Merge "sql: Remove service_provider.relay_state_prefix default"

keystone/api/os_federation.py

@@ -353,8 +353,7 @@ class ServiceProvidersResource(_ResourceBase):
         validation.lazy_validate(schema.service_provider_create, sp)
         sp = self._normalize_dict(sp)
         sp.setdefault('enabled', False)
-        sp.setdefault('relay_state_prefix',
-                      CONF.saml.relay_state_prefix)
+        sp.setdefault('relay_state_prefix', CONF.saml.relay_state_prefix)
         sp_ref = PROVIDERS.federation_api.create_sp(sp_id, sp)
         return self.wrap_member(sp_ref), http.client.CREATED
 

keystone/common/sql/migrations/versions/EXPAND_HEAD

@@ -1 +1 @@
-b4f8b3f584e0
+11c3b243b4cb

keystone/common/sql/migrations/versions/bobcat/expand/11c3b243b4cb_remove_service_provider_relay_state_server_default.py

@@ -0,0 +1,34 @@
+# 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.
+
+"""Remove service_provider.relay_state_prefix server default.
+
+Revision ID: 11c3b243b4cb
+Revises: b4f8b3f584e0
+Create Date: 2023-07-03 12:03:21.649144
+"""
+
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '11c3b243b4cb'
+down_revision = 'b4f8b3f584e0'
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    with op.batch_alter_table('service_provider', schema=None) as batch_op:
+        batch_op.alter_column(
+            'relay_state_prefix',
+            server_default=None,
+        )

keystone/federation/backends/sql.py

@@ -12,7 +12,6 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import oslo_config.cfg
 from oslo_log import log
 from oslo_serialization import jsonutils
 from sqlalchemy import orm
@@ -26,17 +25,6 @@ from keystone.i18n import _
 CONF = keystone.conf.CONF
 LOG = log.getLogger(__name__)
 
-# FIXME(stephenfin): This is necessary to allow Sphinx to auto-generate
-# documentation. Sphinx's autogen extension doesn't/can't initialise
-# oslo.config and register options which means attempts to retrieve this value
-# fail. Using a configurable option for server_default feels like a bad idea
-# and we should probably remove server_default in favour of setting the value
-# in code.
-try:
-    service_provider_relay_state_prefix_default = CONF.saml.relay_state_prefix
-except oslo_config.cfg.NoSuchOptError:
-    service_provider_relay_state_prefix_default = 'ss:mem:'
-
 
 class FederationProtocolModel(sql.ModelBase, sql.ModelDictMixin):
     __tablename__ = 'federation_protocol'
@@ -169,11 +157,7 @@ class ServiceProviderModel(sql.ModelBase, sql.ModelDictMixin):
     description = sql.Column(sql.Text(), nullable=True)
     auth_url = sql.Column(sql.String(256), nullable=False)
     sp_url = sql.Column(sql.String(256), nullable=False)
-    relay_state_prefix = sql.Column(
-        sql.String(256),
-        nullable=False,
-        server_default=service_provider_relay_state_prefix_default,
-    )
+    relay_state_prefix = sql.Column(sql.String(256), nullable=False)
 
     @classmethod
     def from_dict(cls, dictionary):

keystone/tests/unit/test_sql_banned_operations.py

@@ -275,6 +275,32 @@ class KeystoneMigrationsWalk(
         indexes = inspector.get_indexes('project_tag')
         self.assertNotIn('project_id', {x['name'] for x in indexes})
 
+    def _pre_upgrade_11c3b243b4cb(self, connection):
+        inspector = sqlalchemy.inspect(connection)
+        columns = inspector.get_columns('service_provider')
+        found = False
+        for column in columns:
+            if column['name'] != 'relay_state_prefix':
+                continue
+
+            # The default should initially be set to the CONF value
+            self.assertIsNotNone(column['default'])
+            found = True
+        self.assertTrue(found, 'Failed to find column')
+
+    def _check_11c3b243b4cb(self, connection):
+        inspector = sqlalchemy.inspect(connection)
+        columns = inspector.get_columns('service_provider')
+        found = False
+        for column in columns:
+            if column['name'] != 'relay_state_prefix':
+                continue
+
+            # The default should now be unset
+            self.assertIsNone(column['default'])
+            found = True
+        self.assertTrue(found, 'Failed to find column')
+
     def test_single_base_revision(self):
         """Ensure we only have a single base revision.