feat(config): add oci:// prefix, multi-cloud auth, and digest caching to OCI extends#253
Conversation
… to OCI extends Support the oci:// URI scheme in extends references, replace the bare DefaultKeychain with image.GetKeychain for multi-cloud authentication (AWS ECR, GCR, ACR, k8s in-cluster), and introduce SHA-256 digest-based caching that skips full pulls when the remote manifest is unchanged.
✅ Deploy Preview for devsydev canceled.
|
skevetter
changed the title
📝 WalkthroughWalkthroughThis PR threads ChangesExtends Resolution with Context and OCI Caching
Sequence DiagramsequenceDiagram
participant Caller
participant ResolveOCI as resolveOCIExtends
participant Pull as pullOCIExtendsJSON
participant Cache as Cache System
participant Registry as OCI Registry
participant Keychain as Keychain (ctx)
Caller->>ResolveOCI: (ctx, ociRef, visited)
ResolveOCI->>ResolveOCI: stripOCIPrefix for cycle detection
ResolveOCI->>Pull: (ctx, stripped ref)
Pull->>Cache: getCacheDir(ref)
Pull->>Cache: isCacheValid(cached digest, ref)
alt Cache Valid
Cache-->>Pull: return cached devcontainer.json
else Cache Invalid/Missing
Pull->>Keychain: getKeychain(ctx)
Pull->>Registry: remote.Head(ref) → current digest
Pull->>Registry: remote.Image(ref, keychain) → fetch image
Pull->>Pull: extract devcontainer.json from layer
Pull->>Cache: writeCache(dir, json, digest)
end
Pull-->>ResolveOCI: devcontainer.json bytes
ResolveOCI-->>Caller: *DevContainerConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/devcontainer/config/extends_oci.go`:
- Around line 108-117: The cache helpers and OCI calls must accept and use the
request context: add a ctx context.Context parameter to checkExtendsCache and
writeExtendsCache and update their callsites (where cacheDir, ref, kc are
passed) to pass ctx; inside retryOCIExtendsPull's body, add
remote.WithContext(ctx) to remote.Image and remote.Head invocations (in addition
to remote.WithAuthFromKeychain(kc)) so registry fetches honor
cancellation/deadlines; ensure any internal calls within
checkExtendsCache/writeExtendsCache that perform registry I/O also accept and
forward ctx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57035a14-3d53-41b7-b085-b29d4940690b
📒 Files selected for processing (4)
pkg/devcontainer/config/extends.gopkg/devcontainer/config/extends_oci.gopkg/devcontainer/config/extends_oci_test.gopkg/devcontainer/config/parse.go
| if data, ok := checkExtendsCache(cacheDir, ref, kc); ok { | ||
| return data, nil | ||
| } | ||
| } | ||
|
|
||
| var img v1.Image | ||
| err = retryOCIExtendsPull(func() error { | ||
| var fetchErr error | ||
| img, fetchErr = remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain)) | ||
| img, fetchErr = remote.Image(ref, remote.WithAuthFromKeychain(kc)) | ||
| return fetchErr |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'remote\.(Image|Head)\(' pkg/devcontainer/config/extends_oci.go -C2
rg -n 'WithContext\(' pkg/devcontainer/config/extends_oci.go -C2Repository: devsy-org/devsy
Length of output: 569
🏁 Script executed:
# Check the pullOCIExtendsJSON function signature to see if ctx is available
rg -n 'func pullOCIExtendsJSON' pkg/devcontainer/config/extends_oci.go -A 3Repository: devsy-org/devsy
Length of output: 262
🏁 Script executed:
# Check all calls to checkExtendsCache and writeExtendsCache to understand context availability
rg -n '(checkExtendsCache|writeExtendsCache)\(' pkg/devcontainer/config/extends_oci.go -B 2 -A 2Repository: devsy-org/devsy
Length of output: 757
🏁 Script executed:
# Check the complete function signatures for the helper functions mentioned
rg -n 'func (checkExtendsCache|writeExtendsCache)' pkg/devcontainer/config/extends_oci.go -A 1Repository: devsy-org/devsy
Length of output: 390
🌐 Web query:
go-containerregistry remote.Image WithContext option
💡 Result:
Use remote.WithContext(ctx) when calling github.com/google/go-containerregistry/pkg/v1/remote.Image to control the context used for all HTTP requests that remote makes. Example: img, err:= remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain), remote.WithContext(ctx),) remote.WithContext(ctx) is a functional option that sets the context used by the remote package’s HTTP client requests, not only the initial manifest fetch. The GoDoc for WithContext notes that the provided context is used for all HTTP requests performed by the given function, and that for remote.Image it will be used for subsequent operations like RawConfigFile and for methods on layers returned by Layers. The default context is context.Background if you don’t set one [1]. So, when you cancel ctx or it times out, in-flight remote HTTP operations (including later layer/config requests) should be affected because requests are issued with req.WithContext(...) using the stored context [2].
Citations:
Add context parameter to cache helpers and propagate context to registry I/O calls.
Lines 116, 173, and 185 call remote.Image and remote.Head without context options, so caller cancellation and deadlines are not honored during OCI fetch and HEAD check operations. Pass ctx through checkExtendsCache and writeExtendsCache, and add remote.WithContext(ctx) to each registry call.
Proposed fix
- if data, ok := checkExtendsCache(cacheDir, ref, kc); ok {
+ if data, ok := checkExtendsCache(ctx, cacheDir, ref, kc); ok {
return data, nil
}- img, fetchErr = remote.Image(ref, remote.WithAuthFromKeychain(kc))
+ img, fetchErr = remote.Image(ref, remote.WithAuthFromKeychain(kc), remote.WithContext(ctx))- writeExtendsCache(cacheDir, ref, kc, data)
+ writeExtendsCache(ctx, cacheDir, ref, kc, data)-func checkExtendsCache(cacheDir string, ref name.Reference, kc authn.Keychain) ([]byte, bool) {
+func checkExtendsCache(ctx context.Context, cacheDir string, ref name.Reference, kc authn.Keychain) ([]byte, bool) {- desc, err := remote.Head(ref, remote.WithAuthFromKeychain(kc))
+ desc, err := remote.Head(ref, remote.WithAuthFromKeychain(kc), remote.WithContext(ctx))-func writeExtendsCache(cacheDir string, ref name.Reference, kc authn.Keychain, data []byte) {
- desc, err := remote.Head(ref, remote.WithAuthFromKeychain(kc))
+func writeExtendsCache(ctx context.Context, cacheDir string, ref name.Reference, kc authn.Keychain, data []byte) {
+ desc, err := remote.Head(ref, remote.WithAuthFromKeychain(kc), remote.WithContext(ctx))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/devcontainer/config/extends_oci.go` around lines 108 - 117, The cache
helpers and OCI calls must accept and use the request context: add a ctx
context.Context parameter to checkExtendsCache and writeExtendsCache and update
their callsites (where cacheDir, ref, kc are passed) to pass ctx; inside
retryOCIExtendsPull's body, add remote.WithContext(ctx) to remote.Image and
remote.Head invocations (in addition to remote.WithAuthFromKeychain(kc)) so
registry fetches honor cancellation/deadlines; ensure any internal calls within
checkExtendsCache/writeExtendsCache that perform registry I/O also accept and
forward ctx.
| parent, err := resolveExtendsArray( | ||
| context.TODO(), | ||
| devContainer.Extends, | ||
| declaringDir, | ||
| visited, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'context\.TODO\(' pkg/devcontainer/config/parse.go -C2
rg -n 'func ParseDevContainerJSONFile' pkg/devcontainer/config/parse.go -C2Repository: devsy-org/devsy
Length of output: 441
🏁 Script executed:
rg -n 'ParseDevContainerJSONFile' --type goRepository: devsy-org/devsy
Length of output: 3258
🏁 Script executed:
rg -n 'func resolveExtendsArray' pkg/devcontainer/config/parse.go -A 5Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
head -120 pkg/devcontainer/config/parse.go | tail -50Repository: devsy-org/devsy
Length of output: 1341
🏁 Script executed:
rg -n 'func resolveExtendsArray' pkg/devcontainer/config/ -A 15Repository: devsy-org/devsy
Length of output: 1094
🏁 Script executed:
rg -n 'resolveExtendsArray' pkg/devcontainer/config/ -B 2 -A 2Repository: devsy-org/devsy
Length of output: 1769
🏁 Script executed:
grep -r "context\." pkg/devcontainer/config/parse.go | head -20Repository: devsy-org/devsy
Length of output: 78
Use caller-provided context in parse entrypoint instead of context.TODO().
Line 105 uses context.TODO() which prevents caller-driven cancellation/deadline during OCI extends resolution. This is problematic for request-level control flow and can block longer than expected during registry I/O.
The suggested patch's breaking change and incomplete backward compatibility wrapper need refinement. Consider instead:
- Adding a new context-aware function:
ParseDevContainerJSONFileWithContext(ctx context.Context, jsonFilePath string) - Keeping the existing
ParseDevContainerJSONFilesignature for backward compatibility, but have it call the new function with a proper context (note:context.Background()alone doesn't solve this—for CLI commands, use request context; for existing callers that can't provide context, use a timeout wrapper) - Other code paths in the codebase (e.g., extends_oci.go line 85) already properly thread context through
resolveExtendsArray, showing the pattern is established and expected
1 participant
Summary
Adds three tightly-coupled enhancements to OCI extends resolution:
oci://prefix support:isOCIRef()now recognizes theoci://URI scheme andresolveOCIExtendsstrips the prefix before pulling, matching devcontainer spec behavior.authn.DefaultKeychainwithimage.GetKeychain(ctx)which provides k8s in-cluster auth, AWS ECR, GCR, Azure ACR, and Docker config.json in priority order. Falls back to DefaultKeychain on error.HEADrequest to compare digests — only re-pulls when the manifest has changed.Context threading (
context.Context) is wired throughresolveExtendsArray→resolveExtendsSingle→resolveOCIExtends→pullOCIExtendsJSONto support the auth keychain.Summary by CodeRabbit
Release Notes
New Features
oci://prefix in OCI references with automatic normalization.Refactor
Tests