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.

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

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Feb 14, 2023

Related command
az role

Description
Close #23372
Migrate azure-mgmt-authorization SDK to Track 2 and bump API Version to 2022-04-01:

  • role_assignments: 2020-04-01-preview -> 2022-04-01
  • role_definitions: 2018-01-01-preview -> 2022-04-01

Breaking changes in azure-mgmt-authorization SDK

For SDK version 0.61.0 (Track 1) -> 3.0.0 (Track 2):

  • 1-1: definitions_client.config and assignments_client.config are changed to definitions_client._config and assignments_client._config.
  • 1-2: custom_headers are changed to headers.
  • 1-3: When a REST call fails, the error is changed from msrestazure.azure_exceptions.CloudError to azure.core.exceptions.HttpResponseError

For API version 2020-04-01-preview -> 2022-04-01:

For API version 2015-07-01:

  • 3-1: ProviderOperationsMetadataOperations.get changes api_version from positional argument to keyword argument.

@ghost ghost requested a review from yonzhan February 14, 2023 08:26
@ghost ghost added the Auto-Assign Auto assign by bot label Feb 14, 2023
@ghost ghost assigned jiasli Feb 14, 2023
@ghost ghost added this to the Feb 2023 (2023-03-07) milestone Feb 14, 2023
@ghost ghost added the RBAC az role label Feb 14, 2023
@@ -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

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:

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:

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

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.

@jiasli jiasli changed the title [Role] Migrate azure-mgmt-authorization SDK to Track 2 and bump API Version to 2022-04-01 [Role] Migrate azure-mgmt-authorization SDK to Track 2 and bump API Version to 2022-04-01 Feb 14, 2023
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 14, 2023

Migrate azure-mgmt-authorization SDK to Track 2

@jiasli
Copy link
Member Author

jiasli commented Feb 15, 2023

Affected modules: vm aro iot acs resource

@jiasli
Copy link
Member Author

jiasli commented Feb 15, 2023

I accidentally discovered a problem with ams module due to breaking change 2-1, when searching for occurrences of 2020-04-01-preview API.

In 2022-04-01, RoleAssignmentsOperations changes list method to list_for_subscription:

https://github.com/Azure/azure-rest-api-specs/blob/b02ad2011daebdaa4ffc1b0b338181a464d49c47/specification/authorization/resource-manager/Microsoft.Authorization/preview/2020-04-01-preview/authorization-RoleAssignmentsCalls.json#L392-L397

    "/subscriptions/{subscriptionId}/providers/Microsoft.Authorization/roleAssignments": {
      "get": {
        "tags": [
          "RoleAssignments"
        ],
        "operationId": "RoleAssignments_List",

https://github.com/Azure/azure-rest-api-specs/blob/495363bc011ce917f579adc1a5209073565d37f4/specification/authorization/resource-manager/Microsoft.Authorization/stable/2022-04-01/authorization-RoleAssignmentsCalls.json#L37-L42

    "/subscriptions/{subscriptionId}/providers/Microsoft.Authorization/roleAssignments": {
      "get": {
        "tags": [
          "RoleAssignments"
        ],
        "operationId": "RoleAssignments_ListForSubscription",

so this line will fail:

assignments = list(assignments_client.list(filter=f))

However, the retry decorator has a bug:

for retry_time in range(0, _RETRY_TIMES):
try:
return func(*args, **kwargs)
break # pylint: disable=unreachable
except Exception as ex: # pylint: disable=broad-except
if retry_time < _RETRY_TIMES:
time.sleep(5)
logger.warning('Retrying %s creation: %s/%s', kwargs['entity_name_string'],
retry_time + 1, _RETRY_TIMES)
else:
logger.warning("Creating %s failed for appid. Trace followed:\n%s",
kwargs['entity_name_string'], ex.response.headers if hasattr(ex, 'response') else ex)
# pylint: disable=no-member
raise

retry_time will always be < _RETRY_TIMES, so line 255 will never be hit and the error will be silenced.

See the doc for range: https://docs.python.org/3/library/stdtypes.html#typesseq-range

@@ -2141,7 +2141,7 @@ def show_provider_operations(cmd, resource_provider_namespace):
version = getattr(get_api_version(cmd.cli_ctx, ResourceType.MGMT_AUTHORIZATION), 'provider_operations_metadata')
auth_client = _authorization_management_client(cmd.cli_ctx)
if version == '2015-07-01':
return auth_client.provider_operations_metadata.get(resource_provider_namespace, version)
return auth_client.provider_operations_metadata.get(resource_provider_namespace, api_version=version)
Copy link
Member Author

@jiasli jiasli Feb 15, 2023

Choose a reason for hiding this comment

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

This is to handle breaking change 3-1.

@jiasli
Copy link
Member Author

jiasli commented Feb 15, 2023

I discovered a problem with iot module:

create = client.iot_hub_resource.begin_create_or_update(resource_group_name, hub_name, hub_description, polling=True)
if identity_role and identity_scopes:
create.add_done_callback(identity_assignment)

polling=True causes SDK to start a separate thread to do the LRO polling. Even if identity_assignment fails, it fails in that separate thread, so the main thread running the CLI command still succeeds, which is not expected.

@@ -736,7 +737,7 @@ def assign_identity(cli_ctx, getter, setter, identity_role=None, identity_scope=
assignments_client.create(scope=identity_scope, role_assignment_name=assignment_name,
parameters=parameters)
break
except CloudError as ex:
except HttpResponseError as ex:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to handle breaking change 1-3.

@@ -104,7 +107,7 @@ def add_role_assignment_executor(cmd, role, assignee, resource_group_name=None,
operation_group="role_assignments",
)
properties = RoleAssignmentProperties(role_definition_id=role_id, principal_id=object_id)
return assignments_client.create(scope, assignment_name, properties, custom_headers=custom_headers)
return assignments_client.create(scope, assignment_name, properties, headers=custom_headers)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to handle breaking change 1-2.

Comment on lines +50 to +55
# 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 = RoleAssignmentCreateParameters(
role_definition_id=role_id, principal_id=object_id, principal_type=assignee_principal_type,
description=description, condition=condition, condition_version=condition_version)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to handle breaking change 2-2.

@jiasli jiasli changed the title [Role] Migrate azure-mgmt-authorization SDK to Track 2 and bump API Version to 2022-04-01 [Role] Migrate azure-mgmt-authorization SDK to Track 2 and bump API version to 2022-04-01 Feb 16, 2023
@jiasli jiasli requested a review from FumingZhang February 16, 2023 03:14
@jiasli jiasli requested a review from bebound February 16, 2023 05:55
wangzelin007
wangzelin007 previously approved these changes Feb 16, 2023
calvinhzy
calvinhzy previously approved these changes Feb 16, 2023
@jiasli jiasli mentioned this pull request Feb 16, 2023
3 tasks
'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

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:

# 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

evelyn-ys
evelyn-ys previously approved these changes Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot RBAC az role
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release of Microsoft.Authorization 2022-04-01
8 participants