Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Role] Migrate azure-mgmt-authorization SDK to Track 2 and bump API version to 2022-04-01 #25452

Merged
merged 17 commits into from
Feb 17, 2023
Merged

[Role] Migrate azure-mgmt-authorization SDK to Track 2 and bump API version to 2022-04-01 #25452

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
898f5a3
code
jiasli Feb 14, 2023
3639f9d
role tests
jiasli Feb 14, 2023
c25dcd5
Add issue link
jiasli Feb 14, 2023
ee43004
test_provider_operation
jiasli Feb 14, 2023
032e64d
pylint
jiasli Feb 14, 2023
ce4e4f5
acs
jiasli Feb 15, 2023
4dda3fc
RoleAssignmentCreateParameters
jiasli Feb 15, 2023
c43a62d
iot
jiasli Feb 15, 2023
1a2d0ac
aro
jiasli Feb 15, 2023
2ffad8b
resource
jiasli Feb 15, 2023
ce22054
vm
jiasli Feb 15, 2023
d185743
acs-pylint
jiasli Feb 15, 2023
422fa12
pylint
jiasli Feb 16, 2023
8c8bbbf
credscan
jiasli Feb 16, 2023
fa84b6b
Merge remote-tracking branch 'azure/dev' into role-track2
jiasli Feb 16, 2023
6fa073c
acs iot
jiasli Feb 16, 2023
35a3147
vm
jiasli Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/azure-cli-core/azure/cli/core/profiles/_shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,9 @@ def default_api_version(self):
ResourceType.MGMT_RESOURCE_PRIVATELINKS: '2020-05-01',
ResourceType.MGMT_NETWORK_DNS: '2018-05-01',
ResourceType.MGMT_KEYVAULT: '2022-07-01',
ResourceType.MGMT_AUTHORIZATION: SDKProfile('2020-04-01-preview', {
ResourceType.MGMT_AUTHORIZATION: SDKProfile('2022-04-01', {
'classic_administrators': '2015-06-01',
'role_definitions': '2018-01-01-preview',
'role_definitions': '2022-04-01',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'role_definitions': '2022-04-01',

We can simply remove this line if role_definitions uses the same API version with default one~

Copy link
Member Author

@jiasli jiasli Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

role_definitions frequently diverges from role_assignments:

azure-cli/src/azure-cli/azure/cli/command_modules/role/_multi_api_adaptor.py

Lines 62 to 65 in 5f40c5e

# 2015-07-01 RoleDefinition: flattened, RoleAssignment: unflattened
# 2018-01-01-preview RoleDefinition: flattened
# 2020-04-01-preview RoleAssignment: flattened
# Get property_name from properties if the model is unflattened.

so it is merely a placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

role_definitions will again diverge from role_assignments: #26577

'provider_operations_metadata': '2018-01-01-preview'
}),
ResourceType.MGMT_CONTAINERREGISTRY: SDKProfile('2022-02-01-preview', {
Expand Down
9 changes: 7 additions & 2 deletions src/azure-cli/azure/cli/command_modules/role/_multi_api_adaptor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,12 @@ def create_role_assignment(self, client, assignment_name, role_id, object_id, sc
'RoleAssignmentProperties' if self.old_api else 'RoleAssignmentCreateParameters',
mod='models', operation_group='role_assignments')
parameters = RoleAssignmentCreateParameters(role_definition_id=role_id, principal_id=object_id)
if assignee_principal_type:
parameters.principal_type = assignee_principal_type

# In 2022-04-01 API, principal_type is by default 'User', so we have to explicitly set it to None if we can't
# resolve principal type from Graph
# https://github.com/Azure/azure-rest-api-specs/issues/21664
parameters.principal_type = assignee_principal_type

if description:
parameters.description = description
if condition:
Expand All @@ -62,6 +66,7 @@ def get_role_property(self, obj, property_name): # pylint: disable=no-self-use
# 2015-07-01 RoleDefinition: flattened, RoleAssignment: unflattened
# 2018-01-01-preview RoleDefinition: flattened
# 2020-04-01-preview RoleAssignment: flattened
# 2022-04-01 RoleDefinition: flattened RoleAssignment: flattened
# Get property_name from properties if the model is unflattened.
if isinstance(obj, dict):
if 'properties' in obj:
Expand Down
30 changes: 15 additions & 15 deletions src/azure-cli/azure/cli/command_modules/role/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def list_role_definitions(cmd, name=None, resource_group_name=None, scope=None,
custom_role_only=False):
definitions_client = _auth_client_factory(cmd.cli_ctx, scope).role_definitions
scope = _build_role_scope(resource_group_name, scope,
definitions_client.config.subscription_id)
definitions_client._config.subscription_id)
Copy link
Member Author

@jiasli jiasli Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For breaking change 1-1, Track 2 now makes config a protected attribute as _config. I can't find a better way to extract subscription ID from the SDK client.

This pattern has already been used by resource command module:

azure-cli/src/azure-cli/azure/cli/command_modules/resource/custom.py

Lines 3551 to 3552 in 1b62d20

subscriptionId=serialize.url(
"self._config.subscription_id", self.rcf.resources._config.subscription_id, 'str'),

Another option is to let get_mgmt_service_client return the subscription ID during client creation:

azure-cli/src/azure-cli-core/azure/cli/core/commands/client_factory.py

Lines 50 to 51 in 21266e0

def get_mgmt_service_client(cli_ctx, client_or_resource_type, subscription_id=None, api_version=None,
aux_subscriptions=None, aux_tenants=None, credential=None, **kwargs):

but this is such a big breaking change that it will affect almost all command modules and extension which use get_mgmt_service_client to create a client.

Copy link
Member Author

@jiasli jiasli Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was mentioned in doc: https://github.com/Azure/azure-cli/blob/dev/doc/track_2_migration_guidance.md#obtaining-subscription

However, the doc is not accurate as the subscription used to create the client (possibly via --subscription) may not be the same as the current subscription. Assuming the subscription used to create the client being the same as the current subscription can introduce bugs very difficult to track down.

return _search_role_definitions(cmd.cli_ctx, definitions_client, name, [scope], custom_role_only)


Expand Down Expand Up @@ -94,7 +94,7 @@ def _create_update_role_definition(cmd, role_definition, for_update):
definitions_client = _auth_client_factory(cmd.cli_ctx, scope=role_resource_id).role_definitions
scopes_in_definition = role_definition.get('assignableScopes', None)
scopes = (scopes_in_definition if scopes_in_definition else
['/subscriptions/' + definitions_client.config.subscription_id])
['/subscriptions/' + definitions_client._config.subscription_id])
if role_resource_id:
from msrestazure.tools import parse_resource_id
role_id = parse_resource_id(role_resource_id)['name']
Expand Down Expand Up @@ -125,7 +125,7 @@ def delete_role_definition(cmd, name, resource_group_name=None, scope=None,
custom_role_only=False):
definitions_client = _auth_client_factory(cmd.cli_ctx, scope).role_definitions
scope = _build_role_scope(resource_group_name, scope,
definitions_client.config.subscription_id)
definitions_client._config.subscription_id)
roles = _search_role_definitions(cmd.cli_ctx, definitions_client, name, [scope], custom_role_only)
for r in roles:
definitions_client.delete(role_definition_id=r.name, scope=scope)
Expand Down Expand Up @@ -195,7 +195,7 @@ def _create_role_assignment(cli_ctx, role, assignee, resource_group_name=None, s
assignments_client = factory.role_assignments
definitions_client = factory.role_definitions
scope = _build_role_scope(resource_group_name, scope,
assignments_client.config.subscription_id)
assignments_client._config.subscription_id)

role_id = _resolve_role_id(role, scope, definitions_client)
object_id = _resolve_object_id(cli_ctx, assignee) if resolve_assignee else assignee
Expand All @@ -213,25 +213,25 @@ def list_role_assignments(cmd, assignee=None, role=None, resource_group_name=Non
member(transitively).
'''
graph_client = _graph_client_factory(cmd.cli_ctx)
factory = _auth_client_factory(cmd.cli_ctx, scope)
assignments_client = factory.role_assignments
definitions_client = factory.role_definitions
authorization_client = _auth_client_factory(cmd.cli_ctx, scope)
assignments_client = authorization_client.role_assignments
definitions_client = authorization_client.role_definitions

if show_all:
if resource_group_name or scope:
raise CLIError('group or scope are not required when --all is used')
scope = None
else:
scope = _build_role_scope(resource_group_name, scope,
definitions_client.config.subscription_id)
definitions_client._config.subscription_id)

assignments = _search_role_assignments(cmd.cli_ctx, assignments_client, definitions_client,
scope, assignee, role,
include_inherited, include_groups)

results = todict(assignments) if assignments else []
if include_classic_administrators:
results += _backfill_assignments_for_co_admins(cmd.cli_ctx, factory, assignee)
results += _backfill_assignments_for_co_admins(cmd.cli_ctx, authorization_client, assignee)

if not results:
return []
Expand All @@ -240,7 +240,7 @@ def list_role_assignments(cmd, assignee=None, role=None, resource_group_name=Non
# (it's possible that associated roles and principals were deleted, and we just do nothing.)
# 2. fill in role names
role_defs = list(definitions_client.list(
scope=scope or ('/subscriptions/' + definitions_client.config.subscription_id)))
scope=scope or ('/subscriptions/' + definitions_client._config.subscription_id)))
worker = MultiAPIAdaptor(cmd.cli_ctx)
role_dics = {i.id: worker.get_role_property(i, 'role_name') for i in role_defs}
for i in results:
Expand Down Expand Up @@ -470,7 +470,7 @@ def _backfill_assignments_for_co_admins(cli_ctx, auth_client, assignee=None):
'principalName': email,
'roleDefinitionName': admin.role,
'roleDefinitionId': 'NA(classic admin role)',
'scope': '/subscriptions/' + auth_client.config.subscription_id
'scope': '/subscriptions/' + auth_client._config.subscription_id
}
if worker.old_api:
result[-1]['properties'] = properties
Expand Down Expand Up @@ -525,7 +525,7 @@ def delete_role_assignments(cmd, ids=None, assignee=None, role=None, resource_gr
return

scope = _build_role_scope(resource_group_name, scope,
assignments_client.config.subscription_id)
assignments_client._config.subscription_id)
assignments = _search_role_assignments(cmd.cli_ctx, assignments_client, definitions_client,
scope, assignee, role, include_inherited,
include_groups=False)
Expand Down Expand Up @@ -556,9 +556,9 @@ def _search_role_assignments(cli_ctx, assignments_client, definitions_client,
f = "assignedTo('{}')".format(assignee_object_id)
else:
f = "principalId eq '{}'".format(assignee_object_id)
assignments = list(assignments_client.list(filter=f))
assignments = list(assignments_client.list_for_subscription(filter=f))
else:
assignments = list(assignments_client.list())
assignments = list(assignments_client.list_for_subscription())

worker = MultiAPIAdaptor(cli_ctx)
if assignments:
Expand Down Expand Up @@ -604,7 +604,7 @@ def _resolve_role_id(role, scope, definitions_client):
else:
if is_guid(role):
role_id = '/subscriptions/{}/providers/Microsoft.Authorization/roleDefinitions/{}'.format(
definitions_client.config.subscription_id, role)
definitions_client._config.subscription_id, role)
if not role_id: # retrieve role id
role_defs = list(definitions_client.list(scope, "roleName eq '{}'".format(role)))
if not role_defs:
Expand Down
Loading