sql: Remove service_provider.relay_state_prefix default

We shouldn't specify a server default for a configurable option since it
means our initial database schema is not consistently reproducible.
Instead, we should specify the default at runtime. It turns out we
already do this and the server default was overkill. We can remove it.

Change-Id: I74e47a9ed986c7c3af19676ac65f4d290bcb4cc0
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

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.