This repository was archived by the owner on Dec 20, 2025. It is now read-only.
perf(auth/ldap): Use group to user mapping to role sync#573
Merged
mergify[bot] merged 7 commits intospinnaker:masterfrom Apr 3, 2020
kowshikRoy:fiat-ldap-role-sync
Merged
perf(auth/ldap): Use group to user mapping to role sync#573mergify[bot] merged 7 commits intospinnaker:masterfrom kowshikRoy:fiat-ldap-role-sync
mergify[bot] merged 7 commits intospinnaker:masterfrom
kowshikRoy:fiat-ldap-role-sync
Conversation
Currently fiat sync LDAP roles by each users synchronously by making API call to LDAP server. As the number of users grow, this could cause performace issue. Ldap server maintains the Group -> User mapping denoting which users are the part of particular group. So, instead of fetching roles for each users, we can fetch the whole Group->User mapping in one API call and perform the role sync. Trade-off: * As the response of LDAP will huge compared to previous approach. This could put load on network * For small groups, the previous approach could be faster. That's why we encourage people to define a threshold after which the group call should be made.
kowshikRoy
requested review from
ajordens,
cfieber and
robzienert
as code owners
evansb
suggested changes
Mar 4, 2020
kowshikRoy
changed the title
kowshikRoy
changed the title
Contributor
Author
|
@robzienert @ajordens @cfieber |
Contributor
|
this looks fine, but I'd like to see a test that exercises this behaviour in LdapUserRolesProviderSpec? |
added 2 commits
March 28, 2020 03:07
Contributor
Author
|
@cfieber added tests. Thanks. |
cfieber
approved these changes
Apr 3, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently fiat sync LDAP roles by each users synchronously by making API call
to LDAP server. As the number of users grow, this could cause performace
issue.
Ldap server maintains the Group -> User mapping denoting which users are
the part of particular group. So, instead of fetching roles for each users,
we can fetch the whole Group->User mapping in one API call and perform
the role sync.
Trade-off:
This could put load on network
encourage people to define a threshold after which the group call
should be made.
Performance Improvement: From 16 mins -> 14sec.
Linked issue:spinnaker/spinnaker#5508