Change RBAC relationship loading method to "joined"

This patch changes all RBAC relationship method to "joined". This change
enforces that the RBAC associated registers are loaded along with the
parent resource. The rationale of this change is to be able to control
the SQL query executed; the subquery cannot be directly managed by
Neutron.

It is very usual to create the RBAC rules from one single project that
is usually the adminitrator project. That means all RBAC rules will
belong to it. Before this change, the SQL subquery performed to
retrieve the RBAC entries was this (from a network query):

  SELECT networks.id AS networks_id
  FROM networks LEFT OUTER JOIN networkrbacs ON networks.id =
  networkrbacs.object_id
  WHERE networks.project_id = 'bd133e2c499c4bf8aeb16206e31c3c20'
    OR networkrbacs.action = 'access_as_external'
    AND networkrbacs.target_project = 'bd133e2c499c4bf8aeb16206e31c3c20'
    OR networkrbacs.target_project = '*'
    OR networks.project_id = 'bd133e2c499c4bf8aeb16206e31c3c20'
    OR networkrbacs.action IN ('access_as_shared', 'access_as_readonly')
    AND (networkrbacs.target_project = 'bd133e2c499c4bf8aeb16206e31c3c20'
    OR networkrbacs.target_project = '*');

This SQL result has a very low cardinality; that means there are many
duplicated registers. For example, with 10 external network, 1000
projects and 2500 RBAC rules, this query returns 1.4 million rows.
Instead if a "GROUP BY resource_id" (in this case network_id) clause is
added, the number of rows is reduced to 10 (considering this project
has a RBAC per network).

In order to introduce this "GROUP BY" clause, this patch is changing
the loading method. The clause is added in a neutron-lib patch [1].

This change by itself does not improve the query performance. The
neutron-lib patch is needed too. Although this patch does not modify
que SQL query results, the tests added will prove that the neutron-lib
patch does not introduce any regression.

[1]https://review.opendev.org/c/openstack/neutron-lib/+/884878

Closes-Bug: #1918145
Change-Id: Ic6001bd5a57493b8befdf81a41eb0bd1c8022df3
(cherry picked from commit e9da29d)

Author: ralonsoh

neutron/db/models/address_group.py

@@ -46,6 +46,6 @@ class AddressGroup(standard_attr.HasStandardAttributes,
                                  cascade='all, delete-orphan')
     rbac_entries = sa.orm.relationship(rbac_db_models.AddressGroupRBAC,
                                        backref='address_groups',
-                                       lazy='subquery',
+                                       lazy='joined',
                                        cascade='all, delete, delete-orphan')
     api_collections = [ag.ALIAS]

neutron/db/models/address_scope.py

@@ -37,5 +37,5 @@ class AddressScope(model_base.BASEV2, model_base.HasId, model_base.HasProject):
 
     rbac_entries = sa.orm.relationship(rbac_db_models.AddressScopeRBAC,
                                        backref='address_scopes',
-                                       lazy='subquery',
+                                       lazy='joined',
                                        cascade='all, delete, delete-orphan')

neutron/db/models/securitygroup.py

@@ -34,7 +34,7 @@ class SecurityGroup(standard_attr.HasStandardAttributes, model_base.BASEV2,
                          nullable=False)
     rbac_entries = sa.orm.relationship(rbac_db_models.SecurityGroupRBAC,
                                        backref='security_group',
-                                       lazy='subquery',
+                                       lazy='joined',
                                        cascade='all, delete, delete-orphan')
     api_collections = [sg.SECURITYGROUPS]
     collection_resource_map = {sg.SECURITYGROUPS: 'security_group'}

neutron/db/models_v2.py

@@ -228,7 +228,7 @@ class Subnet(standard_attr.HasStandardAttributes, model_base.BASEV2,
     # 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='subquery', uselist=True,
+        rbac_db_models.NetworkRBAC, lazy='joined', uselist=True,
         foreign_keys='Subnet.network_id',
         primaryjoin='Subnet.network_id==NetworkRBAC.object_id',
         viewonly=True)
@@ -282,7 +282,7 @@ class SubnetPool(standard_attr.HasStandardAttributes, model_base.BASEV2,
                                 lazy='subquery')
     rbac_entries = sa.orm.relationship(rbac_db_models.SubnetPoolRBAC,
                                        backref='subnetpools',
-                                       lazy='subquery',
+                                       lazy='joined',
                                        cascade='all, delete, delete-orphan')
     api_collections = [subnetpool_def.COLLECTION_NAME]
     collection_resource_map = {subnetpool_def.COLLECTION_NAME:
@@ -304,7 +304,7 @@ class Network(standard_attr.HasStandardAttributes, model_base.BASEV2,
     rbac_entries = orm.relationship(rbac_db_models.NetworkRBAC,
                                     backref=orm.backref('network',
                                                         load_on_pending=True),
-                                    lazy='subquery',
+                                    lazy='joined',
                                     cascade='all, delete, delete-orphan')
     availability_zone_hints = sa.Column(sa.String(255))
     mtu = sa.Column(sa.Integer, nullable=False,

neutron/db/qos/models.py

@@ -29,7 +29,7 @@ class QosPolicy(standard_attr.HasStandardAttributes, model_base.BASEV2,
     __tablename__ = 'qos_policies'
     name = sa.Column(sa.String(db_const.NAME_FIELD_SIZE))
     rbac_entries = sa.orm.relationship(rbac_db_models.QosPolicyRBAC,
-                                       backref='qos_policy', lazy='subquery',
+                                       backref='qos_policy', lazy='joined',
                                        cascade='all, delete, delete-orphan')
     api_collections = ['policies']
     collection_resource_map = {'policies': 'policy'}

neutron/tests/unit/objects/test_address_group.py

@@ -58,6 +58,7 @@ class AddressGroupRBACDbObjectTestCase(test_rbac.TestRBACObjectMixin,
                                        testlib_api.SqlTestCase):
 
     _test_class = address_group.AddressGroupRBAC
+    _parent_class = address_group.AddressGroup
 
     def setUp(self):
         super(AddressGroupRBACDbObjectTestCase, self).setUp()

neutron/tests/unit/objects/test_address_scope.py

@@ -36,6 +36,7 @@ class AddressScopeRBACDbObjectTestCase(test_rbac.TestRBACObjectMixin,
                                        testlib_api.SqlTestCase):
 
     _test_class = address_scope.AddressScopeRBAC
+    _parent_class = address_scope.AddressScope
 
     def setUp(self):
         super(AddressScopeRBACDbObjectTestCase, self).setUp()

neutron/tests/unit/objects/test_network.py

@@ -12,6 +12,8 @@
 
 from unittest import mock
 
+from neutron_lib.api.definitions import availability_zone as az_def
+
 from neutron.db import rbac_db_models
 from neutron.objects import base as obj_base
 from neutron.objects import network
@@ -27,6 +29,7 @@ class NetworkRBACDbObjectTestCase(test_rbac.TestRBACObjectMixin,
                                   testlib_api.SqlTestCase):
 
     _test_class = network.NetworkRBAC
+    _parent_class = network.Network
 
     def setUp(self):
         self._mock_get_valid_actions = mock.patch.object(
@@ -50,6 +53,13 @@ def test_object_version_degradation_1_1_to_1_0_no_id_no_project_id(self):
                          network_rbac_obj['versioned_object.data'])
         self.assertNotIn('id', network_rbac_obj['versioned_object.data'])
 
+    def _create_random_parent_object(self):
+        objclass_fields = self.get_random_db_fields(self._parent_class)
+        objclass_fields.pop(az_def.AZ_HINTS)
+        _obj = self._parent_class(self.context, **objclass_fields)
+        _obj.create()
+        return _obj
+
 
 class NetworkRBACIfaceOjectTestCase(test_rbac.TestRBACObjectMixin,
                                     obj_test_base.BaseObjectIfaceTestCase):

neutron/tests/unit/objects/test_rbac.py

@@ -9,10 +9,13 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-import random
 
+import random
 from unittest import mock
 
+from neutron_lib import context
+
+from neutron.db import rbac_db_models
 from neutron.objects import address_group
 from neutron.objects import address_scope
 from neutron.objects import network
@@ -26,6 +29,9 @@
 
 class TestRBACObjectMixin(object):
 
+    _test_class = None
+    _parent_class = None
+
     def get_random_object_fields(self, obj_cls=None):
         fields = (super(TestRBACObjectMixin, self).
                   get_random_object_fields(obj_cls))
@@ -34,6 +40,35 @@ def get_random_object_fields(self, obj_cls=None):
         fields['action'] = rnd_actions[idx]
         return fields
 
+    def _create_random_parent_object(self):
+        objclass_fields = self.get_random_db_fields(self._parent_class)
+        _obj = self._parent_class(self.context, **objclass_fields)
+        _obj.create()
+        return _obj
+
+    def test_rbac_shared_on_parent_object(self):
+        if not self._test_class or not self._parent_class:
+            self.skipTest('Mixin class, skipped test')
+        project_id = self.objs[0].project_id
+        _obj_shared = self._create_random_parent_object()
+        # Create a second object that won't be shared and thus won't be
+        # retrieved by the non-admin users.
+        self._create_random_parent_object()
+        for idx in range(3):
+            project = 'project_%s' % idx
+            rbac = self._test_class(
+                self.context, project_id=project_id, target_project=project,
+                action=rbac_db_models.ACCESS_SHARED,
+                object_id=_obj_shared.id)
+            rbac.create()
+
+        for idx in range(3):
+            project = 'project_%s' % idx
+            ctx_no_admin = context.Context(user_id='user', tenant_id=project,
+                                           is_admin=False)
+            objects = self._parent_class.get_objects(ctx_no_admin)
+            self.assertEqual([_obj_shared.id], [_obj.id for _obj in objects])
+
 
 class RBACBaseObjectTestCase(neutron_test_base.BaseTestCase):
 

neutron/tests/unit/objects/test_securitygroup.py

@@ -27,6 +27,7 @@ class SecurityGroupRBACDbObjectTestCase(test_rbac.TestRBACObjectMixin,
                                         testlib_api.SqlTestCase):
 
     _test_class = securitygroup.SecurityGroupRBAC
+    _parent_class = securitygroup.SecurityGroup
 
     def setUp(self):
         super(SecurityGroupRBACDbObjectTestCase, self).setUp()