[ARO] CLI enhancements, including route table checking permissions (Azure#14535)

* [ARO] CLI enhancements, including route table checking permissions

* Adding the new recorded tests.

Author: Kenny Woodson

src/azure-cli/azure/cli/command_modules/aro/_aad.py

@@ -4,14 +4,19 @@
 # --------------------------------------------------------------------------------------------
 
 import datetime
+import time
 import uuid
 
 from azure.cli.core._profile import Profile
 from azure.cli.core.commands.client_factory import configure_common_settings
 from azure.graphrbac import GraphRbacManagementClient
 from azure.graphrbac.models import ApplicationCreateParameters
+from azure.graphrbac.models import GraphErrorException
 from azure.graphrbac.models import PasswordCredential
 from azure.graphrbac.models import ServicePrincipalCreateParameters
+from knack.log import get_logger
+
+logger = get_logger(__name__)
 
 
 class AADManager:
@@ -56,6 +61,15 @@ def get_service_principal(self, app_id):
         return None
 
     def create_service_principal(self, app_id):
-        return self.client.service_principals.create(ServicePrincipalCreateParameters(
-            app_id=app_id,
-        ))
+        max_retries = 3
+        retries = 0
+        while True:
+            try:
+                return self.client.service_principals.create(
+                    ServicePrincipalCreateParameters(app_id=app_id))
+            except GraphErrorException as ex:
+                if retries >= max_retries:
+                    raise
+                retries += 1
+                logger.warning("%s; retry %d of %d", ex, retries, max_retries)
+                time.sleep(10)

src/azure-cli/azure/cli/command_modules/aro/_help.py

@@ -7,66 +7,66 @@
 
 
 helps['aro'] = """
-    type: group
-    short-summary: Manage Azure Red Hat OpenShift clusters.
+  type: group
+  short-summary: Manage Azure Red Hat OpenShift clusters.
 """
 
 helps['aro create'] = """
-    type: command
-    short-summary: Create a cluster.
-    examples:
-      - name: Create an OpenShift cluster
-        text: az aro create --resource-group MyResourceGroup --name MyCluster  --vnet MyVnet --master-subnet MyMasterSubnet --worker-subnet MyWorkerSubnet
-      - name: Create an OpenShift cluster with 5 compute nodes and Red Hat Pull Secret
-        text: az aro create --resource-group MyResourceGroup --name MyCluster --vnet MyVnet --master-subnet MyMasterSubnet --worker-subnet MyWorkerSubnet --worker-count 5 --pull-secret @pullsecret.txt
-      - name: Create a Private OpenShift cluster
-        text: az aro create --resource-group MyResourceGroup --name MyCluster --vnet MyVnet --master-subnet MyMasterSubnet --worker-subnet MyWorkerSubnet --apiserver-visibility Private --ingress-visibility Private
+  type: command
+  short-summary: Create a cluster.
+  examples:
+  - name: Create a cluster.
+    text: az aro create --resource-group MyResourceGroup --name MyCluster --vnet MyVnet --master-subnet MyMasterSubnet --worker-subnet MyWorkerSubnet
+  - name: Create a cluster with 5 compute nodes and Red Hat pull secret.
+    text: az aro create --resource-group MyResourceGroup --name MyCluster --vnet MyVnet --master-subnet MyMasterSubnet --worker-subnet MyWorkerSubnet --worker-count 5 --pull-secret @pullsecret.txt
+  - name: Create a Private cluster
+    text: az aro create --resource-group MyResourceGroup --name MyCluster --vnet MyVnet --master-subnet MyMasterSubnet --worker-subnet MyWorkerSubnet --apiserver-visibility Private --ingress-visibility Private
 """
 
 helps['aro list'] = """
-    type: command
-    short-summary: List clusters.
-    examples:
-      - name: List OpenShift clusters
-        text: az aro list
-      - name: List OpenShift clusters with table view
-        text: az aro list -o table
+  type: command
+  short-summary: List clusters.
+  examples:
+  - name: List clusters.
+    text: az aro list
+  - name: List clusters with table view.
+    text: az aro list -o table
 """
 
 helps['aro delete'] = """
-    type: command
-    short-summary: Delete a cluster.
-    examples:
-      - name: Delete a OpenShift cluster
-        text: az aro delete --name MyCluster --resource-group MyResourceGroup
+  type: command
+  short-summary: Delete a cluster.
+  examples:
+  - name: Delete a cluster.
+    text: az aro delete --name MyCluster --resource-group MyResourceGroup
 """
 
 helps['aro show'] = """
-    type: command
-    short-summary: Get the details of a cluster.
-    examples:
-      - name: Get the details of a Managed OpenShift cluster
-        text: az aro show --name MyCluster --resource-group MyResourceGroup
+  type: command
+  short-summary: Get the details of a cluster.
+  examples:
+  - name: Get the details of a cluster.
+    text: az aro show --name MyCluster --resource-group MyResourceGroup
 """
 
 helps['aro update'] = """
-    type: command
-    short-summary: Update a cluster.
-    examples:
-      - name: Update an existing cluster
-        text: az aro update --name MyCluster --resource-group MyResourceGroup
+  type: command
+  short-summary: Update a cluster.
+  examples:
+  - name: Update a cluster.
+    text: az aro update --name MyCluster --resource-group MyResourceGroup
 """
 
 helps['aro list-credentials'] = """
-    type: command
-    short-summary: List credentials of a cluster.
-    examples:
-      - name: List credentials of a cluster
-        text: az aro list-credentials --name MyCluster --resource-group MyResourceGroup
+  type: command
+  short-summary: List credentials of a cluster.
+  examples:
+  - name: List credentials of a cluster.
+    text: az aro list-credentials --name MyCluster --resource-group MyResourceGroup
 """
 
 helps['aro wait'] = """
-    type: command
-    short-summary: Wait for a cluster to reach a desired state.
-    long-summary: If an operation on a cluster was interrupted or was started with `--no-wait`, use this command to wait for it to complete.
+  type: command
+  short-summary: Wait for a cluster to reach a desired state.
+  long-summary: If an operation on a cluster was interrupted or was started with `--no-wait`, use this command to wait for it to complete.
 """

src/azure-cli/azure/cli/command_modules/aro/_params.py

@@ -18,15 +18,13 @@
 from azure.cli.core.commands.parameters import name_type
 from azure.cli.core.commands.parameters import get_enum_type
 from azure.cli.core.commands.parameters import resource_group_name_type
-from azure.cli.core.commands.parameters import get_location_type
 from azure.cli.core.commands.parameters import tags_type
 from azure.cli.core.commands.validators import get_default_location_from_resource_group
 
 
 def load_arguments(self, _):
     with self.argument_context('aro') as c:
         c.argument('location',
-                   get_location_type(self.cli_ctx),
                    validator=get_default_location_from_resource_group)
         c.argument('resource_name',
                    name_type,
@@ -64,9 +62,11 @@ def load_arguments(self, _):
         c.argument('worker_vm_size',
                    help='Size of worker VMs.')
         c.argument('worker_vm_disk_size_gb',
+                   type=int,
                    help='Disk size in GB of worker VMs.',
                    validator=validate_worker_vm_disk_size_gb)
         c.argument('worker_count',
+                   type=int,
                    help='Count of worker VMs.',
                    validator=validate_worker_count)
 

src/azure-cli/azure/cli/command_modules/aro/_rbac.py

@@ -10,6 +10,7 @@
 from azure.cli.core.profiles import get_sdk
 from azure.cli.core.profiles import ResourceType
 from msrestazure.tools import resource_id
+from msrestazure.tools import parse_resource_id
 
 
 CONTRIBUTOR = 'b24988ac-6180-42a0-ab88-20f7382dd24c'
@@ -20,7 +21,7 @@ def _gen_uuid():
 
 
 def assign_contributor_to_vnet(cli_ctx, vnet, object_id):
-    client = get_mgmt_service_client(cli_ctx, ResourceType.MGMT_AUTHORIZATION)
+    auth_client = get_mgmt_service_client(cli_ctx, ResourceType.MGMT_AUTHORIZATION)
 
     RoleAssignmentCreateParameters = get_sdk(cli_ctx, ResourceType.MGMT_AUTHORIZATION,
                                              'RoleAssignmentCreateParameters', mod='models',
@@ -33,15 +34,63 @@ def assign_contributor_to_vnet(cli_ctx, vnet, object_id):
         name=CONTRIBUTOR,
     )
 
-    for assignment in list(client.role_assignments.list_for_scope(vnet)):
-        if assignment.role_definition_id.lower() == role_definition_id.lower() and \
-                assignment.principal_id.lower() == object_id.lower():
-            return
+    if has_assignment(auth_client.role_assignments.list_for_scope(vnet), role_definition_id, object_id):
+        return
 
+    # generate random uuid for role assignment
     role_uuid = _gen_uuid()
 
-    client.role_assignments.create(vnet, role_uuid, RoleAssignmentCreateParameters(
+    auth_client.role_assignments.create(vnet, role_uuid, RoleAssignmentCreateParameters(
         role_definition_id=role_definition_id,
         principal_id=object_id,
         principal_type='ServicePrincipal',
     ))
+
+
+def assign_contributor_to_routetable(cli_ctx, master_subnet, worker_subnet, object_id):
+    auth_client = get_mgmt_service_client(cli_ctx, ResourceType.MGMT_AUTHORIZATION)
+    network_client = get_mgmt_service_client(cli_ctx, ResourceType.MGMT_NETWORK)
+
+    RoleAssignmentCreateParameters = get_sdk(cli_ctx, ResourceType.MGMT_AUTHORIZATION,
+                                             'RoleAssignmentCreateParameters', mod='models',
+                                             operation_group='role_assignments')
+
+    role_definition_id = resource_id(
+        subscription=get_subscription_id(cli_ctx),
+        namespace='Microsoft.Authorization',
+        type='roleDefinitions',
+        name=CONTRIBUTOR,
+    )
+
+    route_tables = set()
+    for sn in [master_subnet, worker_subnet]:
+        sid = parse_resource_id(sn)
+
+        subnet = network_client.subnets.get(resource_group_name=sid['resource_group'],
+                                            virtual_network_name=sid['name'],
+                                            subnet_name=sid['resource_name'])
+
+        if subnet.route_table is not None:
+            route_tables.add(subnet.route_table.id)
+
+    for rt in route_tables:
+        if has_assignment(auth_client.role_assignments.list_for_scope(rt),
+                          role_definition_id, object_id):
+            continue
+
+        role_uuid = _gen_uuid()
+
+        auth_client.role_assignments.create(rt, role_uuid, RoleAssignmentCreateParameters(
+            role_definition_id=role_definition_id,
+            principal_id=object_id,
+            principal_type='ServicePrincipal',
+        ))
+
+
+def has_assignment(assignments, role_definition_id, object_id):
+    for assignment in assignments:
+        if assignment.role_definition_id.lower() == role_definition_id.lower() and \
+                assignment.principal_id.lower() == object_id.lower():
+            return True
+
+    return False

src/azure-cli/azure/cli/command_modules/aro/_validators.py

@@ -72,15 +72,6 @@ def validate_domain(namespace):
                            namespace.domain)
 
 
-def _validate_int(key, i):
-    try:
-        i = int(i)
-    except ValueError:
-        raise CLIError("Invalid --%s '%s'." % (key.replace('_', '-'), i))
-
-    return i
-
-
 def validate_pull_secret(namespace):
     if namespace.pull_secret is None:
         # TODO: add aka.ms link here
@@ -206,19 +197,13 @@ def validate_vnet_resource_group_name(namespace):
 
 def validate_worker_count(namespace):
     if namespace.worker_count:
-        namespace.worker_count = _validate_int(
-            'worker_count', namespace.worker_count)
-
         if namespace.worker_count < 3:
             raise CLIError(
                 '--worker-count must be greater than or equal to 3.')
 
 
 def validate_worker_vm_disk_size_gb(namespace):
     if namespace.worker_vm_disk_size_gb:
-        namespace.worker_vm_disk_size_gb = _validate_int(
-            'worker_vm_disk_size_gb', namespace.worker_vm_disk_size_gb)
-
         if namespace.worker_vm_disk_size_gb < 128:
             raise CLIError(
                 '--worker-vm-disk-size-gb must be greater than or equal to 128.')

src/azure-cli/azure/cli/command_modules/aro/custom.py

@@ -8,10 +8,13 @@
 
 import azure.mgmt.redhatopenshift.models as v2020_04_30
 from azure.cli.command_modules.aro._aad import AADManager
-from azure.cli.command_modules.aro._rbac import assign_contributor_to_vnet
+from azure.cli.command_modules.aro._rbac import assign_contributor_to_vnet, assign_contributor_to_routetable
 from azure.cli.command_modules.aro._validators import validate_subnets
+from azure.cli.core.commands.client_factory import get_mgmt_service_client
 from azure.cli.core.commands.client_factory import get_subscription_id
+from azure.cli.core.profiles import ResourceType
 from azure.cli.core.util import sdk_no_wait
+from knack.util import CLIError
 
 
 FP_CLIENT_ID = 'f1dd0a37-89c6-4e07-bcd1-ffd3d43d8875'
@@ -41,6 +44,13 @@ def aro_create(cmd,  # pylint: disable=too-many-locals
                ingress_visibility=None,
                tags=None,
                no_wait=False):
+    resource_client = get_mgmt_service_client(
+        cmd.cli_ctx, ResourceType.MGMT_RESOURCE_RESOURCES)
+    provider = resource_client.providers.get('Microsoft.RedHatOpenShift')
+    if provider.registration_state != 'Registered':
+        raise CLIError('Microsoft.RedHatOpenShift provider is not registered.  Run `az provider ' +
+                       'register -n Microsoft.RedHatOpenShift --wait`.')
+
     vnet = validate_subnets(master_subnet, worker_subnet)
 
     subscription_id = get_subscription_id(cmd.cli_ctx)
@@ -49,7 +59,7 @@ def aro_create(cmd,  # pylint: disable=too-many-locals
 
     aad = AADManager(cmd.cli_ctx)
     if client_id is None:
-        app, client_secret = aad.create_application('aro-%s' % random_id)
+        app, client_secret = aad.create_application(cluster_resource_group or 'aro-' + random_id)
         client_id = app.app_id
 
     client_sp = aad.get_service_principal(client_id)
@@ -60,8 +70,20 @@ def aro_create(cmd,  # pylint: disable=too-many-locals
 
     rp_client_sp = aad.get_service_principal(rp_client_id)
 
-    assign_contributor_to_vnet(cmd.cli_ctx, vnet, client_sp.object_id)
-    assign_contributor_to_vnet(cmd.cli_ctx, vnet, rp_client_sp.object_id)
+    for sp_id in [client_sp.object_id, rp_client_sp.object_id]:
+        assign_contributor_to_vnet(cmd.cli_ctx, vnet, sp_id)
+        assign_contributor_to_routetable(cmd.cli_ctx, master_subnet, worker_subnet, sp_id)
+
+    if rp_mode_development():
+        worker_vm_size = worker_vm_size or 'Standard_D2s_v3'
+    else:
+        worker_vm_size = worker_vm_size or 'Standard_D4s_v3'
+
+    if apiserver_visibility is not None:
+        apiserver_visibility = apiserver_visibility.capitalize()
+
+    if ingress_visibility is not None:
+        ingress_visibility = ingress_visibility.capitalize()
 
     oc = v2020_04_30.OpenShiftCluster(
         location=location,
@@ -87,7 +109,7 @@ def aro_create(cmd,  # pylint: disable=too-many-locals
         worker_profiles=[
             v2020_04_30.WorkerProfile(
                 name='worker',  # TODO: 'worker' should not be hard-coded
-                vm_size=worker_vm_size or 'Standard_D4s_v3',
+                vm_size=worker_vm_size,
                 disk_size_gb=worker_vm_disk_size_gb or 128,
                 subnet_id=worker_subnet,
                 count=worker_count or 3,
@@ -146,7 +168,7 @@ def rp_mode_development():
 
 
 def generate_random_id():
-    random_id = (''.join(random.choice('abcdefghijklmnopqrstuvwxyz')) +
+    random_id = (random.choice('abcdefghijklmnopqrstuvwxyz') +
                  ''.join(random.choice('abcdefghijklmnopqrstuvwxyz1234567890')
                          for _ in range(7)))
     return random_id