fix(maven): state cache directives on every read response

David Fernandezrequested to merge
10io/maven-read-cache-headers-1070 into main

Summary

Every Maven read response left the origin with no Cache-Control and no Vary. An edge that caches by file extension by default then stored the .jar 302 (signed storage URL) and the remote cold-fill 200 (the artifact bytes) under a key that ignores the credential, and replayed one principal's authorized response to another for the edge's default TTL. .pom, .tgz and extension-less blob paths were not on that default list, which is why the symptom looked jar-only; the missing directive covered the whole Maven read surface.

This MR makes every Maven read shape state its own cacheability, the rule S15 already fixes for npm remote:

  • every successful GET and HEAD body shape, HEAD, 304 and checksum sidecar, hosted and remote alike, carries Vary: Authorization and Cache-Control: private;
  • the 302 storage redirect carries Vary: Authorization and Cache-Control: no-store, because its signed Location expires and a stored redirect would replay a dead link even from a private cache;
  • the remote artifact cold-fill 200 carries the same private directive as every other body shape.

The directive is unconditional: it does not vary by repository visibility, because closed-beta authorization treats every repository as private (S09, ADR-021) and a stored public copy would outlive a later flip to private. Bare private carries no lifetime on purpose: no Maven response sets Last-Modified, so a private cache has nothing to derive heuristic freshness from and revalidates through the ETag; the cold-fill 200 carries no validator, so a stored copy costs a full re-fetch.

Two unexported helpers in internal/format/maven/download.go stage the two header pairs at each success write site rather than at the dispatch entry, so error responses are byte-identical to before. The six conditional-match 304 arms share one writeNotModified helper, so a new arm cannot ship without the directive.

The spec amendment rides in the same MR: S10's API contract and acceptance criteria gain the rule, S10's Security Considerations bullet that deferred "redirect-cache hygiene" to S06 (which never stated it) now points at the contract, and S14 inherits it with the cold-fill header subset amended to match.

Out of scope, by decision

  • Error responses (404, 416, 422, 5xx) keep their current headers. The read-path 404 is heuristically cacheable and the edge caches it too; that is #1131 (closed).
  • The npm hosted and OCI blob 307 redirects still rely on header omission; #1084 tracks the OCI side.
  • A zone-side cache-bypass rule is defense in depth outside this repository.

Vary is stated for RFC-compliant intermediaries and for consistency with the npm surface; the property this MR relies on against the observed edge is Cache-Control: private / no-store, which that edge honors regardless of Vary.

Governing ADRs

  • ADR-005 (Artifact Delivery Mode) and ADR-013 (Storage Backend Interaction) place CDN caching on the signed storage URL, whose signature the CDN validates at the edge. Neither speaks to caching of the registry's own redirect or body response in front of the origin; this change adds directives on that response and leaves the signed-URL path untouched. Conforms, no deviation.
  • ADR-024 (Infrastructure Delivery) places a shared edge in front of the service. This change is what makes the service's responses safe behind it. Conforms.

Testing

  • TestHostedRead_EveryServeShapeIsCacheSeparated (internal/format/maven/cache_headers_test.go): hosted primary 200 proxy, 302 redirect, 206, HEAD 200, GET and HEAD 304, sidecar 200, HEAD 200, GET and HEAD 304. Asserts on the response snapshot a client receives, that Vary is exactly Authorization once, and the single directive per shape.
  • TestRemoteRead_EveryDirectServeShapeIsCacheSeparated (internal/format/maven/remote_cache_headers_test.go): the kind=2 arms a direct call reaches without a database: the artifact cold-fill stream 200, sidecar synthesis 200, HEAD 200, GET and HEAD 304, and the three row-derived 304 short-circuits.
  • One-line assertions in the existing integration tests for the remote fresh hit 200, redirect-delivery 302, conditional 304, artifact cold-fill 200, metadata cold-fill 200, and every sidecar GET/HEAD.
  • All 18 new unit subtests fail against the unchanged production code and pass with it.
  • Ran locally: go test ./internal/format/maven/ and go test -tags=integration ./internal/format/maven/ (full package, against Colima), golangci-lint run ./internal/format/maven/ and the --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false form (no findings on touched lines), scripts/ci/check-comment-caps.sh --base origin/main, vale, markdownlint and lychee on the three edited documents.
  • e2e catalog: docs/testing/e2e/maven.md gains e2e.maven.consume.read-cache-headers.

The streamArtifactFill doc comment enumerates the content headers that function stages and was left unedited; the cache headers are staged beside them, not inside, so the comment is incomplete rather than false, and editing it would pull a 15-line block into the one-line unexported cap.

Related to https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/1070 Closes #1133 (closed)

Edited by David Fernandez

Merge request reports