From 14c47284f6c64f16ab1e9da442c6fb8317784fcb Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Fri, 18 Sep 2015 10:31:26 -0700 Subject: [PATCH] Simplify join to rbac_entries for subnets The subnet table was joined to the rbac table via an association proxy which required a bunch of hacking in the model_query setup to get it to work right. This patch simplifies it quiet a bit by just using a direct relationship between the subnets and networkrbacs tables joined via the subnet's networkid. It also unrolls an API test that was really difficult to debug because it was hard to tell which iteration it was on. Change-Id: I4da85218158aae624835b97053da9fbb6fb154ef --- neutron/db/common_db_mixin.py | 37 +++++-------------- neutron/db/models_v2.py | 8 +++- .../admin/test_shared_network_extension.py | 26 +++++++------ 3 files changed, 31 insertions(+), 40 deletions(-) diff --git a/neutron/db/common_db_mixin.py b/neutron/db/common_db_mixin.py index 2a65420e5..cf319cd37 100644 --- a/neutron/db/common_db_mixin.py +++ b/neutron/db/common_db_mixin.py @@ -129,9 +129,8 @@ class CommonDbMixin(object): query_filter = None if self.model_query_scope(context, model): if hasattr(model, 'rbac_entries'): - rbac_model, join_params = self._get_rbac_query_params( - model)[:2] - query = query.outerjoin(*join_params) + query = query.outerjoin(model.rbac_entries) + rbac_model = model.rbac_entries.property.mapper.class_ query_filter = ( (model.tenant_id == context.tenant_id) | ((rbac_model.action == 'access_as_shared') & @@ -184,27 +183,6 @@ class CommonDbMixin(object): query = self._model_query(context, model) return query.filter(model.id == id).one() - @staticmethod - def _get_rbac_query_params(model): - """Return the parameters required to query an model's RBAC entries. - - Returns a tuple of 3 containing: - 1. the relevant RBAC model for a given model - 2. the join parameters required to query the RBAC entries for the model - 3. the ID column of the passed in model that matches the object_id - in the rbac entries. - """ - try: - cls = model.rbac_entries.property.mapper.class_ - return (cls, (cls, ), model.id) - except AttributeError: - # an association proxy is being used (e.g. subnets - # depends on network's rbac entries) - rbac_model = (model.rbac_entries.target_class. - rbac_entries.property.mapper.class_) - return (rbac_model, model.rbac_entries.attr, - model.rbac_entries.remote_attr.class_.id) - def _apply_filters_to_query(self, query, model, filters, context=None): if filters: for key, value in six.iteritems(filters): @@ -222,9 +200,8 @@ class CommonDbMixin(object): elif key == 'shared' and hasattr(model, 'rbac_entries'): # translate a filter on shared into a query against the # object's rbac entries - rbac, join_params, oid_col = self._get_rbac_query_params( - model) - query = query.outerjoin(*join_params, aliased=True) + query = query.outerjoin(model.rbac_entries) + rbac = model.rbac_entries.property.mapper.class_ matches = [rbac.target_tenant == '*'] if context: matches.append(rbac.target_tenant == context.tenant_id) @@ -240,6 +217,12 @@ class CommonDbMixin(object): # because that will still give us a network shared to # our tenant (or wildcard) if it's shared to another # tenant. + # This is the column joining the table to rbac via + # the object_id. We can't just use model.id because + # subnets join on network.id so we have to inspect the + # relationship. + join_cols = model.rbac_entries.property.local_columns + oid_col = list(join_cols)[0] is_shared = ~oid_col.in_( query.session.query(rbac.object_id). filter(is_shared) diff --git a/neutron/db/models_v2.py b/neutron/db/models_v2.py index 51a483c00..9ecba83ba 100644 --- a/neutron/db/models_v2.py +++ b/neutron/db/models_v2.py @@ -14,7 +14,6 @@ # under the License. import sqlalchemy as sa -from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy import orm from neutron.api.v2 import attributes as attr @@ -205,7 +204,12 @@ class Subnet(model_base.BASEV2, HasId, HasTenant): constants.DHCPV6_STATEFUL, constants.DHCPV6_STATELESS, name='ipv6_address_modes'), nullable=True) - rbac_entries = association_proxy('networks', 'rbac_entries') + # subnets don't have their own rbac_entries, they just inherit from + # the network rbac entries + rbac_entries = orm.relationship( + rbac_db_models.NetworkRBAC, lazy='joined', + foreign_keys='Subnet.network_id', + primaryjoin='Subnet.network_id==NetworkRBAC.object_id') class SubnetPoolPrefix(model_base.BASEV2): diff --git a/neutron/tests/api/admin/test_shared_network_extension.py b/neutron/tests/api/admin/test_shared_network_extension.py index 87bf8ebb9..1979e2562 100644 --- a/neutron/tests/api/admin/test_shared_network_extension.py +++ b/neutron/tests/api/admin/test_shared_network_extension.py @@ -347,16 +347,20 @@ class RBACSharedNetworksTest(base.BaseAdminNetworkTest): def test_filtering_works_with_rbac_records_present(self): resp = self._make_admin_net_and_subnet_shared_to_tenant_id( self.client.tenant_id) - net = resp['network'] - sub = resp['subnet'] + net = resp['network']['id'] + sub = resp['subnet']['id'] self.admin_client.create_rbac_policy( - object_type='network', object_id=net['id'], + object_type='network', object_id=net, action='access_as_shared', target_tenant='*') - for state, assertion in ((False, self.assertNotIn), - (True, self.assertIn)): - nets = [n['id'] for n in - self.admin_client.list_networks(shared=state)['networks']] - assertion(net['id'], nets) - subs = [s['id'] for s in - self.admin_client.list_subnets(shared=state)['subnets']] - assertion(sub['id'], subs) + self._assert_shared_object_id_listing_presence('subnets', False, sub) + self._assert_shared_object_id_listing_presence('subnets', True, sub) + self._assert_shared_object_id_listing_presence('networks', False, net) + self._assert_shared_object_id_listing_presence('networks', True, net) + + def _assert_shared_object_id_listing_presence(self, resource, shared, oid): + lister = getattr(self.admin_client, 'list_%s' % resource) + objects = [o['id'] for o in lister(shared=shared)[resource]] + if shared: + self.assertIn(oid, objects) + else: + self.assertNotIn(oid, objects) -- 2.45.2