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