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

Add redis lock feature #1063

Merged
merged 106 commits into from
Jun 2, 2021
Merged

Add redis lock feature #1063

merged 106 commits into from
Jun 2, 2021

Conversation

Bibob7
Copy link
Contributor

@Bibob7 Bibob7 commented Feb 23, 2021

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:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@Bibob7 Bibob7 requested a review from a team as a code owner February 23, 2021 07:09
return c.Client.Get(ctx, key).Bytes()
}

func (c *client) Lock(ctx context.Context, key string, expiration time.Duration) error {
Copy link
Member

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).

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 😅

@Bibob7
Copy link
Contributor Author

Bibob7 commented May 11, 2021

@JoelSpeed & @NickMeves: Are there still changes that need to be done? I would like to merge this PR, if possible. 😊

@NickMeves
Copy link
Member

@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 😅

Copy link
Member

@JoelSpeed JoelSpeed left a 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

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/apis/sessions/interfaces.go Outdated Show resolved Hide resolved
Comment on lines 67 to 69
func (l *Lock) lockKey() string {
return fmt.Sprintf("%s.%s", LockPrefix, l.key)
}
Copy link
Member

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

@Bibob7
Copy link
Contributor Author

Bibob7 commented May 29, 2021

What do you say, is this ready to merge? 😊 @JoelSpeed @NickMeves
For the lock key, I also switched to suffix.

Copy link
Member

@JoelSpeed JoelSpeed left a 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!

@JoelSpeed JoelSpeed merged commit f648c54 into oauth2-proxy:master Jun 2, 2021
samirachoadi pushed a commit to samirachoadi/oauth2-proxy that referenced this pull request Jun 13, 2021
* 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>
goshlanguage pushed a commit to goshlanguage/oauth2-proxy that referenced this pull request Sep 20, 2021
* 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>
k-jell pushed a commit to liquidinvestigations/oauth2-proxy that referenced this pull request Apr 6, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants