Draft: Harden the artifact registry client transport and specs

What does this MR do and why?

Groups four small, non-blocking hardening follow-ups from the S02 artifact registry client review. All touch ee/lib/artifact_registry/client.rb or its spec, so they ship together rather than as four MRs racing on the same files.

  1. An unreadable body no longer reclassifies an outcome, and no longer leaks a bare literal to the user (bug). The Faraday::ParsingError rescue in perform_request preserved the status only for 404; every other status with an unreadable body became UnavailableError carrying the client's own string. That misread a terminal conflict as a retryable outage: a 409 means "this will never succeed as sent", so degrading it could drive a provision_namespace retry that can never win. The rescue now delegates to a shared error_class_for(status) classifier that also drives raise_error, so an unreadable body classifies identically to a parseable one of the same status: 401/403 stay AuthorizationError, 429/5xx (and a malformed 2xx or 3xx) stay UnavailableError, and every other 4xx (400/404/409/413/422) keeps ApiError with the status. The unreadable path now passes no message, so the resolver's RendersErrors fallback renders its translated s_() string instead of a bare English literal. The request id is read from the response headers on the rescue path too, so an unreadable 4xx still carries its X-Request-Id and is reported to error tracking before raising.

    User-facing effect: previously an unreadable 409/422 surfaced as the service-unavailable string; it now surfaces as an ApiError with the real status and a translated fallback message, so the S10 provisioning caller branches on :conflict/:unprocessable instead of :service_unavailable. No present caller retries provisioning, so this closes a latent hazard rather than a live incident.

  2. RETRY_OPTIONS cannot silently gain a retry allowlist bypass. faraday-retry evaluates methods.include?(...) || retry_if.call(...), so a future retry_if key would OR-bypass the idempotent-method allowlist that keeps the state-changing POSTs from being replayed. A spec now asserts that invariant directly (RETRY_OPTIONS has no retry_if key), so adding one is a failing test rather than a silent bypass, without pinning the full key set that harmless additions would break.

  3. The parse-rescue boundary and the single-artifact read path gain the coverage they lacked. The unreadable-body taxonomy is now table-driven across 401/403/409/413/302/429/500 and asserts each classifies by its status, with a dedicated example proving an unreadable 4xx carries its response X-Request-Id and is reported before raising. Single-artifact reads gain the symbol dot-segment and reserved-character rows their list siblings carry, an assertion that a malformed id mints no token, and an Integer id reframed as a .to_s coercion (the client does not validate id shape) rather than a pinned contract. The condition ApiError example gains :aggregate_failures.

  4. The service-token wire contract gets a permanent, reproducible home. A new spec drives the real Rails service client over HTTP against a live AR servicetoken guard, gated behind an explicit AR_WIRE_CONTRACT=1 opt-in alongside AR_SERVICE_TOKEN_ENDPOINT and AR_SERVICE_TOKEN so it never makes a real network call just because a token env var is set for another purpose. The accepted-path example asserts the returned namespace id (not merely that no error raised), which is what proves the guard served the request rather than the path drifting to a 404 the client swallows; the reject path asserts a wrong token fails closed with AuthorizationError. A committed README documents how to stand the guard up. The header literal and the emitted wire form are already pinned over WebMock in client_spec.rb, so this file adds only the real-guard check rather than duplicating them.

No feature flag, no migration, no user-visible change (the consumer flag artifact_registry_ui is default-off), so no changelog trailer.

Database changes

None.

Screenshots or screen recordings

N/A. No user-facing change.

How to set up and validate locally

Unit specs (no AR service needed). The wire-contract spec's real-guard group is skipped here and its two examples show as pending:

bundle exec rspec \
  ee/spec/lib/artifact_registry/client_spec.rb \
  ee/spec/lib/artifact_registry/service_token_wire_contract_spec.rb

Result: 678 examples, 0 failures, 2 pending.

RuboCop on the touched files:

bundle exec rubocop \
  ee/lib/artifact_registry/client.rb \
  ee/spec/lib/artifact_registry/client_spec.rb \
  ee/spec/lib/artifact_registry/service_token_wire_contract_spec.rb

Result: no offenses detected.

Wire contract against a real AR guard (optional, needs the AR binary). Follow ee/spec/lib/artifact_registry/README_service_token_wire_contract.md to build and start the servicetoken harness, then:

AR_WIRE_CONTRACT=1 \
AR_SERVICE_TOKEN_ENDPOINT=http://127.0.0.1:8099 \
AR_SERVICE_TOKEN=ar-svc-token \
bundle exec rspec ee/spec/lib/artifact_registry/service_token_wire_contract_spec.rb

Result: 2 examples, 0 failures (the real-guard examples now run). Validated against the real merged AR servicetoken.Middleware (artifact-registry main at 98a7b261): the accepted request arrived carrying Gitlab-Artifact-Registry-Token with an empty Authorization and resolved to the seeded namespace, and a wrong token was rejected fail-closed with AuthorizationError.

Resolves #617755 Resolves #624331 Related to #617828 Related to #621813

Notes on scope:

  • Work item 617754 (compose the GitLab API version constant) is already on master (GITLAB_API_VERSION / NAMESPACES_PATH), so it needs no change here.
  • Work item 617828 Category A is covered (the retry-allowlist spec, the :aggregate_failures add, and the 409-comment correction). Its Category B (observability action: key) and Category C (S10 consumer-facing contract notes) are out of scope and remain on the issue.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

Edited by Narendran

Merge request reports

Loading
Loading