diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index eed3496309..5a97c7fd5f 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -32,6 +32,10 @@ from keystone.identity.backends import sql_model as model CONF = keystone.conf.CONF +def _stale_data_exception_checker(exc): + return isinstance(exc, sqlalchemy.orm.exc.StaleDataError) + + class Identity(base.IdentityDriverBase): # NOTE(henry-nash): Override the __init__() method so as to take a # config parameter to enable sql to be used as a domain-specific driver. @@ -201,6 +205,10 @@ class Identity(base.IdentityDriverBase): return base.filter_user(user_ref.to_dict()) @sql.handle_conflicts(conflict_type='user') + # Explicitly retry on StaleDataErrors, which can happen if two clients + # update the same user's password and the second client has stale password + # information. + @oslo_db_api.wrap_db_retry(exception_checker=_stale_data_exception_checker) def update_user(self, user_id, user): with sql.session_for_write() as session: user_ref = self._get_user(session, user_id) diff --git a/keystone/tests/unit/test_backend_sql.py b/keystone/tests/unit/test_backend_sql.py index 2ec23c31ea..4db5c67865 100644 --- a/keystone/tests/unit/test_backend_sql.py +++ b/keystone/tests/unit/test_backend_sql.py @@ -15,9 +15,11 @@ import datetime import uuid +import fixtures import mock from oslo_db import exception as db_exception from oslo_db import options +from oslo_log import log from six.moves import range import sqlalchemy from sqlalchemy import exc @@ -860,6 +862,42 @@ class SqlIdentity(SqlTests, PROVIDERS.resource_api.check_project_depth, 2) + def test_update_user_with_stale_data_forces_retry(self): + # Capture log output so we know oslo.db attempted a retry + log_fixture = self.useFixture(fixtures.FakeLogger(level=log.DEBUG)) + + # Create a new user + user_dict = unit.new_user_ref( + domain_id=CONF.identity.default_domain_id) + new_user_dict = PROVIDERS.identity_api.create_user(user_dict) + + side_effects = [ + # Raise a StaleDataError simulating that another client has + # updated the user's password while this client's request was + # being processed + sqlalchemy.orm.exc.StaleDataError, + # The oslo.db library will retry the request, so the second + # time this method is called let's return a valid session + # object + sql.session_for_write() + ] + with mock.patch('keystone.common.sql.session_for_write') as m: + m.side_effect = side_effects + + # Update a user's attribute, the first attempt will fail but + # oslo.db will handle the exception and retry, the second attempt + # will succeed + new_user_dict['email'] = uuid.uuid4().hex + PROVIDERS.identity_api.update_user( + new_user_dict['id'], new_user_dict) + + # Make sure oslo.db retried the update by checking the log output + expected_log_message = ( + 'Performing DB retry for function keystone.identity.backends.' + 'sql.update_user' + ) + self.assertIn(expected_log_message, log_fixture.output) + class SqlTrust(SqlTests, trust_tests.TrustTests): diff --git a/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml b/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml new file mode 100644 index 0000000000..f8f2d7f9c8 --- /dev/null +++ b/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + [`bug 1885753 `_] + Keystone's SQL identity backend now retries update user requests to safely + handle stale data when two clients update a user at the same time.