Disable async token refresh for GCP credential providers#1575
Disable async token refresh for GCP credential providers#1575renaudhartert-db merged 3 commits intomainfrom
Conversation
Google's Cloud Go libraries internally wrap token sources in oauth2.ReuseTokenSource, causing the SDK's async refresh (at T-20min) to be swallowed by the inner cache. Unlike M2M OAuth and Azure Client Secret, the inner sources are unexported so we cannot bypass the caching. Disable async refresh for both GoogleDefaultCredentials and GoogleCredentials providers to avoid wasted refresh attempts. Fixes #1549 Co-authored-by: Isaac Signed-off-by: Ubuntu <renaud.hartert@databricks.com>
Co-authored-by: Isaac Signed-off-by: Ubuntu <renaud.hartert@databricks.com>
simonfaltum
left a comment
There was a problem hiding this comment.
Review Swarm: Disable async token refresh for GCP credential providers
Verdict: Approved | Reviewers: Isaac + Cursor | Rounds: 3 (with debate)
0 Critical | 0 Major | 0 Gap | 1 Nit | 2 Suggestion
Debate Summary
The central disagreement across 3 rounds concerned whether disabling async refresh while keeping the SDK's outer cache creates a correctness issue:
-
Cursor's position (maintained through Round 3): With async disabled, the SDK's outer
cachedTokenSourceusesblockingToken()which only calls through to Google's source after exact expiry. Since Google'sReuseTokenSourceis lazy (not background-refreshing), its 10-second early-refresh window is effectively bypassed. This means the PR doesn't fully fix #1549 if interpreted as fixing near-expiry auth failures. -
Isaac's position (maintained through Round 3): The PR's goal is to eliminate wasted async refresh work, and it achieves that. The async path was already broken for GCP (never reliably caught Google's 10-second window). GCP doesn't reject near-expiry tokens the way Azure does. The alternative (removing the outer cache) is a larger change with marginal practical benefit.
-
Synthesis: Both reviewers are technically correct about different aspects. The PR correctly fixes the stated problem (wasted async refresh). Whether #1549 also implies fixing the near-expiry window is a scope question for the PR author. The practical risk is low since GCP doesn't reject near-expiry tokens.
See inline comments for specific findings.
Automated deep review by Isaac + Cursor swarm (3 rounds)
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
Fixes #1549. Related: #1550, #1573.
Google's Cloud Go libraries (
impersonate.IDTokenSource,impersonate.CredentialsTokenSource,idtoken.NewTokenSource,google.CredentialsFromJSON) all internally wrap their token sources inoauth2.ReuseTokenSource. This means the SDK'scachedTokenSourceasync refresh is swallowed by the innerReuseTokenSourcecache, making it entirely wasted work.This is the same double-caching bug as M2M OAuth (#1549). Though, unlike M2M (#1550) and Azure Client Secret (#1573), Google's libraries don't expose an uncached
Token(ctx)method -- the inner sources are unexported types. Recreating the token source on each call is expensive and will require further considerations.The pragmatic fix is to disable async refresh for both GCP providers:
GoogleDefaultCredentials(config/auth_gcp_google_id.go)GoogleCredentials(config/auth_gcp_google_credentials.go)Google's own
ReuseTokenSource(with a 10-second early refresh window) still provides token renewal before expiry. Thorough comments explain the rationale and warn against re-enabling async refresh.Tests
Existing
configtests pass (go test ./config/ -v -count=1). No new tests needed -- this is a configuration change (appendingauth.WithAsyncRefresh(false)to cache options) and the async refresh mechanism itself is already well-tested.