From 28c2bdb9871121e72bd62ba925541e0dc092052a Mon Sep 17 00:00:00 2001 From: gniranjan Date: Mon, 21 Aug 2023 13:37:24 -0500 Subject: [PATCH 1/8] enable preconfigured NSG flag on azure-cli --- python/az/aro/azext_aro/_client_factory.py | 2 +- python/az/aro/azext_aro/_params.py | 2 ++ python/az/aro/azext_aro/commands.py | 2 +- python/az/aro/azext_aro/custom.py | 4 +++- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/python/az/aro/azext_aro/_client_factory.py b/python/az/aro/azext_aro/_client_factory.py index 775fba938d0..488adfe1385 100644 --- a/python/az/aro/azext_aro/_client_factory.py +++ b/python/az/aro/azext_aro/_client_factory.py @@ -4,7 +4,7 @@ import urllib3 from azext_aro.custom import rp_mode_development -from azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2023_04_01 import AzureRedHatOpenShiftClient +from azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2023_09_04 import AzureRedHatOpenShiftClient from azure.cli.core.commands.client_factory import get_mgmt_service_client diff --git a/python/az/aro/azext_aro/_params.py b/python/az/aro/azext_aro/_params.py index fcb56e536a0..fee5832cb16 100644 --- a/python/az/aro/azext_aro/_params.py +++ b/python/az/aro/azext_aro/_params.py @@ -68,6 +68,8 @@ def load_arguments(self, _): c.argument('outbound_type', help='Outbound type of cluster. Must be "Loadbalancer" (default) or "UserDefinedRouting".', validator=validate_outbound_type) + c.argument('enable_preconfigured_nsg', + help='Use Preconfigured NSGs. Allowed values: false, true.') c.argument('disk_encryption_set', help='ResourceID of the DiskEncryptionSet to be used for master and worker VMs.', validator=validate_disk_encryption_set) diff --git a/python/az/aro/azext_aro/commands.py b/python/az/aro/azext_aro/commands.py index e11171dcace..0965220baa1 100644 --- a/python/az/aro/azext_aro/commands.py +++ b/python/az/aro/azext_aro/commands.py @@ -11,7 +11,7 @@ def load_command_table(self, _): aro_sdk = CliCommandType( - operations_tmpl='azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2023_04_01.operations#OpenShiftClustersOperations.{}', # pylint: disable=line-too-long + operations_tmpl='azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2023_09_04.operations#OpenShiftClustersOperations.{}', # pylint: disable=line-too-long client_factory=cf_aro) with self.command_group('aro', aro_sdk, client_factory=cf_aro) as g: diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index 459a5ec12ec..b307d53524d 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -7,7 +7,7 @@ from base64 import b64decode import textwrap -import azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2023_04_01.models as openshiftcluster +import azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2023_09_04.models as openshiftcluster from azure.cli.command_modules.role import GraphError from azure.cli.core.commands.client_factory import get_mgmt_service_client @@ -44,6 +44,7 @@ def aro_create(cmd, # pylint: disable=too-many-locals worker_subnet, vnet=None, # pylint: disable=unused-argument vnet_resource_group_name=None, # pylint: disable=unused-argument + enable_preconfigured_nsg=None, location=None, pull_secret=None, domain=None, @@ -141,6 +142,7 @@ def aro_create(cmd, # pylint: disable=too-many-locals pod_cidr=pod_cidr or '10.128.0.0/14', service_cidr=service_cidr or '172.30.0.0/16', outbound_type=outbound_type or '', + preconfigured_nsg='Enabled' if enable_preconfigured_nsg else 'Disabled', ), master_profile=openshiftcluster.MasterProfile( vm_size=master_vm_size or 'Standard_D8s_v3', From a594ce18afe5f5b4b31a0470750f076f81cb3eef Mon Sep 17 00:00:00 2001 From: gniranjan Date: Mon, 21 Aug 2023 15:29:29 -0500 Subject: [PATCH 2/8] fix option length too long UT failure --- python/az/aro/linter_exclusions.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/az/aro/linter_exclusions.yml b/python/az/aro/linter_exclusions.yml index 22cc6d562e1..0c64d6db6de 100644 --- a/python/az/aro/linter_exclusions.yml +++ b/python/az/aro/linter_exclusions.yml @@ -11,3 +11,6 @@ aro create: vnet_resource_group_name: rule_exclusions: - parameter_should_not_end_in_resource_group + enable_preconfigured_nsg: + rule_exclusions: + - option_length_too_long From 6551d8b0da03cafbc99b43bf63435ceab6941376 Mon Sep 17 00:00:00 2001 From: gniranjan Date: Wed, 23 Aug 2023 18:07:03 -0500 Subject: [PATCH 3/8] add network contributor role to BYO-NSGs when preconfigured NSG feature is enabled --- .../az/aro/azext_aro/_dynamic_validators.py | 2 +- python/az/aro/azext_aro/custom.py | 39 ++++++++++++++++--- .../latest/unit/test_dynamic_validators.py | 21 +++++++++- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/python/az/aro/azext_aro/_dynamic_validators.py b/python/az/aro/azext_aro/_dynamic_validators.py index 67cb57e894b..afb6a24ed9f 100644 --- a/python/az/aro/azext_aro/_dynamic_validators.py +++ b/python/az/aro/azext_aro/_dynamic_validators.py @@ -185,7 +185,7 @@ def _validate_subnet(cmd, namespace): "Microsoft.Network/routeTables/read", "Microsoft.Network/routeTables/write"]) - if subnet_obj.get('networkSecurityGroup', None): + if not namespace.enable_preconfigured_nsg and subnet_obj.get('networkSecurityGroup', None): message = f"A Network Security Group \"{subnet_obj['networkSecurityGroup']['id']}\" "\ "is already assigned to this subnet. Ensure there are no Network "\ "Security Groups assigned to cluster subnets before cluster creation" diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index b307d53524d..c7f4ea51d50 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -74,6 +74,14 @@ def aro_create(cmd, # pylint: disable=too-many-locals if provider.registration_state != 'Registered': raise UnauthorizedError('Microsoft.RedHatOpenShift provider is not registered.', 'Run `az provider register -n Microsoft.RedHatOpenShift --wait`.') + if enable_preconfigured_nsg: + feature_client = get_mgmt_service_client( + cmd.cli_ctx, ResourceType.MGMT_RESOURCE_FEATURES) + feature = feature_client.features.get('Microsoft.RedHatOpenShift', 'PreconfiguredNSG') + if feature.properties.state != 'Registered': + raise ValidationError('PreconfiguredNSG feature is not registered. \ + Run `az feature register --namespace Microsoft.RedHatOpenShift \ + -n PreconfiguredNSG`.') validate_subnets(master_subnet, worker_subnet) @@ -84,6 +92,7 @@ def aro_create(cmd, # pylint: disable=too-many-locals master_subnet, worker_subnet, vnet=vnet, + enable_preconfigured_nsg=enable_preconfigured_nsg, cluster_resource_group=cluster_resource_group, client_id=client_id, client_secret=client_secret, @@ -188,6 +197,7 @@ def validate(cmd, # pylint: disable=too-many-locals,too-many-statements master_subnet, worker_subnet, vnet=None, + enable_preconfigured_nsg=None, cluster_resource_group=None, # pylint: disable=unused-argument client_id=None, client_secret=None, # pylint: disable=unused-argument @@ -200,7 +210,10 @@ def validate(cmd, # pylint: disable=too-many-locals,too-many-statements warnings_as_text=False): class mockoc: # pylint: disable=too-few-public-methods - def __init__(self, disk_encryption_id, master_subnet_id, worker_subnet_id): + def __init__(self, disk_encryption_id, master_subnet_id, worker_subnet_id, preconfigured_nsg): + self.network_profile = openshiftcluster.NetworkProfile( + preconfigured_nsg='Enabled' if preconfigured_nsg else 'Disabled' + ) self.master_profile = openshiftcluster.MasterProfile( subnet_id=master_subnet_id, disk_encryption_set_id=disk_encryption_id @@ -220,7 +233,7 @@ def __init__(self, disk_encryption_id, master_subnet_id, worker_subnet_id): if client_id is not None: sp_obj_ids.append(aad.get_service_principal_id(client_id)) - cluster = mockoc(disk_encryption_set, master_subnet, worker_subnet) + cluster = mockoc(disk_encryption_set, master_subnet, worker_subnet, enable_preconfigured_nsg) try: # Get cluster resources we need to assign permissions on, sort to ensure the same order of operations resources = {ROLE_NETWORK_CONTRIBUTOR: sorted(get_cluster_network_resources(cmd.cli_ctx, cluster, True)), @@ -438,8 +451,9 @@ def generate_random_id(): return random_id -def get_network_resources_from_subnets(cli_ctx, subnets, fail): +def get_network_resources_from_subnets(cli_ctx, subnets, fail, oc): subnet_resources = set() + subnet_with_nsg_preattached = 0 for sn in subnets: sid = parse_resource_id(sn) @@ -460,6 +474,19 @@ def get_network_resources_from_subnets(cli_ctx, subnets, fail): if subnet.get("natGateway", None): subnet_resources.add(subnet['natGateway']['id']) + if oc.network_profile.preconfigured_nsg == 'Enabled': + if subnet.get("networkSecurityGroup", None): + subnet_resources.add(subnet['networkSecurityGroup']['id']) + subnet_with_nsg_preattached = subnet_with_nsg_preattached + 1 + + # when preconfiguredNSG feature is Enabled we either have all subnets NSG attached or none. + if oc.network_profile.preconfigured_nsg == 'Enabled' and \ + subnet_with_nsg_preattached != 0 and \ + subnet_with_nsg_preattached != len(subnets): + raise ValidationError("""(ValidationError) preconfiguredNSG feature is enabled but NSG not attached for subnets. + Please makesure all the subnets have network security group attached and retry. + If issue persists: raise azure support ticket""") + return subnet_resources @@ -481,11 +508,11 @@ def get_cluster_network_resources(cli_ctx, oc, fail): name=master_parts['name'], ) - return get_network_resources(cli_ctx, worker_subnets | {master_subnet}, vnet, fail) + return get_network_resources(cli_ctx, worker_subnets | {master_subnet}, vnet, fail, oc) -def get_network_resources(cli_ctx, subnets, vnet, fail): - subnet_resources = get_network_resources_from_subnets(cli_ctx, subnets, fail) +def get_network_resources(cli_ctx, subnets, vnet, fail, oc): + subnet_resources = get_network_resources_from_subnets(cli_ctx, subnets, fail, oc) resources = set() resources.add(vnet) diff --git a/python/az/aro/azext_aro/tests/latest/unit/test_dynamic_validators.py b/python/az/aro/azext_aro/tests/latest/unit/test_dynamic_validators.py index 3327abb9193..a71b64fbc02 100644 --- a/python/az/aro/azext_aro/tests/latest/unit/test_dynamic_validators.py +++ b/python/az/aro/azext_aro/tests/latest/unit/test_dynamic_validators.py @@ -190,7 +190,7 @@ def test_validate_cidr( ( "should return message when network security group is already attached to subnet", Mock(cli_ctx=Mock(get_progress_controller=Mock(add=Mock(), end=Mock()))), - Mock(vnet='', key="192.168.0.1/32", master_subnet='', worker_subnet='', pod_cidr=None, service_cidr=None), + Mock(vnet='', key="192.168.0.1/32", master_subnet='', worker_subnet='', pod_cidr=None, service_cidr=None, enable_preconfigured_nsg=None), {'networkSecurityGroup': {'id': 'test'}, 'routeTable': {'id': 'test'}}, Mock(**{"permissions.list_for_resource.return_value": [Permission(actions=["Microsoft.Network/routeTables/*"], not_actions=[])]}), { @@ -207,6 +207,25 @@ def test_validate_cidr( "A Network Security Group \"test\" is already assigned to this subnet. " "Ensure there are no Network Security Groups assigned to cluster " "subnets before cluster creation" + ), + ( + "should not return message when Preconfigured NSG is enabled and network security group is attached to subnet", + Mock(cli_ctx=Mock(get_progress_controller=Mock(add=Mock(), end=Mock()))), + Mock(vnet='', key="192.168.0.1/32", master_subnet='', worker_subnet='', pod_cidr=None, service_cidr=None, enable_preconfigured_nsg=True), + {'networkSecurityGroup': {'id': 'test'}, 'routeTable': {'id': 'test'}}, + Mock(**{"permissions.list_for_resource.return_value": [Permission(actions=["Microsoft.Network/routeTables/*"], not_actions=[])]}), + { + "subscription": "subscription", + "namespace": "MICROSOFT.NETWORK", + "type": "virtualnetworks", + "last_child_num": 1, + "child_type_1": "subnets", + "resource_group": None, + "name": None, + "child_name_1": None + }, + Mock(), + None ) ] From 035ae6141644ad4843ce377ed1e8073299351ab4 Mon Sep 17 00:00:00 2001 From: gniranjan Date: Mon, 28 Aug 2023 11:28:34 -0500 Subject: [PATCH 4/8] remove feature check and print subnets with no NSG --- python/az/aro/azext_aro/custom.py | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index c7f4ea51d50..16a45b9e747 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -74,14 +74,6 @@ def aro_create(cmd, # pylint: disable=too-many-locals if provider.registration_state != 'Registered': raise UnauthorizedError('Microsoft.RedHatOpenShift provider is not registered.', 'Run `az provider register -n Microsoft.RedHatOpenShift --wait`.') - if enable_preconfigured_nsg: - feature_client = get_mgmt_service_client( - cmd.cli_ctx, ResourceType.MGMT_RESOURCE_FEATURES) - feature = feature_client.features.get('Microsoft.RedHatOpenShift', 'PreconfiguredNSG') - if feature.properties.state != 'Registered': - raise ValidationError('PreconfiguredNSG feature is not registered. \ - Run `az feature register --namespace Microsoft.RedHatOpenShift \ - -n PreconfiguredNSG`.') validate_subnets(master_subnet, worker_subnet) @@ -453,7 +445,7 @@ def generate_random_id(): def get_network_resources_from_subnets(cli_ctx, subnets, fail, oc): subnet_resources = set() - subnet_with_nsg_preattached = 0 + subnets_with_no_nsg_attached = set() for sn in subnets: sid = parse_resource_id(sn) @@ -477,15 +469,16 @@ def get_network_resources_from_subnets(cli_ctx, subnets, fail, oc): if oc.network_profile.preconfigured_nsg == 'Enabled': if subnet.get("networkSecurityGroup", None): subnet_resources.add(subnet['networkSecurityGroup']['id']) - subnet_with_nsg_preattached = subnet_with_nsg_preattached + 1 + else: + subnets_with_no_nsg_attached.add(sn) # when preconfiguredNSG feature is Enabled we either have all subnets NSG attached or none. if oc.network_profile.preconfigured_nsg == 'Enabled' and \ - subnet_with_nsg_preattached != 0 and \ - subnet_with_nsg_preattached != len(subnets): - raise ValidationError("""(ValidationError) preconfiguredNSG feature is enabled but NSG not attached for subnets. - Please makesure all the subnets have network security group attached and retry. - If issue persists: raise azure support ticket""") + len(subnets_with_no_nsg_attached) != 0 and \ + len(subnets_with_no_nsg_attached) != len(subnets): + raise ValidationError(f"(ValidationError) preconfiguredNSG feature is enabled but NSG not attached for subnets.\ + Please make sure all the following subnets have network security group attached and retry.\ + {subnets_with_no_nsg_attached}") return subnet_resources From 18b74eb5c8f58fde987e80186723326f3e0411b8 Mon Sep 17 00:00:00 2001 From: gniranjan Date: Wed, 30 Aug 2023 15:38:18 -0500 Subject: [PATCH 5/8] use arg_type=get_three_state_flag() --- python/az/aro/azext_aro/_params.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/az/aro/azext_aro/_params.py b/python/az/aro/azext_aro/_params.py index fee5832cb16..4694f6d5845 100644 --- a/python/az/aro/azext_aro/_params.py +++ b/python/az/aro/azext_aro/_params.py @@ -68,7 +68,7 @@ def load_arguments(self, _): c.argument('outbound_type', help='Outbound type of cluster. Must be "Loadbalancer" (default) or "UserDefinedRouting".', validator=validate_outbound_type) - c.argument('enable_preconfigured_nsg', + c.argument('enable_preconfigured_nsg', arg_type=get_three_state_flag(), help='Use Preconfigured NSGs. Allowed values: false, true.') c.argument('disk_encryption_set', help='ResourceID of the DiskEncryptionSet to be used for master and worker VMs.', From 1c53ea0a2a2ec6df7424271a5bfb328260d360c8 Mon Sep 17 00:00:00 2001 From: gniranjan Date: Wed, 6 Sep 2023 10:43:54 -0500 Subject: [PATCH 6/8] include subnets from worker_Profiles_Status --- python/az/aro/azext_aro/custom.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index 99b05662b50..bdd9eeb80ad 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -489,6 +489,11 @@ def get_cluster_network_resources(cli_ctx, oc, fail): if oc.worker_profiles is not None: worker_subnets = {w.subnet_id for w in oc.worker_profiles} + # Ensure that worker_profiles_status exists + # it will not be returned if the cluster resources do not exist + if oc.worker_profiles_status is not None: + worker_subnets |= {w.subnet_id for w in oc.worker_profiles_status} + master_parts = parse_resource_id(master_subnet) vnet = resource_id( subscription=master_parts['subscription'], From 1fa6b41b6c1e0cb5ce53e48de0b4986811417acd Mon Sep 17 00:00:00 2001 From: gniranjan Date: Wed, 6 Sep 2023 12:49:53 -0500 Subject: [PATCH 7/8] add worker_profiles_status to mock class --- python/az/aro/azext_aro/custom.py | 1 + 1 file changed, 1 insertion(+) diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index bdd9eeb80ad..3b2b369b931 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -217,6 +217,7 @@ def __init__(self, disk_encryption_id, master_subnet_id, worker_subnet_id, preco self.worker_profiles = [openshiftcluster.WorkerProfile( subnet_id=worker_subnet_id )] + self.worker_profiles_status = None aad = AADManager(cmd.cli_ctx) From 6bb122a87c8279911497b5d5f954b86a9e8a87bc Mon Sep 17 00:00:00 2001 From: gniranjan Date: Fri, 15 Sep 2023 10:31:29 -0500 Subject: [PATCH 8/8] fix grammers --- python/az/aro/azext_aro/custom.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/az/aro/azext_aro/custom.py b/python/az/aro/azext_aro/custom.py index 3b2b369b931..a2ad63e0b7e 100644 --- a/python/az/aro/azext_aro/custom.py +++ b/python/az/aro/azext_aro/custom.py @@ -474,8 +474,9 @@ def get_network_resources_from_subnets(cli_ctx, subnets, fail, oc): if oc.network_profile.preconfigured_nsg == 'Enabled' and \ len(subnets_with_no_nsg_attached) != 0 and \ len(subnets_with_no_nsg_attached) != len(subnets): - raise ValidationError(f"(ValidationError) preconfiguredNSG feature is enabled but NSG not attached for subnets.\ - Please make sure all the following subnets have network security group attached and retry.\ + raise ValidationError(f"(ValidationError) preconfiguredNSG feature is enabled but an NSG is\ + not attached for all required subnets. Please make sure all the following\ + subnets have a network security groups attached and retry.\ {subnets_with_no_nsg_attached}") return subnet_resources