fix: remove weird host based cred cache#2060
Merged
jakobmoellerdev merged 3 commits intoMar 25, 2026
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRemoved the in-memory, thread-safe OCI credential cache and its tests. Provider now derives an identity per repository call, creates a fresh auth cache, and supplies credentials via an inline Credential callback. Docker credential resolution now accepts host-only hostport values. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider
participant Identity as "Identity\n(v1.IdentityFromOCIRepository)"
participant AuthCache as "auth.Cache\n(new per call)"
participant AuthClient as "auth.Client\n(CredentialFunc)"
participant Registry
Provider->>Identity: derive consumer identity from repository
Provider->>AuthCache: create auth.NewCache()
Provider->>AuthClient: create client with CredentialFunc(identity, creds)
AuthClient->>AuthCache: consult cache for tokens/credentials
AuthClient->>Registry: authenticate requests using computed credentials
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fe61cab to
16e19ba
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/oci/repository/provider/provider.go`:
- Around line 196-209: The toCredential function currently reads raw map keys
like "username", "password", "access_token", and "refresh_token" which
mismatches the keys used when writing credentials (e.g.,
accessToken/refreshToken); update toCredential to use the shared constants
(e.g., CredentialKeyUsername, CredentialKeyPassword, CredentialKeyAccessToken,
CredentialKeyRefreshToken from bindings/go/oci/credentials/docker_config.go)
instead of raw string literals so token-based auth isn't dropped and key names
remain consistent across writers and readers.
- Around line 143-151: The credential keys and cache lifetime are incorrect:
update toCredential to read the canonical credential keys defined in
credentials.CredentialKeyAccessToken / CredentialKeyRefreshToken (instead of
"access_token"/"refresh_token") so it matches how CredentialFunc populates
credentials, and when constructing the ocirepository client in NewFromOCIRepoV1
replace the shared b.authorizationCache with a new per-client cache via
auth.NewCache() (leave b.httpClient and other fields as-is) so each
repository/client gets an isolated authorization cache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 229c1f33-3d86-4e04-b125-5a18603e40a2
📒 Files selected for processing (3)
bindings/go/oci/repository/provider/credential_cache.gobindings/go/oci/repository/provider/credential_cache_test.gobindings/go/oci/repository/provider/provider.go
💤 Files with no reviewable changes (2)
- bindings/go/oci/repository/provider/credential_cache_test.go
- bindings/go/oci/repository/provider/credential_cache.go
- refactor: remove `credentialCache` from provider and related test files - chore: simplify credential handling by inlining logic into `toCredential` function Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
16e19ba to
f59e1af
Compare
frewilhelm
approved these changes
Mar 25, 2026
piotrjanik
approved these changes
Mar 25, 2026
morri-son
approved these changes
Mar 25, 2026
2efef84
into
open-component-model:main
17 of 18 checks passed
1 task
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
credentialCachefrom provider and related test filestoCredentialfunctionWhich issue(s) this PR fixes
the current provider credential cache is not correct. oras can and our cred cache was only able to store by hostname but reusing the creds for differing identities as we have it is invalid.
Verification
ocmthis came up during testing of open-component-model/ocm-website#777