fix: Pace requests to token server for new auth tokens #55

jorge-ibm · 2020-04-27T21:53:57Z

This PR updates the design for token refresh by decoupling the refresh logic from active requests to be consistent with the other cores.

ref: https://github.ibm.com/arf/planning-sdk-squad/issues/1550

jorge-ibm · 2020-04-27T21:56:59Z

core/cp4d_authenticator.go

 }

+var cp4dMuOne sync.Mutex


I decided to use locks instead of goroutines for achieving the synchronous design due to the simplicity. I created two mutexes because I couldn't find a straight forward answer on whether or not I can use the same lock in two places at once, though I don't think the two locks will ever be used at the same time the way the code is setup

jorge-ibm · 2020-04-27T21:59:14Z

core/cp4d_authenticator.go

-			return "", err
+	} else if authenticator.tokenData != nil && authenticator.needsRefresh() {
+		// If refresh needed, kick off a go routine in the background to get a new token
+		ch := make(chan error)


I created a goroutine to achieve the "multi-threading" we want. The select case below it should ensure that no other routine sits around and waits for the newly spawned routine to finish

jorge-ibm · 2020-04-27T22:29:31Z

core/cp4d_authenticator.go

+	}
+
+	// If the refresh time isn't set, then set it
+	if authenticator.tokenRefreshTime == 0 && &authenticator.tokenData.ExpiresIn != nil && &authenticator.tokenData.Expiration != nil {


@mkistler @padamstx - now that I'm looking at this again, should I always be setting the correct refresh time when we request a new token? Currently, it will only be set when you request the first token, and when you refresh the token where we push it forward 1 min.. I think this will result in more token requests than we'd like

Yes ... you should be updating tokenRefreshTime whenever you get a new token.

I realized that as I created the PR 🙂

mkistler

This is a good start but I think there are some issues to be worked out.

mkistler · 2020-04-28T02:41:02Z

core/cp4d_authenticator.go

-		authenticator.tokenData, err = newCp4dTokenData(tokenResponse)
-		if err != nil {
-			return "", err
+	} else if authenticator.tokenData != nil && authenticator.needsRefresh() {


I don't think you need to test authenticator.tokenData != nil here -- it should never be nil here because of the check at line 157.

mkistler · 2020-04-28T03:01:48Z

core/cp4d_authenticator.go

+			if err != nil {
+				return "", err
+			}
+		default:


Don't you need to update tokenRefreshTime here? Otherwise you'll continually refresh the token.

Yes I think I do need to re-calculate the refresh time when we get a new token. Not sure why I kept that out originally.. I think I was following the java core too much

In java, we don't pre-compute the refresh time... the Token object's "needsRefresh()" method will compute it on the fly if needed, and then determine whether or not the token needs to be refreshed.

mkistler · 2020-04-28T03:03:13Z

core/cp4d_authenticator.go

+		return nil
+	}
+
+	err := authenticator.getTokenData()


I think it is very dangerous to perform a network call while holding a mutex. We should find some other solution.

Thinking about this further ... I suppose the other languages must do something similar. Hmmm.

In the Java core, we do this inside the TokenRequestBasedAuthenticator's getToken() method:

if we have no token at all, or the current token has expired, then we invoke a blocking request (synchronized) to the token server

otherwise we have a valid token and we use that as-is, but if the token is within the refresh window (last 20% of lifetime I think), then we ALSO start a refresh in a separate thread.

Not sure what else we can do to improve on that

@mkistler - I think the only other thing I can think of to use in place of the mutex is a go channel ~~blocking go routine~~.. Though I remember reading that under hood it still uses a mutex. I'll try to find that text again

My concern here is ... what happens if the one token request we make "gets lost" -- not unheard of for a network request. Does the client-side time out after a while and simply fail the request ... allowing another to try the request again? If that's the case then this is probably fine.

whatever happens in the Java core would also happen here, so yeah I guess a timeout would occur. If there is some other more fault-tolerant pattern we should be using in the Node, Java, Go cores (at a minimum) to avoid this, then we'll need to put our thinking caps on and come up with a better design.

Agreed. I was being overly cautious. The timeout will cover us so this should be fine.

jorge-ibm · 2020-04-28T17:51:20Z

@mkistler @padamstx - I've pushed an amended commit that adds the missing calculation of the refreshTime everytime we get a new token. I've also moved back the property refreshTime to be a part of the iamTokenData and cp4dTokenData structs, respectfully.

As far as the concerns over using locks for network calls, it looks like by default we set a client side timeout of 30 seconds. I've added two new tests to simulate a client side timeout, TestIamGetTokenTimeoutError and a test to simulate a 504 from the server, TestIamGetTokenServerError.

Note: the client timeout doesn't cover the time spent sending a request rather the time spent reading/receiving.

mkistler

Looks good! 👍

I left some comments/suggestions on stylistic changes that I trust you will handle as appropriate.

mkistler · 2020-04-29T02:13:41Z

core/cp4d_authenticator.go

@@ -63,6 +64,9 @@ type CloudPakForDataAuthenticator struct {
 	tokenData *cp4dTokenData
 }

+var cp4dMuOne sync.Mutex
+var cp4dMuTwo sync.Mutex


I think it would be better to use meaningful names for these mutexes -- to be clear on what they lock/control.

I recommend:

cp4dMuOne -> cp4dRequestTokenMutex

cp4dMuTwo -> cp4dNeedsRefreshMutex

mkistler · 2020-04-29T02:14:38Z

core/iam_authenticator.go

@@ -77,10 +78,12 @@ type IamAuthenticator struct {
 	tokenData *iamTokenData
 }

+var iamMuOne sync.Mutex
+var iamMuTwo sync.Mutex


Here too ... please use meaningful names for these mutexes.

mkistler · 2020-04-29T02:18:10Z

core/iam_authenticator.go

 		}
 	}

+	// return an error if the access token is not valid or was not fetched
+	if authenticator.tokenData == nil || authenticator.tokenData.AccessToken == "" {


It's curious that here you check authenticator.tokenData.AccessToken == "" but above you check !authenticator.tokenData.isTokenValid(). Why the difference? Can't we make them both the same?

Just an oversight on my end. I'll go ahead and add that

So @mkistler I went ahead and tried to make this change, though it's important to noteisTokenValid also checks to see if the token is expired. I think it's a good extra check to have, though the issue is that for cp4d's unit tests, the expiration time is calculated using some jwt token, and not by a mock value from actual token response. The token used in those tests is expired, so all the tests using it would fail.. Getting the unit tests to work would require either a new token that would eventually also expire, changing the GetCurrentTime method to accept a mock clock, or changing how we set and calculate the expired time for a token. I just went ahead and reverted this change.

mkistler · 2020-04-29T02:21:26Z

core/iam_authenticator.go

+		return err
+	}
+
+	return nil


Couldn't the above sequence of 6 lines be expressed simply as:

return authenticator.getTokenData()

padamstx

Discussed a few things with Jorge on slack. LGTM!

## [3.3.1](v3.3.0...v3.3.1) (2020-04-30) ### Bug Fixes * Pace requests to token server for new auth tokens ([#55](#55)) ([578399b](578399b))

ibm-devx-automation · 2020-04-30T15:04:58Z

🎉 This PR is included in version 3.3.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

jorge-ibm requested review from mkistler and padamstx April 27, 2020 21:53

jorge-ibm commented Apr 27, 2020

View reviewed changes

mkistler suggested changes Apr 28, 2020

View reviewed changes

jorge-ibm force-pushed the token-exchange-fix branch 4 times, most recently from 399b740 to fa2f137 Compare April 28, 2020 17:51

jorge-ibm requested a review from mkistler April 28, 2020 17:51

mkistler approved these changes Apr 29, 2020

View reviewed changes

fix: Pace requests to token server for new auth tokens

2c4d670

jorge-ibm force-pushed the token-exchange-fix branch from fa2f137 to 2c4d670 Compare April 29, 2020 17:28

padamstx approved these changes Apr 30, 2020

View reviewed changes

jorge-ibm merged commit 578399b into master Apr 30, 2020

ibm-devx-automation pushed a commit that referenced this pull request Apr 30, 2020

chore(release): 3.3.1 release notes [skip ci]

5bad9fb

## [3.3.1](v3.3.0...v3.3.1) (2020-04-30) ### Bug Fixes * Pace requests to token server for new auth tokens ([#55](#55)) ([578399b](578399b))

ibm-devx-automation added the released label Apr 30, 2020

padamstx deleted the token-exchange-fix branch August 11, 2020 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Pace requests to token server for new auth tokens #55

fix: Pace requests to token server for new auth tokens #55

jorge-ibm commented Apr 27, 2020

jorge-ibm Apr 27, 2020 •

edited

Loading

jorge-ibm Apr 27, 2020

jorge-ibm Apr 27, 2020

mkistler Apr 28, 2020

jorge-ibm Apr 28, 2020

mkistler left a comment

mkistler Apr 28, 2020

mkistler Apr 28, 2020

jorge-ibm Apr 28, 2020

padamstx Apr 28, 2020 •

edited

Loading

mkistler Apr 28, 2020

mkistler Apr 28, 2020

padamstx Apr 28, 2020

jorge-ibm Apr 28, 2020 •

edited

Loading

mkistler Apr 28, 2020

padamstx Apr 28, 2020

mkistler Apr 29, 2020

jorge-ibm commented Apr 28, 2020

mkistler left a comment

mkistler Apr 29, 2020

mkistler Apr 29, 2020

mkistler Apr 29, 2020

jorge-ibm Apr 29, 2020

jorge-ibm Apr 29, 2020

mkistler Apr 29, 2020

padamstx left a comment

ibm-devx-automation commented Apr 30, 2020

fix: Pace requests to token server for new auth tokens #55

fix: Pace requests to token server for new auth tokens #55

Conversation

jorge-ibm commented Apr 27, 2020

jorge-ibm Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padamstx Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorge-ibm Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorge-ibm commented Apr 28, 2020

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

ibm-devx-automation commented Apr 30, 2020

jorge-ibm Apr 27, 2020 •

edited

Loading

padamstx Apr 28, 2020 •

edited

Loading

jorge-ibm Apr 28, 2020 •

edited

Loading