From 784e8cb76c9f214e2b48346f8d072eb60a234fcb Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Fri, 1 Jul 2022 16:52:40 +0800 Subject: [PATCH 1/3] fix nil config in kvsecret provider addon --- .../azure/cli/command_modules/acs/_params.py | 3 +- .../cli/command_modules/acs/_validators.py | 12 +- .../acs/managed_cluster_decorator.py | 70 +++++++- .../latest/test_managed_cluster_decorator.py | 165 ++++++++++++++++++ .../acs/tests/latest/test_validators.py | 45 ++++- 5 files changed, 284 insertions(+), 11 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 f826bc7678c..525b4e334f6 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_params.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_params.py @@ -41,6 +41,7 @@ validate_priority, validate_snapshot_id, validate_snapshot_name, validate_spot_max_price, validate_ssh_key, validate_taints, validate_vm_set_type, validate_vnet_subnet_id, + validate_keyvault_secrets_provider_disable_and_enable_parameters, validate_defender_disable_and_enable_parameters, validate_defender_config_parameter) from azure.cli.core.commands.parameters import ( edge_zone_type, file_type, get_enum_type, @@ -355,7 +356,7 @@ def load_arguments(self, _): c.argument('defender_config', validator=validate_defender_config_parameter) # addons c.argument('enable_secret_rotation', action='store_true') - c.argument('disable_secret_rotation', action='store_true') + c.argument('disable_secret_rotation', action='store_true', validator=validate_keyvault_secrets_provider_disable_and_enable_parameters) c.argument('rotation_poll_interval') # nodepool paramerters c.argument('enable_cluster_autoscaler', options_list=[ 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 6bbef42fe78..dc5246907ad 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_validators.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_validators.py @@ -12,8 +12,11 @@ from azure.cli.core import keys from azure.cli.core.azclierror import ( - InvalidArgumentValueError, RequiredArgumentMissingError, - ArgumentUsageError) + ArgumentUsageError, + InvalidArgumentValueError, + MutuallyExclusiveArgumentError, + RequiredArgumentMissingError, +) from azure.cli.core.commands.validators import validate_tag from azure.cli.core.util import CLIError from knack.log import get_logger @@ -512,6 +515,11 @@ def validate_credential_format(namespace): raise InvalidArgumentValueError("--format can only be azure or exec.") +def validate_keyvault_secrets_provider_disable_and_enable_parameters(namespace): + if namespace.disable_secret_rotation and namespace.enable_secret_rotation: + raise MutuallyExclusiveArgumentError('Providing both --disable-secret-rotation and --enable-secret-rotation flags is invalid') + + def validate_defender_config_parameter(namespace): if namespace.defender_config and not namespace.enable_defender: raise RequiredArgumentMissingError("Please specify --enable-defnder") 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 025c344c3d4..768934eae09 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 @@ -2475,7 +2475,8 @@ def _get_enable_secret_rotation(self, enable_validation: bool = False) -> bool: if not azure_keyvault_secrets_provider_enabled: raise InvalidArgumentValueError( "--enable-secret-rotation can only be specified " - "when azure-keyvault-secrets-provider is enabled" + "when azure-keyvault-secrets-provider is enabled. " + "Please use command 'az aks enable-addons' to enable it." ) return enable_secret_rotation @@ -2522,7 +2523,8 @@ def _get_disable_secret_rotation(self, enable_validation: bool = False) -> bool: if not azure_keyvault_secrets_provider_enabled: raise InvalidArgumentValueError( "--disable-secret-rotation can only be specified " - "when azure-keyvault-secrets-provider is enabled" + "when azure-keyvault-secrets-provider is enabled. " + "Please use command 'az aks enable-addons' to enable it." ) return disable_secret_rotation @@ -2584,7 +2586,8 @@ def _get_rotation_poll_interval(self, enable_validation: bool = False) -> Union[ if not azure_keyvault_secrets_provider_enabled: raise InvalidArgumentValueError( "--rotation-poll-interval can only be specified " - "when azure-keyvault-secrets-provider is enabled" + "when azure-keyvault-secrets-provider is enabled " + "Please use command 'az aks enable-addons' to enable it." ) return rotation_poll_interval @@ -5456,11 +5459,39 @@ def update_identity(self, mc: ManagedCluster) -> ManagedCluster: mc.identity = identity return mc + def ensure_azure_keyvault_secrets_provider_addon_profile( + self, + azure_keyvault_secrets_provider_addon_profile: ManagedClusterAddonProfile, + ) -> ManagedClusterAddonProfile: + # determine the value of constants + addon_consts = self.context.get_addon_consts() + CONST_SECRET_ROTATION_ENABLED = addon_consts.get( + "CONST_SECRET_ROTATION_ENABLED" + ) + CONST_ROTATION_POLL_INTERVAL = addon_consts.get( + "CONST_ROTATION_POLL_INTERVAL" + ) + if ( + azure_keyvault_secrets_provider_addon_profile is None or + not azure_keyvault_secrets_provider_addon_profile.enabled + ): + raise InvalidArgumentValueError( + "Addon azure-keyvault-secrets-provider is not enabled. " + "Please use command 'az aks enable-addons' to enable it." + ) + elif azure_keyvault_secrets_provider_addon_profile.config is None: + # backfill to default + azure_keyvault_secrets_provider_addon_profile.config = { + CONST_SECRET_ROTATION_ENABLED: "false", + CONST_ROTATION_POLL_INTERVAL: "2m", + } + return azure_keyvault_secrets_provider_addon_profile + def update_azure_keyvault_secrets_provider_addon_profile( self, azure_keyvault_secrets_provider_addon_profile: ManagedClusterAddonProfile, - ) -> None: - """Update azure keyvault secrets provider addon profile in-place. + ) -> ManagedClusterAddonProfile: + """Update azure keyvault secrets provider addon profile. :return: None """ @@ -5474,19 +5505,35 @@ def update_azure_keyvault_secrets_provider_addon_profile( ) if self.context.get_enable_secret_rotation(): + azure_keyvault_secrets_provider_addon_profile = ( + self.ensure_azure_keyvault_secrets_provider_addon_profile( + azure_keyvault_secrets_provider_addon_profile + ) + ) azure_keyvault_secrets_provider_addon_profile.config[ CONST_SECRET_ROTATION_ENABLED ] = "true" if self.context.get_disable_secret_rotation(): + azure_keyvault_secrets_provider_addon_profile = ( + self.ensure_azure_keyvault_secrets_provider_addon_profile( + azure_keyvault_secrets_provider_addon_profile + ) + ) azure_keyvault_secrets_provider_addon_profile.config[ CONST_SECRET_ROTATION_ENABLED ] = "false" if self.context.get_rotation_poll_interval() is not None: + azure_keyvault_secrets_provider_addon_profile = ( + self.ensure_azure_keyvault_secrets_provider_addon_profile( + azure_keyvault_secrets_provider_addon_profile + ) + ) azure_keyvault_secrets_provider_addon_profile.config[ CONST_ROTATION_POLL_INTERVAL ] = self.context.get_rotation_poll_interval() + return azure_keyvault_secrets_provider_addon_profile def update_addon_profiles(self, mc: ManagedCluster) -> ManagedCluster: """Update addon profiles for the ManagedCluster object. @@ -5539,8 +5586,17 @@ def update_addon_profiles(self, mc: ManagedCluster) -> ManagedCluster: CONST_AZURE_KEYVAULT_SECRETS_PROVIDER_ADDON_NAME ) - # update azure keyvault secrets provider profile in-place - self.update_azure_keyvault_secrets_provider_addon_profile(azure_keyvault_secrets_provider_addon_profile) + # update azure keyvault secrets provider profile + azure_keyvault_secrets_provider_addon_profile = ( + self.update_azure_keyvault_secrets_provider_addon_profile( + azure_keyvault_secrets_provider_addon_profile + ) + ) + if azure_keyvault_secrets_provider_addon_profile: + # mc.addon_profiles should not be None if azure_keyvault_secrets_provider_addon_profile is not None + mc.addon_profiles[ + CONST_AZURE_KEYVAULT_SECRETS_PROVIDER_ADDON_NAME + ] = azure_keyvault_secrets_provider_addon_profile return mc def update_defender(self, mc: ManagedCluster) -> ManagedCluster: 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 e324501627d..52c87fdaad5 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 @@ -7338,6 +7338,47 @@ def test_update_identity(self): ) self.assertEqual(mc_5, ground_truth_mc_5) + def test_ensure_azure_keyvault_secrets_provider_addon_profile(self): + # custom + dec_1 = AKSManagedClusterUpdateDecorator( + self.cmd, + self.client, + {}, + ResourceType.MGMT_CONTAINERSERVICE, + ) + # fail on addon azure-keyvault-secrets-provider not provided + with self.assertRaises(InvalidArgumentValueError): + dec_1.ensure_azure_keyvault_secrets_provider_addon_profile(None) + + # custom + dec_2 = AKSManagedClusterUpdateDecorator( + self.cmd, + self.client, + {}, + ResourceType.MGMT_CONTAINERSERVICE, + ) + azure_keyvault_secrets_provider_addon_profile_2 = ( + self.models.ManagedClusterAddonProfile(enabled=True) + ) + dec_azure_keyvault_secrets_provider_addon_profile_2 = ( + dec_2.ensure_azure_keyvault_secrets_provider_addon_profile( + azure_keyvault_secrets_provider_addon_profile_2 + ) + ) + ground_truth_azure_keyvault_secrets_provider_addon_profile_2 = ( + self.models.ManagedClusterAddonProfile( + enabled=True, + config={ + CONST_SECRET_ROTATION_ENABLED: "false", + CONST_ROTATION_POLL_INTERVAL: "2m", + }, + ) + ) + self.assertEqual( + dec_azure_keyvault_secrets_provider_addon_profile_2, + ground_truth_azure_keyvault_secrets_provider_addon_profile_2, + ) + def test_update_azure_keyvault_secrets_provider_addon_profile(self): # default dec_1 = AKSManagedClusterUpdateDecorator( @@ -7430,6 +7471,71 @@ def test_update_azure_keyvault_secrets_provider_addon_profile(self): ground_truth_azure_keyvault_secrets_provider_addon_profile_3, ) + # custom value + dec_4 = AKSManagedClusterUpdateDecorator( + self.cmd, + self.client, + { + "enable_secret_rotation": True, + "disable_secret_rotation": False, + "rotation_poll_interval": None, + }, + ResourceType.MGMT_CONTAINERSERVICE, + ) + + azure_keyvault_secrets_provider_addon_profile_4 = self.models.ManagedClusterAddonProfile(enabled=False) + mc_4 = self.models.ManagedCluster( + location="test_location", + addon_profiles={ + CONST_AZURE_KEYVAULT_SECRETS_PROVIDER_ADDON_NAME: azure_keyvault_secrets_provider_addon_profile_4 + }, + ) + dec_4.context.attach_mc(mc_4) + # fail on addon azure-keyvault-secrets-provider not enabled + with self.assertRaises(InvalidArgumentValueError): + dec_4.update_azure_keyvault_secrets_provider_addon_profile(azure_keyvault_secrets_provider_addon_profile_4) + + # backfill nil config to default then update + dec_5 = AKSManagedClusterUpdateDecorator( + self.cmd, + self.client, + { + "enable_secret_rotation": True, + "disable_secret_rotation": False, + "rotation_poll_interval": None, + }, + ResourceType.MGMT_CONTAINERSERVICE, + ) + + azure_keyvault_secrets_provider_addon_profile_5 = ( + self.models.ManagedClusterAddonProfile(enabled=True) + ) + mc_5 = self.models.ManagedCluster( + location="test_location", + addon_profiles={ + CONST_AZURE_KEYVAULT_SECRETS_PROVIDER_ADDON_NAME: azure_keyvault_secrets_provider_addon_profile_5 + }, + ) + dec_5.context.attach_mc(mc_5) + dec_azure_keyvault_secrets_provider_addon_profile_5 = ( + dec_5.update_azure_keyvault_secrets_provider_addon_profile( + azure_keyvault_secrets_provider_addon_profile_5 + ) + ) + ground_truth_azure_keyvault_secrets_provider_addon_profile_5 = ( + self.models.ManagedClusterAddonProfile( + enabled=True, + config={ + CONST_SECRET_ROTATION_ENABLED: "true", + CONST_ROTATION_POLL_INTERVAL: "2m", + }, + ) + ) + self.assertEqual( + dec_azure_keyvault_secrets_provider_addon_profile_5, + ground_truth_azure_keyvault_secrets_provider_addon_profile_5, + ) + def test_update_addon_profiles(self): # default value in `aks_update` dec_1 = AKSManagedClusterUpdateDecorator( @@ -7496,6 +7602,65 @@ def test_update_addon_profiles(self): self.assertEqual(dec_1.context.get_intermediate("ingress_appgw_addon_enabled"), True) self.assertEqual(dec_1.context.get_intermediate("virtual_node_addon_enabled"), True) + # update addon azure_keyvault_secrets_provider with partial profile + dec_2 = AKSManagedClusterUpdateDecorator( + self.cmd, + self.client, + { + "enable_secret_rotation": True, + "disable_secret_rotation": False, + "rotation_poll_interval": "5m", + }, + ResourceType.MGMT_CONTAINERSERVICE, + ) + azure_keyvault_secrets_provider_addon_profile_2 = ( + self.models.ManagedClusterAddonProfile(enabled=True) + ) + mc_2 = self.models.ManagedCluster( + location="test_location", + addon_profiles={ + CONST_AZURE_KEYVAULT_SECRETS_PROVIDER_ADDON_NAME: azure_keyvault_secrets_provider_addon_profile_2 + }, + ) + dec_2.context.attach_mc(mc_2) + dec_mc_2 = dec_2.update_addon_profiles(mc_2) + + ground_truth_azure_keyvault_secrets_provider_addon_profile_2 = ( + self.models.ManagedClusterAddonProfile( + enabled=True, + config={ + CONST_SECRET_ROTATION_ENABLED: "true", + CONST_ROTATION_POLL_INTERVAL: "5m", + }, + ) + ) + ground_truth_mc_2 = self.models.ManagedCluster( + location="test_location", + addon_profiles={ + CONST_AZURE_KEYVAULT_SECRETS_PROVIDER_ADDON_NAME: ground_truth_azure_keyvault_secrets_provider_addon_profile_2, + }, + ) + self.assertEqual(dec_mc_2, ground_truth_mc_2) + + # update addon azure_keyvault_secrets_provider with no profile + dec_3 = AKSManagedClusterUpdateDecorator( + self.cmd, + self.client, + { + "enable_secret_rotation": True, + "disable_secret_rotation": False, + "rotation_poll_interval": None, + }, + ResourceType.MGMT_CONTAINERSERVICE, + ) + mc_3 = self.models.ManagedCluster( + location="test_location", + ) + dec_3.context.attach_mc(mc_3) + # fail on addon azure-keyvault-secrets-provider not enabled + with self.assertRaises(InvalidArgumentValueError): + dec_3.update_addon_profiles(mc_3) + def test_update_defender(self): dec_1 = AKSManagedClusterUpdateDecorator( self.cmd, diff --git a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_validators.py b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_validators.py index 35844dba1bb..31a4b8f3032 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_validators.py +++ b/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_validators.py @@ -3,9 +3,13 @@ # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- import unittest +from types import SimpleNamespace from azure.cli.command_modules.acs import _validators as validators -from azure.cli.core.azclierror import InvalidArgumentValueError +from azure.cli.core.azclierror import ( + InvalidArgumentValueError, + MutuallyExclusiveArgumentError, +) from azure.cli.core.util import CLIError @@ -432,5 +436,44 @@ def test_invalid_kubernetes_version(self): self.assertEqual(str(cm.exception), err) +class TestKeyVaultSecretsProviderAddon(unittest.TestCase): + def test_invalid_keyvault_secret_provider_parameters(self): + namespace = SimpleNamespace( + **{ + "disable_secret_rotation": True, + "enable_secret_rotation": True, + } + ) + with self.assertRaises(MutuallyExclusiveArgumentError): + validators.validate_keyvault_secrets_provider_disable_and_enable_parameters( + namespace + ) + + def test_valid_keyvault_secret_provider_parameters(self): + namespace_1 = SimpleNamespace( + **{ + "disable_secret_rotation": True, + "enable_secret_rotation": False, + } + ) + validators.validate_keyvault_secrets_provider_disable_and_enable_parameters(namespace_1) + + namespace_2 = SimpleNamespace( + **{ + "disable_secret_rotation": False, + "enable_secret_rotation": True, + } + ) + validators.validate_keyvault_secrets_provider_disable_and_enable_parameters(namespace_2) + + namespace_3 = SimpleNamespace( + **{ + "disable_secret_rotation": False, + "enable_secret_rotation": False, + } + ) + validators.validate_keyvault_secrets_provider_disable_and_enable_parameters(namespace_3) + + if __name__ == "__main__": unittest.main() From f894bee71efaf8248c18fb7beb661d72aa81a732 Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Fri, 1 Jul 2022 16:57:06 +0800 Subject: [PATCH 2/3] update test --- .../latest/test_managed_cluster_decorator.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 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 52c87fdaad5..534bd4400ba 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 @@ -69,7 +69,6 @@ from azure.cli.core.profiles import ResourceType from azure.core.exceptions import HttpResponseError from knack.prompting import NoTTYException -from knack.util import CLIError class AKSManagedClusterModelsTestCase(unittest.TestCase): @@ -1698,6 +1697,21 @@ def test_get_network_plugin(self): with self.assertRaises(RequiredArgumentMissingError): ctx_2.get_network_plugin() + # custom + ctx_3 = AKSManagedClusterContext( + self.cmd, + AKSManagedClusterParamDict( + { + "pod_cidr": "test_pod_cidr", + "network_plugin": "azure" + } + ), + self.models, + DecoratorMode.CREATE, + ) + # overwrite warning + self.assertEqual(ctx_3.get_network_plugin(), "azure") + def test_get_pod_cidr_and_service_cidr_and_dns_service_ip_and_docker_bridge_address_and_network_policy( self, ): From 58d04b5240bb6580fdf61830ad48b4e9afa1ef17 Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Fri, 1 Jul 2022 17:59:23 +0800 Subject: [PATCH 3/3] fix lint --- src/azure-cli/azure/cli/command_modules/acs/_validators.py | 4 +++- .../cli/command_modules/acs/managed_cluster_decorator.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) 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 dc5246907ad..126cdd0433b 100644 --- a/src/azure-cli/azure/cli/command_modules/acs/_validators.py +++ b/src/azure-cli/azure/cli/command_modules/acs/_validators.py @@ -517,7 +517,9 @@ def validate_credential_format(namespace): def validate_keyvault_secrets_provider_disable_and_enable_parameters(namespace): if namespace.disable_secret_rotation and namespace.enable_secret_rotation: - raise MutuallyExclusiveArgumentError('Providing both --disable-secret-rotation and --enable-secret-rotation flags is invalid') + raise MutuallyExclusiveArgumentError( + "Providing both --disable-secret-rotation and --enable-secret-rotation flags is invalid" + ) def validate_defender_config_parameter(namespace): 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 768934eae09..994f420fef0 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 @@ -5479,7 +5479,7 @@ def ensure_azure_keyvault_secrets_provider_addon_profile( "Addon azure-keyvault-secrets-provider is not enabled. " "Please use command 'az aks enable-addons' to enable it." ) - elif azure_keyvault_secrets_provider_addon_profile.config is None: + if azure_keyvault_secrets_provider_addon_profile.config is None: # backfill to default azure_keyvault_secrets_provider_addon_profile.config = { CONST_SECRET_ROTATION_ENABLED: "false",