From 5d53dfb8d64186b5b1d2f356fbff8f222e15d1b2 Mon Sep 17 00:00:00 2001 From: Mathieu Rohon Date: Wed, 4 Nov 2015 17:49:40 +0000 Subject: [PATCH] Avoid duplicating tenant check when creating resources The check of the tenant done in the method _get_tenant_id_for_create() is already did by the Neutron Controller in prepare_request_body(), with a call to attributes.populate_tenant_id(). Moreover, when the Controller processes a "create" requests, it will add the 'tenant_id' to the resource dict. Thus, _get_tenant_id_for_create() can be deleted. Calls to this method are replaced by the res['tenant_id']. Changes have to be done in UT to explicitly add the tenant_id while creating resources, since the UT framework is bypassing the controller code that automatically adds the tenant_id to the resource. Co-Authored-By: Hong Hui Xiao Closes-Bug: #1513825 Change-Id: Icea06dc81344e1120bdf986a97a6b1094bbb765e Depends-On: I31022e9230fc5404c6a94edabbb08d2b079c3a09 Depends-On: Iea3f014ef17a1e1b755cd2efe99afd1a36ebbc6a Depends-On: I604602d023e0cbf7f6591149f914d73217d7a574 --- neutron/db/address_scope_db.py | 3 +- neutron/db/common_db_mixin.py | 13 -------- neutron/db/db_base_plugin_v2.py | 8 ++--- neutron/db/l3_db.py | 7 ++-- neutron/db/metering/metering_db.py | 3 +- neutron/db/rbac_db_mixin.py | 3 +- neutron/db/securitygroups_db.py | 9 +++-- neutron/plugins/ml2/plugin.py | 2 +- .../scheduler/test_l3_agent_scheduler.py | 6 ++-- neutron/tests/retargetable/client_fixtures.py | 1 + .../unit/api/rpc/handlers/test_l3_rpc.py | 2 ++ neutron/tests/unit/api/v2/test_attributes.py | 33 +++++++++++++++++++ .../tests/unit/db/test_agentschedulers_db.py | 5 +++ neutron/tests/unit/db/test_l3_hamode_db.py | 4 ++- neutron/tests/unit/extensions/test_l3.py | 2 ++ .../unit/extensions/test_portsecurity.py | 2 +- .../unit/extensions/test_securitygroup.py | 6 ++-- neutron/tests/unit/plugins/ml2/test_plugin.py | 3 +- .../unit/scheduler/test_l3_agent_scheduler.py | 3 +- 19 files changed, 71 insertions(+), 44 deletions(-) diff --git a/neutron/db/address_scope_db.py b/neutron/db/address_scope_db.py index 20f1c963625..d849dcbda8b 100644 --- a/neutron/db/address_scope_db.py +++ b/neutron/db/address_scope_db.py @@ -74,10 +74,9 @@ class AddressScopeDbMixin(ext_address_scope.AddressScopePluginBase): def create_address_scope(self, context, address_scope): """Create an address scope.""" a_s = address_scope['address_scope'] - tenant_id = self._get_tenant_id_for_create(context, a_s) address_scope_id = a_s.get('id') or uuidutils.generate_uuid() with context.session.begin(subtransactions=True): - pool_args = {'tenant_id': tenant_id, + pool_args = {'tenant_id': a_s['tenant_id'], 'id': address_scope_id, 'name': a_s['name'], 'shared': a_s['shared'], diff --git a/neutron/db/common_db_mixin.py b/neutron/db/common_db_mixin.py index 301e0ba5906..ba913bfbab3 100644 --- a/neutron/db/common_db_mixin.py +++ b/neutron/db/common_db_mixin.py @@ -20,8 +20,6 @@ from sqlalchemy import and_ from sqlalchemy import or_ from sqlalchemy import sql -from neutron._i18n import _ -from neutron.common import exceptions as n_exc from neutron.db import sqlalchemyutils @@ -169,17 +167,6 @@ class CommonDbMixin(object): if key in fields)) return resource - def _get_tenant_id_for_create(self, context, resource): - if context.is_admin and 'tenant_id' in resource: - tenant_id = resource['tenant_id'] - elif ('tenant_id' in resource and - resource['tenant_id'] != context.tenant_id): - reason = _('Cannot create resource for another tenant') - raise n_exc.AdminRequired(reason=reason) - else: - tenant_id = context.tenant_id - return tenant_id - def _get_by_id(self, context, model, id): query = self._model_query(context, model) return query.filter(model.id == id).one() diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index fdc92fa2955..aa155048c9c 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -315,7 +315,7 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, n = network['network'] # NOTE(jkoelker) Get the tenant_id outside of the session to avoid # unneeded db action if the operation raises - tenant_id = self._get_tenant_id_for_create(context, n) + tenant_id = n['tenant_id'] with context.session.begin(subtransactions=True): args = {'tenant_id': tenant_id, 'id': n.get('id') or uuidutils.generate_uuid(), @@ -646,7 +646,6 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, net = netaddr.IPNetwork(s['cidr']) subnet['subnet']['cidr'] = '%s/%s' % (net.network, net.prefixlen) - s['tenant_id'] = self._get_tenant_id_for_create(context, s) subnetpool_id = self._get_subnetpool_id(context, s) if subnetpool_id: self.ipam.validate_pools_with_subnetpool(s) @@ -955,9 +954,8 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, self._validate_address_scope_id(context, sp_reader.address_scope_id, id, sp_reader.prefixes, sp_reader.ip_version) - tenant_id = self._get_tenant_id_for_create(context, sp) with context.session.begin(subtransactions=True): - pool_args = {'tenant_id': tenant_id, + pool_args = {'tenant_id': sp['tenant_id'], 'id': sp_reader.id, 'name': sp_reader.name, 'ip_version': sp_reader.ip_version, @@ -1165,7 +1163,7 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, network_id = p['network_id'] # NOTE(jkoelker) Get the tenant_id outside of the session to avoid # unneeded db action if the operation raises - tenant_id = self._get_tenant_id_for_create(context, p) + tenant_id = p['tenant_id'] if p.get('device_owner'): self._enforce_device_owner_not_router_intf_or_device_id( context, p.get('device_owner'), p.get('device_id'), tenant_id) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index ee6ad5142cd..81af764dc53 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -178,8 +178,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): def create_router(self, context, router): r = router['router'] gw_info = r.pop(EXTERNAL_GW_INFO, None) - tenant_id = self._get_tenant_id_for_create(context, r) - router_db = self._create_router_db(context, r, tenant_id) + router_db = self._create_router_db(context, r, r['tenant_id']) try: if gw_info: self._update_router_gw_info(context, router_db['id'], @@ -956,7 +955,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): def _create_floatingip(self, context, floatingip, initial_status=l3_constants.FLOATINGIP_STATUS_ACTIVE): fip = floatingip['floatingip'] - tenant_id = self._get_tenant_id_for_create(context, fip) fip_id = uuidutils.generate_uuid() f_net_id = fip['floating_network_id'] @@ -1003,12 +1001,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): floating_ip_address = floating_fixed_ip['ip_address'] floatingip_db = FloatingIP( id=fip_id, - tenant_id=tenant_id, + tenant_id=fip['tenant_id'], status=initial_status, floating_network_id=fip['floating_network_id'], floating_ip_address=floating_ip_address, floating_port_id=external_port['id']) - fip['tenant_id'] = tenant_id # Update association with internal port # and define external IP address self._update_fip_assoc(context, fip, diff --git a/neutron/db/metering/metering_db.py b/neutron/db/metering/metering_db.py index 51f0f57370a..a9af2e27fcd 100644 --- a/neutron/db/metering/metering_db.py +++ b/neutron/db/metering/metering_db.py @@ -68,12 +68,11 @@ class MeteringDbMixin(metering.MeteringPluginBase, def create_metering_label(self, context, metering_label): m = metering_label['metering_label'] - tenant_id = self._get_tenant_id_for_create(context, m) with context.session.begin(subtransactions=True): metering_db = MeteringLabel(id=uuidutils.generate_uuid(), description=m['description'], - tenant_id=tenant_id, + tenant_id=m['tenant_id'], name=m['name'], shared=m['shared']) context.session.add(metering_db) diff --git a/neutron/db/rbac_db_mixin.py b/neutron/db/rbac_db_mixin.py index d5692f67c45..f2efb6465ed 100644 --- a/neutron/db/rbac_db_mixin.py +++ b/neutron/db/rbac_db_mixin.py @@ -42,12 +42,11 @@ class RbacPluginMixin(common_db_mixin.CommonDbMixin): except c_exc.CallbackFailure as e: raise n_exc.InvalidInput(error_message=e) dbmodel = models.get_type_model_map()[e['object_type']] - tenant_id = self._get_tenant_id_for_create(context, e) with context.session.begin(subtransactions=True): db_entry = dbmodel(object_id=e['object_id'], target_tenant=e['target_tenant'], action=e['action'], - tenant_id=tenant_id) + tenant_id=e['tenant_id']) context.session.add(db_entry) return self._make_rbac_policy_dict(db_entry) diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 8cbcc22e606..a5a7bfea071 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -149,7 +149,7 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): except exceptions.CallbackFailure as e: raise ext_sg.SecurityGroupConflict(reason=e) - tenant_id = self._get_tenant_id_for_create(context, s) + tenant_id = s['tenant_id'] if not default_sg: self._ensure_default_security_group(context, tenant_id) @@ -393,11 +393,10 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): except exceptions.CallbackFailure as e: raise ext_sg.SecurityGroupConflict(reason=e) - tenant_id = self._get_tenant_id_for_create(context, rule_dict) with context.session.begin(subtransactions=True): db = SecurityGroupRule( id=(rule_dict.get('id') or uuidutils.generate_uuid()), - tenant_id=tenant_id, + tenant_id=rule_dict['tenant_id'], security_group_id=rule_dict['security_group_id'], direction=rule_dict['direction'], remote_group_id=rule_dict.get('remote_group_id'), @@ -729,8 +728,8 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): port = port['port'] if port.get('device_owner') and utils.is_port_trusted(port): return - tenant_id = self._get_tenant_id_for_create(context, port) - default_sg = self._ensure_default_security_group(context, tenant_id) + default_sg = self._ensure_default_security_group(context, + port['tenant_id']) if not attributes.is_attr_set(port.get(ext_sg.SECURITYGROUPS)): port[ext_sg.SECURITYGROUPS] = [default_sg] diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 3648deb8404..24f653770a9 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -616,7 +616,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, def _create_network_db(self, context, network): net_data = network[attributes.NETWORK] - tenant_id = self._get_tenant_id_for_create(context, net_data) + tenant_id = net_data['tenant_id'] session = context.session with session.begin(subtransactions=True): self._ensure_default_security_group(context, tenant_id) diff --git a/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py b/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py index 45d22b6d446..2f94b8d0dc3 100644 --- a/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py +++ b/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py @@ -47,7 +47,8 @@ class L3SchedulerBaseTest(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): return agent def _create_router(self, name): - router = {'name': name, 'admin_state_up': True} + router = {'name': name, 'admin_state_up': True, + 'tenant_id': self.adminContext.tenant_id} return self.l3_plugin.create_router( self.adminContext, {'router': router}) @@ -304,7 +305,8 @@ class L3AZSchedulerBaseTest(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): def _create_router(self, az_hints, ha): router = {'name': 'router1', 'admin_state_up': True, - 'availability_zone_hints': az_hints} + 'availability_zone_hints': az_hints, + 'tenant_id': self._tenant_id} if ha: router['ha'] = True return self.l3_plugin.create_router( diff --git a/neutron/tests/retargetable/client_fixtures.py b/neutron/tests/retargetable/client_fixtures.py index 6c4efb75072..4c4e9a2a795 100644 --- a/neutron/tests/retargetable/client_fixtures.py +++ b/neutron/tests/retargetable/client_fixtures.py @@ -96,6 +96,7 @@ class PluginClientFixture(AbstractClientFixture): # framework kwargs.setdefault('admin_state_up', True) kwargs.setdefault('shared', False) + kwargs.setdefault('tenant_id', self.ctx.tenant_id) data = dict(network=kwargs) result = self.plugin.create_network(self.ctx, data) return base.AttributeDict(result) diff --git a/neutron/tests/unit/api/rpc/handlers/test_l3_rpc.py b/neutron/tests/unit/api/rpc/handlers/test_l3_rpc.py index 341436af045..eac6d97371f 100644 --- a/neutron/tests/unit/api/rpc/handlers/test_l3_rpc.py +++ b/neutron/tests/unit/api/rpc/handlers/test_l3_rpc.py @@ -37,11 +37,13 @@ class TestL3RpcCallback(testlib_api.SqlTestCase): def _prepare_network(self): network = {'network': {'name': 'abc', 'shared': False, + 'tenant_id': 'tenant_id', 'admin_state_up': True}} return self.plugin.create_network(self.ctx, network) def _prepare_ipv6_pd_subnet(self): subnet = {'subnet': {'network_id': self.network['id'], + 'tenant_id': 'tenant_id', 'cidr': None, 'ip_version': 6, 'name': 'ipv6_pd', diff --git a/neutron/tests/unit/api/v2/test_attributes.py b/neutron/tests/unit/api/v2/test_attributes.py index e83fbf84c38..c3324793c80 100644 --- a/neutron/tests/unit/api/v2/test_attributes.py +++ b/neutron/tests/unit/api/v2/test_attributes.py @@ -18,11 +18,15 @@ import testtools import mock import netaddr +import webob.exc + +from oslo_utils import uuidutils from neutron._i18n import _ from neutron.api.v2 import attributes from neutron.common import constants from neutron.common import exceptions as n_exc +from neutron import context from neutron.tests import base from neutron.tests import tools @@ -1009,3 +1013,32 @@ class TestResDict(base.BaseTestCase): attr_info, {'key': 1}, {'key': 1}) self.assertRaises(self._EXC_CLS, attributes.convert_value, attr_info, {'key': 1}, self._EXC_CLS) + + def test_populate_tenant_id(self): + tenant_id_1 = uuidutils.generate_uuid() + tenant_id_2 = uuidutils.generate_uuid() + # apart from the admin, nobody can create a res on behalf of another + # tenant + ctx = context.Context(user_id=None, tenant_id=tenant_id_1) + res_dict = {'tenant_id': tenant_id_2} + self.assertRaises(webob.exc.HTTPBadRequest, + attributes.populate_tenant_id, + ctx, res_dict, None, None) + ctx.is_admin = True + self.assertIsNone(attributes.populate_tenant_id(ctx, res_dict, + None, None)) + + # for each create request, the tenant_id should be added to the + # req body + res_dict2 = {} + attributes.populate_tenant_id(ctx, res_dict2, None, True) + self.assertEqual({'tenant_id': ctx.tenant_id}, res_dict2) + + # if the tenant_id is mandatory for the resource and not specified + # in the request nor in the context, an exception should be raised + res_dict3 = {} + attr_info = {'tenant_id': {'allow_post': True}, } + ctx.tenant_id = None + self.assertRaises(webob.exc.HTTPBadRequest, + attributes.populate_tenant_id, + ctx, res_dict3, attr_info, True) diff --git a/neutron/tests/unit/db/test_agentschedulers_db.py b/neutron/tests/unit/db/test_agentschedulers_db.py index 582794f099b..65d9ddb84e5 100644 --- a/neutron/tests/unit/db/test_agentschedulers_db.py +++ b/neutron/tests/unit/db/test_agentschedulers_db.py @@ -783,6 +783,7 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): self._set_net_external(net_id) router = {'name': 'router1', 'admin_state_up': True, + 'tenant_id': 'tenant_id', 'external_gateway_info': {'network_id': net_id}, 'distributed': True} r = self.l3plugin.create_router( @@ -1087,6 +1088,7 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): router = {'name': 'router1', 'external_gateway_info': {'network_id': net_id}, + 'tenant_id': 'tenant_id', 'admin_state_up': True, 'distributed': True} r = self.l3plugin.create_router(self.adminContext, @@ -1119,6 +1121,7 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): self._set_net_external(net_id) router = {'name': 'router1', + 'tenant_id': 'tenant_id', 'admin_state_up': True, 'distributed': True} r = self.l3plugin.create_router(self.adminContext, @@ -1158,6 +1161,7 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): router = {'name': 'router1', 'external_gateway_info': {'network_id': net_id}, + 'tenant_id': 'tenant_id', 'admin_state_up': True, 'distributed': True} r = self.l3plugin.create_router(self.adminContext, @@ -1186,6 +1190,7 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): router = {'name': 'router1', 'external_gateway_info': {'network_id': net_id}, + 'tenant_id': 'tenant_id', 'admin_state_up': True, 'distributed': True} r = self.l3plugin.create_router(self.adminContext, diff --git a/neutron/tests/unit/db/test_l3_hamode_db.py b/neutron/tests/unit/db/test_l3_hamode_db.py index 8e22257eeb2..c4a00094fa6 100644 --- a/neutron/tests/unit/db/test_l3_hamode_db.py +++ b/neutron/tests/unit/db/test_l3_hamode_db.py @@ -68,7 +68,9 @@ class L3HATestFramework(testlib_api.SqlTestCase): if ctx is None: ctx = self.admin_ctx ctx.tenant_id = tenant_id - router = {'name': 'router1', 'admin_state_up': True} + router = {'name': 'router1', + 'admin_state_up': True, + 'tenant_id': tenant_id} if ha is not None: router['ha'] = ha if distributed is not None: diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index 7d57c6b1214..b1b27326fe7 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -2471,6 +2471,7 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): plugin = manager.NeutronManager.get_service_plugins()[ service_constants.L3_ROUTER_NAT] router_req = {'router': {'id': _uuid(), 'name': 'router', + 'tenant_id': 'foo', 'admin_state_up': True}} result = plugin.create_router(context.Context('', 'foo'), router_req) self.assertEqual(result['id'], router_req['router']['id']) @@ -2976,6 +2977,7 @@ class L3NatDBTestCaseMixin(object): with self.network() as n: data = {'router': { 'name': 'router1', 'admin_state_up': True, + 'tenant_id': ctx.tenant_id, 'external_gateway_info': {'network_id': n['network']['id']}}} self.assertRaises(MyException, plugin.create_router, ctx, data) diff --git a/neutron/tests/unit/extensions/test_portsecurity.py b/neutron/tests/unit/extensions/test_portsecurity.py index 734ad8ca1b2..0179ca57521 100644 --- a/neutron/tests/unit/extensions/test_portsecurity.py +++ b/neutron/tests/unit/extensions/test_portsecurity.py @@ -60,7 +60,7 @@ class PortSecurityTestPlugin(db_base_plugin_v2.NeutronDbPluginV2, supported_extension_aliases = ["security-group", "port-security"] def create_network(self, context, network): - tenant_id = self._get_tenant_id_for_create(context, network['network']) + tenant_id = network['network'].get('tenant_id') self._ensure_default_security_group(context, tenant_id) with context.session.begin(subtransactions=True): neutron_db = super(PortSecurityTestPlugin, self).create_network( diff --git a/neutron/tests/unit/extensions/test_securitygroup.py b/neutron/tests/unit/extensions/test_securitygroup.py index c7195803b9d..e8fb5573429 100644 --- a/neutron/tests/unit/extensions/test_securitygroup.py +++ b/neutron/tests/unit/extensions/test_securitygroup.py @@ -177,7 +177,7 @@ class SecurityGroupTestPlugin(db_base_plugin_v2.NeutronDbPluginV2, supported_extension_aliases = ["security-group"] def create_port(self, context, port): - tenant_id = self._get_tenant_id_for_create(context, port['port']) + tenant_id = port['port']['tenant_id'] default_sg = self._ensure_default_security_group(context, tenant_id) if not attr.is_attr_set(port['port'].get(ext_sg.SECURITYGROUPS)): port['port'][ext_sg.SECURITYGROUPS] = [default_sg] @@ -207,8 +207,8 @@ class SecurityGroupTestPlugin(db_base_plugin_v2.NeutronDbPluginV2, return port def create_network(self, context, network): - tenant_id = self._get_tenant_id_for_create(context, network['network']) - self._ensure_default_security_group(context, tenant_id) + self._ensure_default_security_group(context, + network['network']['tenant_id']) return super(SecurityGroupTestPlugin, self).create_network(context, network) diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 2d13438acd4..1e3a22482c2 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -867,7 +867,8 @@ class TestMl2DvrPortsV2(TestMl2PortsV2): p_const.L3_ROUTER_NAT] r = plugin.create_router( self.context, - {'router': {'name': 'router', 'admin_state_up': True}}) + {'router': {'name': 'router', 'admin_state_up': True, + 'tenant_id': self.context.tenant_id}}) with self.subnet() as s: p = plugin.add_router_interface(self.context, r['id'], {'subnet_id': s['subnet']['id']}) diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py index 243d6361e57..b320d1a5fa9 100644 --- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py +++ b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py @@ -1797,7 +1797,8 @@ class L3HATestCaseMixin(testlib_api.SqlTestCase, def _create_ha_router(self, ha=True, tenant_id='tenant1', az_hints=None): self.adminContext.tenant_id = tenant_id - router = {'name': 'router1', 'admin_state_up': True} + router = {'name': 'router1', 'admin_state_up': True, + 'tenant_id': tenant_id} if ha is not None: router['ha'] = ha if az_hints is None: