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

[AKS] az aks update: Fix the issue of NoneType error when updating the config of keyvault secret provider #23088

Merged
merged 3 commits into from
Jul 5, 2022
Merged

[AKS] az aks update: Fix the issue of NoneType error when updating the config of keyvault secret provider #23088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
784e8cb
fix nil config in kvsecret provider addon
FumingZhang Jul 1, 2022
f894bee
update test
FumingZhang Jul 1, 2022
58d04b5
fix lint
FumingZhang Jul 1, 2022
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
3 changes: 2 additions & 1 deletion src/azure-cli/azure/cli/command_modules/acs/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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=[
Expand Down
14 changes: 12 additions & 2 deletions src/azure-cli/azure/cli/command_modules/acs/_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -512,6 +515,13 @@ 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")
Expand Down
70 changes: 63 additions & 7 deletions src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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."
)
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",
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
"""
Expand All @@ -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.
Expand Down Expand Up @@ -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:
Expand Down
181 changes: 180 additions & 1 deletion src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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,
):
Expand Down Expand Up @@ -7338,6 +7352,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(
Expand Down Expand Up @@ -7430,6 +7485,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(
Expand Down Expand Up @@ -7496,6 +7616,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,
Expand Down
Loading