From 0dd8cdddd1869ce7973b3939314cbd01b08d4710 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Thu, 17 Sep 2026 20:27:50 +0200 Subject: [PATCH 1/2] fix(ci): run the authorization gate's diagnostics on a real mismatch The gate's step runs two independent commands over the same surface: `go test` is the verdict, and `go run` is the only caller of surfaceMismatchReport, the moved-entry diagnostics added in #3836. A real aggregate mismatch fails BOTH, and the step stopped at the first -- so on exactly the failure the diagnostics exist to explain, they never ran. Measured on #3878 (CI run 35257229458, job 105324573289): the approval base checked out fine, `go test` failed with a4781e58, and zero moved-entry lines were emitted. Identifying the one entry that moved took a purpose-written conservation render, which is the cost #3836 set out to remove. Both commands now run unconditionally and the step fails if either did. The cd.yaml copy had the same ordering and gets the same treatment. The new test executes the extracted step against stub `go` and `git` binaries under the runner's own shell flags, so the assertions are about behaviour rather than YAML shape: which commands ran, and what the step exited with. Each is ablated -- swallowing the status fires the negative control, and removing the diagnostics call fires the short-circuit assertion. Fixes #3879 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/cd.yaml | 13 +- .github/workflows/ci.yaml | 27 ++- .../test-authorization-gate-diagnostics.sh | 222 ++++++++++++++++++ 3 files changed, 257 insertions(+), 5 deletions(-) create mode 100755 scripts/tests/test-authorization-gate-diagnostics.sh diff --git a/.github/workflows/cd.yaml b/.github/workflows/cd.yaml index bfc0c5b70..a6f38505f 100644 --- a/.github/workflows/cd.yaml +++ b/.github/workflows/cd.yaml @@ -162,9 +162,18 @@ jobs: kubectl version --client - name: 🔐 Validate static production authorization controls + # Both commands run unconditionally, and the step fails if either did. + # They are independent outputs over the same surface: `go test` is the + # verdict, `go run` is the only caller of the moved-entry diagnostics. A + # real aggregate mismatch fails BOTH, so short-circuiting on the first + # left the one failure that most needs explaining with no entry names at + # all (#3879). Covered by scripts/tests/test-authorization-gate-diagnostics.sh. run: | - go test ./scripts/validate-eks-ci-role-policy - go run ./scripts/validate-eks-ci-role-policy . + set -euo pipefail + status=0 + go test ./scripts/validate-eks-ci-role-policy || status=1 + go run ./scripts/validate-eks-ci-role-policy . || status=1 + exit "$status" - name: ⚙️ Setup KSail for effective authorization validation shell: bash diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f5e583072..b0fb6a8f9 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -230,6 +230,19 @@ jobs: shellcheck scripts/tests/test-regenerate-publish-workflow-approved-revisions.sh bash scripts/tests/test-regenerate-publish-workflow-approved-revisions.sh + # The authorization gate's verdict and its moved-entry diagnostics are two + # commands in one step, and a real aggregate mismatch fails BOTH — so a step + # that stopped at the first never ran the diagnostics on the one failure they + # exist to explain (#3879). That is a property of the STEP's shape, which no + # script it calls can enforce, so the test executes the step against stub + # binaries and ablates each assertion. Unconditional for the same reason as + # the steps above: its subjects are ci.yaml and cd.yaml themselves, which sit + # outside the k8s paths. + - name: 🔐 Validate the authorization gate reports its diagnostics + run: | + shellcheck scripts/tests/test-authorization-gate-diagnostics.sh + bash scripts/tests/test-authorization-gate-diagnostics.sh + # Both subject guards above examine a file only when it already carries a # shared-publish-workflow subject, so an OCIRepository pointed at a devantler-tech # artifact with NO spec.verify at all is invisible to them — and refused by nothing @@ -906,16 +919,24 @@ jobs: # The HEAD^1 checkout is diagnostic only: on an unapproved aggregate the # validator names the surface entries that differ from it. Without it the # verdict is identical and the moved entries are reported as unknown. + # Both commands run unconditionally, and the step fails if either did. + # They are independent outputs over the same surface: `go test` is the + # verdict, `go run` is the only caller of the moved-entry diagnostics. A + # real aggregate mismatch fails BOTH, so short-circuiting on the first + # left the one failure that most needs explaining with no entry names at + # all (#3879). Covered by scripts/tests/test-authorization-gate-diagnostics.sh. run: | set -euo pipefail - go test ./scripts/validate-eks-ci-role-policy + status=0 + go test ./scripts/validate-eks-ci-role-policy || status=1 base_root="$RUNNER_TEMP/approval-base-tree" if git worktree add --detach "$base_root" HEAD^1; then - go run ./scripts/validate-eks-ci-role-policy . "$base_root" + go run ./scripts/validate-eks-ci-role-policy . "$base_root" || status=1 else echo "::warning::approval base checkout failed; an aggregate mismatch will not name moved entries" - go run ./scripts/validate-eks-ci-role-policy . + go run ./scripts/validate-eks-ci-role-policy . || status=1 fi + exit "$status" - name: ⚙️ Setup KSail for effective authorization validation shell: bash diff --git a/scripts/tests/test-authorization-gate-diagnostics.sh b/scripts/tests/test-authorization-gate-diagnostics.sh new file mode 100755 index 000000000..59be7b887 --- /dev/null +++ b/scripts/tests/test-authorization-gate-diagnostics.sh @@ -0,0 +1,222 @@ +#!/usr/bin/env bash +# Prove the authorization gate reports BOTH of its outputs on a real mismatch +# (devantler-tech/platform#3879). +# +# WHY THIS EXISTS. The "🔐 Validate static production authorization controls" +# step runs two independent commands over the same surface: +# +# * `go test ./scripts/validate-eks-ci-role-policy` — the verdict. On an +# unapproved aggregate TestValidateAuthorizationAcceptsCommittedPolicy +# t.Fatalf's with the bare fingerprint. +# * `go run ./scripts/validate-eks-ci-role-policy . "$base_root"` — the +# diagnostics. This is the ONLY caller of surfaceMismatchReport, the code +# #3836 added to name the surface entries that moved. +# +# A real aggregate mismatch fails BOTH. So a step that short-circuits on the +# first command never reaches the second, and the failure that most needs an +# explanation is the one reported with no entry names at all — measured on +# #3878 (CI run 35257229458, job 105324573289): the approval base checked out +# fine, `go test` failed with `a4781e58…`, and zero moved-entry lines were +# emitted. +# +# The step's script is EXECUTED here against stub `go` and `git` binaries rather +# than pattern-matched, so the assertions are about behaviour: which commands +# ran, and what the step exited with. Every assertion is ABLATED against the +# short-circuiting form the fix replaces; a check that cannot fail is not a +# check. +# +# yq (mikefarah v4) reads the YAML. No network, no secrets, no Go toolchain. +# Bash 3.2 compatible. +set -euo pipefail + +root_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +readonly root_dir +readonly job='validate-eks-authorization' +readonly step='🔐 Validate static production authorization controls' +readonly diagnostic_line='changed helm.toolkit.fluxcd.io/v2|HelmRelease|kube-system|hcloud-csi' + +work_dir="$(mktemp -d)" +readonly work_dir +trap 'rm -rf "${work_dir}"' EXIT + +failures=0 + +fail() { + printf 'FAIL: %s\n' "$1" >&2 + failures=$((failures + 1)) +} + +# extract_step prints the step's `run:` script for one workflow. +extract_step() { + local workflow="$1" + yq -r \ + ".jobs[\"${job}\"].steps[] | select(.name == \"${step}\") | .run" \ + "${workflow}" +} + +# step_shell_flags prints the flags the runner gives the step's interpreter, so +# the script is exercised the way GitHub actually runs it rather than under a +# bare `bash`. A step with no `shell:` key gets `bash -e {0}`; an explicit +# `shell: bash` gets `bash --noprofile --norc -eo pipefail {0}`. The difference +# decides whether a failing first command aborts the step, which is the very +# property under test — running these under a plain `bash` reports a swallowed +# failure that the runner would never produce. +step_shell_flags() { + local workflow="$1" declared + declared="$(yq -r \ + ".jobs[\"${job}\"].steps[] | select(.name == \"${step}\") | .shell // \"\"" \ + "${workflow}")" + case "${declared}" in + bash) printf -- '-e -o pipefail' ;; + '') printf -- '-e' ;; + *) printf -- '-e' ;; + esac +} + +# make_stubs builds a PATH directory whose `go` records each invocation and +# exits with the caller's chosen status, and whose `git` always succeeds so the +# approval-base branch is taken. +# +# $1 work root $2 `go test` exit status $3 `go run` exit status +make_stubs() { + local dir="$1" test_status="$2" run_status="$3" + mkdir -p "${dir}/bin" + cat >"${dir}/bin/go" <>"${dir}/invocations" +case "\$1" in + test) printf 'FAIL\tgithub.com/devantler-tech/platform/scripts/validate-eks-ci-role-policy\n'; exit ${test_status} ;; + run) printf 'EKS CI role policy: ${diagnostic_line}\n'; exit ${run_status} ;; +esac +exit 0 +EOF + cat >"${dir}/bin/git" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF + chmod +x "${dir}/bin/go" "${dir}/bin/git" + : >"${dir}/invocations" +} + +# run_step executes a step script under the stubs and prints +# "||". +# +# $1 script path $2 `go test` exit status $3 `go run` exit status +# $4 the runner's shell flags for that step +run_step() { + local script="$1" test_status="$2" run_status="$3" shell_flags="$4" + local sandbox status output + sandbox="$(mktemp -d "${work_dir}/sandbox.XXXXXX")" + make_stubs "${sandbox}" "${test_status}" "${run_status}" + set +e + # shellcheck disable=SC2086 # shell_flags is a controlled flag list, not a path. + output="$(PATH="${sandbox}/bin:${PATH}" RUNNER_TEMP="${sandbox}" \ + bash ${shell_flags} "${script}" 2>&1)" + status=$? + set -e + printf '%s|%s|%s\n' \ + "${status}" \ + "$(tr '\n' ' ' <"${sandbox}/invocations" | sed 's/ *$//')" \ + "$(printf '%s' "${output}" | tr '\n' ' ')" +} + +# field reads one of run_step's three fields. The captured output is LAST and is +# read with an open-ended range, because a surface entry is itself pipe-separated +# (`apiVersion|Kind|namespace|name`) — a fixed `-f3` truncates the diagnostics at +# their first separator and reports a line that was printed as missing. +field() { + case "$2" in + 3) printf '%s' "$1" | cut -d'|' -f3- ;; + *) printf '%s' "$1" | cut -d'|' -f"$2" ;; + esac +} + +# assert_gate_behaviour runs the three cases every copy of the step must satisfy. +# +# $1 label $2 script path $3 shell flags +# $4 "diagnostics" to require the moved-entry line +assert_gate_behaviour() { + local label="$1" script="$2" shell_flags="$3" mode="${4:-}" + local result + + # Case A — a real aggregate mismatch: both commands fail. The step must run + # the diagnostics anyway, and must still fail. + result="$(run_step "${script}" 1 1 "${shell_flags}")" + case " $(field "${result}" 2) " in + *' run '*) : ;; + *) fail "${label}: a failing 'go test' short-circuited the step before 'go run' — the moved-entry diagnostics never execute on the one failure they exist to explain (got invocations: '$(field "${result}" 2)')" ;; + esac + [ "$(field "${result}" 1)" != "0" ] || + fail "${label}: the step exited 0 while both commands failed" + if [ "${mode}" = "diagnostics" ]; then + case "$(field "${result}" 3)" in + *"${diagnostic_line}"*) : ;; + *) fail "${label}: the moved-entry diagnostics were not reported (got: '$(field "${result}" 3)')" ;; + esac + fi + + # Case B — negative control: `go test` fails for a reason unrelated to the + # aggregate, so `go run` passes. Running both must not swallow the failure. + result="$(run_step "${script}" 1 0 "${shell_flags}")" + [ "$(field "${result}" 1)" != "0" ] || + fail "${label}: a failing 'go test' was swallowed when 'go run' passed — the step reports success on a broken validator" + + # Case C — control: both pass, so the step passes. + result="$(run_step "${script}" 0 0 "${shell_flags}")" + [ "$(field "${result}" 1)" = "0" ] || + fail "${label}: the step failed while both commands passed (exit $(field "${result}" 1): '$(field "${result}" 3)')" +} + +for workflow_name in ci cd; do + workflow="${root_dir}/.github/workflows/${workflow_name}.yaml" + [ -f "${workflow}" ] || { fail "${workflow_name}.yaml is missing"; continue; } + + script="${work_dir}/${workflow_name}-step.sh" + extract_step "${workflow}" >"${script}" + shell_flags="$(step_shell_flags "${workflow}")" + if [ ! -s "${script}" ] || grep -qx 'null' "${script}"; then + fail "${workflow_name}.yaml: job '${job}' has no step named '${step}' — the gate this test pins was renamed or removed" + continue + fi + grep -q 'validate-eks-ci-role-policy' "${script}" || + fail "${workflow_name}.yaml: the extracted step does not invoke the authorization validator" + + # ci.yaml checks out the approval base, so its diagnostics name the entries. + if grep -q 'approval-base-tree' "${script}"; then + assert_gate_behaviour "${workflow_name}.yaml" "${script}" "${shell_flags}" diagnostics + else + assert_gate_behaviour "${workflow_name}.yaml" "${script}" "${shell_flags}" + fi +done + +# ABLATION. The short-circuiting form this fix replaces must fail Case A, and +# must fail naming the short-circuit rather than something incidental. A test +# that passes against the defect proves nothing. +ablation="${work_dir}/ablation-step.sh" +cat >"${ablation}" <<'EOF' +set -euo pipefail +go test ./scripts/validate-eks-ci-role-policy +base_root="$RUNNER_TEMP/approval-base-tree" +if git worktree add --detach "$base_root" HEAD^1; then + go run ./scripts/validate-eks-ci-role-policy . "$base_root" +else + go run ./scripts/validate-eks-ci-role-policy . +fi +EOF +ablation_result="$(run_step "${ablation}" 1 1 '-e')" +case " $(field "${ablation_result}" 2) " in + *' run '*) + fail "ABLATION: the short-circuiting step reached 'go run', so Case A cannot detect the defect it is written for" + ;; + *) : ;; +esac +[ "$(field "${ablation_result}" 1)" != "0" ] || + fail "ABLATION: the short-circuiting step exited 0, so Case B's control is not exercised" + +if [ "${failures}" -ne 0 ]; then + printf '%s: %d assertion(s) failed\n' "$(basename "$0")" "${failures}" >&2 + exit 1 +fi + +printf '%s: authorization gate reports its verdict AND its diagnostics in ci.yaml and cd.yaml\n' \ + "$(basename "$0")" From b34aef05073fec1beeb0ceeee9fed44ce722faa3 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Thu, 17 Sep 2026 23:22:33 +0200 Subject: [PATCH 2/2] test(ci): pin the half of the gate contract that only the diagnostics can break The step's contract is that both commands run and the step fails if either did. Its test covered a failing `go test` in both directions but never the mirror -- `go test` passing while only `go run` fails -- which is the half the diagnostics themselves live in. Cases A, B and D are all satisfied by a step that ignores `go run` entirely, so swallowing its failure would have landed silently and the moved-entry diagnostics would have stopped being checked at all. Ablation: replacing `|| status=1` with `|| true` on both `go run` lines fails the new case on ci.yaml and cd.yaml and leaves every other case passing. Dropping the `||` clause outright does not fire it, because `set -e` then aborts the step anyway -- which is why the case is written against the swallow. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/test-authorization-gate-diagnostics.sh | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/scripts/tests/test-authorization-gate-diagnostics.sh b/scripts/tests/test-authorization-gate-diagnostics.sh index 59be7b887..8d6584f38 100755 --- a/scripts/tests/test-authorization-gate-diagnostics.sh +++ b/scripts/tests/test-authorization-gate-diagnostics.sh @@ -131,7 +131,7 @@ field() { esac } -# assert_gate_behaviour runs the three cases every copy of the step must satisfy. +# assert_gate_behaviour runs the four cases every copy of the step must satisfy. # # $1 label $2 script path $3 shell flags # $4 "diagnostics" to require the moved-entry line @@ -161,7 +161,16 @@ assert_gate_behaviour() { [ "$(field "${result}" 1)" != "0" ] || fail "${label}: a failing 'go test' was swallowed when 'go run' passed — the step reports success on a broken validator" - # Case C — control: both pass, so the step passes. + # Case C — the mirror of B, and the half of "the step fails if either did" that + # nothing else here covers: the verdict passes and only the diagnostics fail. + # Cases A, B and D are all satisfied by a step that ignores `go run` entirely, + # so dropping `|| status=1` from either `go run` line would otherwise land + # silently — and the diagnostics this gate exists for would stop being checked. + result="$(run_step "${script}" 0 1 "${shell_flags}")" + [ "$(field "${result}" 1)" != "0" ] || + fail "${label}: a failing 'go run' was swallowed when 'go test' passed — the step reports success although the moved-entry diagnostics did not run" + + # Case D — control: both pass, so the step passes. result="$(run_step "${script}" 0 0 "${shell_flags}")" [ "$(field "${result}" 1)" = "0" ] || fail "${label}: the step failed while both commands passed (exit $(field "${result}" 1): '$(field "${result}" 3)')"