From e8cd39b3d75cbc10f69bb99aed449b751f067940 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Fri, 26 May 2023 17:48:57 +0200 Subject: [PATCH] Make DB migration creating indexes in RBACs conditional This patch makes conditional the existing DB migration that adds the new indexes "target_tenant" and "action" in the "*rbacs" tables. The rationale of this patch is to be able to manually improve older systems by just manually creating the indexes in the database. Once these indexes are added, those operations including RBACs checks (all these called from non-admin user to RBAC administrated resourced) will be improved. This patch is avoiding the migration issue a system could find if these indexes have been manually added and then the system is upgraded. The new check added will first retrieve the table indexes; if the index is already present, the index addition is skipped. Closes-Bug: #2020802 Change-Id: I1962fbc844bb67180e9071bcee01f8e95853bdda --- doc/source/admin/config-rbac.rst | 34 ++++++++++++ .../ba859d649675_add_indexes_to_rbacs.py | 26 ++++++++- .../test_ba859d649675_add_indexes_to_rbacs.py | 55 +++++++++++++++++++ 3 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 neutron/tests/functional/db/migrations/test_ba859d649675_add_indexes_to_rbacs.py diff --git a/doc/source/admin/config-rbac.rst b/doc/source/admin/config-rbac.rst index acf86b3c9ef..6701b0692e0 100644 --- a/doc/source/admin/config-rbac.rst +++ b/doc/source/admin/config-rbac.rst @@ -804,3 +804,37 @@ IDs. If an operator wants to prevent normal users from doing this, the ``"create_rbac_policy":`` entry in ``policy.yaml`` can be adjusted from ``""`` to ``"rule:admin_only"``. + + +Improve database RBAC query operations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Since [1]_, present in Yoga version, Neutron has indexes for +"target_tenant" (now "target_project") and "action" columns in all +RBAC related tables. That improves the SQL queries involving the +RBAC tables [2]_. Any system before Yoga won't have these indexes +but the system administrator can manually add them to the Neutron +database following the next steps: + +* Find the RBAC tables: + +.. code-block:: console + + $ tables=`mysql -e "use ovs_neutron; show tables;" | grep rbac` + + +* Insert the indexes for the "target_tenant" and "action" columns: + + $ for table in $tables do; mysql -e \ + "alter table $table add key (action); alter table $table add key (target_tenant);"; done + + +In order to prevent errors during a system upgrade, [3]_ was +implemented and backported up to Yoga. This patch checks if any index +is already present in the Neutron tables and avoids executing the +index creation command again. + + +.. [1] https://review.opendev.org/c/openstack/neutron/+/810072 +.. [2] https://github.com/openstack/neutron-lib/blob/890d62a3df3f35bb18bf1a11e79a9e97e7dd2d2c/neutron_lib/db/model_query.py#L123-L131 +.. [3] https://review.opendev.org/c/openstack/neutron/+/884617 diff --git a/neutron/db/migration/alembic_migrations/versions/yoga/expand/ba859d649675_add_indexes_to_rbacs.py b/neutron/db/migration/alembic_migrations/versions/yoga/expand/ba859d649675_add_indexes_to_rbacs.py index affd7e03fa0..cb38491c805 100644 --- a/neutron/db/migration/alembic_migrations/versions/yoga/expand/ba859d649675_add_indexes_to_rbacs.py +++ b/neutron/db/migration/alembic_migrations/versions/yoga/expand/ba859d649675_add_indexes_to_rbacs.py @@ -14,6 +14,7 @@ # from alembic import op +import sqlalchemy as sa """Add indexes to RBACs @@ -31,6 +32,27 @@ down_revision = 'c181bb1d89e4' OBJECTS = ('network', 'qospolicy', 'securitygroup', 'addressscope', 'subnetpool', 'addressgroup') COLUMNS = ('target_tenant', 'action') +_INSPECTOR = None + + +def get_inspector(): + global _INSPECTOR + if _INSPECTOR: + return _INSPECTOR + else: + _INSPECTOR = sa.inspect(op.get_bind()) + + return _INSPECTOR + + +def has_index(table, column): + """Check if the table has an index *using only* the column name provided""" + inspector = get_inspector() + table_indexes = inspector.get_indexes(table) + for index in table_indexes: + if [column] == index['column_names']: + return True + return False def upgrade(): @@ -38,4 +60,6 @@ def upgrade(): table = object + 'rbacs' ix = 'ix_' + table + '_' for column in COLUMNS: - op.create_index(op.f(ix + column), table, [column], unique=False) + if not has_index(table, column): + op.create_index(op.f(ix + column), table, [column], + unique=False) diff --git a/neutron/tests/functional/db/migrations/test_ba859d649675_add_indexes_to_rbacs.py b/neutron/tests/functional/db/migrations/test_ba859d649675_add_indexes_to_rbacs.py new file mode 100644 index 00000000000..4e3aa72d463 --- /dev/null +++ b/neutron/tests/functional/db/migrations/test_ba859d649675_add_indexes_to_rbacs.py @@ -0,0 +1,55 @@ +# Copyright 2017 OpenStack Foundation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +from oslo_db.sqlalchemy import utils as db_utils + +from neutron.db.migration.alembic_migrations.versions.yoga.expand import \ + ba859d649675_add_indexes_to_rbacs as _migration +from neutron.tests.functional.db import test_migrations + + +class TestAddIndexesToRbacsMixin(object): + """Validates binding_index for NetworkDhcpAgentBinding migration.""" + + @staticmethod + def get_index(table_indexes, column): + for index in table_indexes: + if [column] == index['column_names']: + return True + return False + + def _pre_upgrade_ba859d649675(self, engine): + for table in _migration.OBJECTS: + table_indexes = db_utils.get_indexes(engine, table + 'rbacs') + for column in _migration.COLUMNS: + self.assertFalse(self.get_index(table_indexes, column)) + + def _check_ba859d649675(self, engine, data): + for table in _migration.OBJECTS: + table_indexes = db_utils.get_indexes(engine, table + 'rbacs') + for column in _migration.COLUMNS: + self.assertTrue(self.get_index(table_indexes, column)) + + +class TestAddIndexesToRbacsMySQL( + TestAddIndexesToRbacsMixin, + test_migrations.TestWalkMigrationsMySQL): + pass + + +class TestAddIndexesToRbacsPostgreSQL( + TestAddIndexesToRbacsMixin, + test_migrations.TestWalkMigrationsPostgreSQL): + pass