Merge "Retry update_user when sqlalchemy raises StaleDataErrors" into stable/rocky

3 files changed, 51 insertions, 0 deletions

diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py
index fefbdf21f..2421fa901 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 e709cf204..a7b0a35bc 100644
--- a/keystone/tests/unit/test_backend_sql.py
+++ b/keystone/tests/unit/test_backend_sql.py
@@ -14,9 +14,11 @@
 
 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
@@ -750,6 +752,41 @@ 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'
+        )
+        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 000000000..f8f2d7f9c
--- /dev/null
+++ b/releasenotes/notes/bug-1885753-51df25f3ff1d9ae8.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    [`bug 1885753 <https://bugs.launchpad.net/keystone/+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.