From d541ee97317b5830d1e057ecc7e58a5b5063c130 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Fri, 26 May 2023 17:48:57 +0200 Subject: [PATCH 1/2] 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 (cherry picked from commit e8cd39b3d75cbc10f69bb99aed449b751f067940) --- 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 @@ 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 From 3328be0994db663f07a2adb78e2a35e59869a68c Mon Sep 17 00:00:00 2001 From: Fernando Royo Date: Fri, 28 Apr 2023 16:16:27 +0200 Subject: [PATCH 2/2] Return 409 Conflict to tenant user deleting port attached to FIP When a tenant user try to delete a port that has attached a FIP by an admin user is getting a 500 ServerError. This patch improves the error to 409 Conflict doing some additionals checks on the delete_port method. New exception has been included locally, but will be removed as soon neutron-lib bumps to a newer release. Closes-Bug: 2017680 Change-Id: Iab77c64c03fd0d44ff7a3fc1c556d85a8c480bb9 (cherry picked from commit 9f6f6d5082b4341529144e992d5293675146ae88) --- neutron/db/l3_db.py | 24 +++++++++++++++++++++++- neutron/tests/unit/db/test_l3_db.py | 21 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index a5e038e8ee6..f135c68e85e 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -81,6 +81,13 @@ 'port: %(port_id)s.') +# TODO(froyo): Move this exception to neutron-lib as soon as possible, and when +# a new release is created and pointed to in the requirements remove this code. +class FipAssociated(n_exc.InUse): + message = _('Unable to complete the operation on port "%(port_id)s" ' + 'because the port still has an associated floating IP.') + + @registry.has_registry_receivers class L3_NAT_dbonly_mixin(l3.RouterPluginBase, base_services.WorkerBase, @@ -1766,12 +1773,27 @@ def disassociate_floatingips(self, context, port_id, do_notify=True): @return: set of router-ids that require notification updates """ with db_api.CONTEXT_WRITER.using(context): + # NOTE(froyo): Context is elevated to confirm the presence of at + # least one FIP associated to the port_id. Additional checks + # regarding the tenant's grants will be carried out in following + # lines. if not l3_obj.FloatingIP.objects_exist( - context, fixed_port_id=port_id): + context.elevated(), fixed_port_id=port_id): return [] floating_ip_objs = l3_obj.FloatingIP.get_objects( context, fixed_port_id=port_id) + + # NOTE(froyo): To ensure that a FIP assigned by an admin user + # cannot be disassociated by a tenant user, we raise exception to + # generate a 409 Conflict response message that prompts the tenant + # user to contact an admin, rather than a 500 error message. + if not context.is_admin: + floating_ip_objs_admin = l3_obj.FloatingIP.get_objects( + context.elevated(), fixed_port_id=port_id) + if floating_ip_objs_admin != floating_ip_objs: + raise FipAssociated(port_id=port_id) + router_ids = {fip.router_id for fip in floating_ip_objs} old_fips = {fip.id: self._make_floatingip_dict(fip) for fip in floating_ip_objs} diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index b45ff2374a2..8b54e8d4666 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -328,6 +328,27 @@ def test_prevent_l3_port_existing_floating_ip(self, gp): self.db.prevent_l3_port_deletion(ctx, None) + @mock.patch.object(l3_obj.FloatingIP, 'objects_exist') + @mock.patch.object(l3_obj.FloatingIP, 'get_objects') + def test_disassociate_floatingips_conflict_by_fip_attached(self, + get_objects, + objects_exist): + context_tenant = context.Context('tenant', 'tenant', is_admin=False) + objects_exist.return_value = True + get_objects.side_effect = [ + [], + [{'id': 'floating_ip1', 'port_id': 'port_id'}]] + self.assertRaises(l3_db.FipAssociated, + self.db.disassociate_floatingips, + context_tenant, + 'port_id') + objects_exist.assert_called_once_with( + mock.ANY, fixed_port_id='port_id') + expected_calls = [ + mock.call(context_tenant, fixed_port_id='port_id'), + mock.call(mock.ANY, fixed_port_id='port_id')] + get_objects.assert_has_calls(expected_calls) + @mock.patch.object(directory, 'get_plugin') def test_subscribe_address_scope_of_subnetpool(self, gp): l3_db.L3RpcNotifierMixin()