Skip to content

Ldap role mapping deadlock fix#24431

Merged
alexey-milovidov merged 4 commits intoClickHouse:masterfrom
traceon:ldap-role-mappind-deadlock-fix
May 31, 2021
Merged

Ldap role mapping deadlock fix#24431
alexey-milovidov merged 4 commits intoClickHouse:masterfrom
traceon:ldap-role-mappind-deadlock-fix

Conversation

@traceon
Copy link
Copy Markdown
Contributor

@traceon traceon commented May 23, 2021 •
edited
Loading

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
Copy Markdown
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)

@traceon traceon mentioned this pull request May 23, 2021
Closed
@alexey-milovidov alexey-milovidov self-assigned this May 24, 2021
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented May 25, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented May 25, 2021

Command update: success

Branch has been successfully updated

@vzakaznikov
Copy link
Copy Markdown
Contributor

vzakaznikov commented May 27, 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)

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
Copy Markdown
Member

alexey-milovidov commented May 27, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented May 27, 2021

Command update: success

Branch has been successfully updated

@alexey-milovidov alexey-milovidov merged commit e246016 into ClickHouse:master May 31, 2021
@traceon traceon deleted the ldap-role-mappind-deadlock-fix branch May 31, 2021 19:21
This was referenced May 31, 2021
Merged
Merged
Merged
Merged
Merged
robot-clickhouse pushed a commit that referenced this pull request May 31, 2021
Backport #24431 to 20.8: Ldap role mapping deadlock fix
773d727
@robot-clickhouse robot-clickhouse mentioned this pull request May 31, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request May 31, 2021
Backport #24431 to 21.5: Ldap role mapping deadlock fix
4b2c886
@robot-clickhouse robot-clickhouse mentioned this pull request May 31, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request May 31, 2021
Backport #24431 to 21.4: Ldap role mapping deadlock fix
ce57338
@robot-clickhouse robot-clickhouse mentioned this pull request May 31, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request May 31, 2021
Backport #24431 to 21.3: Ldap role mapping deadlock fix
c0e49b4
@robot-clickhouse robot-clickhouse mentioned this pull request May 31, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request May 31, 2021
Backport #24431 to 21.6: Ldap role mapping deadlock fix
d008393
@robot-clickhouse robot-clickhouse mentioned this pull request May 31, 2021
Merged
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
@alexey-milovidov
Merge pull request #24827 from ClickHouse/backport/21.6/24431
39210a3 
Backport #24431 to 21.6: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
@alexey-milovidov
Merge pull request #24826 from ClickHouse/backport/21.3/24431
64a30da 
Backport #24431 to 21.3: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
@alexey-milovidov
Merge pull request #24824 from ClickHouse/backport/21.5/24431
98c62cf 
Backport #24431 to 21.5: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
@alexey-milovidov
Merge pull request #24823 from ClickHouse/backport/20.8/24431
5064fa4 
Backport #24431 to 20.8: Ldap role mapping deadlock fix
alexey-milovidov added a commit that referenced this pull request Jun 1, 2021
@alexey-milovidov
Merge pull request #24825 from ClickHouse/backport/21.4/24431
bc0320e 
Backport #24431 to 21.4: Ldap role mapping deadlock fix
@vzakaznikov
Copy link
Copy Markdown
Contributor

vzakaznikov commented Jun 14, 2021

@traceon, see 33f270c for the new test.

@jmaicher jmaicher mentioned this pull request Sep 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@traceon @alexey-milovidov @vzakaznikov @filimonov @robot-clickhouse