Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ldap role mapping deadlock fix #24431

Merged

Conversation

traceon
Copy link
Contributor

@traceon traceon commented May 23, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category:

  • Bug Fix

Changelog entry:

  • Fixed the deadlock that can happen during LDAP role (re)mapping, when LDAP group is mapped to a nonexistent local role

Detailed description:

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label May 23, 2021
@traceon
Copy link
Contributor Author

traceon commented May 23, 2021

@vzakaznikov Should we add a test for this case?

  1. the ldap server is configured as a user directory in clickhouse
  2. typical role mapping is configured there, with a clickhouse_ prefix
  3. user1 user exists only in ldap
  4. role1 role exists in clickhouse
  5. unknown role does not exist in clickhouse
  6. user1 is a member of clickhouse_role1 group in ldap initially
  7. user1 logs in successfully (show grants shows only role1), then logs out
  8. user1 also becomes a member of clickhouse_unknown group in ldap (clickhouse server process is not restarted during this)
  9. user1 should be able to log in successfully again (with show grants still showing only role1)

@alexey-milovidov
Copy link
Member

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented May 25, 2021

Command update: success

Branch has been successfully updated

@vzakaznikov
Copy link
Contributor

@vzakaznikov Should we add a test for this case?

  1. the ldap server is configured as a user directory in clickhouse
  2. typical role mapping is configured there, with a clickhouse_ prefix
  3. user1 user exists only in ldap
  4. role1 role exists in clickhouse
  5. unknown role does not exist in clickhouse
  6. user1 is a member of clickhouse_role1 group in ldap initially
  7. user1 logs in successfully (show grants shows only role1), then logs out
  8. user1 also becomes a member of clickhouse_unknown group in ldap (clickhouse server process is not restarted during this)
  9. user1 should be able to log in successfully again (with show grants still showing only role1)

Yes, I can add an explicit test case for this. Also, we need to compare this scenario to xfailed test cases we already have. Right now LDAP tests are disabled in CI/CD, therefore you need to run them manually to check if there are no regressions.

@alexey-milovidov
Copy link
Member

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented May 27, 2021

Command update: success

Branch has been successfully updated

alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
Backport #24431 to 21.6: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
Backport #24431 to 21.3: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
Backport #24431 to 21.5: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
Backport #24431 to 20.8: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
Backport #24431 to 21.4: Ldap role mapping deadlock fix
@vzakaznikov
Copy link
Contributor

@traceon, see 33f270c for the new test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDAP: changing groups for user makes Clickhouse fail to log in
5 participants