From b910a27d5ff0b924f3fba3933e6f90621f3c1bf3 Mon Sep 17 00:00:00 2001 From: tengqm Date: Thu, 4 Jun 2015 10:09:28 -0400 Subject: [PATCH] Fix filter name inconsistency in stack_list As described in the bug report, filter names used in stack_list API are not consistent with what users see when creating or retrieving stacks. We cannot simply change the exposed filter key names because that will break existing users. This patch adds a translation logic from STACK_KEYS like 'stack_name' to its corresponding db column name, i.e. 'name'. The intent is to provide compatibility with existing use cases while enforcing API consistency regarding the "properties" a user sees from the API. At engine API layer, we also try a simple parsing of the 'status' key, which may be of simple form 'FAILED' or complex form 'CREATE_COMPLETE'. Change-Id: Ia6fe4345ca9feaf20c3bd020ff454e5d701a458e Closes-Bug: 1461838 --- heat/api/openstack/v1/stacks.py | 22 +++- heat/engine/api.py | 84 ++++++++++++++ heat/engine/service.py | 3 + heat/tests/api/openstack_v1/test_stacks.py | 43 +++++-- heat/tests/test_engine_api_utils.py | 123 +++++++++++++++++++++ heat/tests/test_engine_service.py | 21 ++++ 6 files changed, 285 insertions(+), 11 deletions(-) diff --git a/heat/api/openstack/v1/stacks.py b/heat/api/openstack/v1/stacks.py index b051d60b9a..61575c2488 100644 --- a/heat/api/openstack/v1/stacks.py +++ b/heat/api/openstack/v1/stacks.py @@ -186,6 +186,8 @@ class StackController(object): def _index(self, req, tenant_safe=True): filter_whitelist = { + # usage of keys in this list are not encouraged, please use + # rpc_api.STACK_KEYS instead 'id': 'mixed', 'status': 'mixed', 'name': 'mixed', @@ -208,7 +210,25 @@ class StackController(object): 'not_tags_any': 'single', } params = util.get_allowed_params(req.params, whitelist) - filter_params = util.get_allowed_params(req.params, filter_whitelist) + stack_keys = dict.fromkeys(rpc_api.STACK_KEYS, 'mixed') + unsupported = ( + rpc_api.STACK_ID, # not user visible + rpc_api.STACK_CAPABILITIES, # not supported + rpc_api.STACK_CREATION_TIME, # don't support timestamp + rpc_api.STACK_DELETION_TIME, # don't support timestamp + rpc_api.STACK_DESCRIPTION, # not supported + rpc_api.STACK_NOTIFICATION_TOPICS, # not supported + rpc_api.STACK_OUTPUTS, # not in database + rpc_api.STACK_PARAMETERS, # not in this table + rpc_api.STACK_TAGS, # tags query following a specific guideline + rpc_api.STACK_TMPL_DESCRIPTION, # not supported + rpc_api.STACK_UPDATED_TIME, # don't support timestamp + ) + for key in unsupported: + stack_keys.pop(key) + # downward compatibility + stack_keys.update(filter_whitelist) + filter_params = util.get_allowed_params(req.params, stack_keys) show_deleted = False p_name = rpc_api.PARAM_SHOW_DELETED diff --git a/heat/engine/api.py b/heat/engine/api.py index def6d89567..855a551476 100644 --- a/heat/engine/api.py +++ b/heat/engine/api.py @@ -86,6 +86,90 @@ def extract_args(params): return kwargs +def _parse_object_status(status): + """Parse input status into action and status if possible. + + This function parses a given string (or list of strings) and see if it + contains the action part. The action part is exacted if found. + + :param status: A string or a list of strings where each string contains + a status to be checked. + :returns: (actions, statuses) tuple, where actions is a set of actions + extracted from the input status and statuses is a set of pure + object status. + """ + + if not isinstance(status, list): + status = [status] + + status_set = set() + action_set = set() + for val in status: + # Note: cannot reference Stack.STATUSES due to circular reference issue + for s in ('COMPLETE', 'FAILED', 'IN_PROGRESS'): + index = val.rfind(s) + if index != -1: + status_set.add(val[index:]) + if index > 1: + action_set.add(val[:index - 1]) + break + + return action_set, status_set + + +def translate_filters(params): + """Translate filter names to their corresponding DB field names. + + :param params: A dictionary containing keys from engine.api.STACK_KEYS + and other keys previously leaked to users. + :returns: A dict containing only valid DB filed names. + """ + key_map = { + rpc_api.STACK_NAME: 'name', + rpc_api.STACK_ACTION: 'action', + rpc_api.STACK_STATUS: 'status', + rpc_api.STACK_STATUS_DATA: 'status_reason', + rpc_api.STACK_DISABLE_ROLLBACK: 'disable_rollback', + rpc_api.STACK_TIMEOUT: 'timeout', + rpc_api.STACK_OWNER: 'username', + rpc_api.STACK_PARENT: 'owner_id', + rpc_api.STACK_USER_PROJECT_ID: 'stack_user_project_id', + } + + for key, field in key_map.items(): + value = params.pop(key, None) + if not value: + continue + + fld_value = params.get(field, None) + if fld_value: + if not isinstance(fld_value, list): + fld_value = [fld_value] + if not isinstance(value, list): + value = [value] + + value.extend(fld_value) + + params[field] = value + + # Deal with status which might be of form _, e.g. + # "CREATE_FAILED". Note this logic is still not ideal due to the fact + # that action and status are stored separately. + if 'status' in params: + a_set, s_set = _parse_object_status(params['status']) + statuses = sorted(s_set) + params['status'] = statuses[0] if len(statuses) == 1 else statuses + + if a_set: + a = params.get('action', []) + action_set = set(a) if isinstance(a, list) else set([a]) + actions = sorted(action_set.union(a_set)) + + params['action'] = actions[0] if len(actions) == 1 else actions + + return params + + def format_stack_outputs(stack, outputs): ''' Return a representation of the given output template for the given stack diff --git a/heat/engine/service.py b/heat/engine/service.py index 9fa93c1e2d..710eecf9d8 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -500,6 +500,9 @@ class EngineService(service.Service): multiple tags using the boolean OR expression :returns: a list of formatted stacks """ + if filters is not None: + filters = api.translate_filters(filters) + stacks = parser.Stack.load_all(cnxt, limit, marker, sort_keys, sort_dir, filters, tenant_safe, show_deleted, resolve_data=False, diff --git a/heat/tests/api/openstack_v1/test_stacks.py b/heat/tests/api/openstack_v1/test_stacks.py index 2e9b450525..fa2728426e 100644 --- a/heat/tests/api/openstack_v1/test_stacks.py +++ b/heat/tests/api/openstack_v1/test_stacks.py @@ -321,7 +321,27 @@ class StackControllerTest(tools.ControllerTest, common.HeatTestCase): 'username': 'fake username', 'tenant': 'fake tenant', 'owner_id': 'fake owner-id', - 'balrog': 'you shall not pass!' + 'stack_name': 'fake stack name', + 'stack_identity': 'fake identity', + 'creation_time': 'create timestamp', + 'updated_time': 'update timestamp', + 'deletion_time': 'deletion timestamp', + 'notification_topics': 'fake topic', + 'description': 'fake description', + 'template_description': 'fake description', + 'parameters': 'fake params', + 'outputs': 'fake outputs', + 'stack_action': 'fake action', + 'stack_status': 'fake status', + 'stack_status_reason': 'fake status reason', + 'capabilities': 'fake capabilities', + 'disable_rollback': 'fake value', + 'timeout_mins': 'fake timeout', + 'stack_owner': 'fake owner', + 'parent': 'fake parent', + 'stack_user_project_id': 'fake project id', + 'tags': 'fake tags', + 'barlog': 'you shall not pass!' } req = self._get('/stacks', params=params) mock_call.return_value = [] @@ -333,15 +353,18 @@ class StackControllerTest(tools.ControllerTest, common.HeatTestCase): self.assertIn('filters', engine_args) filters = engine_args['filters'] - self.assertEqual(7, len(filters)) - self.assertIn('id', filters) - self.assertIn('status', filters) - self.assertIn('name', filters) - self.assertIn('action', filters) - self.assertIn('username', filters) - self.assertIn('tenant', filters) - self.assertIn('owner_id', filters) - self.assertNotIn('balrog', filters) + self.assertEqual(16, len(filters)) + for key in ('id', 'status', 'name', 'action', 'username', 'tenant', + 'owner_id', 'stack_name', 'stack_action', 'stack_status', + 'stack_status_reason', 'disable_rollback', 'timeout_mins', + 'stack_owner', 'parent', 'stack_user_project_id'): + self.assertIn(key, filters) + + for key in ('stack_identity', 'creation_time', 'updated_time', + 'deletion_time', 'notification_topics', 'description', + 'template_description', 'parameters', 'outputs', + 'capabilities', 'tags', 'barlog'): + self.assertNotIn(key, filters) def test_index_returns_stack_count_if_with_count_is_true( self, mock_enforce): diff --git a/heat/tests/test_engine_api_utils.py b/heat/tests/test_engine_api_utils.py index cfb7045c5e..048a14fbf9 100644 --- a/heat/tests/test_engine_api_utils.py +++ b/heat/tests/test_engine_api_utils.py @@ -1110,3 +1110,126 @@ class TestExtractArgs(common.HeatTestCase): exc = self.assertRaises(ValueError, api.extract_args, p) self.assertIn('Invalid tag, "tag2," contains a comma', six.text_type(exc)) + + +class TranslateFilterTest(common.HeatTestCase): + scenarios = [ + ( + 'single+single', + dict(inputs={'stack_status': 'COMPLETE', 'status': 'FAILED'}, + expected={'status': ['COMPLETE', 'FAILED']}) + ), ( + 'none+single', + dict(inputs={'name': 'n1'}, + expected={'name': 'n1'}) + ), ( + 'single+none', + dict(inputs={'stack_name': 'n1'}, + expected={'name': 'n1'}) + ), ( + 'none+list', + dict(inputs={'action': ['a1', 'a2']}, + expected={'action': ['a1', 'a2']}) + ), ( + 'list+none', + dict(inputs={'stack_action': ['a1', 'a2']}, + expected={'action': ['a1', 'a2']}) + ), ( + 'single+list', + dict(inputs={'stack_owner': 'u1', 'username': ['u2', 'u3']}, + expected={'username': ['u1', 'u2', 'u3']}) + ), ( + 'list+single', + dict(inputs={'parent': ['s1', 's2'], 'owner_id': 's3'}, + expected={'owner_id': ['s1', 's2', 's3']}) + ), ( + 'list+list', + dict(inputs={'stack_name': ['n1', 'n2'], 'name': ['n3', 'n4']}, + expected={'name': ['n1', 'n2', 'n3', 'n4']}) + ), ( + 'full_status_split', + dict(inputs={'stack_status': 'CREATE_COMPLETE'}, + expected={'action': 'CREATE', 'status': 'COMPLETE'}) + ), ( + 'full_status_split_merge', + dict(inputs={'stack_status': 'CREATE_COMPLETE', + 'status': 'CREATE_FAILED'}, + expected={'action': 'CREATE', + 'status': ['COMPLETE', 'FAILED']}) + ), ( + 'action_status_merge', + dict(inputs={'action': ['UPDATE', 'CREATE'], + 'status': 'CREATE_FAILED'}, + expected={'action': ['CREATE', 'UPDATE'], + 'status': 'FAILED'}) + ) + ] + + def test_stack_filter_translate(self): + actual = api.translate_filters(self.inputs) + self.assertEqual(self.expected, actual) + + +class ParseStatusTest(common.HeatTestCase): + scenarios = [ + ( + 'single_bogus', + dict(inputs='bogus status', + expected=(set(), set())) + ), ( + 'list_bogus', + dict(inputs=['foo', 'bar'], + expected=(set(), set())) + ), ( + 'single_partial', + dict(inputs='COMPLETE', + expected=(set(), set(['COMPLETE']))) + ), ( + 'multi_partial', + dict(inputs=['FAILED', 'COMPLETE'], + expected=(set(), set(['FAILED', 'COMPLETE']))) + ), ( + 'multi_partial_dup', + dict(inputs=['FAILED', 'FAILED'], + expected=(set(), set(['FAILED']))) + ), ( + 'single_full', + dict(inputs=['DELETE_FAILED'], + expected=(set(['DELETE']), set(['FAILED']))) + ), ( + 'multi_full', + dict(inputs=['DELETE_FAILED', 'CREATE_COMPLETE'], + expected=(set(['CREATE', 'DELETE']), + set(['COMPLETE', 'FAILED']))) + ), ( + 'mix_bogus_partial', + dict(inputs=['delete_failed', 'COMPLETE'], + expected=(set(), set(['COMPLETE']))) + ), ( + 'mix_bogus_full', + dict(inputs=['delete_failed', 'action_COMPLETE'], + expected=(set(['action']), set(['COMPLETE']))) + ), ( + 'mix_bogus_full_incomplete', + dict(inputs=['delete_failed', '_COMPLETE'], + expected=(set(), set(['COMPLETE']))) + ), ( + 'mix_partial_full', + dict(inputs=['FAILED', 'b_COMPLETE'], + expected=(set(['b']), + set(['COMPLETE', 'FAILED']))) + ), ( + 'mix_full_dup', + dict(inputs=['a_FAILED', 'a_COMPLETE'], + expected=(set(['a']), + set(['COMPLETE', 'FAILED']))) + ), ( + 'mix_full_dup_2', + dict(inputs=['a_FAILED', 'b_FAILED'], + expected=(set(['a', 'b']), set(['FAILED']))) + ) + ] + + def test_stack_parse_status(self): + actual = api._parse_object_status(self.inputs) + self.assertEqual(self.expected, actual) diff --git a/heat/tests/test_engine_service.py b/heat/tests/test_engine_service.py index 06a8a14099..31801655da 100644 --- a/heat/tests/test_engine_service.py +++ b/heat/tests/test_engine_service.py @@ -1709,6 +1709,27 @@ class StackServiceTest(common.HeatTestCase): mock.ANY, ) + @mock.patch.object(stack_object.Stack, 'get_all') + def test_stack_list_passes_filter_translated(self, mock_stack_get_all): + filters = {'stack_name': 'bar'} + self.eng.list_stacks(self.ctx, filters=filters) + translated = {'name': 'bar'} + mock_stack_get_all.assert_called_once_with(mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + translated, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + ) + @mock.patch.object(stack_object.Stack, 'get_all') def test_stack_list_tenant_safe_defaults_to_true(self, mock_stack_get_all): self.eng.list_stacks(self.ctx)