-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add redis lock feature #1063
Add redis lock feature #1063
Conversation
Update Fork
Merge Changes from Base Repo
Update Repo from origin
Update master branch
This reverts commit 408ba1a.
pkg/sessions/redis/client.go
Outdated
return c.Client.Get(ctx, key).Bytes() | ||
} | ||
|
||
func (c *client) Lock(ctx context.Context, key string, expiration time.Duration) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like redislock
doesn't have a way to Peek
at whether a lock is already taken without trying to obtain it ourselves? I really only see Obtain
exposed.
That functionality would be nice for us to block and wait for a lock to be done without trying to take it (so we can then pull a fresh session from the store).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to avoid a long queue of requests waiting for the session refresh lock to be done - only to have to all lock/unlock themselves to pull the fresh session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, unfortunately, they are not supporting that right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I solved it with just doing retries with Load, if LoadWithLock failed once. What do you think. This is not yet tested, just an idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't have a way to Peek at whether a lock is already taken
Is this something we could implement on top of it possibly?
I want to avoid a long queue of requests waiting for the session refresh lock to be done - only to have to all lock/unlock themselves to pull the fresh session.
Lock/unlock should be pretty quick once we know that the session has been revalidated right? What I don't want to do is overcomplicate the original implementation by having strict requirements that may not be 100% necessary. This PR is quite large, I wonder if we can break it into steps and get a minimal implementation that may not be polished, and iterate to make it more efficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna try to spike out an implementation of the proactive refreshing that is tied to this -- that being separate might make us able to fully focus on the locking functionality in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickMeves: Great, let me know, when this is done and I can continue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Draft is up here: #1086
Once I get some feedback on the design choices from @JoelSpeed I'll wrap it up. It's admittedly raw since I did it from my couch while watching TV 😅
@JoelSpeed & @NickMeves: Are there still changes that need to be done? I would like to merge this PR, if possible. 😊 |
I'll need to defer to @JoelSpeed on getting this one across the finish line. I just started at a new job so I won't have much time for any deep OAuth2 Proxy work or code reviews for a few weeks. Saving all my mental stamina for all the new stuff I need to learn during onboarding 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty good to go IMO. Would like to see some documentation on the interface so we know what the expectations are for future implementors. And there's one comment still outstanding that I had from march around whether we should use a suffix for the lock, this seems to be more common from what I've seen.
Otherwise looks great! Appreciate all the work you've put into this @Bibob7
pkg/sessions/redis/lock.go
Outdated
func (l *Lock) lockKey() string { | ||
return fmt.Sprintf("%s.%s", LockPrefix, l.key) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This question is still outstanding
What do you say, is this ready to merge? 😊 @JoelSpeed @NickMeves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go now, if there are any further issues, let's merge them in follow ups
Thanks for your patience @Bibob7, I think we've got to a state of high quality code for the addition of this feature!
* Add sensible logging flag to default setup for logger * Add Redis lock * Fix default value flag for sensitive logging * Split RefreshSessionIfNeeded in two methods and use Redis lock * Small adjustments to doc and code * Remove sensible logging * Fix method names in ticket.go * Revert "Fix method names in ticket.go" This reverts commit 408ba1a. * Fix methods name in ticket.go * Remove block in Redis client get * Increase lock time to 1 second * Perform retries, if session store is locked * Reverse if condition, because it should return if session does not have to be refreshed * Update go.sum * Update MockStore * Return error if loading session fails * Fix and update tests * Change validSession to session in docs and strings * Change validSession to session in docs and strings * Fix docs * Fix wrong field name * Fix linting * Fix imports for linting * Revert changes except from locking functionality * Add lock feature on session state * Update from master * Remove errors package, because it is not used * Only pass context instead of request to lock * Use lock key * By default use NoOpLock * Remove debug output * Update ticket_test.go * Map internal error to sessions error * Add ErrLockNotObtained * Enable lock peek for all redis clients * Use lock key prefix consistent * Fix imports * Use exists method for peek lock * Fix imports * Fix imports * Fix imports * Remove own Dockerfile * Fix imports * Fix tests for ticket and session store * Fix session store test * Update pkg/apis/sessions/interfaces.go Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Do not wrap lock method Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Use errors package for lock constants * Use better naming for initLock function * Add comments * Add session store lock test * Fix tests * Fix tests * Fix tests * Fix tests * Add cookies after saving session * Add mock lock * Fix imports for mock_lock.go * Store mock lock for key * Apply elapsed time on mock lock * Check if lock is initially applied * Reuse existing lock * Test all lock methods * Update CHANGELOG.md * Use redis client methods in redis.lock for release an refresh * Use lock key suffix instead of prefix for lock key * Add comments for Lock interface * Update comment for Lock interface * Update CHANGELOG.md * Change LockSuffix to const * Check lock on already loaded session * Use global var for loadedSession in lock tests * Use lock instance for refreshing and releasing of lock * Update possible error type for Refresh Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
* Add sensible logging flag to default setup for logger * Add Redis lock * Fix default value flag for sensitive logging * Split RefreshSessionIfNeeded in two methods and use Redis lock * Small adjustments to doc and code * Remove sensible logging * Fix method names in ticket.go * Revert "Fix method names in ticket.go" This reverts commit 408ba1a. * Fix methods name in ticket.go * Remove block in Redis client get * Increase lock time to 1 second * Perform retries, if session store is locked * Reverse if condition, because it should return if session does not have to be refreshed * Update go.sum * Update MockStore * Return error if loading session fails * Fix and update tests * Change validSession to session in docs and strings * Change validSession to session in docs and strings * Fix docs * Fix wrong field name * Fix linting * Fix imports for linting * Revert changes except from locking functionality * Add lock feature on session state * Update from master * Remove errors package, because it is not used * Only pass context instead of request to lock * Use lock key * By default use NoOpLock * Remove debug output * Update ticket_test.go * Map internal error to sessions error * Add ErrLockNotObtained * Enable lock peek for all redis clients * Use lock key prefix consistent * Fix imports * Use exists method for peek lock * Fix imports * Fix imports * Fix imports * Remove own Dockerfile * Fix imports * Fix tests for ticket and session store * Fix session store test * Update pkg/apis/sessions/interfaces.go Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Do not wrap lock method Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Use errors package for lock constants * Use better naming for initLock function * Add comments * Add session store lock test * Fix tests * Fix tests * Fix tests * Fix tests * Add cookies after saving session * Add mock lock * Fix imports for mock_lock.go * Store mock lock for key * Apply elapsed time on mock lock * Check if lock is initially applied * Reuse existing lock * Test all lock methods * Update CHANGELOG.md * Use redis client methods in redis.lock for release an refresh * Use lock key suffix instead of prefix for lock key * Add comments for Lock interface * Update comment for Lock interface * Update CHANGELOG.md * Change LockSuffix to const * Check lock on already loaded session * Use global var for loadedSession in lock tests * Use lock instance for refreshing and releasing of lock * Update possible error type for Refresh Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
* Add sensible logging flag to default setup for logger * Add Redis lock * Fix default value flag for sensitive logging * Split RefreshSessionIfNeeded in two methods and use Redis lock * Small adjustments to doc and code * Remove sensible logging * Fix method names in ticket.go * Revert "Fix method names in ticket.go" This reverts commit 408ba1a. * Fix methods name in ticket.go * Remove block in Redis client get * Increase lock time to 1 second * Perform retries, if session store is locked * Reverse if condition, because it should return if session does not have to be refreshed * Update go.sum * Update MockStore * Return error if loading session fails * Fix and update tests * Change validSession to session in docs and strings * Change validSession to session in docs and strings * Fix docs * Fix wrong field name * Fix linting * Fix imports for linting * Revert changes except from locking functionality * Add lock feature on session state * Update from master * Remove errors package, because it is not used * Only pass context instead of request to lock * Use lock key * By default use NoOpLock * Remove debug output * Update ticket_test.go * Map internal error to sessions error * Add ErrLockNotObtained * Enable lock peek for all redis clients * Use lock key prefix consistent * Fix imports * Use exists method for peek lock * Fix imports * Fix imports * Fix imports * Remove own Dockerfile * Fix imports * Fix tests for ticket and session store * Fix session store test * Update pkg/apis/sessions/interfaces.go Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Do not wrap lock method Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Use errors package for lock constants * Use better naming for initLock function * Add comments * Add session store lock test * Fix tests * Fix tests * Fix tests * Fix tests * Add cookies after saving session * Add mock lock * Fix imports for mock_lock.go * Store mock lock for key * Apply elapsed time on mock lock * Check if lock is initially applied * Reuse existing lock * Test all lock methods * Update CHANGELOG.md * Use redis client methods in redis.lock for release an refresh * Use lock key suffix instead of prefix for lock key * Add comments for Lock interface * Update comment for Lock interface * Update CHANGELOG.md * Change LockSuffix to const * Check lock on already loaded session * Use global var for loadedSession in lock tests * Use lock instance for refreshing and releasing of lock * Update possible error type for Refresh Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
Description
Based on the discussion in my previous PR: #1031, I tried to implement the proposal.
Motivation and Context
If parallel requests hitting the oauth2-proxy with an expired session, oauth2-proxy will perform multiple requests to the token endpoint to refresh it.
Because of refresh token rotation, a refresh token can only be used once, so that the second request to the token endpoint will fail.
In this case, it is possible to repeat the retrieval of the session from the persistent storage, as the successful request must have already updated the session in the persistent storage.
Issue: #1006
How Has This Been Tested?
I used the implemented "Lock" and "Release" method from SessionState before and after refreshing the session. The Redis lock feature worked properly.
I also added tests in
pkg/sessions/tests/session_store_tests.go
Checklist: