From e04a06b106c8f093add77faee8c5ea544160a45e Mon Sep 17 00:00:00 2001 From: david liu Date: Fri, 13 Nov 2015 14:54:56 +0800 Subject: [PATCH] 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 Change-Id: I63942fbf28eff5da58de754ef870506fb246d2d6 --- refstack_client/refstack_client.py | 82 +++++++------- refstack_client/tests/unit/test_client.py | 132 ++++++++++++++++------ 2 files changed, 142 insertions(+), 72 deletions(-) diff --git a/refstack_client/refstack_client.py b/refstack_client/refstack_client.py index 8e49e30..fc0a684 100755 --- a/refstack_client/refstack_client.py +++ b/refstack_client/refstack_client.py @@ -184,46 +184,50 @@ class RefstackClient: else: args['tenant_name'] = tenant_name - 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 \ - len(service['endpoints']) > 0: - return service['endpoints'][0]['id'] - else: - message = "Unable to retrieve CPID. " + \ - "Identity service endpoint was " + \ - "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', - 'domain_name') - args['user_domain_name'] = conf_file.get('identity', - 'domain_name') - if conf_file.has_option('identity', 'region'): - args['region_name'] = conf_file.get('identity', - 'region') - client = ksclient3.Client(**args) - token = client.auth_ref - for service in token['catalog']: - if service['type'] == 'identity' and \ - 'id' in service and service['id'] is not None: + 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 service['endpoints'][0]['id']: + return service['endpoints'][0]['id'] + # Raise a key error if 'identity' was not found so that it + # can be caught and have an appropriate error displayed. + raise KeyError + 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', 'domain_name') + args['user_domain_name'] = conf_file.get('identity', + 'domain_name') + if conf_file.has_option('identity', 'region'): + args['region_name'] = conf_file.get('identity', + 'region') + client = ksclient3.Client(**args) + token = client.auth_ref + for service in token['catalog']: + if service['type'] == 'identity' and service['id']: return service['id'] - else: - message = "Unable to retrive CPID. " + \ - "Identity service ID was not " + \ - "found in Keystone v3 catalog." - self.logger.error(message) - raise RuntimeError(message) - else: - raise ValueError('Auth_version %s is unsupported' - '' % auth_version) + # Raise a key error if 'identity' was not found. It will + # be caught below as well. + raise KeyError + 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. diff --git a/refstack_client/tests/unit/test_client.py b/refstack_client/tests/unit/test_client.py index f324a2a..5314951 100755 --- a/refstack_client/tests/unit/test_client.py +++ b/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.