diff options
| -rw-r--r-- | .zuul.yaml | 13 | ||||
| -rw-r--r-- | doc/source/admin/caching-layer.rst | 8 | ||||
| -rw-r--r-- | keystone/credential/backends/sql.py | 3 | ||||
| -rw-r--r-- | keystone/identity/backends/ldap/common.py | 34 | ||||
| -rw-r--r-- | keystone/identity/backends/ldap/core.py | 7 | ||||
| -rw-r--r-- | keystone/tests/unit/test_backend_ldap.py | 55 | ||||
| -rw-r--r-- | keystone/tests/unit/test_v3_credential.py | 34 | ||||
| -rw-r--r-- | releasenotes/notes/bug-1782922-db822fda486ac773.yaml | 10 | ||||
| -rw-r--r-- | releasenotes/notes/bug-1840291-35af1ac7ba06e166.yaml | 6 |
9 files changed, 155 insertions, 15 deletions
diff --git a/.zuul.yaml b/.zuul.yaml index 547d423f1..fa38264ea 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -68,6 +68,17 @@ tox_env: upgrade osa_test_repo: openstack/openstack-ansible-os_keystone +- job: + name: keystone-dsvm-ldap-domain-specific-driver + parent: devstack-tempest + vars: + devstack_localrc: + KEYSTONE_CLEAR_LDAP: 'yes' + LDAP_PASSWORD: 'nomoresecret' + USE_PYTHON3: True + devstack_services: + ldap: true + # Experimental - job: name: keystone-tox-patch_cover @@ -180,7 +191,7 @@ - keystoneclient-devstack-functional: voting: false irrelevant-files: *irrelevant-files - - legacy-tempest-dsvm-ldap-domain-specific-driver: + - keystone-dsvm-ldap-domain-specific-driver: voting: false irrelevant-files: &tempest-irrelevant-files - ^(test-|)requirements.txt$ diff --git a/doc/source/admin/caching-layer.rst b/doc/source/admin/caching-layer.rst index e54ebceee..419791073 100644 --- a/doc/source/admin/caching-layer.rst +++ b/doc/source/admin/caching-layer.rst @@ -147,18 +147,18 @@ will perform proper invalidations of the cached methods listed above. For more information about the different back ends (and configuration options), see: -- `dogpile.cache.memory <https://dogpilecache.readthedocs.io/en/latest/api.html#memory-backends>`__ +- `dogpile.cache.memory <https://dogpilecache.sqlalchemy.org/en/latest/api.html#memory-backends>`__ -- `dogpile.cache.memcached <https://dogpilecache.readthedocs.io/en/latest/api.html#memcached-backends>`__ +- `dogpile.cache.memcached <https://dogpilecache.sqlalchemy.org/en/latest/api.html#memcached-backends>`__ .. note:: The memory back end is not suitable for use in a production environment. -- `dogpile.cache.redis <https://dogpilecache.readthedocs.io/en/latest/api.html#redis-backends>`__ +- `dogpile.cache.redis <https://dogpilecache.sqlalchemy.org/en/latest/api.html#redis-backends>`__ -- `dogpile.cache.dbm <https://dogpilecache.readthedocs.io/en/latest/api.html#file-backends>`__ +- `dogpile.cache.dbm <https://dogpilecache.sqlalchemy.org/en/latest/api.html#file-backends>`__ Cache invalidation ------------------ diff --git a/keystone/credential/backends/sql.py b/keystone/credential/backends/sql.py index 144307161..e499e2a86 100644 --- a/keystone/credential/backends/sql.py +++ b/keystone/credential/backends/sql.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_db import api as oslo_db_api + from keystone.common import driver_hints from keystone.common import sql from keystone.credential.backends import base @@ -96,6 +98,7 @@ class Credential(base.CredentialDriverBase): query = query.filter_by(project_id=project_id) query.delete() + @oslo_db_api.wrap_db_retry(retry_on_deadlock=True) def delete_credentials_for_user(self, user_id): with sql.session_for_write() as session: query = session.query(CredentialModel) diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index e2f85a7ee..b9becea74 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1293,9 +1293,37 @@ class BaseLdap(object): else: return self._id_to_dn_string(object_id) - @staticmethod - def _dn_to_id(dn): - return ldap.dn.str2dn(dn)[0][0][1] + def _dn_to_id(self, dn): + # Check if the naming attribute in the DN is the same as keystone's + # configured 'id' attribute'. If so, extract the ID value from the DN + if self.id_attr == ldap.dn.str2dn(dn)[0][0][0].lower(): + return ldap.dn.str2dn(dn)[0][0][1] + else: + # The 'ID' attribute is NOT in the DN, so we need to perform an + # LDAP search to look it up from the user entry itself. + with self.get_connection() as conn: + search_result = conn.search_s(dn, ldap.SCOPE_BASE) + + if search_result: + try: + id_list = search_result[0][1][self.id_attr] + except KeyError: + message = ('ID attribute %(id_attr)s not found in LDAP ' + 'object %(dn)s.') % ({'id_attr': self.id_attr, + 'dn': search_result}) + LOG.warning(message) + raise exception.NotFound(message=message) + if len(id_list) > 1: + message = ('In order to keep backward compatibility, in ' + 'the case of multivalued ids, we are ' + 'returning the first id %(id_attr) in the ' + 'DN.') % ({'id_attr': id_list[0]}) + LOG.warning(message) + return id_list[0] + else: + message = _('DN attribute %(dn)s not found in LDAP') % ( + {'dn': dn}) + raise exception.NotFound(message=message) def _ldap_res_to_model(self, res): # LDAP attribute names may be returned in a different case than diff --git a/keystone/identity/backends/ldap/core.py b/keystone/identity/backends/ldap/core.py index b6b87185f..2afa730e9 100644 --- a/keystone/identity/backends/ldap/core.py +++ b/keystone/identity/backends/ldap/core.py @@ -311,8 +311,11 @@ class UserApi(common_ldap.EnabledEmuMixIn, common_ldap.BaseLdap): return obj def get_filtered(self, user_id): - user = self.get(user_id) - return self.filter_attributes(user) + try: + user = self.get(user_id) + return self.filter_attributes(user) + except ldap.NO_SUCH_OBJECT: + raise self.NotFound(user_id=user_id) def get_all(self, ldap_filter=None, hints=None): objs = super(UserApi, self).get_all(ldap_filter=ldap_filter, diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py index a13c718bc..aa7a50747 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -1819,7 +1819,33 @@ class LDAPIdentity(BaseLDAPIdentity): self.assertEqual(self.user_foo['email'], user_ref['id']) @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') - def test_get_id_from_dn_for_multivalued_attribute_id(self, mock_ldap_get): + def test_get_multivalued_attribute_id_from_dn(self, + mock_ldap_get): + driver = PROVIDERS.identity_api._select_identity_driver( + CONF.identity.default_domain_id) + driver.user.id_attr = 'mail' + + # make 'email' multivalued so we can test the error condition + email1 = uuid.uuid4().hex + email2 = uuid.uuid4().hex + # Mock the ldap search results to return user entries with + # user_name_attribute('sn') value has emptyspaces, emptystring + # and attibute itself is not set. + mock_ldap_get.return_value = ( + 'cn=users,dc=example,dc=com', + { + 'mail': [email1, email2], + } + ) + + # This is not a valid scenario, since we do not support multiple value + # attribute id on DN. + self.assertRaises(exception.NotFound, + PROVIDERS.identity_api.get_user, email1) + + @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') + def test_raise_not_found_dn_for_multivalued_attribute_id(self, + mock_ldap_get): driver = PROVIDERS.identity_api._select_identity_driver( CONF.identity.default_domain_id) driver.user.id_attr = 'mail' @@ -1836,10 +1862,29 @@ class LDAPIdentity(BaseLDAPIdentity): } ) - user_ref = PROVIDERS.identity_api.get_user(email1) - # make sure we get the ID from DN (old behavior) if the ID attribute - # has multiple values - self.assertEqual('nobodycares', user_ref['id']) + # This is not a valid scenario, since we do not support multiple value + # attribute id on DN. + self.assertRaises(exception.NotFound, + PROVIDERS.identity_api.get_user, email1) + + @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') + def test_get_id_not_in_dn(self, + mock_ldap_get): + driver = PROVIDERS.identity_api._select_identity_driver( + CONF.identity.default_domain_id) + driver.user.id_attr = 'sAMAccountName' + + user_id = uuid.uuid4().hex + mock_ldap_get.return_value = ( + 'cn=someuser,dc=example,dc=com', + { + 'cn': 'someuser', + 'sn': [uuid.uuid4().hex], + 'sAMAccountName': [user_id], + } + ) + user_ref = PROVIDERS.identity_api.get_user(user_id) + self.assertEqual(user_id, user_ref['id']) @mock.patch.object(common_ldap.BaseLdap, '_ldap_get') def test_id_attribute_not_found(self, mock_ldap_get): diff --git a/keystone/tests/unit/test_v3_credential.py b/keystone/tests/unit/test_v3_credential.py index b5e08249a..2538c3a10 100644 --- a/keystone/tests/unit/test_v3_credential.py +++ b/keystone/tests/unit/test_v3_credential.py @@ -17,6 +17,8 @@ import json import uuid from keystoneclient.contrib.ec2 import utils as ec2_utils +import mock +from oslo_db import exception as oslo_db_exception from six.moves import http_client from testtools import matchers @@ -262,6 +264,38 @@ class CredentialTestCase(CredentialBaseTestCase): '/credentials/%(credential_id)s' % { 'credential_id': self.credential['id']}) + def test_delete_credential_retries_on_deadlock(self): + patcher = mock.patch('sqlalchemy.orm.query.Query.delete', + autospec=True) + + class FakeDeadlock(object): + def __init__(self, mock_patcher): + self.deadlock_count = 2 + self.mock_patcher = mock_patcher + self.patched = True + + def __call__(self, *args, **kwargs): + if self.deadlock_count > 1: + self.deadlock_count -= 1 + else: + self.mock_patcher.stop() + self.patched = False + raise oslo_db_exception.DBDeadlock + + sql_delete_mock = patcher.start() + side_effect = FakeDeadlock(patcher) + sql_delete_mock.side_effect = side_effect + + try: + PROVIDERS.credential_api.delete_credentials_for_user( + user_id=self.user['id']) + finally: + if side_effect.patched: + patcher.stop() + + # initial attempt + 1 retry + self.assertEqual(sql_delete_mock.call_count, 2) + def test_create_ec2_credential(self): """Call ``POST /credentials`` for creating ec2 credential.""" blob, ref = unit.new_ec2_credential(user_id=self.user['id'], diff --git a/releasenotes/notes/bug-1782922-db822fda486ac773.yaml b/releasenotes/notes/bug-1782922-db822fda486ac773.yaml new file mode 100644 index 000000000..d8f5e26d7 --- /dev/null +++ b/releasenotes/notes/bug-1782922-db822fda486ac773.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + [`bug 1782922 <https://bugs.launchpad.net/keystone/+bug/1782922>`_] + Fixed the problem where Keystone indiscriminately return the first RDN + as the user ID, regardless whether it matches the configured + 'user_id_attribute' or not. This will break deployments where + 'group_members_are_ids' are set to False and 'user_id_attribute' is not + in the DN. This patch will perform a lookup by DN if the first RND does + not match the configured 'user_id_attribute'. diff --git a/releasenotes/notes/bug-1840291-35af1ac7ba06e166.yaml b/releasenotes/notes/bug-1840291-35af1ac7ba06e166.yaml new file mode 100644 index 000000000..142f8e0b5 --- /dev/null +++ b/releasenotes/notes/bug-1840291-35af1ac7ba06e166.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + [`bug 1840291 <https://bugs.launchpad.net/keystone/+bug/1840291>`_] + Adds retries for ``delete_credential_for_user`` method to avoid + DBDeadlocks when deleting large number of credentials concurrently. |