From fc954c9426a58e6bdcb744b1e5d131eff99d65e8 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Wed, 17 Feb 2016 00:49:43 -0800 Subject: [PATCH] Add Nova notifier hook calls to pecan This patch adds a the Nova notifier calls to the pecan notification hook and adds some simple functional tests for them as well. This patch also alters the operations of the policy enforcement hook, which now stores in the request context the original value of the object rather than the value of the object as it would have appeared to the plugin - that is to say a merge between the original objects and the parameters passed on to the request body. Such 'frankenobject' is indeed of no use for the notifiers. Partial-Bug: #1552979 Change-Id: I4a37197eb38afe15d2b368f4e355226824478792 --- neutron/notifiers/nova.py | 3 + neutron/pecan_wsgi/hooks/notifier.py | 40 +++++++--- .../pecan_wsgi/hooks/policy_enforcement.py | 55 +++++++------- .../tests/functional/pecan_wsgi/test_hooks.py | 75 ++++++++++++++++++- 4 files changed, 135 insertions(+), 38 deletions(-) diff --git a/neutron/notifiers/nova.py b/neutron/notifiers/nova.py index c444c19afc6..57f636e0c06 100644 --- a/neutron/notifiers/nova.py +++ b/neutron/notifiers/nova.py @@ -46,6 +46,9 @@ class Notifier(object): # and each Notifier is handling it's own auth. That means that we are # authenticating the exact same thing len(controllers) times. This # should be an easy thing to optimize. + # FIXME(kevinbenton): remove this comment and the one above once the + # switch to pecan is complete since only one notifier is constructed + # in the pecan notification hook. auth = ks_loading.load_auth_from_conf_options(cfg.CONF, 'nova') session = ks_loading.load_session_from_conf_options( diff --git a/neutron/pecan_wsgi/hooks/notifier.py b/neutron/pecan_wsgi/hooks/notifier.py index 7bd2d6506b0..acfe3d56b15 100644 --- a/neutron/pecan_wsgi/hooks/notifier.py +++ b/neutron/pecan_wsgi/hooks/notifier.py @@ -29,9 +29,16 @@ LOG = log.getLogger(__name__) class NotifierHook(hooks.PecanHook): priority = 135 - # TODO(kevinbenton): implement - # ceilo notifier - # nova notifier + # TODO(kevinbenton): implement ceilo notifier + + def _nova_notify(self, action, resource, *args): + action_resource = '%s_%s' % (action, resource) + if not hasattr(self, '_nova_notifier'): + # this is scoped to avoid a dependency on nova client when nova + # notifications aren't enabled + from neutron.notifiers import nova + self._nova_notifier = nova.Notifier() + self._nova_notifier.send_network_change(action_resource, *args) def _notify_dhcp_agent(self, context, resource_name, action, resources): plugin = manager.NeutronManager.get_plugin_for_resource(resource_name) @@ -67,23 +74,38 @@ class NotifierHook(hooks.PecanHook): # The object has been deleted, so we must notify the agent with the # data of the original object data = {collection_name: - state.request.context.get('request_resources', [])} + state.request.context.get('original_resources', [])} else: try: data = jsonutils.loads(state.response.body) except ValueError: if not state.response.body: data = {} - # Send a notification only if a resource can be identified in the - # response. This means that for operations such as add_router_interface - # no notification will be sent - if cfg.CONF.dhcp_agent_notification and data: - resources = [] + resources = [] + if data: if resource_name in data: resources = [data[resource_name]] elif collection_name in data: # This was a bulk request resources = data[collection_name] + # Send a notification only if a resource can be identified in the + # response. This means that for operations such as add_router_interface + # no notification will be sent + if cfg.CONF.dhcp_agent_notification and data: self._notify_dhcp_agent( neutron_context, resource_name, action, resources) + + if cfg.CONF.notify_nova_on_port_data_changes: + orig = {} + if action == 'update': + orig = state.request.context.get('original_resources')[0] + elif action == 'delete': + # NOTE(kevinbenton): the nova notifier is a bit strange because + # it expects the original to be in the last argument on a + # delete rather than in the 'original_obj' position + resources = ( + state.request.context.get('original_resources') or []) + for resource in resources: + self._nova_notify(action, resource_name, orig, + {resource_name: resource}) diff --git a/neutron/pecan_wsgi/hooks/policy_enforcement.py b/neutron/pecan_wsgi/hooks/policy_enforcement.py index c855a1a66b7..582986a3e19 100644 --- a/neutron/pecan_wsgi/hooks/policy_enforcement.py +++ b/neutron/pecan_wsgi/hooks/policy_enforcement.py @@ -36,31 +36,32 @@ def _custom_getter(resource, resource_id): return quota.get_tenant_quotas(resource_id)[quotasv2.RESOURCE_NAME] +def fetch_resource(neutron_context, resource, resource_id): + attrs = v2_attributes.get_resource_info(resource) + if not attrs: + # this isn't a request for a normal resource. it could be + # an action like removing a network from a dhcp agent. + # return None and assume the custom controller for this will + # handle the necessary logic. + return + field_list = [name for (name, value) in attrs.items() + if (value.get('required_by_policy') or + value.get('primary_key') or 'default' not in value)] + plugin = manager.NeutronManager.get_plugin_for_resource(resource) + if plugin: + getter = getattr(plugin, 'get_%s' % resource) + # TODO(kevinbenton): the parent_id logic currently in base.py + return getter(neutron_context, resource_id, fields=field_list) + else: + # Some legit resources, like quota, do not have a plugin yet. + # Retrieving the original object is nevertheless important + # for policy checks. + return _custom_getter(resource, resource_id) + + class PolicyHook(hooks.PecanHook): priority = 140 - def _fetch_resource(self, neutron_context, resource, resource_id): - attrs = v2_attributes.get_resource_info(resource) - if not attrs: - # this isn't a request for a normal resource. it could be - # an action like removing a network from a dhcp agent. - # return None and assume the custom controller for this will - # handle the necessary logic. - return - field_list = [name for (name, value) in attrs.items() - if (value.get('required_by_policy') or - value.get('primary_key') or 'default' not in value)] - plugin = manager.NeutronManager.get_plugin_for_resource(resource) - if plugin: - getter = getattr(plugin, 'get_%s' % resource) - # TODO(kevinbenton): the parent_id logic currently in base.py - return getter(neutron_context, resource_id, fields=field_list) - else: - # Some legit resources, like quota, do not have a plugin yet. - # Retrieving the original object is nevertheless important - # for policy checks. - return _custom_getter(resource, resource_id) - def before(self, state): # This hook should be run only for PUT,POST and DELETE methods and for # requests targeting a neutron resource @@ -87,7 +88,7 @@ class PolicyHook(hooks.PecanHook): # identifier would have been already retrieved by the lookup process; # in the case of DELETE requests there won't be any item to process in # the request body - merged_resources = [] + original_resources = [] if needs_prefetch: try: item = resources_copy.pop() @@ -95,12 +96,12 @@ class PolicyHook(hooks.PecanHook): # Ops... this was a delete after all! item = {} resource_id = state.request.context.get('resource_id') - resource_obj = self._fetch_resource(neutron_context, - resource, resource_id) + resource_obj = fetch_resource(neutron_context, + resource, resource_id) if resource_obj: + original_resources.append(resource_obj) obj = copy.copy(resource_obj) obj.update(item) - merged_resources.append(obj.copy()) obj[const.ATTRIBUTES_TO_UPDATE] = item.keys() # Put back the item in the list so that policies could be # enforced @@ -108,7 +109,7 @@ class PolicyHook(hooks.PecanHook): # TODO(salv-orlando): as other hooks might need to prefetch resources, # store them in the request context. However, this should be done in a # separate hook which is conventietly called before all other hooks - state.request.context['request_resources'] = merged_resources + state.request.context['original_resources'] = original_resources for item in resources_copy: try: policy.enforce( diff --git a/neutron/tests/functional/pecan_wsgi/test_hooks.py b/neutron/tests/functional/pecan_wsgi/test_hooks.py index 1c81681ab0e..a408ef0a1b4 100644 --- a/neutron/tests/functional/pecan_wsgi/test_hooks.py +++ b/neutron/tests/functional/pecan_wsgi/test_hooks.py @@ -23,6 +23,7 @@ from neutron import context from neutron.db.quota import driver as quota_driver from neutron import manager from neutron.pecan_wsgi.controllers import resource +from neutron.pecan_wsgi.hooks import policy_enforcement as pe from neutron import policy from neutron.tests.functional.pecan_wsgi import test_functional @@ -220,7 +221,7 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest): self.assertNotIn('restricted_attr', json_response['mehs'][0]) -class TestNotifierHook(test_functional.PecanFunctionalTest): +class TestDHCPNotifierHook(test_functional.PecanFunctionalTest): def setUp(self): # the DHCP notifier needs to be mocked so that correct operations can @@ -230,7 +231,7 @@ class TestNotifierHook(test_functional.PecanFunctionalTest): patcher = mock.patch('neutron.api.rpc.agentnotifiers.' 'dhcp_rpc_agent_api.DhcpAgentNotifyAPI.notify') self.mock_notifier = patcher.start() - super(TestNotifierHook, self).setUp() + super(TestDHCPNotifierHook, self).setUp() def test_dhcp_notifications_disabled(self): cfg.CONF.set_override('dhcp_agent_notification', False) @@ -290,3 +291,73 @@ class TestNotifierHook(test_functional.PecanFunctionalTest): self.mock_notifier.assert_has_calls( [mock.call(mock.ANY, {'network': item_1}, 'network.create.end'), mock.call(mock.ANY, {'network': item_2}, 'network.create.end')]) + + +class TestNovaNotifierHook(test_functional.PecanFunctionalTest): + + def setUp(self): + patcher = mock.patch('neutron.pecan_wsgi.hooks.notifier.NotifierHook.' + '_nova_notify') + self.mock_notifier = patcher.start() + super(TestNovaNotifierHook, self).setUp() + + def test_nova_notifications_disabled(self): + cfg.CONF.set_override('notify_nova_on_port_data_changes', False) + self.app.post_json( + '/v2.0/networks.json', + params={'network': {'name': 'meh'}}, + headers={'X-Project-Id': 'tenid'}) + self.assertFalse(self.mock_notifier.called) + + def test_post_put_delete_triggers_notification(self): + req_headers = {'X-Project-Id': 'tenid', 'X-Roles': 'admin'} + response = self.app.post_json( + '/v2.0/networks.json', + params={'network': {'name': 'meh'}}, headers=req_headers) + self.assertEqual(201, response.status_int) + json_body = jsonutils.loads(response.body) + self.mock_notifier.assert_called_once_with('create', 'network', {}, + json_body) + self.mock_notifier.reset_mock() + network_id = json_body['network']['id'] + + # NOTE(kevinbenton): the original passed into the notifier does + # not contain all of the fields of the object. Only those required + # by the policy engine are included. + orig = pe.fetch_resource(context.get_admin_context(), + 'network', network_id) + response = self.app.put_json( + '/v2.0/networks/%s.json' % network_id, + params={'network': {'name': 'meh-2'}}, + headers=req_headers) + self.assertEqual(200, response.status_int) + json_body = jsonutils.loads(response.body) + self.mock_notifier.assert_called_once_with('update', 'network', + orig, json_body) + self.mock_notifier.reset_mock() + + orig = pe.fetch_resource(context.get_admin_context(), + 'network', network_id) + response = self.app.delete( + '/v2.0/networks/%s.json' % network_id, headers=req_headers) + self.assertEqual(204, response.status_int) + # No need to validate data content sent to the notifier as it's just + # going to load the object from the database + self.mock_notifier.assert_called_once_with('delete', 'network', {}, + {'network': orig}) + + def test_bulk_create_triggers_notifications(self): + req_headers = {'X-Project-Id': 'tenid', 'X-Roles': 'admin'} + response = self.app.post_json( + '/v2.0/networks.json', + params={'networks': [{'name': 'meh_1'}, + {'name': 'meh_2'}]}, + headers=req_headers) + self.assertEqual(201, response.status_int) + json_body = jsonutils.loads(response.body) + item_1 = json_body['networks'][0] + item_2 = json_body['networks'][1] + self.assertEqual( + [mock.call('create', 'network', {}, {'network': item_1}), + mock.call('create', 'network', {}, {'network': item_2})], + self.mock_notifier.mock_calls)