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

fillcache pkg: trigger cache update immediately #271

Merged
merged 7 commits into from
Dec 10, 2019

Conversation

Jusshersmith
Copy link
Contributor

Problem

The fillcache package is what we use with the Google (and soon to be Cognito) providers to keep group membership cached.

Once a request goes through to sso_auth to validate a users group membership, the ValidateGroup function looks in the local cache for the relevant groups:

for _, group := range allGroups {
memberSet, ok := p.GroupsCache.Get(group)
if !ok {
useGroupsResource = true
if started := p.GroupsCache.RefreshLoop(group); started {

If the group doesn't exist in the cache, it triggers a refresh of the cache which in turn kicks off a goroutine to keep the group cache up to date (based off an input TTL).

Right now, the triggered goroutine only fills the cache after the given TTL, not immediately. Any subsequent calls to ValidateGroupMembership between the first call, and the first tick of ttl won’t have the groups cached.

Solution

Break out the logic that the goroutine uses to fill the cache into a separate method, and call it once before the loop, and then upon each tick of ttl.

Going to look at updating this PR with some logging examples to better verify the bug + expected behaviour

Notes

  • There are few different ways we could implement this, but the last commit includes one that is perhaps most clear

@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #271 into master will increase coverage by 0.32%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   62.21%   62.54%   +0.32%     
==========================================
  Files          54       54              
  Lines        4200     4202       +2     
==========================================
+ Hits         2613     2628      +15     
+ Misses       1399     1385      -14     
- Partials      188      189       +1
Impacted Files Coverage Δ
internal/pkg/groups/mock_cache.go 0% <0%> (ø) ⬆️
internal/pkg/groups/fillcache.go 73.91% <76.47%> (+20.18%) ⬆️

@Jusshersmith Jusshersmith added the bug Something isn't working label Nov 21, 2019
@Jusshersmith Jusshersmith self-assigned this Nov 21, 2019
jphines
jphines previously approved these changes Nov 26, 2019
@Jusshersmith Jusshersmith force-pushed the jusshersmith-test-sso-fillcache-initial-cache branch from b67d6ec to 7165315 Compare December 9, 2019 17:09
@Jusshersmith
Copy link
Contributor Author

Now that #273 is merged, we're back and once again ✅!

@Jusshersmith Jusshersmith merged commit 7edfee7 into master Dec 10, 2019
@Jusshersmith Jusshersmith deleted the jusshersmith-test-sso-fillcache-initial-cache branch December 10, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants