fix(oci): cache tag lists and normalize manifest variants - #280
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves OCI offline behavior by caching tag-list responses and making manifest caching more robust across equivalent Accept header variants, so previously warmed caches can be reused when upstream registry/token services are unavailable.
Changes:
- Add automatic caching (with stale fallback) for OCI
/v2/<name>/tags/listresponses, independent of general metadata caching. - Normalize manifest
Acceptheaders for cache-key derivation and validate cached manifest content types against the caller’sAccept. - Add tests and documentation for OCI tag-list + manifest offline caching behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/handler/container.go | Routes tag-list requests through the new cached implementation. |
| internal/handler/container_tags.go | Implements tag-list caching, ETag revalidation, and stale fallback behavior. |
| internal/handler/container_manifest.go | Normalizes Accept for cache keys and validates cached manifest content type against request. |
| internal/handler/container_test.go | Adds coverage for stale tag-list fallback and manifest variant normalization/content-type gating. |
| docs/configuration.md | Documents always-cached OCI manifests and tag lists + stale behavior. |
| docs/architecture.md | Updates architecture notes to include tag lists as always-cached metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Please address these cache compatibility issues:
- Preserve reads of existing manifest cache keys. Previous releases hashed the raw
Acceptvalue, so the normalized key misses warmed entries after an upgrade. On a normalized-key miss, try the legacy key and rewrite the entry under the normalized key. Add an upgrade test using previous-format data. - Apply quality to the most specific matching
Acceptmedia range.Accept: application/vnd.oci.image.index.v1+json;q=0, */*;q=1currently accepts the explicitly excluded index through the wildcard. Add a regression test.
|
Thanks, implemented the legacy raw-Accept cache-key fallback with migration and upgrade coverage. Also fixed specificity-aware Accept matching so an explicit q=0 exclusion overrides a wildcard, with regression coverage. |
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Please preserve mixed-version and rollback cache access. Fresh manifests are now stored only under the normalized Accept key. Older replicas and rollback releases still hash the raw Accept value, so they cannot read entries first cached by the new release. Dual-write the raw key when it differs, including the digest alias. Add a handler-level test that warms through the public route and verifies the previous-format key is written.
|
Implemented dual-write support for normalized and legacy raw-Accept keys, including digest aliases. Added a handler-level warm-route test that verifies both previous-format keys are written. |
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Please address the two cache correctness issues in the inline comments.
|
Addressed both cache-correctness issues and added handler-level regression coverage. |
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Please rewrite relative tag-list pagination Link targets for named registries. rewriteContainerTagsLink currently returns non-absolute targets unchanged, so a link such as </v2/owner/repo/tags/list?...> drops the upstream/<registry>/ selector and routes the next page to the default registry. Add a named-registry route test that follows the returned link.
|
Thanks, Fixed. Relative tag-list pagination links now retain the named-registry selector when rewritten through the proxy, with a route-level test that follows the returned link. |
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Thanks for sticking with this — the relative-Link rewrite now keeps the named-registry selector and the follow-the-link test proves it end to end.
goconst tripped on main after #259 landed on top of #280: five composite-literal occurrences each of "Content-Length" and "Content-Type" across container.go, container_manifest.go, container_tags.go, handler.go, and swift.go crossed the min-occurrences: 5 threshold. Neither PR hit it alone. Add headerContentType and headerContentLength beside headerAcceptEncoding and use them throughout the package rather than only at the flagged sites, so the next handler that adds one does not re-trip the check.
Compute real manifest digests in #280's fixture upstreams so the new verification accepts them, and add headerETag / headerLastModified to
* Add Homebrew JSON API and bottle proxy support * Fix Homebrew HEAD offline fallback and non-sha256 OCI manifest handling Route Homebrew API HEAD requests through ProxyCached so a warm cache answers without an upstream call and stale entries are served when the upstream is unreachable. HEAD still reaches upstream as HEAD when metadata caching is disabled. Limit OCI manifest digest verification to sha256 references and Docker-Content-Digest headers so other digest algorithms are proxied instead of rejected, and log the failing expected value. * Reconcile with #280 and #301 after rebase Compute real manifest digests in #280's fixture upstreams so the new verification accepts them, and add headerETag / headerLastModified to * Send fixed Accept for Homebrew API and match If-None-Match properly The Homebrew API cache key does not include Accept, so replaying the client header could serve one representation under another; the API does not negotiate anyway. Compare If-None-Match with weak comparison, list splitting and "*" per RFC 7232 instead of string equality, and apply the same helper to the metadata and swift responders. * Reconcile with #298 and #304 after rebase Move the configureScanning doc comment back to its function after the auto-merge stacked it on mountProtocolHandlers, and drop the second ETag/Last-Modified set in writeMetadataCachedResponse now that the pre-304 set covers both response paths.
Fixes #249
Summary
/v2/<name>/tags/listresponses automatically, including when general metadata caching is disabled.Acceptheaders before deriving cache keys, so equivalent media-type lists reuse the same cached variant.Validation
gofmtgo tool golangci-lint run ./...go vet ./...go test -race ./...git diff --check