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

feat(fiat) - Cache fetched LDAP roles to speed up role syncs. #1066

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

kirangodishala
Copy link
Contributor

  • BaseUserRolesProvider: New class to enable caching layer for concrete UserRolesProvider implementations.
  • LdapUserRolesProvider: Update class to consume caching layer.
  • When call for roles is initiated, now the local cache responds with cached roles else LDAP is contacted and the result gets cached.
  • This feature can be enabled by adding the below configuration:
auth:
  groupMembership:
    ldap:
      cache:
        enabled: true
        expireAfterWriteSeconds: 600        

* BaseUserRolesProvider: New class to enable caching layer for concrete UserRolesProvider implementations.
* LdapUserRolesProvider: Update class to consume caching layer.
@dbyron-sf
Copy link
Contributor

FWIW we've been running this for ~2 years at Salesforce.

@dbyron-sf
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 14, 2023

update

✅ Branch has been successfully updated

@mattgogerly
Copy link
Member

mattgogerly commented Jun 15, 2023

I don't use the LDAP provider, but I'm curious about the side effects of this. If user A is added to a new AD group with permissions on an application, does this mean they have to wait until cache expiry to start using it?

Alternatively, if a user is removed from an AD group, will they still have access until cache expiry?

@kirangodishala
Copy link
Contributor Author

I don't use the LDAP provider, but I'm curious about the side effects of this. If user A is added to a new AD group with permissions on an application, does this mean they have to wait until cache expiry to start using it?

@mattgogerly - No. Since the entry of user A is not present in the cache, the request goes to LDAP. Expiry is at the entry level.

Alternatively, if a user is removed from an AD group, will they still have access until cache expiry?

Yes, this is possible as with any caching mechanism (unless there is a way to invalidate explicitly).

@jasonmcintosh
Copy link
Member

#804 MIGHT have been of interest FYI

Ignoring that... this is minor and not requesting changes, just more of. "hey here's another custom cache system." Is there a reason NOT to use standard spring cache system? Just using @Cachable with the advantage of being able to pick the cache implementation.

@dbyron-sf
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 26, 2023

update

✅ Branch has been successfully updated

@dbyron-sf
Copy link
Contributor

this is minor and not requesting changes, just more of. "hey here's another custom cache system." Is there a reason NOT to use standard spring cache system? Just using @Cachable with the advantage of being able to pick the cache implementation.

You're right @jasonmcintosh. No doubt our ignorance when we implemented this. Since we've been running this code for awhile and have good confidence in it, I'm gonna go ahead and merge it and treat using something more standard as a future improvement.

@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Jun 26, 2023
@mergify mergify bot added the auto merged label Jun 26, 2023
@mergify mergify bot merged commit f286b68 into spinnaker:master Jun 26, 2023
5 checks passed
aman-agrawal pushed a commit to OpsMx/fiat-oes that referenced this pull request May 7, 2024
…ker#1066)

* BaseUserRolesProvider: New class to enable caching layer for concrete UserRolesProvider implementations.
* LdapUserRolesProvider: Update class to consume caching layer.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants