From 1c4d73da71df48ad4a537e9f6624722d1ffc7dd7 Mon Sep 17 00:00:00 2001 From: jovieir Date: Fri, 9 May 2025 07:31:58 +0000 Subject: [PATCH 1/7] added assignee-principal-type param to az aks update --attach-acr --- .../azure/cli/command_modules/acs/_params.py | 7 ++--- .../command_modules/acs/_roleassignments.py | 27 ++++++++++++------- .../cli/command_modules/acs/_validators.py | 12 ++++++++- .../azure/cli/command_modules/acs/custom.py | 1 + .../acs/managed_cluster_decorator.py | 20 +++++++++++++- .../acs/tests/latest/test_aks_commands.py | 21 +++++++++++++++ 6 files changed, 73 insertions(+), 15 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/acs/_params.py b/src/azure-cli/azure/cli/command_modules/acs/_params.py index 1ea258aea7b..57d07442908 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_params.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_params.py @@ -580,8 +580,10 @@ def load_arguments(self, _): c.argument('gmsa_dns_server') c.argument('gmsa_root_domain_name') c.argument('disable_windows_gmsa', action='store_true') - c.argument('attach_acr', acr_arg_type, validator=validate_acr) - c.argument('detach_acr', acr_arg_type, validator=validate_acr) + c.argument('attach_acr', acr_arg_type, validator=validate_acr, help='Attach an ACR to the AKS cluster.') + c.argument('detach_acr', acr_arg_type, validator=validate_acr, help='Detach an ACR from the AKS cluster.') + c.argument('assignee_principal_type', validator=validate_acr, options_list=['--assignee-principal-type'], + help='Use this parameter in conjunction with --attach-acr to explicitly set the principal type in the ACR role assignment. This helps avoid RBAC-related errors by ensuring the correct principal type is applied. Valid values are \'User\', \'Group\' or \'ServicePrincipal\'') c.argument('disable_defender', action='store_true', validator=validate_defender_disable_and_enable_parameters) c.argument('enable_defender', action='store_true') c.argument('defender_config', validator=validate_defender_config_parameter) @@ -693,7 +695,6 @@ def load_arguments(self, _): c.argument('disable_cost_analysis', action='store_true') c.argument("if_match") c.argument("if_none_match") - with self.argument_context('aks disable-addons', resource_type=ResourceType.MGMT_CONTAINERSERVICE, operation_group='managed_clusters') as c: c.argument('addons', options_list=['--addons', '-a']) diff --git a/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py b/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py index 224f1792f13..c765d6f3ab1 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py @@ -63,7 +63,8 @@ def build_role_scope(resource_group_name: str, scope: str, subscription_id: str) return scope -def add_role_assignment_executor(cmd, role, assignee, resource_group_name=None, scope=None, resolve_assignee=True): +def add_role_assignment_executor(cmd, role, assignee, resource_group_name=None, + scope=None, resolve_assignee=True, assignee_principal_type=None): factory = get_auth_management_client(cmd.cli_ctx, scope) assignments_client = factory.role_assignments definitions_client = factory.role_definitions @@ -94,7 +95,7 @@ def add_role_assignment_executor(cmd, role, assignee, resource_group_name=None, ) if cmd.supported_api_version(min_api="2018-01-01-preview", resource_type=ResourceType.MGMT_AUTHORIZATION): parameters = RoleAssignmentCreateParameters(role_definition_id=role_id, principal_id=object_id, - principal_type=None) + principal_type=assignee_principal_type) return assignments_client.create(scope, assignment_name, parameters, headers=custom_headers) # for backward compatibility @@ -109,7 +110,8 @@ def add_role_assignment_executor(cmd, role, assignee, resource_group_name=None, return assignments_client.create(scope, assignment_name, properties, headers=custom_headers) -def add_role_assignment(cmd, role, service_principal_msi_id, is_service_principal=True, delay=2, scope=None): +def add_role_assignment(cmd, role, service_principal_msi_id, is_service_principal=True, + delay=2, scope=None, assignee_principal_type=None): # AAD can have delays in propagating data, so sleep and retry hook = cmd.cli_ctx.get_progress_controller(True) hook.add(message="Waiting for AAD role to propagate", value=0, total_val=1.0) @@ -124,6 +126,7 @@ def add_role_assignment(cmd, role, service_principal_msi_id, is_service_principa service_principal_msi_id, scope=scope, resolve_assignee=is_service_principal, + assignee_principal_type=assignee_principal_type, ) break except HttpResponseError as ex: @@ -304,8 +307,10 @@ def ensure_cluster_identity_permission_on_kubelet_identity(cmd, cluster_identity "Could not grant Managed Identity Operator permission to cluster identity at scope {}".format(scope) ) - -def ensure_aks_acr_role_assignment(cmd, assignee, registry_id, detach=False, is_service_principal=True): +def ensure_aks_acr_role_assignment(cmd, assignee, registry_id, detach=False, + is_service_principal=True, assignee_principal_type=None): + + if detach: if not delete_role_assignments( cmd.cli_ctx, "acrpull", assignee, scope=registry_id, is_service_principal=is_service_principal @@ -313,13 +318,14 @@ def ensure_aks_acr_role_assignment(cmd, assignee, registry_id, detach=False, is_ raise AzCLIError("Could not delete role assignments for ACR. Are you an Owner on this subscription?") return - if not add_role_assignment(cmd, "acrpull", assignee, scope=registry_id, is_service_principal=is_service_principal): + if not add_role_assignment(cmd, "acrpull", assignee, scope=registry_id, is_service_principal=is_service_principal, + assignee_principal_type=assignee_principal_type): raise AzCLIError("Could not create a role assignment for ACR. Are you an Owner on this subscription?") return - # pylint: disable=unused-argument -def ensure_aks_acr(cmd, assignee, acr_name_or_id, subscription_id, detach=False, is_service_principal=True): +def ensure_aks_acr(cmd, assignee, acr_name_or_id, subscription_id, detach=False, + is_service_principal=True, assignee_principal_type=None): from azure.mgmt.core.tools import is_valid_resource_id, parse_resource_id # Check if the ACR exists by resource ID. @@ -330,7 +336,8 @@ def ensure_aks_acr(cmd, assignee, acr_name_or_id, subscription_id, detach=False, registry = acr_client.registries.get(parsed_registry["resource_group"], parsed_registry["name"]) except HttpResponseError as ex: raise AzCLIError(ex.message) - ensure_aks_acr_role_assignment(cmd, assignee, registry.id, detach, is_service_principal) + ensure_aks_acr_role_assignment(cmd, assignee, registry.id, detach, + is_service_principal, assignee_principal_type) return # Check if the ACR exists by name accross all resource groups. @@ -342,5 +349,5 @@ def ensure_aks_acr(cmd, assignee, acr_name_or_id, subscription_id, detach=False, if "was not found" in ex.message: raise AzCLIError("ACR {} not found. Have you provided the right ACR name?".format(registry_name)) raise AzCLIError(ex.message) - ensure_aks_acr_role_assignment(cmd, assignee, registry.id, detach, is_service_principal) + ensure_aks_acr_role_assignment(cmd, assignee, registry.id, detach, is_service_principal, assignee_principal_type) return diff --git a/src/azure-cli/azure/cli/command_modules/acs/_validators.py b/src/azure-cli/azure/cli/command_modules/acs/_validators.py index 3e83cc80e5d..d7afba6292a 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_validators.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_validators.py @@ -341,7 +341,17 @@ def validate_spot_max_price(namespace): def validate_acr(namespace): if namespace.attach_acr and namespace.detach_acr: raise CLIError('Cannot specify "--attach-acr" and "--detach-acr" at the same time.') - + if namespace.assignee_principal_type and not namespace.attach_acr: + raise CLIError('The "--assignee-principal-type" argument can only be used with "--attach-acr".') + if namespace.attach_acr: + # Validate assignee_principal_type if specified + if namespace.assignee_principal_type: + valid_types = ['User', 'ServicePrincipal', 'Group'] + if namespace.assignee_principal_type not in valid_types: + raise CLIError( + f"Invalid value for --assignee_principal_type. " + f"Allowed values are: {', '.join(valid_types)}" +) def validate_nodepool_tags(ns): """ Extracts multiple space-separated tags in key[=value] format """ diff --git a/src/azure-cli/azure/cli/command_modules/acs/custom.py b/src/azure-cli/azure/cli/command_modules/acs/custom.py index eb8685e4509..7175b2e7f52 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/custom.py +++ b/src/azure-cli/azure/cli/command_modules/acs/custom.py @@ -756,6 +756,7 @@ def aks_update( disable_windows_gmsa=False, attach_acr=None, detach_acr=None, + assignee_principal_type=None, nrg_lockdown_restriction_level=None, enable_defender=False, disable_defender=False, diff --git a/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py b/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py index 82c1277430f..d12732ffe8b 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py +++ b/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py @@ -1872,6 +1872,21 @@ def get_detach_acr(self) -> Union[str, None]: # this parameter does not need validation return detach_acr + def get_assignee_principal_type(self) -> Union[str, None]: + """Obtain the value of assignee_principal_type. + + This function will return the override value of the assignee principal type (e.g., User, ServicePrincipal, Group) + to determine the type of identity being assigned for the ACR role. This is used + during the attach ACR operation to ensure proper role assignment based on the identity type. + + :return: string or None + """ + # read the original value passed by the command + assignee_principal_type = self.raw_param.get("assignee_principal_type") + # this parameter does not need dynamic completion + # this parameter does not need validation + return assignee_principal_type + def get_http_proxy_config(self) -> Union[Dict, ManagedClusterHTTPProxyConfig, None]: """Obtain the value of http_proxy_config. @@ -7374,7 +7389,7 @@ def process_attach_detach_acr(self, mc: ManagedCluster) -> None: assignee, is_service_principal = self.context.get_assignee_from_identity_or_sp_profile() attach_acr = self.context.get_attach_acr() detach_acr = self.context.get_detach_acr() - + assignee_principal_type = self.context.get_assignee_principal_type() if attach_acr: self.context.external_functions.ensure_aks_acr( self.cmd, @@ -7382,6 +7397,7 @@ def process_attach_detach_acr(self, mc: ManagedCluster) -> None: acr_name_or_id=attach_acr, subscription_id=subscription_id, is_service_principal=is_service_principal, + assignee_principal_type=assignee_principal_type, ) if detach_acr: @@ -7392,6 +7408,7 @@ def process_attach_detach_acr(self, mc: ManagedCluster) -> None: subscription_id=subscription_id, detach=True, is_service_principal=is_service_principal, + assignee_principal_type=assignee_principal_type, ) def update_azure_service_mesh_profile(self, mc: ManagedCluster) -> ManagedCluster: @@ -8925,6 +8942,7 @@ def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None: acr_name_or_id=attach_acr, subscription_id=self.context.get_subscription_id(), is_service_principal=False, + assignee_principal_type=self.context.get_assignee_principal_type() ) enable_azure_container_storage = self.context.get_intermediate("enable_azure_container_storage") diff --git a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py index 5ad5ce8fa48..62258e20fba 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py +++ b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py @@ -7233,6 +7233,27 @@ def test_aks_update_attatch_acr(self, resource_group, resource_group_location, s # check role assignment self.cmd(role_assignment_check_cmd, checks=[self.is_empty()]) + # Test with --assignee-principal-type + attach_with_principal_cmd = "aks update --resource-group={resource_group} --name={name} --attach-acr={acr_name} --assignee-principal-type=ServicePrincipal" + self.cmd(attach_with_principal_cmd) + + # check role assignment with principal type + role_assignment_with_principal_check_cmd = ( + "role assignment list --scope {acr_scope} --assignee " + + assignee_object_id if assignee_object_id else sp_name + ) + self.cmd(role_assignment_with_principal_check_cmd, checks=[ + self.check('length(@) == `1`', True), + self.check('[0].principalType', 'ServicePrincipal') + ]) + + # detach acr + attach_cmd = 'aks update --resource-group={resource_group} --name={name} --detach-acr={acr_name}' + self.cmd(attach_cmd) + + # check role assignment + self.cmd(role_assignment_check_cmd, checks=[self.is_empty()]) + # delete self.cmd( 'aks delete -g {resource_group} -n {name} --yes --no-wait', checks=[self.is_empty()]) From 8f0c4e6e14d569ecb252824ab41621bfb6024dcc Mon Sep 17 00:00:00 2001 From: jovieir Date: Mon, 12 May 2025 17:28:41 +0000 Subject: [PATCH 2/7] Fixed pep8 style --- .../azure/cli/command_modules/acs/_roleassignments.py | 10 ++++++---- .../azure/cli/command_modules/acs/_validators.py | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py b/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py index c765d6f3ab1..1912efefee2 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_roleassignments.py @@ -307,10 +307,10 @@ def ensure_cluster_identity_permission_on_kubelet_identity(cmd, cluster_identity "Could not grant Managed Identity Operator permission to cluster identity at scope {}".format(scope) ) -def ensure_aks_acr_role_assignment(cmd, assignee, registry_id, detach=False, - is_service_principal=True, assignee_principal_type=None): - - + +def ensure_aks_acr_role_assignment(cmd, assignee, registry_id, detach=False, is_service_principal=True, + assignee_principal_type=None): + if detach: if not delete_role_assignments( cmd.cli_ctx, "acrpull", assignee, scope=registry_id, is_service_principal=is_service_principal @@ -323,9 +323,11 @@ def ensure_aks_acr_role_assignment(cmd, assignee, registry_id, detach=False, raise AzCLIError("Could not create a role assignment for ACR. Are you an Owner on this subscription?") return + # pylint: disable=unused-argument def ensure_aks_acr(cmd, assignee, acr_name_or_id, subscription_id, detach=False, is_service_principal=True, assignee_principal_type=None): + from azure.mgmt.core.tools import is_valid_resource_id, parse_resource_id # Check if the ACR exists by resource ID. diff --git a/src/azure-cli/azure/cli/command_modules/acs/_validators.py b/src/azure-cli/azure/cli/command_modules/acs/_validators.py index d7afba6292a..41ae44db12c 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_validators.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_validators.py @@ -351,7 +351,8 @@ def validate_acr(namespace): raise CLIError( f"Invalid value for --assignee_principal_type. " f"Allowed values are: {', '.join(valid_types)}" -) + ) + def validate_nodepool_tags(ns): """ Extracts multiple space-separated tags in key[=value] format """ From 8afc4ca2855dda15c06779116a299f1efca8fabf Mon Sep 17 00:00:00 2001 From: jovieir Date: Tue, 13 May 2025 05:42:53 +0000 Subject: [PATCH 3/7] fixed unittest SP validation --- .../cli/command_modules/acs/tests/latest/test_aks_commands.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py index 18074740364..54083477e5f 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py +++ b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py @@ -7181,7 +7181,7 @@ def test_aks_update_attatch_acr(self, resource_group, resource_group_location, s assignee_object_id = _get_test_sp_object_id(sp_name) role_assignment_check_cmd = ( "role assignment list --scope {acr_scope} --assignee " + - assignee_object_id if assignee_object_id else sp_name + assignee_object_id if assignee_object_id else "role assignment list --scope {acr_scope} --assignee " + sp_name ) # attach acr @@ -7205,7 +7205,7 @@ def test_aks_update_attatch_acr(self, resource_group, resource_group_location, s # check role assignment with principal type role_assignment_with_principal_check_cmd = ( "role assignment list --scope {acr_scope} --assignee " + - assignee_object_id if assignee_object_id else sp_name + assignee_object_id if assignee_object_id else "role assignment list --scope {acr_scope} --assignee " + sp_name ) self.cmd(role_assignment_with_principal_check_cmd, checks=[ self.check('length(@) == `1`', True), From 1fe0c59327e08e77c59d6453210bb311974ac598 Mon Sep 17 00:00:00 2001 From: jovieir Date: Sat, 24 May 2025 00:12:20 +0000 Subject: [PATCH 4/7] Fixed tests --- .../acs/tests/latest/test_managed_cluster_decorator.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py index 7d6ecdfb629..b9f1b4aeecf 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py +++ b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py @@ -8222,6 +8222,7 @@ def test_postprocessing_after_mc_created(self): acr_name_or_id="test_attach_acr", subscription_id="1234-5678-9012", is_service_principal=False, + assignee_principal_type=None, ) def test_put_mc(self): @@ -8946,6 +8947,7 @@ def test_process_attach_detach_acr(self): acr_name_or_id="test_attach_acr", subscription_id="test_subscription_id", is_service_principal=False, + assignee_principal_type=None, ), call( self.cmd, @@ -8954,6 +8956,7 @@ def test_process_attach_detach_acr(self): subscription_id="test_subscription_id", detach=True, is_service_principal=False, + assignee_principal_type=None, ), ] ) @@ -11929,6 +11932,7 @@ def test_postprocessing_after_mc_created(self): acr_name_or_id="test_attach_acr", subscription_id="1234-5678-9012", is_service_principal=False, + assignee_principal_type=None, ) def test_put_mc(self): From 5f91d940f6383fad5f347b88e72fad1bb438bcf2 Mon Sep 17 00:00:00 2001 From: jovieir Date: Sat, 24 May 2025 02:44:08 +0000 Subject: [PATCH 5/7] Adding short command option '-t' for assignee-principal-type --- src/azure-cli/azure/cli/command_modules/acs/_params.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/azure-cli/azure/cli/command_modules/acs/_params.py b/src/azure-cli/azure/cli/command_modules/acs/_params.py index 57d07442908..dae6d90b1ed 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_params.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_params.py @@ -582,7 +582,7 @@ def load_arguments(self, _): c.argument('disable_windows_gmsa', action='store_true') c.argument('attach_acr', acr_arg_type, validator=validate_acr, help='Attach an ACR to the AKS cluster.') c.argument('detach_acr', acr_arg_type, validator=validate_acr, help='Detach an ACR from the AKS cluster.') - c.argument('assignee_principal_type', validator=validate_acr, options_list=['--assignee-principal-type'], + c.argument('assignee_principal_type', validator=validate_acr, options_list=['--assignee-principal-type', '-t'], help='Use this parameter in conjunction with --attach-acr to explicitly set the principal type in the ACR role assignment. This helps avoid RBAC-related errors by ensuring the correct principal type is applied. Valid values are \'User\', \'Group\' or \'ServicePrincipal\'') c.argument('disable_defender', action='store_true', validator=validate_defender_disable_and_enable_parameters) c.argument('enable_defender', action='store_true') From 6bb42fb16f8f19459e55fd2c6546493d6d99e66b Mon Sep 17 00:00:00 2001 From: jovieir Date: Mon, 26 May 2025 19:34:32 +0000 Subject: [PATCH 6/7] Fixed ensure_assignment assert call --- .../acs/tests/latest/test_managed_cluster_decorator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py index b9f1b4aeecf..369e96e892f 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py +++ b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py @@ -6578,7 +6578,7 @@ def test_process_attach_acr(self): return_value=registry, ), patch("azure.cli.command_modules.acs._roleassignments.ensure_aks_acr_role_assignment") as ensure_assignment: dec_3.process_attach_acr(mc_3) - ensure_assignment.assert_called_once_with(self.cmd, "test_service_principal", "test_registry_id", False, True) + ensure_assignment.assert_called_once_with(self.cmd, "test_service_principal", "test_registry_id", False, True, None) def test_set_up_network_profile(self): # default value in `aks_create` From a70eb819bea56b966f235a2795ae48ce44f0f0c6 Mon Sep 17 00:00:00 2001 From: jovieir Date: Tue, 27 May 2025 02:33:32 +0000 Subject: [PATCH 7/7] fixed unittest when assignee_principal_type kwarg is not explicitly called --- .../azure/cli/command_modules/acs/managed_cluster_decorator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py b/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py index d12732ffe8b..664cb8bb97a 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py +++ b/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py @@ -7021,6 +7021,7 @@ def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None: acr_name_or_id=attach_acr, subscription_id=self.context.get_subscription_id(), is_service_principal=False, + assignee_principal_type=self.context.get_assignee_principal_type() ) # azure monitor metrics addon (v2)