openstack/neutron
Code Issues Proposed changes

Pecan: get rid of member action hook

Browse Source 
The MemberAction hook, which parses the URL to find out if there is
anything beyond the resource identifier which might be interpreted as
an action to execute is an abomination and a horrible hack.
This patch does some justice by casting it into the abyss of git
history, hopefully forever.

As a matter of fact, the only Neutron API resource that uses this
capability is the router, with the actions 'add_router_interface' and
'remove_reouter_interface'. Therefore, not without a good deal of
deliberate opinionated thinking, this patch adds a specialiized Pecan
controller for the router resource rather than adding the abiity to
handle member actions in the generic resource controller.
The main driver behind this decision is that "member actions" should
be avoided as much as possile, and there is not yet an agreed-upon API
guideline on how they should be expressed in OpenStack APIs.

The 'router' extension (neutron/extensions/l3.py) has also been
"pecanized", by returning a RouterController for routers and an
instance of the generic ItemController for floating IPs.

Moreover, references to the 'member_action' parameter in the request
context are removed. The BodyValidation hook has been updated to
process the request body only if a resource or collection name can be
found in it (member actions don't have any according to the Neutron
API specification).

Finally, this patch adds functional tests specific for validating
member action processing in the router controller.

Related-Blueprint: wsgi-pecan-switch

Change-Id: Ib57472b38f5b1576387c8bf49c082ce25d3e5bd6
This commit is contained in:
Salvatore Orlando
2015-12-21 16:01:50 -08:00
parent 171351d5fb
commit 12646fa68f
10 changed files with 239 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
neutron/extensions/l3.py
View File

@@ -22,6 +22,7 @@ from neutron.api import extensions
from neutron.api.v2 import attributes as attr
from neutron.api.v2 import resource_helper
from neutron.common import exceptions as nexception
from neutron.pecan_wsgi import controllers
from neutron.plugins.common import constants
@@ -78,6 +79,8 @@ class RouterExternalGatewayInUseByFloatingIp(nexception.InUse):
"more floating IPs.")
ROUTERS = 'routers'
FLOATINGIP = 'floatingip'
FLOATINGIPS = '%ss' % FLOATINGIP
EXTERNAL_GW_INFO = 'external_gateway_info'
FLOATINGIPS = 'floatingips'
@@ -204,6 +207,12 @@ class L3(extensions.ExtensionDescriptor):
super(L3, self).update_attributes_map(
attributes, extension_attrs_map=RESOURCE_ATTRIBUTE_MAP)
@classmethod
def get_pecan_controllers(cls):
return ((ROUTERS, controllers.RoutersController()),
(FLOATINGIPS, controllers.CollectionsController(FLOATINGIPS,
FLOATINGIP)))
def get_extended_resources(self, version):
if version == "2.0":
return RESOURCE_ATTRIBUTE_MAP

1
neutron/pecan_wsgi/app.py
View File

@@ -45,7 +45,6 @@ def setup_app(*args, **kwargs):
app_hooks = [
hooks.ExceptionTranslationHook(), # priority 100
hooks.ContextHook(), # priority 95
hooks.MemberActionHook(), # piority 95
hooks.BodyValidationHook(), # priority 120
hooks.OwnershipValidationHook(), # priority 125
hooks.QuotaEnforcementHook(), # priority 130

4
neutron/pecan_wsgi/controllers/__init__.py
View File

@@ -11,6 +11,10 @@
# under the License.
from neutron.pecan_wsgi.controllers import quota
from neutron.pecan_wsgi.controllers import resource
from neutron.pecan_wsgi.controllers import router
CollectionsController = resource.CollectionsController
QuotasController = quota.QuotasController
RoutersController = router.RoutersController

91
neutron/pecan_wsgi/controllers/resource.py
View File

@@ -18,13 +18,56 @@ from pecan import request
from neutron.pecan_wsgi.controllers import utils
class ItemController(utils.NeutronPecanController):
def __init__(self, resource, item):
super(ItemController, self).__init__(None, resource)
self.item = item
@utils.expose(generic=True)
def index(self, *args, **kwargs):
return self.get()
def get(self, *args, **kwargs):
getter = getattr(self.plugin, 'get_%s' % self.resource)
neutron_context = request.context['neutron_context']
return {self.resource: getter(neutron_context, self.item)}
@utils.when(index, method='HEAD')
@utils.when(index, method='POST')
@utils.when(index, method='PATCH')
def not_supported(self):
pecan.abort(405)
@utils.when(index, method='PUT')
def put(self, *args, **kwargs):
neutron_context = request.context['neutron_context']
resources = request.context['resources']
# TODO(kevinbenton): bulk?
updater = getattr(self.plugin, 'update_%s' % self.resource)
# Bulk update is not supported, 'resources' always contains a single
# elemenet
data = {self.resource: resources[0]}
return {self.resource: updater(neutron_context, self.item, data)}
@utils.when(index, method='DELETE')
def delete(self):
# TODO(kevinbenton): setting code could be in a decorator
pecan.response.status = 204
neutron_context = request.context['neutron_context']
deleter = getattr(self.plugin, 'delete_%s' % self.resource)
return deleter(neutron_context, self.item)
class CollectionsController(utils.NeutronPecanController):
item_controller_class = ItemController
@utils.expose()
def _lookup(self, item, *remainder):
# Store resource identifier in request context
request.context['resource_id'] = item
return ItemController(self.resource, item), remainder
return self.item_controller_class(self.resource, item), remainder
@utils.expose(generic=True)
def index(self, *args, **kwargs):
@@ -69,49 +112,3 @@ class CollectionsController(utils.NeutronPecanController):
creator = getattr(self.plugin, method)
neutron_context = request.context['neutron_context']
return {key: creator(neutron_context, data)}
class ItemController(utils.NeutronPecanController):
def __init__(self, resource, item):
super(ItemController, self).__init__(None, resource)
self.item = item
@utils.expose(generic=True)
def index(self, *args, **kwargs):
return self.get()
def get(self, *args, **kwargs):
getter = getattr(self.plugin, 'get_%s' % self.resource)
neutron_context = request.context['neutron_context']
return {self.resource: getter(neutron_context, self.item)}
@utils.when(index, method='HEAD')
@utils.when(index, method='POST')
@utils.when(index, method='PATCH')
def not_supported(self):
pecan.abort(405)
@utils.when(index, method='PUT')
def put(self, *args, **kwargs):
neutron_context = request.context['neutron_context']
if request.member_action:
member_action_method = getattr(self.plugin,
request.member_action)
return member_action_method(neutron_context, self.item,
request.prepared_data)
# TODO(kevinbenton): bulk?
updater = getattr(self.plugin, 'update_%s' % self.resource)
resources = request.context['resources']
# Bulk update is not supported, 'resources' always contains a single
# elemenet
data = {self.resource: resources[0]}
return {self.resource: updater(neutron_context, self.item, data)}
@utils.when(index, method='DELETE')
def delete(self):
# TODO(kevinbenton): setting code could be in a decorator
pecan.response.status = 204
neutron_context = request.context['neutron_context']
deleter = getattr(self.plugin, 'delete_%s' % self.resource)
return deleter(neutron_context, self.item)

82
neutron/pecan_wsgi/controllers/router.py Normal file
View File

@@ -0,0 +1,82 @@
# Copyright (c) 2015 Taturiello Consulting, Meh.
# All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
from neutron._i18n import _LE
from oslo_log import log
import pecan
from pecan import request
from neutron.pecan_wsgi.controllers import resource
from neutron.pecan_wsgi.controllers import utils
LOG = log.getLogger(__name__)
class RouterController(resource.ItemController):
"""Customize ResourceController for member actions"""
### Pecan generic controllers don't work very well with inheritance
@utils.expose(generic=True)
def index(self, *args, **kwargs):
return super(RouterController, self).index(*args, **kwargs)
@utils.when(index, method='HEAD')
@utils.when(index, method='POST')
@utils.when(index, method='PATCH')
def not_supported(self):
return super(RouterController, self).not_supported()
@utils.when(index, method='PUT')
def put(self, *args, **kwargs):
neutron_context = request.context['neutron_context']
if args:
# There is a member action to process
member_action = args[0]
LOG.debug("Processing member action %(action)s for resource "
"%(resource)s identified by %(item)s",
{'action': member_action,
'resource': self.resource,
'item': self.item})
# NOTE(salv-orlando): The following simply verify that the plugin
# has a method for a given action. It therefore enables plugins to
# implement actions which are not part of the API specification.
# Unfortunately the API extension descriptor does not do a good job
# of sanctioning which actions are available on a given resource.
# TODO(salv-orlando): prevent plugins from implementing actions
# which are not part of the Neutron API spec
try:
member_action_method = getattr(self.plugin, member_action)
return member_action_method(neutron_context, self.item,
request.context['request_data'])
except AttributeError:
LOG.error(_LE("Action %(action)s is not defined on resource "
"%(resource)s"),
{'action': member_action, 'resource': self.resource})
pecan.abort(404)
# Do standard PUT processing
return super(RouterController, self).put(*args, **kwargs)
@utils.when(index, method='DELETE')
def delete(self):
return super(RouterController, self).delete()
class RoutersController(resource.CollectionsController):
item_controller_class = RouterController
def __init__(self):
super(RoutersController, self).__init__('routers', 'router')

2
neutron/pecan_wsgi/hooks/__init__.py
View File

@@ -15,7 +15,6 @@
from neutron.pecan_wsgi.hooks import body_validation
from neutron.pecan_wsgi.hooks import context
from neutron.pecan_wsgi.hooks import member_action
from neutron.pecan_wsgi.hooks import notifier
from neutron.pecan_wsgi.hooks import ownership_validation
from neutron.pecan_wsgi.hooks import policy_enforcement
@@ -25,7 +24,6 @@ from neutron.pecan_wsgi.hooks import translation
ExceptionTranslationHook = translation.ExceptionTranslationHook
ContextHook = context.ContextHook
MemberActionHook = member_action.MemberActionHook
BodyValidationHook = body_validation.BodyValidationHook
OwnershipValidationHook = ownership_validation.OwnershipValidationHook
PolicyHook = policy_enforcement.PolicyHook

20
neutron/pecan_wsgi/hooks/body_validation.py
View File

@@ -13,11 +13,15 @@
# License for the specific language governing permissions and limitations
# under the License.
from oslo_log import log
from oslo_serialization import jsonutils
from pecan import hooks
from neutron.api.v2 import attributes as v2_attributes
from neutron.api.v2 import base as v2_base
LOG = log.getLogger(__name__)
class BodyValidationHook(hooks.PecanHook):
@@ -32,10 +36,24 @@ class BodyValidationHook(hooks.PecanHook):
is_create = state.request.method == 'POST'
if not resource:
return
try:
json_data = jsonutils.loads(state.request.body)
except ValueError:
LOG.debug("No JSON Data in %(method)s request for %(collection)s",
{'method': state.request.method,
'collections': collection})
return
# Raw data are consumed by member actions such as add_router_interface
state.request.context['request_data'] = json_data
if not (resource in json_data or collection in json_data):
# there is no resource in the request. This can happen when a
# member action is being processed.
return
# Prepare data to be passed to the plugin from request body
data = v2_base.Controller.prepare_request_body(
neutron_context,
state.request.json,
json_data,
is_create,
resource,
v2_attributes.get_collection_info(collection),

69
neutron/pecan_wsgi/hooks/member_action.py
View File

@@ -1,69 +0,0 @@
# Copyright (c) 2015 Mirantis, Inc.
# All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
from pecan import abort
from pecan import hooks
from neutron.api import extensions
from neutron.api.v2 import attributes
class MemberActionHook(hooks.PecanHook):
priority = 95
def before(self, state):
# TODO(salv-orlando): This hook must go. Handling actions like this is
# shameful
state.request.member_action = None
resource = state.request.context.get('resource')
if not resource:
return
try:
# Remove the format suffix if any
uri = state.request.path.rsplit('.', 1)[0].split('/')[2:]
if not uri:
# there's nothing to process in the URI
return
except IndexError:
return
collection = None
for (collection, res) in attributes.PLURALS.items():
if res == resource:
break
else:
return
state.request.member_action = self._parse_action(
resource, collection, uri[1:])
def _parse_action(self, resource, collection, remainder):
# NOTE(salv-orlando): This check is revolting and makes me
# puke, but avoids silly failures when dealing with API actions
# such as "add_router_interface".
if len(remainder) > 1:
action = remainder[1]
else:
return
ext_mgr = extensions.PluginAwareExtensionManager.get_instance()
resource_exts = ext_mgr.get_resources()
for ext in resource_exts:
if (ext.collection == collection and action in ext.member_actions):
return action
# Action or resource extension not found
if action:
abort(404, detail="Action %(action)s for resource "
"%(resource)s undefined" %
{'action': action,
'resource': resource})

25
neutron/pecan_wsgi/hooks/notifier.py
View File

@@ -74,15 +74,16 @@ class NotifierHook(hooks.PecanHook):
except ValueError:
if not state.response.body:
data = {}
if cfg.CONF.dhcp_agent_notification:
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]
else:
resources = []
self._notify_dhcp_agent(neutron_context, resource_name,
action, resources)
# 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 = []
if resource_name in data:
resources = [data[resource_name]]
elif collection_name in data:
# This was a bulk request
resources = data[collection_name]
self._notify_dhcp_agent(
neutron_context, resource_name,
action, resources)

68
neutron/tests/functional/pecan_wsgi/test_controllers.py
View File

@@ -13,6 +13,7 @@
from collections import namedtuple
import mock
from oslo_config import cfg
from oslo_serialization import jsonutils
import pecan
from pecan import request
@@ -21,6 +22,7 @@ from neutron.api.v2 import attributes
from neutron import context
from neutron import manager
from neutron.pecan_wsgi.controllers import root as controllers
from neutron.plugins.common import constants
from neutron.tests.functional.pecan_wsgi import test_functional
_SERVICE_PLUGIN_RESOURCE = 'serviceplugin'
@@ -394,3 +396,69 @@ class TestRequestProcessing(TestResourceController):
response = self.do_request('/v2.0/dummy/serviceplugins.json')
self.assertEqual(200, response.status_int)
self.assertEqual(_SERVICE_PLUGIN_INDEX_BODY, response.json_body)
class TestRouterController(TestResourceController):
"""Specialized tests for the router resource controller
This test class adds tests specific for the router controller in
order to verify the 'member_action' functionality, which this
controller uses for adding and removing router interfaces.
"""
def setUp(self):
cfg.CONF.set_override(
'service_plugins',
['neutron.services.l3_router.l3_router_plugin.L3RouterPlugin'])
super(TestRouterController, self).setUp()
# Create a network, a subnet, and a router
pl = manager.NeutronManager.get_plugin()
service_plugins = manager.NeutronManager.get_service_plugins()
l3_plugin = service_plugins[constants.L3_ROUTER_NAT]
ctx = context.get_admin_context()
network_id = pl.create_network(
ctx,
{'network':
{'name': 'pecannet',
'tenant_id': 'tenid',
'shared': False,
'admin_state_up': True,
'status': 'ACTIVE'}})['id']
self.subnet = pl.create_subnet(
ctx,
{'subnet':
{'tenant_id': 'tenid',
'network_id': network_id,
'name': 'pecansub',
'ip_version': 4,
'cidr': '10.20.30.0/24',
'gateway_ip': '10.20.30.1',
'enable_dhcp': True,
'allocation_pools': [
{'start': '10.20.30.2',
'end': '10.20.30.254'}],
'dns_nameservers': [],
'host_routes': []}})
self.router = l3_plugin.create_router(
ctx,
{'router':
{'name': 'pecanrtr',
'tenant_id': 'tenid',
'admin_state_up': True}})
def test_member_actions_processing(self):
response = self.app.put_json(
'/v2.0/routers/%s/add_router_interface.json' % self.router['id'],
params={'subnet_id': self.subnet['id']},
headers={'X-Project-Id': 'tenid'})
self.assertEqual(200, response.status_int)
def test_non_existing_member_action_returns_404(self):
response = self.app.put_json(
'/v2.0/routers/%s/do_meh.json' % self.router['id'],
params={'subnet_id': 'doesitevenmatter'},
headers={'X-Project-Id': 'tenid'},
expect_errors=True)
self.assertEqual(404, response.status_int)