Merge pull request #50 from stackhpc/upstream/yoga-2023-06-26

Synchronise yoga with upstream

Author: markgoddard

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

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}

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,11 +32,34 @@
 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():
     for object in OBJECTS:
         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)

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

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