Classify secret-resolution failures by cause

Kam Kyralarequested to merge
kkyrala/gitlab-runner:kkyrala/classify-secret-resolution-failures into main

What does this MR do?

It classifies CI secret-resolution failures by what caused them, instead of always reporting them as runner_system_failure:

  • Provider client errors that point at a job/provider configuration problem (400, 401, 403, 404) now report as runner_configuration_error (which maps to script_failure on instances that don't support it natively).
  • Provider server-side failures (5xx) report as runner_external_dependency_failure (maps to runner_system_failure).
  • Everything else — network errors, timeouts, 429, anything we can't read a status code from — is left alone and keeps today's runner_system_failure default.

The mechanism is generic, but this MR only wires up the Vault resolver. The other secret backends (AWS, GCP, Azure, GitLab Secrets Manager) can follow the same pattern in later MRs.

Why was this MR needed?

When a job's secrets: can't be resolved because the provider refuses the request — a Vault 403 permission denied for the requested path, or a 400 missing role — the runner reported the job as runner_system_failure:

ERROR: Job failed (system failure): resolving secrets: reading secret:
reading from Vault: api error: status code 403: permission denied

Nothing on the runner or its host failed here. The runner reached the provider and the provider refused on purpose, because of the secret path and policy the job declared. That's a job-configuration problem for the job owner to fix, not a platform failure.

runner_system_failure is the reason operators alert and page on. Routing user-config denials into it means operators get paged for things they can't fix, and one misconfigured project is enough to push a shared runner shard past its error SLO and open an infrastructure incident. That's exactly what happened in the incident behind this change.

Root cause

Build.Run returns the error from resolveSecrets directly, and the secret resolvers hand back a plain wrapped error rather than a *BuildError with a FailureReason. In setTraceStatus, anything that can't be unwrapped to a *BuildError falls through to the default and is reported as RunnerSystemFailure. So these were never classified as system failures on purpose — they were untagged, and untagged defaults to system failure.

Solution

  • helpers/vault: expose StatusCode() on the unwrapped API response error, so callers can classify without reaching into the concrete (unexported) error type.
  • helpers/secrets: add two semantic wrapper error types, ResolvingConfigurationError and ResolvingExternalDependencyError.
  • helpers/secrets/resolvers/vault: classifyError reads the API status code and wraps 4xx (config/auth) and 5xx (external dependency) accordingly; anything else passes through untouched.
  • common/build.go: wrapSecretResolvingError maps those semantic errors to ConfigurationError / RunnerExternalDependencyFailure; unclassified errors are returned as-is and keep the previous RunnerSystemFailure behaviour.

Both failure reasons already exist and are already in the compatibility map, so nothing new is needed on the Rails side.

What's the best way to test this MR?

There are unit tests at each layer:

  • helpers/vault/utils_test.go — the StatusCode() accessor.
  • helpers/secrets/resolvers/vault/resolver_test.go — TestClassifyError covers 400/401/403/404 → configuration, 5xx → external dependency, and 429 / no-status / network errors passing through unchanged, including a check that the original error chain is preserved.
  • common/build_test.go — TestWrapSecretResolvingError covers the mapping to ConfigurationError / RunnerExternalDependencyFailure, pass-through of an existing *BuildError, and unmodified pass-through of an unclassified error.
go test ./helpers/vault/... ./helpers/secrets/... ./common/...

To check it by hand: a job whose secrets: references a Vault path its role can't read should now fail with runner_configuration_error (shown to the user as a configuration error) rather than runner_system_failure. A job that hits a Vault 5xx should come through as runner_external_dependency_failure.

Known follow-ups (out of scope)

  • Adopt the same classification in the AWS, GCP, Azure, and GitLab Secrets Manager resolvers.
  • Revisit retry behaviour — attemptResolveSecrets retries every failure, and retrying a 403 is pointless.
  • SLO robustness — this fixes one class of misattributed failure, but doesn't on its own stop every user-caused failure from reaching an operator SLO. Per-project error attribution is a separate effort.

What are the relevant issue numbers?

Closes #39573 (closed)

Merge request reports