From 32328de6e37e1f9f55d563f8a55087dc7d6f46e1 Mon Sep 17 00:00:00 2001 From: Ronald De Rose Date: Wed, 7 Sep 2016 23:51:09 +0000 Subject: [PATCH] Fixes password created_at errors due to the server_default Migration 002 sets the password created_at column to a TIMESTAMP type with a server_default=sql.func.now(). There are a couple problems that have been uncovered with this change: * We cannot guarantee that func.now() will generate a UTC timestamp. * For some older versions of MySQL, the default TIMESTAMP column will automatically be updated when other columns are updated: https://dev.mysql.com/doc/refman/5.5/en/timestamp-initialization.html This patch fixes the problem by recreating the password created_at column back to a DateTime type without a server_default: 1) Drop and recreate the created_at column 2) Update the created_at value 3) Set the created_at column as not nullable Closes-Bug: #1621200 Change-Id: Id5c607a777afb6565d66a336028eba796e3846b2 --- .../versions/004_reset_password_created_at.py | 37 +++++++++++++++++++ .../versions/004_reset_password_created_at.py | 15 ++++++++ .../versions/004_reset_password_created_at.py | 15 ++++++++ keystone/identity/backends/sql_model.py | 2 +- keystone/tests/unit/test_backend_sql.py | 2 +- .../tests/unit/test_sql_banned_operations.py | 25 +++++++++++-- keystone/tests/unit/test_sql_upgrade.py | 22 +++++++++++ 7 files changed, 113 insertions(+), 5 deletions(-) create mode 100644 keystone/common/sql/contract_repo/versions/004_reset_password_created_at.py create mode 100644 keystone/common/sql/data_migration_repo/versions/004_reset_password_created_at.py create mode 100644 keystone/common/sql/expand_repo/versions/004_reset_password_created_at.py diff --git a/keystone/common/sql/contract_repo/versions/004_reset_password_created_at.py b/keystone/common/sql/contract_repo/versions/004_reset_password_created_at.py new file mode 100644 index 0000000000..f453d135d8 --- /dev/null +++ b/keystone/common/sql/contract_repo/versions/004_reset_password_created_at.py @@ -0,0 +1,37 @@ +# 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 datetime + +import sqlalchemy as sql +import sqlalchemy.sql.expression as expression + + +def upgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + password = sql.Table('password', meta, autoload=True) + # reset created_at column + password.c.created_at.drop() + created_at = sql.Column('created_at', sql.DateTime(), + nullable=True, + default=datetime.datetime.utcnow) + password.create_column(created_at) + # update created_at value + now = datetime.datetime.utcnow() + values = {'created_at': now} + stmt = password.update().where( + password.c.created_at == expression.null()).values(values) + stmt.execute() + # set not nullable + password.c.created_at.alter(nullable=False) diff --git a/keystone/common/sql/data_migration_repo/versions/004_reset_password_created_at.py b/keystone/common/sql/data_migration_repo/versions/004_reset_password_created_at.py new file mode 100644 index 0000000000..9cb40b4541 --- /dev/null +++ b/keystone/common/sql/data_migration_repo/versions/004_reset_password_created_at.py @@ -0,0 +1,15 @@ +# 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. + + +def upgrade(migrate_engine): + pass diff --git a/keystone/common/sql/expand_repo/versions/004_reset_password_created_at.py b/keystone/common/sql/expand_repo/versions/004_reset_password_created_at.py new file mode 100644 index 0000000000..9cb40b4541 --- /dev/null +++ b/keystone/common/sql/expand_repo/versions/004_reset_password_created_at.py @@ -0,0 +1,15 @@ +# 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. + + +def upgrade(migrate_engine): + pass diff --git a/keystone/identity/backends/sql_model.py b/keystone/identity/backends/sql_model.py index 6854cf8ebf..5ef4ff5436 100644 --- a/keystone/identity/backends/sql_model.py +++ b/keystone/identity/backends/sql_model.py @@ -234,7 +234,7 @@ class Password(sql.ModelBase, sql.DictBase): ondelete='CASCADE')) password = sql.Column(sql.String(128), nullable=True) # created_at default set here to safe guard in case it gets missed - created_at = sql.Column(sql.TIMESTAMP, nullable=False, + created_at = sql.Column(sql.DateTime, nullable=False, default=datetime.datetime.utcnow) expires_at = sql.Column(sql.DateTime, nullable=True) self_service = sql.Column(sql.Boolean, default=False, nullable=False, diff --git a/keystone/tests/unit/test_backend_sql.py b/keystone/tests/unit/test_backend_sql.py index c4583f4aab..7a1e9805e2 100644 --- a/keystone/tests/unit/test_backend_sql.py +++ b/keystone/tests/unit/test_backend_sql.py @@ -152,7 +152,7 @@ class SqlModels(SqlTests): cols = (('id', sql.Integer, None), ('local_user_id', sql.Integer, None), ('password', sql.String, 128), - ('created_at', sql.TIMESTAMP, None), + ('created_at', sql.DateTime, None), ('expires_at', sql.DateTime, None), ('self_service', sql.Boolean, False)) self.assertExpectedSchema('password', cols) diff --git a/keystone/tests/unit/test_sql_banned_operations.py b/keystone/tests/unit/test_sql_banned_operations.py index 244eabd1dd..0553a54ec3 100644 --- a/keystone/tests/unit/test_sql_banned_operations.py +++ b/keystone/tests/unit/test_sql_banned_operations.py @@ -236,7 +236,12 @@ class TestKeystoneExpandSchemaMigrations( # to make `blob` nullable. This allows the triggers added in 003 to # catch writes when the `blob` attribute isn't populated. We do this so # that the triggers aren't aware of the encryption implementation. - 3 + 3, + # Migration 004 changes the password created_at column type, from + # timestamp to datetime and updates the initial value in the contract + # phase. Adding an exception here to pass expand banned tests, + # otherwise fails. + 4 ] def setUp(self): @@ -271,7 +276,12 @@ class TestKeystoneDataMigrations( # Migration 002 changes the column type, from datetime to timestamp in # the contract phase. Adding exception here to pass banned data # migration tests. Fails otherwise. - 2 + 2, + # Migration 004 changes the password created_at column type, from + # timestamp to datetime and updates the initial value in the contract + # phase. Adding an exception here to pass data migrations banned tests, + # otherwise fails. + 4 ] def setUp(self): @@ -315,7 +325,16 @@ class TestKeystoneContractSchemaMigrations( # Migration 002 changes the column type, from datetime to timestamp. # To do this, the column is first dropped and recreated. This should # not have any negative impact on a rolling upgrade deployment. - 2 + 2, + # Migration 004 changes the password created_at column type, from + # timestamp to datetime and updates the created_at value. This is + # likely not going to impact a rolling upgrade as the contract repo is + # executed once the code has been updated; thus the created_at column + # would be populated for any password changes. That being said, there + # could be a performance issue for existing large password tables, as + # the migration is not batched. However, it's a compromise and not + # likely going to be a problem for operators. + 4 ] def setUp(self): diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index 7e29518514..2238bf0c5a 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/keystone/tests/unit/test_sql_upgrade.py @@ -1767,6 +1767,28 @@ class FullMigration(SqlMigrateBase, unit.TestCase): 'type': 'cert'} self.insert_dict(session, credential_table_name, credential) + def test_migration_004_reset_password_created_at(self): + # upgrade each repository to 003 and test + self.expand(3) + self.migrate(3) + self.contract(3) + password = sqlalchemy.Table('password', self.metadata, autoload=True) + # postgresql returns 'TIMESTAMP WITHOUT TIME ZONE' + self.assertTrue( + str(password.c.created_at.type).startswith('TIMESTAMP')) + # upgrade each repository to 004 and test + self.expand(4) + self.migrate(4) + self.contract(4) + password = sqlalchemy.Table('password', self.metadata, autoload=True) + # type would still be TIMESTAMP with postgresql + if self.engine.name == 'postgresql': + self.assertTrue( + str(password.c.created_at.type).startswith('TIMESTAMP')) + else: + self.assertEqual('DATETIME', str(password.c.created_at.type)) + self.assertFalse(password.c.created_at.nullable) + class MySQLOpportunisticFullMigration(FullMigration): FIXTURE = test_base.MySQLOpportunisticFixture