From 118f1a8fbcb1f500a9509afa399241529dc37d69 Mon Sep 17 00:00:00 2001 From: Goutham Muguluvalli Niranjan <31833540+gouthamMN@users.noreply.github.com> Date: Tue, 19 Sep 2023 11:06:09 -0500 Subject: [PATCH] Enable preconfigured NSG flag on azure-cli (#3112) * enable preconfigured NSG flag on azure-cli * fix option length too long UT failure * add network contributor role to BYO-NSGs when preconfigured NSG feature is enabled * remove feature check and print subnets with no NSG * use arg_type=get_three_state_flag() * include subnets from worker_Profiles_Status * add worker_profiles_status to mock class * fix grammers --------- Co-authored-by: gniranjan --- python/az/aro/azext_aro/_client_factory.py | 2 +- .../az/aro/azext_aro/_dynamic_validators.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 | 43 ++++++++++++++++--- .../latest/unit/test_dynamic_validators.py | 21 ++++++++- python/az/aro/linter_exclusions.yml | 3 ++ 7 files changed, 64 insertions(+), 11 deletions(-) diff --git a/python/az/aro/azext_aro/_client_factory.py b/python/az/aro/azext_aro/_client_factory.py index 1dda842fc26..7bd20999e26 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/_dynamic_validators.py b/python/az/aro/azext_aro/_dynamic_validators.py index c9dd364f2fc..3dda7b78404 100644 --- a/python/az/aro/azext_aro/_dynamic_validators.py +++ b/python/az/aro/azext_aro/_dynamic_validators.py @@ -184,7 +184,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/_params.py b/python/az/aro/azext_aro/_params.py index d548130fb27..94e355e9057 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', 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.', 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 6561d799a98..a2ad63e0b7e 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 @@ -49,6 +49,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, @@ -88,6 +89,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, @@ -145,6 +147,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', @@ -190,6 +193,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 @@ -202,7 +206,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 @@ -210,6 +217,7 @@ def __init__(self, disk_encryption_id, master_subnet_id, worker_subnet_id): self.worker_profiles = [openshiftcluster.WorkerProfile( subnet_id=worker_subnet_id )] + self.worker_profiles_status = None aad = AADManager(cmd.cli_ctx) @@ -222,7 +230,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)), @@ -432,8 +440,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() + subnets_with_no_nsg_attached = set() for sn in subnets: sid = parse_resource_id(sn) @@ -455,6 +464,21 @@ 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']) + 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 \ + 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 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 @@ -467,6 +491,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'], @@ -476,11 +505,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 ) ] 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