From 9e81843719dab3c66d89adf100bb3e1d5c9cedca Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Mon, 28 Mar 2016 10:50:12 -0700 Subject: [PATCH] Correct `role_name` constraint dropping The `role_name` constraint was not properly dropped in some cases because the unique constraint was not consistently named. In all cases we must search for the constraint expected, not assume the name of the constraint will be consistent (especially from older installs that have been moved forward in releases). This change fixes migration 88, updates 96 (for backport to stable/mitaka), and creates migration 101 for anyone who has moved beyond the mitaka -> newton placeholders. This is being overly cautious, but specifically is to handle the case where someone performed the upgrade and manually fixed the migration resulting in duplicated constraints in the role_table. Co-Authored-By: "Matthew Thode" Change-Id: Ie0dc3d2449bace57d3e9323b281a2abd2ad0c983 closes-bug: #1562934 --- .../versions/088_domain_specific_roles.py | 31 ++++- .../versions/096_drop_role_name_constraint.py | 50 +++++++ .../migrate_repo/versions/096_placeholder.py | 18 --- .../versions/101_drop_role_name_constraint.py | 53 ++++++++ keystone/tests/unit/test_sql_upgrade.py | 123 ++++++++++++++++++ 5 files changed, 253 insertions(+), 22 deletions(-) create mode 100644 keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py delete mode 100644 keystone/common/sql/migrate_repo/versions/096_placeholder.py create mode 100644 keystone/common/sql/migrate_repo/versions/101_drop_role_name_constraint.py diff --git a/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py b/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py index 92f2b906dc..8b792dfa75 100644 --- a/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py +++ b/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py @@ -13,9 +13,10 @@ import migrate import sqlalchemy as sql -_ROLE_NAME_OLD_CONSTRAINT = 'ixu_role_name' + _ROLE_NAME_NEW_CONSTRAINT = 'ixu_role_name_domain_id' _ROLE_TABLE_NAME = 'role' +_ROLE_NAME_COLUMN_NAME = 'name' _DOMAIN_ID_COLUMN_NAME = 'domain_id' _NULL_DOMAIN_ID = '<>' @@ -27,10 +28,32 @@ def upgrade(migrate_engine): role_table = sql.Table(_ROLE_TABLE_NAME, meta, autoload=True) domain_id = sql.Column(_DOMAIN_ID_COLUMN_NAME, sql.String(64), nullable=False, server_default=_NULL_DOMAIN_ID) - role_table.create_column(domain_id) - migrate.UniqueConstraint(role_table.c.name, - name=_ROLE_NAME_OLD_CONSTRAINT).drop() + # NOTE(morganfainberg): the `role_name` unique constraint is not + # guaranteed to be a fixed name, such as 'ixu_role_name`, so we need to + # search for the correct constraint that only affects role_table.c.name + # and drop that constraint. + to_drop = None + if migrate_engine.name == 'mysql': + for c in role_table.indexes: + if (c.unique and len(c.columns) == 1 and + _ROLE_NAME_COLUMN_NAME in c.columns): + to_drop = c + break + else: + for c in role_table.constraints: + if len(c.columns) == 1 and _ROLE_NAME_COLUMN_NAME in c.columns: + to_drop = c + break + + if to_drop is not None: + migrate.UniqueConstraint(role_table.c.name, + name=to_drop.name).drop() + + # perform changes after constraint is dropped. + if 'domain_id' not in role_table.columns: + # Only create the column if it doesn't already exist. + role_table.create_column(domain_id) migrate.UniqueConstraint(role_table.c.name, role_table.c.domain_id, diff --git a/keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py b/keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py new file mode 100644 index 0000000000..0156de2172 --- /dev/null +++ b/keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py @@ -0,0 +1,50 @@ +# 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. + +import migrate +import sqlalchemy as sql + +_ROLE_TABLE_NAME = 'role' +_ROLE_NAME_COLUMN_NAME = 'name' + + +def upgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + role_table = sql.Table(_ROLE_TABLE_NAME, meta, autoload=True) + + # NOTE(morganfainberg): the `role_name` unique constraint is not + # guaranteed to be named 'ixu_role_name', so we need to search for the + # correct constraint that only affects role_table.c.name and drop + # that constraint. + # + # This is an idempotent change that reflects the fix to migration + # 88 if the role_name unique constraint was not named consistently and + # someone manually fixed the migrations / db without dropping the + # old constraint. + to_drop = None + if migrate_engine.name == 'mysql': + for c in role_table.indexes: + if (c.unique and len(c.columns) == 1 and + _ROLE_NAME_COLUMN_NAME in c.columns): + to_drop = c + break + else: + for c in role_table.constraints: + if len(c.columns) == 1 and _ROLE_NAME_COLUMN_NAME in c.columns: + to_drop = c + break + + if to_drop is not None: + migrate.UniqueConstraint(role_table.c.name, + name=to_drop.name).drop() diff --git a/keystone/common/sql/migrate_repo/versions/096_placeholder.py b/keystone/common/sql/migrate_repo/versions/096_placeholder.py deleted file mode 100644 index 295e0f45bf..0000000000 --- a/keystone/common/sql/migrate_repo/versions/096_placeholder.py +++ /dev/null @@ -1,18 +0,0 @@ -# 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. - -# This is a placeholder for Mitaka backports. Do not use this number for new -# Newton work. New Newton work starts after all the placeholders. - - -def upgrade(migrate_engine): - pass diff --git a/keystone/common/sql/migrate_repo/versions/101_drop_role_name_constraint.py b/keystone/common/sql/migrate_repo/versions/101_drop_role_name_constraint.py new file mode 100644 index 0000000000..ff90d441dd --- /dev/null +++ b/keystone/common/sql/migrate_repo/versions/101_drop_role_name_constraint.py @@ -0,0 +1,53 @@ +# 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. + +import migrate +import sqlalchemy as sql + +_ROLE_TABLE_NAME = 'role' +_ROLE_NAME_COLUMN_NAME = 'name' + + +def upgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + role_table = sql.Table(_ROLE_TABLE_NAME, meta, autoload=True) + + # NOTE(morganfainberg): the `role_name` unique constraint is not + # guaranteed to be named 'ixu_role_name', so we need to search for the + # correct constraint that only affects role_table.c.name and drop + # that constraint. + # + # This is an idempotent change that reflects the fix to migration + # 88 if the role_name unique constraint was not named consistently and + # someone manually fixed the migrations / db without dropping the + # old constraint. + # This is a copy of migration 96 to catch any/all deployments that + # are close to master. migration 96 will be backported to + # stable/mitaka. + to_drop = None + if migrate_engine.name == 'mysql': + for c in role_table.indexes: + if (c.unique and len(c.columns) == 1 and + _ROLE_NAME_COLUMN_NAME in c.columns): + to_drop = c + break + else: + for c in role_table.constraints: + if len(c.columns) == 1 and _ROLE_NAME_COLUMN_NAME in c.columns: + to_drop = c + break + + if to_drop is not None: + migrate.UniqueConstraint(role_table.c.name, + name=to_drop.name).drop() diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index cec4561de9..7e3d8c5c73 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/keystone/tests/unit/test_sql_upgrade.py @@ -32,6 +32,7 @@ WARNING:: import json import uuid +import migrate from migrate.versioning import api as versioning_api from migrate.versioning import repository import mock @@ -440,6 +441,11 @@ class SqlUpgradeTests(SqlMigrateBase): table = sqlalchemy.Table(table_name, meta, autoload=True) return index_name in [idx.name for idx in table.indexes] + def does_constraint_exist(self, table_name, constraint_name): + meta = sqlalchemy.MetaData(bind=self.engine) + table = sqlalchemy.Table(table_name, meta, autoload=True) + return constraint_name in [con.name for con in table.constraints] + def test_endpoint_policy_upgrade(self): self.assertTableDoesNotExist('policy_association') self.upgrade(81) @@ -1031,6 +1037,123 @@ class SqlUpgradeTests(SqlMigrateBase): # assert id column is an integer (after) self.assertIsInstance(revocation_event_table.c.id.type, sql.Integer) + def _add_unique_constraint_to_role_name(self, + constraint_name='ixu_role_name'): + meta = sqlalchemy.MetaData() + meta.bind = self.engine + role_table = sqlalchemy.Table('role', meta, autoload=True) + migrate.UniqueConstraint(role_table.c.name, + name=constraint_name).create() + + def _drop_unique_constraint_to_role_name(self, + constraint_name='ixu_role_name'): + role_table = sqlalchemy.Table('role', self.metadata, autoload=True) + migrate.UniqueConstraint(role_table.c.name, + name=constraint_name).drop() + + def test_migration_88_drops_unique_constraint(self): + self.upgrade(87) + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + 'ixu_role_name')) + self.upgrade(88) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_88_inconsistent_constraint_name(self): + self.upgrade(87) + self._drop_unique_constraint_to_role_name() + + constraint_name = uuid.uuid4().hex + self._add_unique_constraint_to_role_name( + constraint_name=constraint_name) + + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', constraint_name)) + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + constraint_name)) + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + self.upgrade(88) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', constraint_name)) + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + constraint_name)) + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_96(self): + self.upgrade(95) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + self.upgrade(96) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_96_constraint_exists(self): + self.upgrade(95) + self._add_unique_constraint_to_role_name() + + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + 'ixu_role_name')) + + self.upgrade(96) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_101(self): + self.upgrade(100) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + self.upgrade(101) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_101_constraint_exists(self): + self.upgrade(100) + self._add_unique_constraint_to_role_name() + + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + 'ixu_role_name')) + self.upgrade(101) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + class MySQLOpportunisticUpgradeTestCase(SqlUpgradeTests): FIXTURE = test_base.MySQLOpportunisticFixture