openstack
/
neutron
Code Issues Proposed changes

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
This commit is contained in:
Rodolfo Alonso Hernandez 2023-05-28 17:28:03 +02:00
parent 0de6a4d620
commit e9da29d16c
11 changed files with 57 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
neutron/db/models/address_group.py
View File

@ -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]

2
neutron/db/models/address_scope.py
View File

@ -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')

2
neutron/db/models/securitygroup.py
View File

@ -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'}

6
neutron/db/models_v2.py
View File

@ -251,7 +251,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)
@ -306,7 +306,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:
@ -328,7 +328,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,

2
neutron/db/qos/models.py
View File

@ -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'}

1
neutron/tests/unit/objects/test_address_group.py
View File

@ -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()

1
neutron/tests/unit/objects/test_address_scope.py
View File

@ -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()

10
neutron/tests/unit/objects/test_network.py
View File

@ -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 @@ class NetworkRBACDbObjectTestCase(test_rbac.TestRBACObjectMixin,
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):

37
neutron/tests/unit/objects/test_rbac.py
View File

@ -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 @@ from neutron.tests.unit.objects import test_base
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 @@ class TestRBACObjectMixin(object):
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):

1
neutron/tests/unit/objects/test_securitygroup.py
View File

@ -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()

1
neutron/tests/unit/objects/test_subnetpool.py
View File

@ -195,6 +195,7 @@ class SubnetPoolRBACDbObjectTestCase(test_rbac.TestRBACObjectMixin,
SubnetPoolTestMixin):
_test_class = subnetpool.SubnetPoolRBAC
_parent_class = subnetpool.SubnetPool
def setUp(self):
super(SubnetPoolRBACDbObjectTestCase, self).setUp()