git.iko.soy/typeable/oauth2-proxy
on master help sign in
Implement refresh relying on obtaining lock
commit54d42c58298aa2d041b51a67d3a50617bb7e7e7c authorJoel Speed <joel.speed@hotmail.co.uk> dateSun 5 Dec 2021, 23:56 (5 years ago) parente2c7ff6
2 files changed+209−277
compare with master
unifiedsplit
pkg/middleware/stored_session.go+79−92 pkg/middleware/stored_session_test.go+130−185
pkg/middleware/stored_session.go+79−92
@@ -15,8 +15,20 @@ import (
1515)
1616
1717const (
18− SessionLockExpireTime = 5 * time.Second
19− SessionLockPeekDelay = 50 * time.Millisecond
18+ // When attempting to obtain the lock, if it's not done before this timeout
19+ // then exit and fail the refresh attempt.
20+ // TODO: This should probably be configurable by the end user.
21+ sessionRefreshObtainTimeout = 5 * time.Second
22+
23+ // Maximum time allowed for a session refresh attempt.
24+ // If the refresh request isn't finished within this time, the lock will be
25+ // released.
26+ // TODO: This should probably be configurable by the end user.
27+ sessionRefreshLockDuration = 2 * time.Second
28+
29+ // How long to wait after failing to obtain the lock before trying again.
30+ // TODO: This should probably be configurable by the end user.
31+ sessionRefreshRetryPeriod = 10 * time.Millisecond
2032)
2133
2234// StoredSessionLoaderOptions contains all of the requirements to construct
@@ -113,47 +125,86 @@ func (s *storedSessionLoader) getValidatedSession(rw http.ResponseWriter, req *h
113125// is older than the refresh period.
114126// Success or fail, we will then validate the session.
115127func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) error {
116− if s.refreshPeriod <= time.Duration(0) || session.Age() < s.refreshPeriod {
128+ if !needsRefresh(s.refreshPeriod, session) {
117129 // Refresh is disabled or the session is not old enough, do nothing
118130 return nil
119131 }
120132
121− wasRefreshed, err := s.checkForConcurrentRefresh(session, req)
122− if err != nil {
123− return err
133+ var lockObtained bool
134+ ctx, cancel := context.WithTimeout(context.Background(), sessionRefreshObtainTimeout)
135+ defer cancel()
136+
137+ for !lockObtained {
138+ select {
139+ case <-ctx.Done():
140+ return errors.New("timeout obtaining session lock")
141+ default:
142+ err := session.ObtainLock(req.Context(), sessionRefreshLockDuration)
143+ if err != nil && !errors.Is(err, sessionsapi.ErrLockNotObtained) {
144+ return fmt.Errorf("error occurred while trying to obtain lock: %v", err)
145+ } else if errors.Is(err, sessionsapi.ErrLockNotObtained) {
146+ time.Sleep(sessionRefreshRetryPeriod)
147+ continue
148+ }
149+ // No error means we obtained the lock
150+ lockObtained = true
151+ }
124152 }
125153
126− // If session was already refreshed via a concurrent request locked skip refreshing,
127− // because the refreshed session is already loaded from storage
128− if !wasRefreshed {
129− logger.Printf("Refreshing session - User: %s; SessionAge: %s", session.User, session.Age())
130− err = s.refreshSession(rw, req, session)
131− if err != nil {
132− // If a preemptive refresh fails, we still keep the session
133− // if validateSession succeeds.
134− logger.Errorf("Unable to refresh session: %v", err)
154+ // The rest of this function is carried out under lock, but we must release it
155+ // wherever we exit from this function.
156+ defer func() {
157+ if session == nil {
158+ return
159+ }
160+ if err := session.ReleaseLock(req.Context()); err != nil {
161+ logger.Errorf("unable to release lock: %v", err)
135162 }
163+ }()
164+
165+ // Reload the session in case it was changed underneath us.
166+ freshSession, err := s.store.Load(req)
167+ if err != nil {
168+ return fmt.Errorf("could not load session: %v", err)
169+ }
170+ if freshSession == nil {
171+ return errors.New("session no longer exists, it may have been removed by another request")
172+ }
173+ // Restore the state of the fresh session into the original pointer.
174+ // This is important so that changes are passed up the to the parent scope.
175+ lock := session.Lock
176+ *session = *freshSession
177+
178+ // Ensure we maintain the session lock after we have refreshed the session.
179+ // Loading from the session store creates a new lock in the session.
180+ session.Lock = lock
181+
182+ if !needsRefresh(s.refreshPeriod, session) {
183+ // The session must have already been refreshed while we were waiting to
184+ // obtain the lock.
185+ return nil
186+ }
187+
188+ // We are holding the lock and the session needs a refresh
189+ logger.Printf("Refreshing session - User: %s; SessionAge: %s", session.User, session.Age())
190+ if err := s.refreshSession(rw, req, session); err != nil {
191+ // If a preemptive refresh fails, we still keep the session
192+ // if validateSession succeeds.
193+ logger.Errorf("Unable to refresh session: %v", err)
136194 }
137195
138196 // Validate all sessions after any Redeem/Refresh operation (fail or success)
139197 return s.validateSession(req.Context(), session)
140198}
141199
200+// needsRefresh determines whether we should attempt to refresh a session or not.
201+func needsRefresh(refreshPeriod time.Duration, session *sessionsapi.SessionState) bool {
202+ return refreshPeriod > time.Duration(0) && session.Age() > refreshPeriod
203+}
204+
142205// refreshSession attempts to refresh the session with the provider
143206// and will save the session if it was updated.
144207func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) error {
145− err := session.ObtainLock(req.Context(), SessionLockExpireTime)
146− if err != nil {
147− logger.Errorf("Unable to obtain lock: %v", err)
148− return s.handleObtainLockError(req, session)
149− }
150− defer func() {
151− err = session.ReleaseLock(req.Context())
152− if err != nil {
153− logger.Errorf("unable to release lock: %v", err)
154− }
155− }()
156−
157208 refreshed, err := s.sessionRefresher(req.Context(), session)
158209 if err != nil && !errors.Is(err, providers.ErrNotImplemented) {
159210 return fmt.Errorf("error refreshing tokens: %v", err)
@@ -182,75 +233,11 @@ func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.R
182233 err = s.store.Save(rw, req, session)
183234 if err != nil {
184235 logger.PrintAuthf(session.Email, req, logger.AuthError, "error saving session: %v", err)
185− err = fmt.Errorf("error saving session: %v", err)
236+ return fmt.Errorf("error saving session: %v", err)
186237 }
187− return err
188−}
189−
190−func (s *storedSessionLoader) handleObtainLockError(req *http.Request, session *sessionsapi.SessionState) error {
191− wasRefreshed, err := s.checkForConcurrentRefresh(session, req)
192− if err != nil {
193− logger.Errorf("Unable to wait for obtained lock: %v", err)
194− return err
195− }
196− if !wasRefreshed {
197− return errors.New("unable to obtain lock and session was also not refreshed via concurrent request")
198− }
199− return nil
200−}
201−
202−func (s *storedSessionLoader) updateSessionFromStore(req *http.Request, session *sessionsapi.SessionState) error {
203− sessionStored, err := s.store.Load(req)
204− if err != nil {
205− return fmt.Errorf("unable to load updated session from store: %v", err)
206− }
207−
208− if sessionStored == nil {
209− return fmt.Errorf("no session available to udpate from store")
210− }
211− *session = *sessionStored
212−
213238 return nil
214239}
215240
216−func (s *storedSessionLoader) waitForPossibleSessionLock(session *sessionsapi.SessionState, req *http.Request) (bool, error) {
217− var wasLocked bool
218− isLocked, err := session.PeekLock(req.Context())
219− for isLocked {
220− wasLocked = true
221− // delay next peek lock
222− time.Sleep(SessionLockPeekDelay)
223− isLocked, err = session.PeekLock(req.Context())
224− }
225−
226− if err != nil {
227− return false, err
228− }
229−
230− return wasLocked, nil
231−}
232−
233−// checkForConcurrentRefresh returns true if the session is already refreshed via a concurrent request.
234−func (s *storedSessionLoader) checkForConcurrentRefresh(session *sessionsapi.SessionState, req *http.Request) (bool, error) {
235− wasLocked, err := s.waitForPossibleSessionLock(session, req)
236− if err != nil {
237− return false, err
238− }
239−
240− refreshed := false
241− if wasLocked {
242− logger.Printf("Update session from store instead of refreshing")
243− err = s.updateSessionFromStore(req, session)
244− if err != nil {
245− logger.Errorf("Unable to update session from store: %v", err)
246− return false, err
247− }
248− refreshed = true
249− }
250−
251− return refreshed, nil
252−}
253−
254241// validateSession checks whether the session has expired and performs
255242// provider validation on the session.
256243// An error implies the session is not longer valid.
pkg/middleware/stored_session_test.go+130−185
@@ -18,97 +18,67 @@ import (
1818 . "github.com/onsi/gomega"
1919)
2020
21−type TestLock struct {
22− Locked bool
23− WasObtained bool
24− WasRefreshed bool
25− WasReleased bool
26− PeekedCount int
27− LockedOnPeekCount int
28− ObtainError error
29− PeekError error
30− RefreshError error
31− ReleaseError error
21+type testLock struct {
22+ locked bool
23+ obtainOnAttempt int
24+ obtainAttempts int
25+ obtainError error
3226}
3327
34−func (l *TestLock) Obtain(_ context.Context, _ time.Duration) error {
35− if l.ObtainError != nil {
36− return l.ObtainError
28+func (l *testLock) Obtain(_ context.Context, _ time.Duration) error {
29+ l.obtainAttempts++
30+ if l.obtainAttempts < l.obtainOnAttempt {
31+ return sessionsapi.ErrLockNotObtained
32+ }
33+ if l.obtainError != nil {
34+ return l.obtainError
3735 }
38− l.Locked = true
39− l.WasObtained = true
36+ l.locked = true
4037 return nil
4138}
4239
43−func (l *TestLock) Peek(_ context.Context) (bool, error) {
44− if l.PeekError != nil {
45− return false, l.PeekError
46− }
47− locked := l.Locked
48− l.Locked = false
49− l.PeekedCount++
50− // mainly used to test case when peek initially returns false,
51− // but when trying to obtain lock, it returns true.
52− if l.LockedOnPeekCount == l.PeekedCount {
53− return true, nil
54− }
55− return locked, nil
40+func (l *testLock) Peek(_ context.Context) (bool, error) {
41+ return l.locked, nil
5642}
5743
58−func (l *TestLock) Refresh(_ context.Context, _ time.Duration) error {
59− if l.RefreshError != nil {
60− return l.ReleaseError
61− }
62− l.WasRefreshed = true
44+func (l *testLock) Refresh(_ context.Context, _ time.Duration) error {
6345 return nil
6446}
6547
66−func (l *TestLock) Release(_ context.Context) error {
67− if l.ReleaseError != nil {
68− return l.ReleaseError
69− }
70− l.Locked = false
71− l.WasReleased = true
48+func (l *testLock) Release(_ context.Context) error {
49+ l.locked = false
7250 return nil
7351}
7452
75−type LockConc struct {
76− mu sync.Mutex
77− lock bool
78− disablePeek bool
53+type testLockConcurrent struct {
54+ mu sync.RWMutex
55+ locked bool
7956}
8057
81−func (l *LockConc) Obtain(_ context.Context, _ time.Duration) error {
58+func (l *testLockConcurrent) Obtain(_ context.Context, _ time.Duration) error {
8259 l.mu.Lock()
83− if l.lock {
84− l.mu.Unlock()
60+ defer l.mu.Unlock()
61+ if l.locked {
8562 return sessionsapi.ErrLockNotObtained
8663 }
87− l.lock = true
88− l.mu.Unlock()
64+ l.locked = true
8965 return nil
9066}
9167
92−func (l *LockConc) Peek(_ context.Context) (bool, error) {
93− var response bool
94− l.mu.Lock()
95− if l.disablePeek {
96− response = false
97− } else {
98− response = l.lock
99− }
100− l.mu.Unlock()
101− return response, nil
68+func (l *testLockConcurrent) Peek(_ context.Context) (bool, error) {
69+ l.mu.RLock()
70+ defer l.mu.RUnlock()
71+ return l.locked, nil
10272}
10373
104−func (l *LockConc) Refresh(_ context.Context, _ time.Duration) error {
74+func (l *testLockConcurrent) Refresh(_ context.Context, _ time.Duration) error {
10575 return nil
10676}
10777
108−func (l *LockConc) Release(_ context.Context) error {
78+func (l *testLockConcurrent) Release(_ context.Context) error {
10979 l.mu.Lock()
110− l.lock = false
111− l.mu.Unlock()
80+ defer l.mu.Unlock()
81+ l.locked = false
11282 return nil
11383}
11484
@@ -374,22 +344,29 @@ var _ = Describe("Stored Session Suite", func() {
374344
375345 DescribeTable("when serving concurrent requests",
376346 func(in storedSessionLoaderConcurrentTableInput) {
377− lockConc := &LockConc{}
347+ lockConc := &testLockConcurrent{}
348+
349+ lock := &sync.RWMutex{}
350+ existingSession := *in.existingSession // deep copy existingSession state
351+ existingSession.Lock = lockConc
352+ store := &fakeSessionStore{
353+ LoadFunc: func(req *http.Request) (*sessionsapi.SessionState, error) {
354+ lock.RLock()
355+ defer lock.RUnlock()
356+ session := existingSession
357+ return &session, nil
358+ },
359+ SaveFunc: func(_ http.ResponseWriter, _ *http.Request, session *sessionsapi.SessionState) error {
360+ lock.Lock()
361+ defer lock.Unlock()
362+ existingSession = *session
363+ return nil
364+ },
365+ }
378366
379367 refreshedChan := make(chan bool, in.numConcReqs)
380368 for i := 0; i < in.numConcReqs; i++ {
381369 go func(refreshedChan chan bool, lockConc sessionsapi.Lock) {
382− existingSession := *in.existingSession // deep copy existingSession state
383− existingSession.Lock = lockConc
384− store := &fakeSessionStore{
385− LoadFunc: func(req *http.Request) (*sessionsapi.SessionState, error) {
386− return &existingSession, nil
387− },
388− SaveFunc: func(http.ResponseWriter, *http.Request, *sessionsapi.SessionState) error {
389− return nil
390− },
391− }
392−
393370 scope := &middlewareapi.RequestScope{
394371 Session: nil,
395372 }
@@ -461,13 +438,13 @@ var _ = Describe("Stored Session Suite", func() {
461438
462439 Context("refreshSessionIfNeeded", func() {
463440 type refreshSessionIfNeededTableInput struct {
464− refreshPeriod time.Duration
465− sessionStored bool
466− session *sessionsapi.SessionState
467− expectedErr error
468− expectRefreshed bool
469− expectValidated bool
470− expectedLockState TestLock
441+ refreshPeriod time.Duration
442+ session *sessionsapi.SessionState
443+ concurrentSessionRefresh bool
444+ expectedErr error
445+ expectRefreshed bool
446+ expectValidated bool
447+ expectedLockObtained bool
471448 }
472449
473450 createdPast := time.Now().Add(-5 * time.Minute)
@@ -478,13 +455,23 @@ var _ = Describe("Stored Session Suite", func() {
478455 refreshed := false
479456 validated := false
480457
481− store := &fakeSessionStore{}
482− if in.sessionStored {
483− store = &fakeSessionStore{
484− LoadFunc: func(req *http.Request) (*sessionsapi.SessionState, error) {
485− return in.session, nil
486− },
487− }
458+ session := &sessionsapi.SessionState{}
459+ *session = *in.session
460+ if in.concurrentSessionRefresh {
461+ // Update the session that Load returns.
462+ // This simulates a concurrent refresh in the background.
463+ session.CreatedAt = &createdFuture
464+ }
465+ store := &fakeSessionStore{
466+ LoadFunc: func(req *http.Request) (*sessionsapi.SessionState, error) {
467+ // Loading the session from the provider creates a new lock
468+ session.Lock = &testLock{}
469+ return session, nil
470+ },
471+ SaveFunc: func(_ http.ResponseWriter, _ *http.Request, s *sessionsapi.SessionState) error {
472+ *session = *s
473+ return nil
474+ },
488475 }
489476
490477 s := &storedSessionLoader{
@@ -518,117 +505,90 @@ var _ = Describe("Stored Session Suite", func() {
518505 }
519506 Expect(refreshed).To(Equal(in.expectRefreshed))
520507 Expect(validated).To(Equal(in.expectValidated))
521− testLock, ok := in.session.Lock.(*TestLock)
508+ testLock, ok := in.session.Lock.(*testLock)
522509 Expect(ok).To(Equal(true))
523510
524− Expect(testLock).To(Equal(&in.expectedLockState))
511+ if in.expectedLockObtained {
512+ Expect(testLock.obtainAttempts).Should(BeNumerically(">", 0), "Expected at least one attempt at obtaining the session lock")
513+ }
514+ Expect(testLock.locked).To(BeFalse(), "Expected lock should always be released")
525515 },
526516 Entry("when the refresh period is 0, and the session does not need refreshing", refreshSessionIfNeededTableInput{
527517 refreshPeriod: time.Duration(0),
528518 session: &sessionsapi.SessionState{
529519 RefreshToken: refresh,
530520 CreatedAt: &createdFuture,
531− Lock: &TestLock{},
521+ Lock: &testLock{},
532522 },
533− expectedErr: nil,
534− expectRefreshed: false,
535− expectValidated: false,
536− expectedLockState: TestLock{},
523+ expectedErr: nil,
524+ expectRefreshed: false,
525+ expectValidated: false,
526+ expectedLockObtained: false,
537527 }),
538528 Entry("when the refresh period is 0, and the session needs refreshing", refreshSessionIfNeededTableInput{
539529 refreshPeriod: time.Duration(0),
540530 session: &sessionsapi.SessionState{
541531 RefreshToken: refresh,
542532 CreatedAt: &createdPast,
543− Lock: &TestLock{},
533+ Lock: &testLock{},
544534 },
545− expectedErr: nil,
546− expectRefreshed: false,
547− expectValidated: false,
548− expectedLockState: TestLock{},
535+ expectedErr: nil,
536+ expectRefreshed: false,
537+ expectValidated: false,
538+ expectedLockObtained: false,
549539 }),
550540 Entry("when the session does not need refreshing", refreshSessionIfNeededTableInput{
551541 refreshPeriod: 1 * time.Minute,
552542 session: &sessionsapi.SessionState{
553543 RefreshToken: refresh,
554544 CreatedAt: &createdFuture,
555− Lock: &TestLock{},
545+ Lock: &testLock{},
556546 },
557− expectedErr: nil,
558− expectRefreshed: false,
559− expectValidated: false,
560− expectedLockState: TestLock{},
547+ expectedErr: nil,
548+ expectRefreshed: false,
549+ expectValidated: false,
550+ expectedLockObtained: false,
561551 }),
562552 Entry("when the session is refreshed by the provider", refreshSessionIfNeededTableInput{
563553 refreshPeriod: 1 * time.Minute,
564554 session: &sessionsapi.SessionState{
565555 RefreshToken: refresh,
566556 CreatedAt: &createdPast,
567− Lock: &TestLock{},
568− },
569− expectedErr: nil,
570− expectRefreshed: true,
571− expectValidated: true,
572− expectedLockState: TestLock{
573− Locked: false,
574− WasObtained: true,
575− WasReleased: true,
576− PeekedCount: 1,
577− },
578− }),
579− Entry("when the session is locked and instead loaded from storage", refreshSessionIfNeededTableInput{
580− refreshPeriod: 1 * time.Minute,
581− session: &sessionsapi.SessionState{
582− RefreshToken: noRefresh,
583− CreatedAt: &createdPast,
584− Lock: &TestLock{
585− Locked: true,
586− },
587− },
588− sessionStored: true,
589− expectedErr: nil,
590− expectRefreshed: false,
591− expectValidated: true,
592− expectedLockState: TestLock{
593− Locked: false,
594− PeekedCount: 2,
557+ Lock: &testLock{},
595558 },
559+ expectedErr: nil,
560+ expectRefreshed: true,
561+ expectValidated: true,
562+ expectedLockObtained: true,
596563 }),
597564 Entry("when obtaining lock failed, but concurrent request refreshed", refreshSessionIfNeededTableInput{
598565 refreshPeriod: 1 * time.Minute,
599566 session: &sessionsapi.SessionState{
600567 RefreshToken: noRefresh,
601568 CreatedAt: &createdPast,
602− Lock: &TestLock{
603− ObtainError: errors.New("not able to obtain lock"),
604− LockedOnPeekCount: 2,
569+ Lock: &testLock{
570+ obtainOnAttempt: 4,
605571 },
606572 },
607− expectedErr: nil,
608− expectRefreshed: false,
609− expectValidated: true,
610− expectedLockState: TestLock{
611− PeekedCount: 3,
612− LockedOnPeekCount: 2,
613− ObtainError: errors.New("not able to obtain lock"),
614− },
573+ concurrentSessionRefresh: true,
574+ expectedErr: nil,
575+ expectRefreshed: false,
576+ expectValidated: false,
577+ expectedLockObtained: true,
615578 }),
616− Entry("when obtaining lock failed", refreshSessionIfNeededTableInput{
579+ Entry("when obtaining lock failed with a valid session", refreshSessionIfNeededTableInput{
617580 refreshPeriod: 1 * time.Minute,
618581 session: &sessionsapi.SessionState{
619582 RefreshToken: noRefresh,
620583 CreatedAt: &createdPast,
621− Lock: &TestLock{
622− ObtainError: errors.New("not able to obtain lock"),
584+ Lock: &testLock{
585+ obtainError: sessionsapi.ErrLockNotObtained,
623586 },
624587 },
625− expectedErr: nil,
626− expectRefreshed: false,
627− expectValidated: true,
628− expectedLockState: TestLock{
629− PeekedCount: 2,
630− ObtainError: errors.New("not able to obtain lock"),
631− },
588+ expectedErr: errors.New("timeout obtaining session lock"),
589+ expectRefreshed: false,
590+ expectValidated: false,
591+ expectedLockObtained: true,
632592 }),
633593 Entry("when the session is not refreshed by the provider", refreshSessionIfNeededTableInput{
634594 refreshPeriod: 1 * time.Minute,
@@ -636,34 +596,24 @@ var _ = Describe("Stored Session Suite", func() {
636596 RefreshToken: noRefresh,
637597 CreatedAt: &createdPast,
638598 ExpiresOn: &createdFuture,
639− Lock: &TestLock{},
640− },
641− expectedErr: nil,
642− expectRefreshed: true,
643− expectValidated: true,
644− expectedLockState: TestLock{
645− Locked: false,
646− WasObtained: true,
647− WasReleased: true,
648− PeekedCount: 1,
599+ Lock: &testLock{},
649600 },
601+ expectedErr: nil,
602+ expectRefreshed: true,
603+ expectValidated: true,
604+ expectedLockObtained: true,
650605 }),
651606 Entry("when the provider doesn't implement refresh", refreshSessionIfNeededTableInput{
652607 refreshPeriod: 1 * time.Minute,
653608 session: &sessionsapi.SessionState{
654609 RefreshToken: notImplemented,
655610 CreatedAt: &createdPast,
656− Lock: &TestLock{},
657− },
658− expectedErr: nil,
659− expectRefreshed: true,
660− expectValidated: true,
661− expectedLockState: TestLock{
662− Locked: false,
663− WasObtained: true,
664− WasReleased: true,
665− PeekedCount: 1,
611+ Lock: &testLock{},
666612 },
613+ expectedErr: nil,
614+ expectRefreshed: true,
615+ expectValidated: true,
616+ expectedLockObtained: true,
667617 }),
668618 Entry("when the session is not refreshed by the provider", refreshSessionIfNeededTableInput{
669619 refreshPeriod: 1 * time.Minute,
@@ -672,17 +622,12 @@ var _ = Describe("Stored Session Suite", func() {
672622 RefreshToken: noRefresh,
673623 CreatedAt: &createdPast,
674624 ExpiresOn: &createdFuture,
675− Lock: &TestLock{},
676− },
677− expectedErr: errors.New("session is invalid"),
678− expectRefreshed: true,
679− expectValidated: true,
680− expectedLockState: TestLock{
681− Locked: false,
682− WasObtained: true,
683− WasReleased: true,
684− PeekedCount: 1,
625+ Lock: &testLock{},
685626 },
627+ expectedErr: errors.New("session is invalid"),
628+ expectRefreshed: true,
629+ expectValidated: true,
630+ expectedLockObtained: true,
686631 }),
687632 )
688633 })