-
Notifications
You must be signed in to change notification settings - Fork 30
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: use auth DetectDefault over oauth2 FindDefaultCredentials #909
Conversation
@quartzmo mind taking a quick pass and letting me know if this looks okay to you? 😄 |
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.
Will you add a few sentences about why we're not force refreshing the token anymore? That was a tricky piece of code to write, but is now obviated by the better caching period in the new auth library. We're definitely going to forget this, so recording it for posterity would be a cheap and useful thing to do.
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.
What do you think about converting this test setup to use DetectDefault
instead of DefaultTokenSource
at this time as well?
@quartzmo I think you accidentally pasted the wrong link, not sure exactly what test you are referring to... I do plan to eventually remove the rest of the test usage of TokenSource and add a message to the
|
The new Google
auth
library should be used over the oldoauth2
library.DetectDefault
should be used over the oldFindDefaultCredentials
tosource ADC from the environment.
TokenProvider
should be used over the oldTokenSource
httptransport.NewClient
to gain advantage of built-in universe domain checksThis will help fix certain non-GDU paths as
FindDefaultCredentials
does not supportself-signed JWTs.
Switching to the new
auth
library also allows us to remove some trickycustom token refresh logic:
cloud-sql-go-connector/internal/cloudsql/refresh.go
Lines 143 to 162 in 3e8203a
Previously
TokenSource
tokens were tricked into performing a refresh by settingthe
Expiry
to one minute in the future. This was required as tokens were observedto be cached sometimes up until <30s until expiry.
We no longer need this logic as the
TokenProvider
implementation by defaultuses
NewCachedTokenProvider
which caches tokens and automatically refreshesthem when the token is close to expiring (within ~4 minutes).
Fixes #904