Extract shared OAuthTokenSource into pkg/auth/tokensource#5090
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
There was a problem hiding this comment.
Pull request overview
Refactors duplicated OAuth/OIDC token acquisition logic (in-memory cache → secrets-cached access token → refresh token restore → interactive browser PKCE) out of pkg/llm and pkg/registry/auth into a shared pkg/auth/tokensource package, and rewires both consumers to use it. This also brings the registry path in line with the LLM path by persisting rotated refresh tokens after cache restore.
Changes:
- Added
pkg/auth/tokensourcesharedOAuthTokenSourceimplementation (plus unit/regression tests). - Updated LLM and registry token source wrappers to delegate to the shared package.
- Updated/expanded registry auth tests and adjusted LLM tests to reflect the extraction.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/auth/oauth_token_source.go | Removed duplicated registry-specific OAuth token source implementation. |
| pkg/registry/auth/auth.go | Replaced registry token source construction with tokensource.New(...) and centralized config persistence. |
| pkg/registry/auth/auth_test.go | Reworked tests to validate behavior via the public API and new shared behavior (persisting rotated RTs). |
| pkg/registry/auth/helpers_test.go | Added OIDC discovery test server helper for registry auth tests. |
| pkg/llm/tokensource.go | Replaced LLM token source implementation with a thin wrapper around tokensource.OAuthTokenSource. |
| pkg/llm/tokensource_test.go | Trimmed old LLM-specific tests; added a smaller set aligned with shared implementation. |
| pkg/auth/tokensource/tokensource.go | New shared OAuth/OIDC token source implementation used by both LLM and registry. |
| pkg/auth/tokensource/tokensource_test.go | New black-box tests covering tiering, caching, key provider, and config persistence behavior. |
| pkg/auth/tokensource/preemptive_test.go | New internal regression tests for preemptive refresh composition behavior. |
| pkg/auth/tokensource/doc.go | New package documentation for the shared token source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5090 +/- ##
==========================================
+ Coverage 66.92% 67.01% +0.09%
==========================================
Files 595 595
Lines 60080 59994 -86
==========================================
- Hits 40208 40205 -3
+ Misses 16807 16729 -78
+ Partials 3065 3060 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both pkg/llm and pkg/registry/auth implemented the same three-tier OAuth token strategy (in-memory cache → secrets-cached access token → refresh token → browser OIDC/PKCE). This extracts that logic into a single shared package, pkg/auth/tokensource, and rewires both consumers as thin wrappers. As a side effect, the registry path now wraps its restored token source with PersistingTokenSource, matching the LLM path and ensuring rotated refresh tokens are re-persisted after a cache restore. Closes #5050
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aponcedeleonch
left a comment
There was a problem hiding this comment.
Three small follow-ups inline. Nice refactor overall.
aponcedeleonch
left a comment
There was a problem hiding this comment.
Thanks for the clean refactor. All three of my comments are addressed. The regression surface looks tight: LLM is a pure extraction, registry's behavior changes are intentional and well-tested. LGTM.
Summary
Both pkg/llm and pkg/registry/auth implemented the same four-tier OAuth token strategy (in-memory cache → secrets-cached access token → refresh token → browser OIDC/PKCE). This extracts that logic into a single shared package, pkg/auth/tokensource, and rewires both consumers as thin wrappers.
Behavior changes (intentional):
Registry path now has cross-process access-token caching. The shared
OAuthTokenSourcewrites a<key>_ATsecret after each successful token fetch (expiry encoded in the value). Concurrent short-livedthvprocesses can reuse this cached AT instead of all racing to exchange the same refresh token — preventinginvalid_granterrors from OIDC providers that rotate refresh tokens on each exchange. This is new for the registry path (the oldoauth_token_source.godid not have this tier).Registry path now wraps its restored token source with
PersistingTokenSource. Rotated refresh tokens are re-persisted after a cache restore, matching the LLM path and fixing a gap where a provider-rotated refresh token would be lost on the next invocation.Fixes #5050
Incorporates #5066
Type of change
Test plan
task test)task test-e2e)task lint-fix)API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/auth/tokensource/tokensource.goOAuthTokenSource,DeriveSecretKey,EnsureOfflineAccesspkg/auth/tokensource/preemptive.gowithPreemptiveRefresh,withPreemptiveRefreshFrom)pkg/auth/oauth/noncaching.goNonCachingRefresher— always performs a network round-trip, prevents preemptive-window thrashingpkg/llm/tokensource.gopkg/registry/auth/auth.goupdateRegistryTokenRefsimplifiedpkg/registry/auth/oauth_token_source.goDoes this introduce a user-facing change?
The registry path gains cross-process AT caching (extra keyring/secrets writes). Users with very restrictive keyring write policies may need to allow writes to
<key>_ATkeys. In practice this is transparent and improves reliability for concurrentthvinvocations.Special notes for reviewers
_ATcache key isKeyProvider() + "_AT". For registry,KeyProviderreturns eitherCachedRefreshTokenReforREGISTRY_OAUTH_<8hex>, so the AT key is e.g.REGISTRY_OAUTH_ab12cd34_AT.cacheAccessTokensuppresses write errors (debug log only) — a keyring write failure degrades gracefully to a full refresh on the next call.makeTokenPersisterwrites""tokey+"_AT"to invalidate the AT cache after a refresh token rotation (rather thanDeleteSecret, for provider compatibility).Large PR Justification
This is a refactor to unify OAuthTokenSource used in several places. It reduces code duplication and provides better integration with cross-process token caching as a side effect.