feat(oci): Step 13 part 1 - container remote refusals and outcome mapping (S16 plan: 13/26)

What this adds

The upstream-outcome to OCI-response mapping the container remote read arms will share once they fetch, and the cache-store refusals it reads. Part 1 of 2 for S16 Step 13; What part 2 owes below says what the other half carries.

MapFetchOutcome is HTTP-free. It returns a decision (an action, a status, an OCI error code, a Retry-After flag and the upstream's own Retry-After value) and never touches an http.ResponseWriter, so the table is exhaustive over remote.FetchOutcome, the upstream status classes, and this package's refusal sentinels, and the suite covers every row with no server, no database, and no cache store behind it.

Nothing calls it yet. The read arms 11b landed serve cached content and answer the interim 501 on a miss, so there is no upstream outcome to map until the miss-fill arms (Steps 14 and 15) replace that 501.

What part 2 owes

!1785 (merged) targets this branch and carries the rest of Step 13:

  • internal/format/oci/remote_operations.go: constructing Lookuper and Fetcher per fetch and composing Lookup, freshness, Fetch, and CacheFallback.
  • The Fetch body bound at container.manifest_max_payload. This MR threads the configured value into NewRemoteCacheStore; part 2 threads the same value into the fetch, which is what makes the two bounds equal.
  • The format's remote.HealthSweepSource and its append to the aggregate in cmd/artifact-registry/wire_remote.go.

Step 13's Acceptance asks for a table over every row of the spec's Error Cases table. This MR covers the rows MapFetchOutcome decides on its own: the upstream status classes, the remote.FetchOutcome values, and this package's refusal sentinels. The rows only a composed Fetch reaches are part 2's, the rejected-coding case among them, where a failed Fetch reports the upstream's own 200. Rows in that table outside Step 13 belong to their own steps: the 405 write-verb rows to Step 10, the list-proxy mapping to Step 16.

Four defects this closes

Three of four manifest refusals answered the wrong status. docs/specs/S16-container-remote.md puts four classification refusals on one 503 UNAVAILABLE row and says all four take it. Three of them left DetectManifestType as a bare *DetectError, which classifyRemoteCacheManifest wrapped with no sentinel, so they matched no arm and fell to the fail-closed 500 INTERNAL with no Retry-After. Measured before the fix:

schema1 mediaType    -> 500 INTERNAL     (spec: 503 UNAVAILABLE + Retry-After)
schemaVersion not 2  -> 500 INTERNAL     (spec: 503 UNAVAILABLE + Retry-After)
ambiguous structure  -> 500 INTERNAL     (spec: 503 UNAVAILABLE + Retry-After)
unaccepted mediaType -> 503 UNAVAILABLE  (correct)

The spec notes a real Docker Schema 1 manifest carries no mediaType and lands on the ambiguity guard, so the realistic legacy-image case was in the broken set. ErrUnclassifiableManifestPayload now wraps the chain's refusal, keeping the *DetectError reachable by errors.As so the hosted push path's 400 MANIFEST_INVALID mapping is unaffected.

A discovery-HEAD 403 propagated DENIED with no cache fallback, which is the token exchange's row. That HEAD is unauthenticated, so no status it returns is a verdict on a credential; the spec puts every discovery status that establishes no auth verdict on the cache-or-503 outcome. The leg is now read before the status. Only 403 was affected. Every other discovery status already reached the handshake arm's default, and a discovery 401 is the challenge itself.

An upstream 405, 407, or 426 was relayed bare. RFC 9110 makes a header mandatory on each: Allow on a 405, Proxy-Authenticate on a 407, Upgrade on a 426. A decision carries none of them, and the unenumerated-4xx arm propagated all three, which emits exactly the malformed response this spec's own blob-upload rows call out when they require an Allow on every 405. None of the three can be answered honestly from this seam either: the client's method is allowed on the route it asked, the proxy demanding authentication is this service's own, and the protocol upgrade is between this service and its upstream. The three now take the unusable-response route a 200 carrying an undecoded content coding takes, serving a cached copy when one exists and 503 UNAVAILABLE otherwise.

A non-retryable 401 answered two conditions with one sentinel. ErrUpstreamAuthNotRetryable covered both a token this service minted and sent, which the upstream refused, and a 401 answered to a request that carried no credential because discovery found no usable challenge. Both reached the handshake arm as an *UpstreamAuthError with a zero Status, so both served a cached copy, or answered 503 with nothing cached, where the Error Cases row for a 401 after a fresh post-retry token fixes 401 UNAUTHORIZED. UpstreamAuthenticator.Reauthorize is the last point that holds the prior Authorization, and its Header is the only thing that separates the two, so the split is made there: ErrUpstreamCredentialsRejected propagates 401 with no cache fallback, and ErrUpstreamNeverAuthenticated keeps the cache-or-503 route. ErrUpstreamAuthStatusInvalid and errAuthDiscoveryWrite reached the same arm with the same zero Status; both are defects on this side and now answer 500 INTERNAL.

Also here

  • The configured container.manifest_max_payload becomes a constructor argument on NewRemoteCacheStore in place of the package constant, so the read-back ceiling is whatever an operator configured. Until part 2 merges the fetch applies no manifest bound at all; part 2 threads the same value into it, and the two bounds are equal from that point.
  • BumpLastDownloadedAt joins the embedded remoteCachePromoted and the var _ remote.CacheStore anchor goes in beside the backing anchor, completing the four-method seam. 11a supplied the datastore method the embed promotes; 11b and 11c touch the file not at all, which their merged MRs bear out.
  • Two Error Cases rows: one for an unenumerated upstream 4xx, which the mapping answers by propagating the status under INTERNAL, and the 405/407/426 carve-out above. The first behavior was already reasoned about with no row describing it; the second is the defect's fix.
  • RemoteFetchOutcome's zero value is pinned by a test. The doc comment called it a safe zero value on the strength of its action alone, while the Status it pairs that action with is 0, which no response can carry. The comment now says which half is safe and names the renderer's obligation to treat a non-positive Status on an error action as its own 500.
  • CodeUnauthorized and CodeTooManyRequests, defined beside the mapping that emits them, with errors.go pointing at them.

Testing

MapFetchOutcome gets a row per outcome, status class, sentinel and handshake leg, plus three whole-table invariants: every 503 carries Retry-After, no 500 does, and no decision carries an upstream header, hostname, or credential. The last is checked by reflection over every field, so a field added later is covered. The carve-out adds six rows, each of the three statuses at both hasCache values, because it changes the answer twice. The 401 split adds eight more: the two refusal sentinels and the two handshake faults on this side, each at both hasCache values, because all four carry a zero Status and only one of them wants the handshake arm's default.

The chain-refusal rows are built by running the real classifier rather than by naming the sentinel, so a wrap that stopped carrying it fails them rather than passing.

The cache store gets a runtime remote.CacheStore satisfaction test that calls all four methods through the interface value rather than restating the compile-time anchor, the promotion contract for the third method, and the threaded ceiling at a value the package default deliberately is not.

Reviewable LOC

2638 total, measured at the branch tip with git diff origin/main...HEAD --numstat: production 1013 (remote_errors.go 619, remote_cache_store.go 305, remote_auth.go 55, container_remote_cache.go 18, errors.go 10, remote_requestbuilder.go 6), tests 1621 (remote_errors_test.go 1292, remote_cache_store_test.go 307, remote_auth_test.go 22), spec 4.

Past the 500 ceiling and not split further, because the two halves are bound in both directions. remote_errors.go needs ErrUnclassifiableManifestPayload, which remote_cache_store.go defines: a compile dependency. Seven comments in remote_cache_store.go name MapFetchOutcome and isRefusedUpstreamContent, a documentation dependency the other way. Whichever half went first, one of them would be wrong on main: the mapping would not compile, or the sentinels would forward-reference a symbol that does not exist. This is the case docs/dev/development-model.md covers by asking for a justification instead of a split.

e2e scenarios

No docs/testing/ scenario is added or affected. Nothing in this MR is reachable from a request: the mapping has no caller, and the cache-store changes are a constructor argument and a promoted method on a type nothing constructs in production yet. The scenarios land with the miss-fill arms that make these paths reachable.

Related to #288

Edited by Sylvia Shen

Merge request reports

Loading
Loading