OSA - Remove FQDN parameters and update ensure_osa_aad flow

src/command_modules/azure-cli-acs/HISTORY.rst

@@ -2,6 +2,9 @@
 
 Release History
 ===============
+2.4.0
+++++++
+* BREAKING CHANGE: Removing `--fqdn` flag on az openshift commands
 
 2.3.22
 ++++++

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_format.py

@@ -45,7 +45,7 @@ def _osa_table_format(result):
         resourceGroup: resourceGroup,
         openShiftVersion: openShiftVersion,
         provisioningState: provisioningState,
-        fqdn: fqdn
+        publicHostname: publicHostname
     }""")
     # use ordered dicts so headers are predictable
     return parsed.search(result, Options(dict_cls=OrderedDict))

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

@@ -641,9 +641,6 @@
   - name: --compute-count -c
     type: int
     short-summary: Number of nodes in the OpenShift node pool.
-  - name: --fqdn
-    type: string
-    short-summary: FQDN for OpenShift API server loadbalancer internal hostname. For example myopenshiftcluster.eastus.cloudapp.azure.com
   - name: --aad-client-app-id
     type: string
     short-summary: The ID of an Azure Active Directory client application. If not specified, a new Azure Active Directory client is created.
@@ -669,13 +666,13 @@
 
 examples:
   - name: Create an OpenShift cluster and auto create an AAD Client
-    text: az openshift create -g MyResourceGroup -n MyManagedCluster --fqdn {FQDN}
+    text: az openshift create -g MyResourceGroup -n MyManagedCluster
   - name: Create an OpenShift cluster and auto create an AAD Client and setup cluster admin group
-    text: az openshift create -g MyResourceGroup -n MyManagedCluster --fqdn {FQDN} --customer-admin-group-id {GROUP_ID}
+    text: az openshift create -g MyResourceGroup -n MyManagedCluster --customer-admin-group-id {GROUP_ID}
   - name: Create an OpenShift cluster with 5 compute nodes and a custom AAD Client.
-    text: az openshift create -g MyResourceGroup -n MyManagedCluster --fqdn {FQDN} --aad-client-app-id {APP_ID} --aad-client-app-secret {APP_SECRET} --aad-tenant-id {TENANT_ID} --compute-count 5
+    text: az openshift create -g MyResourceGroup -n MyManagedCluster --aad-client-app-id {APP_ID} --aad-client-app-secret {APP_SECRET} --aad-tenant-id {TENANT_ID} --compute-count 5
   - name: Create an Openshift cluster using a custom vnet
-    text: az openshift create -g MyResourceGroup -n MyManagedCluster --fqdn {FQDN} --vnet-peer "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/openshift-vnet/providers/Microsoft.Network/virtualNetworks/test"
+    text: az openshift create -g MyResourceGroup -n MyManagedCluster --vnet-peer "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/openshift-vnet/providers/Microsoft.Network/virtualNetworks/test"
 """
 
 helps['openshift delete'] = """

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

@@ -41,7 +41,9 @@
 from azure.cli.core.commands.client_factory import get_mgmt_service_client
 from azure.cli.core.keys import is_valid_ssh_rsa_public_key
 from azure.cli.core.util import in_cloud_console, shell_safe_json_parse, truncate_text, sdk_no_wait
+from azure.cli.core.commands import LongRunningOperation
 from azure.graphrbac.models import (ApplicationCreateParameters,
+                                    ApplicationUpdateParameters,
                                     PasswordCredential,
                                     KeyCredential,
                                     ServicePrincipalCreateParameters,
@@ -1219,6 +1221,8 @@ def update_application(client, object_id, display_name, homepage, identifier_uri
             client.update_key_credentials(object_id, key_creds)
         if password_creds:
             client.update_password_credentials(object_id, password_creds)
+        if reply_urls:
+            client.patch(object_id, ApplicationUpdateParameters(reply_urls=reply_urls))
         return
     except GraphErrorException as ex:
         if 'insufficient privileges' in str(ex).lower():
@@ -2255,13 +2259,14 @@ def _ensure_osa_aad(cli_ctx,
                     aad_client_app_secret=None,
                     aad_tenant_id=None,
                     identifier=None,
-                    name=None, update=False,
+                    name=None, create=False,
                     customer_admin_group_id=None):
     rbac_client = get_graph_rbac_management_client(cli_ctx)
-    if not aad_client_app_id:
-        if not aad_client_app_secret and update:
+    if create:
+        # This reply_url is temporary set since Azure need one to create the AAD.
+        app_id_name = 'https://{}'.format(name)
+        if not aad_client_app_secret:
             aad_client_app_secret = _create_client_secret()
-        reply_url = 'https://{}/oauth2callback/Azure%20AD'.format(identifier)
 
         # Delegate Sign In and Read User Profile permissions on Windows Azure Active Directory API
         resource_access = ResourceAccess(id="311a71cc-e848-46a1-bdf8-97ff7156d8e6",
@@ -2275,36 +2280,33 @@ def _ensure_osa_aad(cli_ctx,
                                                          resource_app_id="00000002-0000-0000-c000-000000000000")
 
         list_aad_filtered = list(rbac_client.applications.list(filter="identifierUris/any(s:s eq '{}')"
-                                                               .format(reply_url)))
-        if update:
-            if list_aad_filtered:
-                update_application(client=rbac_client.applications,
-                                   object_id=list_aad_filtered[0].object_id,
-                                   display_name=identifier,
-                                   identifier_uris=[reply_url],
-                                   reply_urls=[reply_url],
-                                   homepage=reply_url,
-                                   password=aad_client_app_secret,
-                                   required_resource_accesses=[required_osa_aad_access])
-                aad_client_app_id = list_aad_filtered[0].app_id
-                logger.info('Updated AAD: %s', aad_client_app_id)
-            else:
-                result = create_application(client=rbac_client.applications,
-                                            display_name=identifier,
-                                            identifier_uris=[reply_url],
-                                            reply_urls=[reply_url],
-                                            homepage=reply_url,
-                                            password=aad_client_app_secret,
-                                            required_resource_accesses=[required_osa_aad_access])
-                aad_client_app_id = result.app_id
-                logger.info('Created an AAD: %s', aad_client_app_id)
-        else:
+                                                               .format(app_id_name)))
+        if list_aad_filtered:
             aad_client_app_id = list_aad_filtered[0].app_id
-            aad_client_app_secret = 'whatever'
-    # Get the TenantID
-    if aad_tenant_id is None:
-        profile = Profile(cli_ctx=cli_ctx)
-        _, _, aad_tenant_id = profile.get_login_credentials()
+            # Updating reply_url with the correct FQDN information returned by the RP
+            reply_url = 'https://{}/oauth2callback/Azure%20AD'.format(identifier)
+            update_application(client=rbac_client.applications,
+                               object_id=list_aad_filtered[0].object_id,
+                               display_name=name,
+                               identifier_uris=[app_id_name],
+                               reply_urls=[reply_url],
+                               homepage=app_id_name,
+                               password=aad_client_app_secret,
+                               required_resource_accesses=[required_osa_aad_access])
+            logger.info('Updated AAD: %s', aad_client_app_id)
+        else:
+            result = create_application(client=rbac_client.applications,
+                                        display_name=name,
+                                        identifier_uris=[app_id_name],
+                                        homepage=app_id_name,
+                                        password=aad_client_app_secret,
+                                        required_resource_accesses=[required_osa_aad_access])
+            aad_client_app_id = result.app_id
+            logger.info('Created an AAD: %s', aad_client_app_id)
+        # Get the TenantID
+        if aad_tenant_id is None:
+            profile = Profile(cli_ctx=cli_ctx)
+            _, _, aad_tenant_id = profile.get_login_credentials()
     return OpenShiftManagedClusterAADIdentityProvider(
         client_id=aad_client_app_id,
         secret=aad_client_app_secret,
@@ -2435,7 +2437,7 @@ def _remove_osa_nulls(managed_clusters):
     This works around a quirk of the SDK for python behavior. These fields are not sent
     by the server, but get recreated by the CLI's own "to_dict" serialization.
     """
-    attrs = ['tags', 'public_hostname', 'plan', 'type', 'id']
+    attrs = ['tags', 'plan', 'type', 'id']
     ap_master_attrs = ['name', 'os_type']
     net_attrs = ['peer_vnet_id']
     for managed_cluster in managed_clusters:
@@ -2491,7 +2493,6 @@ def osa_list(cmd, client, resource_group_name=None):
 
 
 def openshift_create(cmd, client, resource_group_name, name,  # pylint: disable=too-many-locals
-                     fqdn,
                      location=None,
                      compute_vm_size="Standard_D4s_v3",
                      compute_count=3,
@@ -2538,17 +2539,21 @@ def openshift_create(cmd, client, resource_group_name, name,  # pylint: disable=
     )
     identity_providers = []
 
+    # Validating if aad_client_app_id aad_client_app_secret aad_tenant_id are set
+    create_aad = False
+    if aad_client_app_id is None and aad_client_app_secret is None and aad_tenant_id is None:
+        create_aad = True
+
     # Validating if the cluster is not existing since we are not supporting the AAD rotation on OSA for now
-    update_aad_secret = False
     try:
         client.get(resource_group_name, name)
     except CloudError:
-        update_aad_secret = True
+        create_aad = True
     osa_aad_identity = _ensure_osa_aad(cmd.cli_ctx,
                                        aad_client_app_id=aad_client_app_id,
                                        aad_client_app_secret=aad_client_app_secret,
-                                       aad_tenant_id=aad_tenant_id, identifier=fqdn,
-                                       name=name, update=update_aad_secret,
+                                       aad_tenant_id=aad_tenant_id, identifier=None,
+                                       name=name, create=create_aad,
                                        customer_admin_group_id=customer_admin_group_id)
     identity_providers.append(
         OpenShiftManagedClusterIdentityProvider(
@@ -2576,7 +2581,6 @@ def openshift_create(cmd, client, resource_group_name, name,  # pylint: disable=
     osamc = OpenShiftManagedCluster(
         location=location, tags=tags,
         open_shift_version="v3.11",
-        fqdn=fqdn,
         network_profile=network_profile,
         auth_profile=auth_profile,
         agent_pool_profiles=agent_pool_profiles,
@@ -2585,8 +2589,15 @@ def openshift_create(cmd, client, resource_group_name, name,  # pylint: disable=
 
     try:
         # long_running_operation_timeout=300
-        return sdk_no_wait(no_wait, client.create_or_update,
-                           resource_group_name=resource_group_name, resource_name=name, parameters=osamc)
+        result = sdk_no_wait(no_wait, client.create_or_update,
+                             resource_group_name=resource_group_name, resource_name=name, parameters=osamc)
+        result = LongRunningOperation(cmd.cli_ctx)(result)
+        instance = client.get(resource_group_name, name)
+        _ensure_osa_aad(cmd.cli_ctx,
+                        aad_client_app_id=osa_aad_identity.client_id,
+                        aad_client_app_secret=osa_aad_identity.secret,
+                        aad_tenant_id=osa_aad_identity.tenant_id, identifier=instance.public_hostname,
+                        name=name, create=True)
     except CloudError as ex:
         raise ex
 

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/tests/latest/test_osa_commands.py

@@ -29,7 +29,6 @@ def test_openshift_create_default_service(self, resource_group, resource_group_l
         self.kwargs.update({
             'resource_group': resource_group,
             'name': osa_name,
-            'fqdn': self.generate_random_fqdn(osa_name, resource_group_location),
             'location': resource_group_location,
             'aad_client_app_id': aad_client_app_id,
             'aad_client_app_secret': aad_client_app_secret,
@@ -38,7 +37,7 @@ def test_openshift_create_default_service(self, resource_group, resource_group_l
 
         # create
         create_cmd = 'openshift create --resource-group={resource_group} --name={name} --location={location} ' \
-                     '--fqdn={fqdn} --compute-count=1 ' \
+                     '--compute-count=1 ' \
                      '--aad-client-app-id {aad_client_app_id} --aad-client-app-secret {aad_client_app_secret}'
         self.cmd(create_cmd, checks=[
             self.exists('fqdn'),
@@ -53,7 +52,6 @@ def test_openshift_create_default_service(self, resource_group, resource_group_l
             self.check('agentPoolProfiles[0].count', 1),
             self.check('agentPoolProfiles[0].osType', 'Linux'),
             self.check('agentPoolProfiles[0].vmSize', 'Standard_D4s_v3'),
-            self.check('fqdn', '{fqdn}'),
             self.exists('openShiftVersion')
         ])
 
@@ -80,14 +78,13 @@ def test_openshift_create_service_no_wait(self, resource_group, resource_group_l
         self.kwargs.update({
             'resource_group': resource_group,
             'name': osa_name,
-            'fqdn': self.generate_random_fqdn(osa_name, resource_group_location),
             'location': resource_group_location,
             'aad_client_app_id': aad_client_app_id,
             'aad_client_app_secret': aad_client_app_secret
         })
 
         # create --no-wait
-        create_cmd = 'openshift create -g {resource_group} -n {name} --fqdn {fqdn} ' \
+        create_cmd = 'openshift create -g {resource_group} -n {name} ' \
                      '-l {location} -c 1 --aad-client-app-id {aad_client_app_id} ' \
                      '--aad-client-app-secret {aad_client_app_secret} ' \
                      '--tags scenario_test --no-wait'
@@ -102,7 +99,6 @@ def test_openshift_create_service_no_wait(self, resource_group, resource_group_l
             self.check('resourceGroup', '{resource_group}'),
             self.check('agentPoolProfiles[0].count', 1),
             self.check('agentPoolProfiles[0].vmSize', 'Standard_D4s_v3'),
-            self.check('fqdn', '{fqdn}'),
             self.check('provisioningState', 'Succeeded'),
             self.exists('openShiftVersion')
         ])
@@ -122,14 +118,13 @@ def test_openshift_create_default_service_no_aad(self, resource_group, resource_
         self.kwargs.update({
             'resource_group': resource_group,
             'name': osa_name,
-            'fqdn': self.generate_random_fqdn(osa_name, resource_group_location),
             'location': resource_group_location,
             'resource_type': 'Microsoft.ContainerService/OpenShiftManagedClusters'
         })
 
         # create
         create_cmd = 'openshift create --resource-group={resource_group} --name={name} --location={location} ' \
-                     '--fqdn={fqdn} --compute-count=1 '
+                     '--compute-count=1 '
         self.cmd(create_cmd, checks=[
             self.exists('fqdn'),
             self.check('provisioningState', 'Succeeded')
@@ -143,7 +138,6 @@ def test_openshift_create_default_service_no_aad(self, resource_group, resource_
             self.check('agentPoolProfiles[0].count', 1),
             self.check('agentPoolProfiles[0].osType', 'Linux'),
             self.check('agentPoolProfiles[0].vmSize', 'Standard_D4s_v3'),
-            self.check('fqdn', '{fqdn}'),
             self.exists('openShiftVersion')
         ])
 
@@ -159,7 +153,3 @@ def test_openshift_create_default_service_no_aad(self, resource_group, resource_
 
         # delete
         self.cmd('openshift delete -g {resource_group} -n {name} --yes --no-wait', checks=[self.is_empty()])
-
-    @classmethod
-    def generate_random_fqdn(self, name, location):
-        return "{}.{}.cloudapp.azure.com".format(name, location)

src/command_modules/azure-cli-acs/setup.py

@@ -14,7 +14,7 @@
     logger.warn("Wheel is not available, disabling bdist_wheel hook")
     cmdclass = {}
 
-VERSION = "2.3.22"
+VERSION = "2.4.0"
 CLASSIFIERS = [
     'Development Status :: 5 - Production/Stable',
     'Intended Audience :: Developers',