Merge "Simplify join to rbac_entries for subnets"

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)

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):

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)