diff --git a/src/azure-cli/azure/cli/command_modules/acs/_help.py b/src/azure-cli/azure/cli/command_modules/acs/_help.py index fd630e62126..29571e3b82c 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_help.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_help.py @@ -279,7 +279,9 @@ long-summary: |- These addons are available: http_application_routing - configure ingress with automatic public DNS name creation. - monitoring - turn on Log Analytics monitoring. Uses the Log Analytics Default Workspace if it exists, else creates one. Specify "--workspace-resource-id" to use an existing workspace. + monitoring - turn on Log Analytics monitoring. Uses the Log Analytics Default Workspace if it exists, else creates one. + Specify "--workspace-resource-id" to use an existing workspace. + If monitoring addon is enabled --no-wait argument will have no effect virtual-node - enable AKS Virtual Node (PREVIEW). Requires --subnet-name to provide the name of an existing subnet for the Virtual Node to use. - name: --disable-rbac type: bool @@ -447,6 +449,7 @@ These addons are available: http_application_routing - configure ingress with automatic public DNS name creation. monitoring - turn on Log Analytics monitoring. Requires "--workspace-resource-id". + If monitoring addon is enabled --no-wait argument will have no effect virtual-node - enable AKS Virtual Node (PREVIEW). Requires --subnet-name to provide the name of an existing subnet for the Virtual Node to use. parameters: - name: --addons -a 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 49c93d24c3c..464c404d3dc 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/custom.py +++ b/src/azure-cli/azure/cli/command_modules/acs/custom.py @@ -551,7 +551,7 @@ def _build_service_principal(rbac_client, cli_ctx, name, url, client_secret): return service_principal -def _add_role_assignment(cli_ctx, role, service_principal, delay=2, scope=None): +def _add_role_assignment(cli_ctx, role, service_principal_msi_id, is_service_principal=True, delay=2, scope=None): # AAD can have delays in propagating data, so sleep and retry hook = cli_ctx.get_progress_controller(True) hook.add(message='Waiting for AAD role to propagate', value=0, total_val=1.0) @@ -560,7 +560,7 @@ def _add_role_assignment(cli_ctx, role, service_principal, delay=2, scope=None): hook.add(message='Waiting for AAD role to propagate', value=0.1 * x, total_val=1.0) try: # TODO: break this out into a shared utility library - create_role_assignment(cli_ctx, role, service_principal, scope=scope) + create_role_assignment(cli_ctx, role, service_principal_msi_id, is_service_principal, scope=scope) break except CloudError as ex: if ex.message == 'The role assignment already exists.': @@ -1395,11 +1395,14 @@ def create_service_principal(cli_ctx, identifier, resolve_app=True, rbac_client= return rbac_client.service_principals.create(ServicePrincipalCreateParameters(app_id=app_id, account_enabled=True)) -def create_role_assignment(cli_ctx, role, assignee, resource_group_name=None, scope=None): - return _create_role_assignment(cli_ctx, role, assignee, resource_group_name, scope) +def create_role_assignment(cli_ctx, role, assignee, is_service_principal, resource_group_name=None, scope=None): + return _create_role_assignment(cli_ctx, + role, assignee, resource_group_name, + scope, resolve_assignee=is_service_principal) -def _create_role_assignment(cli_ctx, role, assignee, resource_group_name=None, scope=None, resolve_assignee=True): +def _create_role_assignment(cli_ctx, role, assignee, + resource_group_name=None, scope=None, resolve_assignee=True): from azure.cli.core.profiles import ResourceType, get_sdk factory = get_auth_management_client(cli_ctx, scope) assignments_client = factory.role_assignments @@ -1408,7 +1411,11 @@ def _create_role_assignment(cli_ctx, role, assignee, resource_group_name=None, s scope = _build_role_scope(resource_group_name, scope, assignments_client.config.subscription_id) role_id = _resolve_role_id(role, scope, definitions_client) + + # If the cluster has service principal resolve the service principal client id to get the object id, + # if not use MSI object id. object_id = _resolve_object_id(cli_ctx, assignee) if resolve_assignee else assignee + RoleAssignmentCreateParameters = get_sdk(cli_ctx, ResourceType.MGMT_AUTHORIZATION, 'RoleAssignmentCreateParameters', mod='models', operation_group='role_assignments') @@ -1597,6 +1604,38 @@ def _validate_ssh_key(no_ssh_key, ssh_key_value): raise CLIError('Provided ssh key ({}) is invalid or non-existent'.format(shortened_key)) +def _add_monitoring_role_assignment(result, cluster_resource_id, cmd): + service_principal_msi_id = None + # Check if service principal exists, if it does, assign permissions to service principal + # Else, provide permissions to MSI + if ( + hasattr(result, 'service_principal_profile') and + hasattr(result.service_principal_profile, 'client_id') and + result.service_principal_profile.client_id != 'msi' + ): + logger.info('valid service principal exists, using it') + service_principal_msi_id = result.service_principal_profile.client_id + is_service_principal = True + elif ( + (hasattr(result, 'addon_profiles')) and + ('omsagent' in result.addon_profiles) and + (hasattr(result.addon_profiles['omsagent'], 'identity')) and + (hasattr(result.addon_profiles['omsagent'].identity, 'object_id')) + ): + logger.info('omsagent MSI exists, using it') + service_principal_msi_id = result.addon_profiles['omsagent'].identity.object_id + is_service_principal = False + + if service_principal_msi_id is not None: + if not _add_role_assignment(cmd.cli_ctx, 'Monitoring Metrics Publisher', + service_principal_msi_id, is_service_principal, scope=cluster_resource_id): + logger.warning('Could not create a role assignment for Monitoring addon. ' + 'Are you an Owner on this subscription?') + else: + logger.warning('Could not find service principal or user assigned MSI for role' + 'assignment') + + # pylint: disable=too-many-statements,too-many-branches def aks_create(cmd, client, resource_group_name, name, ssh_key_value, # pylint: disable=too-many-locals dns_name_prefix=None, @@ -1789,25 +1828,29 @@ def aks_create(cmd, client, resource_group_name, name, ssh_key_value, # pylint: retry_exception = Exception(None) for _ in range(0, max_retry): try: - result = sdk_no_wait(no_wait, - client.create_or_update, - resource_group_name=resource_group_name, - resource_name=name, parameters=mc) - # add cluster spn with Monitoring Metrics Publisher role assignment to the cluster resource - # mdm metrics supported only in azure public cloud so add the role assignment only in this cloud - cloud_name = cmd.cli_ctx.cloud.name - if cloud_name.lower() == 'azurecloud' and monitoring: - from msrestazure.tools import resource_id - cluster_resource_id = resource_id( - subscription=subscription_id, - resource_group=resource_group_name, - namespace='Microsoft.ContainerService', type='managedClusters', - name=name - ) - if not _add_role_assignment(cmd.cli_ctx, 'Monitoring Metrics Publisher', - service_principal_profile.client_id, scope=cluster_resource_id): - logger.warning('Could not create a role assignment for monitoring addon. ' - 'Are you an Owner on this subscription?') + if monitoring: + # adding a wait here since we rely on the result for role assignment + result = LongRunningOperation(cmd.cli_ctx)(client.create_or_update( + resource_group_name=resource_group_name, + resource_name=name, + parameters=mc)) + cloud_name = cmd.cli_ctx.cloud.name + # add cluster spn/msi Monitoring Metrics Publisher role assignment to publish metrics to MDM + # mdm metrics is supported only in azure public cloud, so add the role assignment only in this cloud + if cloud_name.lower() == 'azurecloud': + from msrestazure.tools import resource_id + cluster_resource_id = resource_id( + subscription=subscription_id, + resource_group=resource_group_name, + namespace='Microsoft.ContainerService', type='managedClusters', + name=name + ) + _add_monitoring_role_assignment(result, cluster_resource_id, cmd) + else: + result = sdk_no_wait(no_wait, + client.create_or_update, + resource_group_name=resource_group_name, + resource_name=name, parameters=mc) return result except CloudError as ex: retry_exception = ex @@ -1840,12 +1883,14 @@ def aks_enable_addons(cmd, client, resource_group_name, name, addons, workspace_ subnet_name=None, no_wait=False): instance = client.get(resource_group_name, name) subscription_id = get_subscription_id(cmd.cli_ctx) - service_principal_client_id = instance.service_principal_profile.client_id + instance = _update_addons(cmd, instance, subscription_id, resource_group_name, addons, enable=True, workspace_resource_id=workspace_resource_id, subnet_name=subnet_name, no_wait=no_wait) if 'omsagent' in instance.addon_profiles and instance.addon_profiles['omsagent'].enabled: _ensure_container_insights_for_monitoring(cmd, instance.addon_profiles['omsagent']) + # adding a wait here since we rely on the result for role assignment + result = LongRunningOperation(cmd.cli_ctx)(client.create_or_update(resource_group_name, name, instance)) cloud_name = cmd.cli_ctx.cloud.name # mdm metrics supported only in Azure Public cloud so add the role assignment only in this cloud if cloud_name.lower() == 'azurecloud': @@ -1856,13 +1901,12 @@ def aks_enable_addons(cmd, client, resource_group_name, name, addons, workspace_ namespace='Microsoft.ContainerService', type='managedClusters', name=name ) - if not _add_role_assignment(cmd.cli_ctx, 'Monitoring Metrics Publisher', - service_principal_client_id, scope=cluster_resource_id): - logger.warning('Could not create a role assignment for Monitoring addon. ' - 'Are you an Owner on this subscription?') + _add_monitoring_role_assignment(result, cluster_resource_id, cmd) + else: + result = sdk_no_wait(no_wait, client.create_or_update, + resource_group_name, name, instance) - # send the managed cluster representation to update the addon profiles - return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, name, instance) + return result def aks_get_versions(cmd, client, location): diff --git a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py index 8df3e825578..56b1c424dff 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py +++ b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py @@ -56,9 +56,20 @@ def test_add_role_assignment_basic(self): with mock.patch( 'azure.cli.command_modules.acs.custom.create_role_assignment') as create_role_assignment: ok = _add_role_assignment(cli_ctx, role, sp, delay=0) - create_role_assignment.assert_called_with(cli_ctx, role, sp, scope=None) + create_role_assignment.assert_called_with(cli_ctx, role, sp, True, scope=None) self.assertTrue(ok, 'Expected _add_role_assignment to succeed') + def test_add_role_assignment_msi_basic(self): + role = 'Owner' + sp = '1234567' + cli_ctx = mock.MagicMock() + + with mock.patch( + 'azure.cli.command_modules.acs.custom.create_role_assignment') as create_role_assignment: + ok = _add_role_assignment(cli_ctx, role, sp, False, delay=0) + create_role_assignment.assert_called_with(cli_ctx, role, sp, False, scope=None) + self.assertTrue(ok, 'Expected _add_role_assignment with msi to succeed') + def test_add_role_assignment_exists(self): role = 'Owner' sp = '1234567' @@ -74,7 +85,7 @@ def test_add_role_assignment_exists(self): create_role_assignment.side_effect = err ok = _add_role_assignment(cli_ctx, role, sp, delay=0) - create_role_assignment.assert_called_with(cli_ctx, role, sp, scope=None) + create_role_assignment.assert_called_with(cli_ctx, role, sp, True, scope=None) self.assertTrue(ok, 'Expected _add_role_assignment to succeed') def test_add_role_assignment_fails(self): @@ -92,7 +103,7 @@ def test_add_role_assignment_fails(self): create_role_assignment.side_effect = err ok = _add_role_assignment(cli_ctx, role, sp, delay=0) - create_role_assignment.assert_called_with(cli_ctx, role, sp, scope=None) + create_role_assignment.assert_called_with(cli_ctx, role, sp, True, scope=None) self.assertFalse(ok, 'Expected _add_role_assignment to fail') @mock.patch('azure.cli.core.commands.client_factory.get_subscription_id')