Address regression and strengthen cpid checks

Use a try/except block instead of checking if keys exists
for cleaner code. Now if the identity key is not found in the
catalog, the user will get an error instead of the function
returning None as the cpid.

Closes-Bug: #1502949
Closes-Bug: #1515880

Co-Authored-By: Paul Van Eck <pvaneck@us.ibm.com>
Change-Id: I63942fbf28eff5da58de754ef870506fb246d2d6

refstack_client/refstack_client.py

@@ -184,26 +184,23 @@ class RefstackClient:
             else:
                 args['tenant_name'] = tenant_name
 
+            try:
                 if auth_version == 'v2':
                     args['auth_url'] = conf_file.get('identity', 'uri')
                     client = ksclient2.Client(**args)
                     token = client.auth_ref
                     for service in token['serviceCatalog']:
                         if service['type'] == 'identity':
-                        if 'endpoints' in service and \
+                            if service['endpoints'][0]['id']:
-                            len(service['endpoints']) > 0:
                                 return service['endpoints'][0]['id']
-                        else:
+                    # Raise a key error if 'identity' was not found so that it
-                            message = "Unable to retrieve CPID. " + \
+                    # can be caught and have an appropriate error displayed.
-                                      "Identity service endpoint was " + \
+                    raise KeyError
-                                      "not found in Keystone v2 catalog."
-                            self.logger.error(message)
-                            raise RuntimeError(message)
                 elif auth_version == 'v3':
                     args['auth_url'] = conf_file.get('identity', 'uri_v3')
                     if conf_file.has_option('identity', 'domain_name'):
-                    args['project_domain_name'] = conf_file.get('identity',
+                        args['project_domain_name'] = \
-                                                                'domain_name')
+                            conf_file.get('identity', 'domain_name')
                         args['user_domain_name'] = conf_file.get('identity',
                                                                  'domain_name')
                     if conf_file.has_option('identity', 'region'):
@@ -212,18 +209,25 @@ class RefstackClient:
                     client = ksclient3.Client(**args)
                     token = client.auth_ref
                     for service in token['catalog']:
-                    if service['type'] == 'identity' and \
+                        if service['type'] == 'identity' and service['id']:
-                        'id' in service and service['id'] is not None:
                             return service['id']
-                    else:
+                    # Raise a key error if 'identity' was not found. It will
-                        message = "Unable to retrive CPID. " + \
+                    # be caught below as well.
-                                  "Identity service ID was not " + \
+                    raise KeyError
-                                  "found in Keystone v3 catalog."
-                        self.logger.error(message)
-                        raise RuntimeError(message)
                 else:
                     raise ValueError('Auth_version %s is unsupported'
                                      '' % auth_version)
+            # If a Key or Index Error was raised, one of the expected keys or
+            # indices for retrieving the identity service ID was not found.
+            except (KeyError, IndexError) as e:
+                raise RuntimeError('Unable to retrieve CPID from Keystone %s '
+                                   'catalog. The catalog or the identity '
+                                   'service endpoint was not '
+                                   'found.' % auth_version)
+            except Exception as e:
+                self.logger.error('Problems retreiving CPID from Keystone '
+                                  'using %s endpoint' % auth_version)
+                raise
 
         except ConfigParser.Error as e:
             # Most likely a missing section or option in the config file.

refstack_client/tests/unit/test_client.py

@@ -94,33 +94,6 @@ class TestRefstackClient(unittest.TestCase):
             return_value=self.mock_ks3_client
         )
 
-    def mock_keystone_with_wrong_service(self):
-        """
-        Mock the Keystone client methods.
-        """
-        self.mock_identity_service_v2 = {'type': 'identity',
-                                         'endpoints': []}
-        self.mock_identity_service_v3 = {'type': 'identity',
-                                         'id': None}
-        self.mock_ks2_client = MagicMock(
-            name='ks_client',
-            **{'auth_ref':
-               {'serviceCatalog': [self.mock_identity_service_v2]}}
-        )
-        self.mock_ks3_client = MagicMock(
-            name='ks_client',
-            **{'auth_ref':
-               {'catalog': [self.mock_identity_service_v3]}}
-        )
-        self.ks2_client_builder = self.patch(
-            'refstack_client.refstack_client.ksclient2.Client',
-            return_value=self.mock_ks2_client
-        )
-        self.ks3_client_builder = self.patch(
-            'refstack_client.refstack_client.ksclient3.Client',
-            return_value=self.mock_ks3_client
-        )
-
     def setUp(self):
         """
         Test case setup
@@ -356,22 +329,70 @@ class TestRefstackClient(unittest.TestCase):
         )
         self.assertEqual('test-id', cpid)
 
-    def test_get_cpid_from_keystone_v2_exits(self):
+    def test_get_cpid_from_keystone_v2_varying_catalogs(self):
         """
-        Test getting the CPID from keystone API v2 exits with wrong service.
+        Test getting the CPID from keystone API v2 varying catalogs.
         """
         argv = self.mock_argv()
         args = rc.parse_cli_args(argv)
         client = rc.RefstackClient(args)
         client.tempest_dir = self.test_path
         client._prep_test()
-        self.mock_keystone_with_wrong_service()
+
+        # Test when the identity endpoints is empty
+        self.mock_ks2_client = MagicMock(
+            name='ks_client',
+            **{'auth_ref':
+               {'serviceCatalog': [{'type': 'identity', 'endpoints': []}]}}
+        )
+        self.ks2_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient2.Client',
+            return_value=self.mock_ks2_client
+        )
         with self.assertRaises(RuntimeError):
             client._get_cpid_from_keystone(client.conf)
 
-    def test_get_cpid_from_keystone_v3_exits(self):
+        # Test when the catalog is empty
+        self.mock_ks2_client = MagicMock(
+            name='ks_client',
+            **{'auth_ref': {'serviceCatalog': []}}
+        )
+        self.ks2_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient2.Client',
+            return_value=self.mock_ks2_client
+        )
+        with self.assertRaises(RuntimeError):
+            client._get_cpid_from_keystone(client.conf)
+
+        # Test when there is no service catalog
+        self.mock_ks2_client = MagicMock(name='ks_client', **{'auth_ref': {}})
+        self.ks2_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient2.Client',
+            return_value=self.mock_ks2_client
+        )
+        with self.assertRaises(RuntimeError):
+            client._get_cpid_from_keystone(client.conf)
+
+        # Test when catalog has other non-identity services.
+        self.mock_ks2_client = MagicMock(
+            name='ks_client',
+            **{'auth_ref':
+                {'serviceCatalog': [{'type': 'compute',
+                                     'endpoints': [{'id': 'test-id1'}]},
+                                    {'type': 'identity',
+                                     'endpoints': [{'id': 'test-id2'}]}]}
+               }
+        )
+        self.ks2_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient2.Client',
+            return_value=self.mock_ks2_client
+        )
+        cpid = client._get_cpid_from_keystone(client.conf)
+        self.assertEqual('test-id2', cpid)
+
+    def test_get_cpid_from_keystone_v3_varying_catalogs(self):
         """
-        Test getting the CPID from keystone API v3 exits.
+        Test getting the CPID from keystone API v3 with varying catalogs.
         """
         args = rc.parse_cli_args(self.mock_argv())
         client = rc.RefstackClient(args)
@@ -380,10 +401,55 @@ class TestRefstackClient(unittest.TestCase):
         client.conf.remove_option('identity', 'tenant_id')
         client.conf.set('identity', 'tenant_name', 'tenant_name')
         client.conf.set('identity-feature-enabled', 'api_v3', 'true')
-        self.mock_keystone_with_wrong_service()
+
+        # Test when the identity ID is None.
+        self.mock_ks3_client = MagicMock(
+            name='ks_client',
+            **{'auth_ref': {'catalog': [{'type': 'identity', 'id': None}]}}
+        )
+        self.ks3_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient3.Client',
+            return_value=self.mock_ks3_client
+        )
         with self.assertRaises(RuntimeError):
             client._get_cpid_from_keystone(client.conf)
 
+        # Test when the catalog is empty.
+        self.mock_ks3_client = MagicMock(
+            name='ks_client',
+            **{'auth_ref': {'catalog': []}}
+        )
+        self.ks3_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient3.Client',
+            return_value=self.mock_ks3_client
+        )
+        with self.assertRaises(RuntimeError):
+            client._get_cpid_from_keystone(client.conf)
+
+        # Test when there is no service catalog.
+        self.mock_ks3_client = MagicMock(name='ks_client', **{'auth_ref': {}})
+        self.ks3_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient3.Client',
+            return_value=self.mock_ks3_client
+        )
+        with self.assertRaises(RuntimeError):
+            client._get_cpid_from_keystone(client.conf)
+
+        #Test when catalog has other non-identity services.
+        self.mock_ks3_client = MagicMock(
+            name='ks_client',
+            **{'auth_ref': {'catalog': [{'type': 'compute',
+                                         'id': 'test-id1'},
+                                        {'type': 'identity',
+                                         'id': 'test-id2'}]}}
+        )
+        self.ks3_client_builder = self.patch(
+            'refstack_client.refstack_client.ksclient3.Client',
+            return_value=self.mock_ks3_client
+        )
+        cpid = client._get_cpid_from_keystone(client.conf)
+        self.assertEqual('test-id2', cpid)
+
     def test_form_result_content(self):
         """
         Test that the request content is formed into the expected format.