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

diff --git a/neutron/db/common_db_mixin.py b/neutron/db/common_db_mixin.py
index 2a65420e5e49230323488efb570e03623cc0c74a..cf319cd3797c8279620d0011f8d5622cb8dc1006 100644 (file)
--- 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 51a483c0084bb2518736de9d4889bc2ab2cdd45a..9ecba83ba9c5c1a46de62403f02dc599a513337b 100644 (file)
--- 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 87bf8ebb91c86a0b543ac4d375bee0a7c02b6301..1979e25622a27abf797d944b247abfd629cdd6ff 100644 (file)
--- 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)