-
Notifications
You must be signed in to change notification settings - Fork 118
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
fix: Lazy refresh should refresh tokens 4 minutes before expiration. #2063
Conversation
67856b9
to
cb64a3a
Compare
We don't know for sure that this will fix the above issue right? We should probably mark this as "Related to #2509" and have it tested in the wild first before marking as fixed. Thoughts? |
cb64a3a
to
67a3226
Compare
Good point. I set this as "related to." |
67a3226
to
62dbf7d
Compare
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.
LGTM. One small nit.
.getAccessToken() | ||
.getExpirationTime() | ||
.toInstant() | ||
.minus(4, ChronoUnit.MINUTES) |
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.
Do we have a constant for this value somewhere? I think we should use it to avoid future mistakes in buffering.
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.
➕ 1
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.
Good idea, yes. RefreshCalculator.DEFAULT_REFRESH_BUFFER
core/src/main/java/com/google/cloud/sql/core/DefaultAccessTokenSupplier.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/cloud/sql/core/DefaultAccessTokenSupplier.java
Outdated
Show resolved
Hide resolved
62dbf7d
to
9decc4f
Compare
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.
LGTM
|| "".equals(credentials.getAccessToken().getTokenValue()); | ||
} | ||
|
||
private void refreshIfNearExpiration(GoogleCredentials credentials) throws IOException { |
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.
nit: Since we also refresh for reasons other than expiry maybe a better name is refreshIfRequired or refreshIfNecessary or something similar.
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.
Fixed.
9decc4f
to
29ae338
Compare
Added a 4 minute buffer to refreshing tokens and certificates to avoid creating race condition that would allow the connector to create an ephemeral certificate with an expired auth token.
Now, IAM auth tokens are now refreshed 4 minutes before they token expire. Also, the Lazy Refresh Strategy will refresh the client certificate 4 minutes before the expiration of the certificate and the IAM auth token.
This should mitigate some of the strange certificate expiration errors commonly found in Cloud Run, see: #2059