From 9375d8222b0869c5b0e27d6a434caa8ebffafc58 Mon Sep 17 00:00:00 2001 From: Sanjana Lawande Date: Wed, 2 Oct 2024 12:17:11 -0700 Subject: [PATCH] CLI Phase 1 - add UpgradeableTo field to update functionality (#3844) * CLI Phase 1 - add UpgradeableTo field to update functionality * fix upgradeableTo parameter * apply suggestions from code review * add unit tests for upgradeableTo * fix unit tests * Disallow refresh_cluster_credentials together with upgradeable_to * apply code review suggestions * fix python lint issue --- python/az/aro/azext_aro/_params.py | 4 ++ python/az/aro/azext_aro/_validators.py | 14 +++- python/az/aro/azext_aro/custom.py | 29 ++++---- .../tests/latest/unit/test_validators.py | 67 +++++++++++++++++-- 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/python/az/aro/azext_aro/_params.py b/python/az/aro/azext_aro/_params.py index c532cf13abb..30652368e04 100644 --- a/python/az/aro/azext_aro/_params.py +++ b/python/az/aro/azext_aro/_params.py @@ -23,6 +23,7 @@ validate_enable_managed_identity, validate_platform_workload_identities, validate_cluster_identity, + validate_upgradeable_to_format, ) from azure.cli.core.commands.parameters import ( name_type, @@ -160,6 +161,9 @@ def load_arguments(self, _): options_list=['--assign-platform-workload-identity', '--assign-platform-wi'], validator=validate_platform_workload_identities(isCreate=False), action=AROPlatformWorkloadIdentityAddAction, nargs='+') + c.argument('upgradeable_to', arg_group='Identity', options_list=['--upgradeable-to'], + help='OpenShift version to upgrade to.', is_preview=True, + validator=validate_upgradeable_to_format) with self.argument_context('aro get-admin-kubeconfig') as c: c.argument('file', diff --git a/python/az/aro/azext_aro/_validators.py b/python/az/aro/azext_aro/_validators.py index 9fdee64cf32..96b8941323a 100644 --- a/python/az/aro/azext_aro/_validators.py +++ b/python/az/aro/azext_aro/_validators.py @@ -45,7 +45,6 @@ def validate_client_id(namespace): raise MutuallyExclusiveArgumentError('Must not specify --client-id when --enable-managed-identity is True') # pylint: disable=line-too-long if namespace.platform_workload_identities is not None: raise MutuallyExclusiveArgumentError('Must not specify --client-id when --assign-platform-workload-identity is used') # pylint: disable=line-too-long - try: uuid.UUID(namespace.client_id) except ValueError as e: @@ -53,6 +52,8 @@ def validate_client_id(namespace): if namespace.client_secret is None or not str(namespace.client_secret): raise RequiredArgumentMissingError('Must specify --client-secret with --client-id.') # pylint: disable=line-too-long + if namespace.upgradeable_to is not None: + raise MutuallyExclusiveArgumentError('Must not specify --client-id when --upgradeable-to is used.') # pylint: disable=line-too-long def validate_client_secret(isCreate): @@ -65,6 +66,8 @@ def _validate_client_secret(namespace): raise MutuallyExclusiveArgumentError('Must not specify --client-secret when --assign-platform-workload-identity is used') # pylint: disable=line-too-long if isCreate and (namespace.client_id is None or not str(namespace.client_id)): raise RequiredArgumentMissingError('Must specify --client-id with --client-secret.') + if namespace.upgradeable_to is not None: + raise MutuallyExclusiveArgumentError('Must not specify --client-secret when --upgradeable-to is used.') # pylint: disable=line-too-long return _validate_client_secret @@ -281,6 +284,8 @@ def validate_refresh_cluster_credentials(namespace): return if namespace.client_secret is not None or namespace.client_id is not None: raise RequiredArgumentMissingError('--client-id and --client-secret must be not set with --refresh-credentials.') # pylint: disable=line-too-long + if namespace.upgradeable_to is not None: + raise MutuallyExclusiveArgumentError('Must not specify --refresh-credentials when --upgradeable-to is used.') # pylint: disable=line-too-long def validate_version_format(namespace): @@ -288,6 +293,13 @@ def validate_version_format(namespace): raise InvalidArgumentValueError('--version is invalid') +def validate_upgradeable_to_format(namespace): + if not namespace.upgradeable_to: + return + if not re.match(r'^[4-9]{1}\.(1[4-9]|[1-9][0-9])\.[0-9]{1,2}$', namespace.upgradeable_to): + raise InvalidArgumentValueError('--upgradeable-to format is invalid') + + def validate_load_balancer_managed_outbound_ip_count(namespace): if namespace.load_balancer_managed_outbound_ip_count is None: return diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index 056661f596a..c8fe7f24531 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -455,6 +455,7 @@ def aro_update(cmd, client_secret=None, platform_workload_identities=None, load_balancer_managed_outbound_ip_count=None, + upgradeable_to=None, no_wait=False): # if we can't read cluster spec, we will not be able to do much. Fail. oc = client.open_shift_clusters.get(resource_group_name, resource_name) @@ -479,20 +480,24 @@ def aro_update(cmd, if client_id is not None: oc_update.service_principal_profile.client_id = client_id - if platform_workload_identities is not None: - pwis = {} - for i in oc.platform_workload_identity_profile.platform_workload_identities: - pwis[i.operator_name] = openshiftcluster.PlatformWorkloadIdentity( - operator_name=i.operator_name, - resource_id=i.resource_id - ) + if oc.platform_workload_identity_profile is not None: + if platform_workload_identities is not None or upgradeable_to is not None: + oc_update.platform_workload_identity_profile = openshiftcluster.PlatformWorkloadIdentityProfile() - for i in platform_workload_identities: - pwis[i.operator_name] = i + if platform_workload_identities is not None: + pwis = {} + for i in oc.platform_workload_identity_profile.platform_workload_identities: + pwis[i.operator_name] = openshiftcluster.PlatformWorkloadIdentity( + operator_name=i.operator_name, + resource_id=i.resource_id + ) - oc_update.platform_workload_identity_profile = openshiftcluster.PlatformWorkloadIdentityProfile( - platform_workload_identities=list(pwis.values()) - ) + for i in platform_workload_identities: + pwis[i.operator_name] = i + + oc_update.platform_workload_identity_profile.platform_workload_identities = list(pwis.values()) + + oc_update.platform_workload_identity_profile.upgradeable_to = upgradeable_to if load_balancer_managed_outbound_ip_count is not None: oc_update.network_profile = openshiftcluster.NetworkProfile() diff --git a/python/az/aro/azext_aro/tests/latest/unit/test_validators.py b/python/az/aro/azext_aro/tests/latest/unit/test_validators.py index 4fa9018be51..0df05cf9c94 100644 --- a/python/az/aro/azext_aro/tests/latest/unit/test_validators.py +++ b/python/az/aro/azext_aro/tests/latest/unit/test_validators.py @@ -21,7 +21,8 @@ validate_load_balancer_managed_outbound_ip_count, validate_enable_managed_identity, validate_platform_workload_identities, - validate_cluster_identity + validate_cluster_identity, + validate_upgradeable_to_format ) from azure.cli.core.azclierror import ( InvalidArgumentValueError, RequiredArgumentMissingError, @@ -112,7 +113,7 @@ def test_validate_cidr(test_description, dummyclass, attribute_to_get_from_objec ), ( "should not raise any exception when namespace.client_id is a valid input for creating a UUID and namespace.client_secret has a valid str representation", - Mock(client_id="12345678123456781234567812345678", platform_workload_identities=None, client_secret="12345"), + Mock(upgradeable_to=None, client_id="12345678123456781234567812345678", platform_workload_identities=None, client_secret="12345"), None ) ] @@ -171,15 +172,27 @@ def test_validate_client_id(test_description, namespace, expected_exception): ( "should not raise any exception when isCreate is true and all arguments valid", True, - Mock(client_id="12345678123456781234567812345678", client_secret="123", platform_workload_identities=None), + Mock(upgradeable_to=None, client_id="12345678123456781234567812345678", client_secret="123", platform_workload_identities=None), None ), ( "should not raise any exception when isCreate is false and all arguments valid", False, - Mock(client_secret="123", platform_workload_identities=None), + Mock(upgradeable_to=None, client_secret="123", platform_workload_identities=None), None ), + ( + "should raise MutuallyExclusiveArgumentError exception when isCreate is true and upgradeable_to, client_id and client_secret are present", + True, + Mock(upgradeable_to="4.14.2", client_id="12345678123456781234567812345678", client_secret="123", platform_workload_identities=None), + MutuallyExclusiveArgumentError + ), + ( + "should raise MutuallyExclusiveArgumentError exception when isCreate is false and upgradeable_to, client_id and client_secret are present", + False, + Mock(upgradeable_to="4.14.2", client_id="12345678123456781234567812345678", client_secret="123", platform_workload_identities=None), + MutuallyExclusiveArgumentError + ), ] @@ -804,9 +817,15 @@ def test_validate_worker_vm_disk_size_gb(test_description, namespace, expected_e ), ( "should not raise any Exception because namespace.client_secret is None and namespace.client_id is None", - Mock(client_secret=None, client_id=None), + Mock(upgradeable_to=None, client_secret=None, client_id=None), None - ) + ), + ( + "should raise MutuallyExclusiveArgumentError exception because namespace.upgradeable_to is not None", + Mock(upgradeable_to="4.14.2", client_id=None, client_secret=None), + MutuallyExclusiveArgumentError + ), + ] @@ -1276,3 +1295,39 @@ def test_validate_cluster_identity(test_description, namespace, expected_excepti if expected_identity is not None: assert (expected_identity == namespace.mi_user_assigned) + + +test_validate_upgradeable_to_data = [ + ( + "should not raise any Exception because namespace.upgradeable_to is empty", + Mock(upgradeable_to="", client_id=None, client_secret=None), + None, None + ), + + ( + "should raise InvalidArgumentValueError Exception because upgradeable_to format is invalid", + Mock(upgradeable_to="a", client_id=None, client_secret=None), + InvalidArgumentValueError, "--upgradeable-to is invalid" + ), + + ( + "Should raise InvalidArgumentValueError when --upgradeable-to < 4.14.0", + Mock(upgradeable_to="4.0.4", + client_id=None, client_secret=None), + InvalidArgumentValueError, 'Enabling managed identity requires --upgradeable-to >= 4.14.0' + ), + +] + + +@pytest.mark.parametrize( + "test_description, namespace, expected_exception, expected_exception_message", + test_validate_upgradeable_to_data, + ids=[i[0] for i in test_validate_upgradeable_to_data] +) +def test_validate_upgradeable_to_data(test_description, namespace, expected_exception, expected_exception_message): + if expected_exception is None: + validate_upgradeable_to_format(namespace) + else: + with pytest.raises(expected_exception): + validate_upgradeable_to_format(namespace)