diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b81e734bd126..e5346ad304c5 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -141,6 +141,42 @@ jobs: retention-days: 1 if-no-files-found: error + # The chart tests under deploy/helm/paperclip/tests/ were never wired into any + # workflow -- 21 assertions about the manifests we actually ship (Penstock + # routing, ServiceMonitor RBAC, runtime caches, probes) that no CI run has ever + # executed. This job runs them, and gates the BLO-20733 approval-plan marker: + # the release channel's completion check is bound to a marker computed from an + # unstamped render, which is only sound while stamping perturbs nothing else. + # + # Deliberately its own job rather than a step in `policy`: it needs helm and + # kubectl, and a tooling regression in provisioning those should not take down + # the gate that every unrelated PR depends on. + helm_chart: + name: Helm chart + runs-on: arc-light + timeout-minutes: 10 + + steps: + - name: Checkout repository + uses: actions/checkout@v6 + + - name: Setup Node.js + uses: actions/setup-node@v6 + with: + node-version: 24 + + - name: Install helm + uses: azure/setup-helm@v4 + + # Only ever invoked as `kubectl create --dry-run=client -o json`, i.e. as + # an offline YAML reader so the render comparisons are structural rather + # than textual. No cluster credential is used or needed. + - name: Install kubectl + uses: azure/setup-kubectl@v4 + + - name: Test Helm chart renders + run: node --test ./deploy/helm/paperclip/tests/*.test.mjs + typecheck_release_registry: name: Typecheck + Release Registry needs: [policy] @@ -351,7 +387,7 @@ jobs: # Preserve the legacy required-check name while the underlying work runs in parallel. name: verify if: ${{ always() }} - needs: [typecheck_release_registry, general_tests, worktree_install, build] + needs: [typecheck_release_registry, general_tests, worktree_install, build, helm_chart] runs-on: arc-light timeout-minutes: 5 @@ -382,12 +418,14 @@ jobs: GENERAL_TESTS_RESULT: ${{ needs.general_tests.result }} WORKTREE_INSTALL_RESULT: ${{ needs.worktree_install.result }} BUILD_RESULT: ${{ needs.build.result }} + HELM_CHART_RESULT: ${{ needs.helm_chart.result }} run: | declare -A lane_results=( [typecheck_release_registry]="$TYPECHECK_RELEASE_REGISTRY_RESULT" [general_tests]="$GENERAL_TESTS_RESULT" [worktree_install]="$WORKTREE_INSTALL_RESULT" [build]="$BUILD_RESULT" + [helm_chart]="$HELM_CHART_RESULT" ) cancelled_lanes=() diff --git a/deploy/helm/paperclip/templates/deployment-api.yaml b/deploy/helm/paperclip/templates/deployment-api.yaml index 50e8a90c338d..6a3ad2414f2e 100644 --- a/deploy/helm/paperclip/templates/deployment-api.yaml +++ b/deploy/helm/paperclip/templates/deployment-api.yaml @@ -41,7 +41,49 @@ spec: {{- with .Values.pod.labels }} {{- toYaml . | nindent 8 }} {{- end }} - {{- with .Values.pod.annotations }} + {{- /* + BLO-20733: stamp the approval plan marker onto the POD TEMPLATE, so it is + part of the rolled-out spec rather than top-level metadata a config-only + edit could carry. scripts/approve-paperclip-api-digest.sh refuses any plan + whose template lacks it, records it on the in-flight lock, and its + ROLLOUT_COMPLETE_JQ requires the live template to carry this exact value + before retiring that lock -- so a healthy rollout of a DIFFERENT plan that + happens to reuse the approved digest can no longer retire it. + + The marker's value is the SHA-256 of the canonical rendered Deployment + with this annotation removed, which is why it cannot be computed inside + the template (it would hash a document containing itself). The deploy job + renders once unstamped, hashes, then re-renders with the value set. + + That two-pass scheme is only sound if setting this value changes NOTHING + ELSE in the rendered output. Keeping the stamp confined to this one merged + key is what preserves it, and + deploy/helm/paperclip/tests/approval-plan-marker.test.mjs pins it by + diffing a stamped render against an unstamped one. + + The marker key is RELEASE-CONTROLLED, so pod.annotations may not carry it. + Without this guard the key has two silent failure modes, both of which + defeat the binding above. With api.approvalPlanSha256 unset, a value from + pod.annotations would be copied straight through -- stamping an + unvalidated marker into the render the release job treats as "unstamped", + so the hash it computes is taken over a document that already contains a + marker and can never match what the approve script recomputes. With it + set, `set` would silently overwrite the caller's value, hiding the + conflict rather than reporting it. Rejecting the key outright is the only + behaviour that keeps render #1 genuinely unstamped. + */}} + {{- $markerKey := "paperclip.blockcast.net/approval-plan-sha256" }} + {{- $podAnnotations := deepCopy (.Values.pod.annotations | default dict) }} + {{- if hasKey $podAnnotations $markerKey }} + {{- fail (printf "pod.annotations must not set %s: it is release-controlled and stamped from api.approvalPlanSha256" $markerKey) }} + {{- end }} + {{- with .Values.api.approvalPlanSha256 }} + {{- if not (regexMatch "^[0-9a-f]{64}$" .) }} + {{- fail (printf "api.approvalPlanSha256 must be 64 lowercase hex characters, got %q" .) }} + {{- end }} + {{- $_ := set $podAnnotations $markerKey . }} + {{- end }} + {{- with $podAnnotations }} annotations: {{- toYaml . | nindent 8 }} {{- end }} diff --git a/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs b/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs new file mode 100644 index 000000000000..1051a535a365 --- /dev/null +++ b/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs @@ -0,0 +1,192 @@ +// BLO-20733 — the approval plan marker the release channel binds completion to. +// +// scripts/approve-paperclip-api-digest.sh (Blockcast/onprem-k8s, and the +// vendored copy here) refuses any planned Deployment whose POD TEMPLATE lacks +// `paperclip.blockcast.net/approval-plan-sha256`, and computes the value it +// expects as: +// +// sha256( jq -cS 'del(.spec.template.metadata.annotations[marker])' ) +// +// i.e. the canonical full Deployment with the marker itself removed. The marker +// therefore cannot be produced inside the Helm template — it would have to hash +// a document containing its own value. The release job instead renders the +// chart TWICE: once unstamped to obtain the hash, then again with +// `--set api.approvalPlanSha256=` to produce the manifest it both hands +// to the approve script and deploys. +// +// That scheme is sound if and only if stamping changes NOTHING ELSE in the +// rendered output. If it did, the hash taken from render #1 would not match the +// hash the approve script recomputes from render #2, and every release would +// die at "planned Deployment pod template must carry ...". These tests pin that +// invariant — they are the "rendered chart output and the manifest the approve +// script hashes are demonstrated to agree" acceptance criterion. +import { execFileSync } from "node:child_process"; +import assert from "node:assert/strict"; +import { test } from "node:test"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../..", +); + +const MARKER = "paperclip.blockcast.net/approval-plan-sha256"; +// Any 64-hex string exercises the plumbing; the real value is a SHA-256 the +// release job computes from the unstamped render. +const SAMPLE = "a".repeat(64); + +function renderApiDeployment(extraArgs = []) { + const yaml = execFileSync( + "helm", + [ + "template", + "paperclip", + "deploy/helm/paperclip", + "--namespace", + "paperclip", + "-f", + "deploy/helm/paperclip/values.blockcast.yaml", + "--show-only", + "templates/deployment-api.yaml", + ...extraArgs, + ], + { cwd: repoRoot, encoding: "utf8" }, + ); + // kubectl is the YAML->JSON reader here purely so the comparison below is + // structural rather than textual; --dry-run=client needs no cluster. + return JSON.parse( + execFileSync("kubectl", ["create", "--dry-run=client", "-o", "json", "-f", "-"], { + input: yaml, + encoding: "utf8", + stdio: ["pipe", "pipe", "ignore"], + }), + ); +} + +// Mirrors CANONICAL_UNSTAMPED_PLAN in approve-paperclip-api-digest.sh, including +// its empty-map cleanup: after deleting the only annotation, jq's `del` leaves +// `annotations: {}` behind, which is NOT what an unstamped render produces. The +// script prunes it, so the equivalence asserted here is the one it actually +// computes. Keep the two in step. +function stripMarker(deployment) { + const stripped = structuredClone(deployment); + const templateMeta = stripped.spec?.template?.metadata; + if (!templateMeta?.annotations) return stripped; + + delete templateMeta.annotations[MARKER]; + if (Object.keys(templateMeta.annotations).length === 0) { + delete templateMeta.annotations; + } + if (Object.keys(templateMeta).length === 0) { + delete stripped.spec.template.metadata; + } + return stripped; +} + +test("no marker is stamped by default (non-Blockcast deploys skip the approval channel)", () => { + const rendered = renderApiDeployment(); + + assert.equal( + rendered.spec.template.metadata.annotations?.[MARKER], + undefined, + "unset api.approvalPlanSha256 must render no marker annotation", + ); +}); + +test("the marker lands on the POD TEMPLATE, not top-level Deployment metadata", () => { + const rendered = renderApiDeployment([`--set`, `api.approvalPlanSha256=${SAMPLE}`]); + + assert.equal( + rendered.spec.template.metadata.annotations[MARKER], + SAMPLE, + "the marker must be on spec.template.metadata.annotations so it is part of the rolled-out pod spec", + ); + // Top-level metadata survives a config-only edit that never rolls pods, which + // is exactly the evidence ROLLOUT_COMPLETE_JQ must not accept. + assert.equal( + rendered.metadata.annotations?.[MARKER], + undefined, + "the marker must NOT be stamped on top-level Deployment metadata", + ); +}); + +// The load-bearing test: without this, a two-pass render is unsound. +test("stamping the marker changes nothing else in the rendered Deployment", () => { + const unstamped = renderApiDeployment(); + const stamped = renderApiDeployment([`--set`, `api.approvalPlanSha256=${SAMPLE}`]); + + assert.notDeepEqual( + stamped, + unstamped, + "sanity: the stamped render must actually differ, or this test proves nothing", + ); + assert.deepEqual( + stripMarker(stamped), + unstamped, + "stamped render minus the marker must equal the unstamped render, or the hash " + + "computed from render #1 cannot match what the approve script recomputes from render #2", + ); +}); + +test("the invariant also holds when pod.annotations is already non-empty", () => { + // values.blockcast.yaml currently leaves pod.annotations empty, so the stamp + // is what creates the annotations map. Pin the other branch too: an operator + // adding a pod annotation later must not break the release channel, and + // toYaml's alphabetical ordering must not shift anything. + const existing = [`--set`, `pod.annotations.example\\.com/team=paperclip`]; + const unstamped = renderApiDeployment(existing); + const stamped = renderApiDeployment([ + ...existing, + `--set`, + `api.approvalPlanSha256=${SAMPLE}`, + ]); + + assert.equal( + stamped.spec.template.metadata.annotations["example.com/team"], + "paperclip", + "stamping must not clobber pre-existing pod annotations", + ); + assert.deepEqual(stripMarker(stamped), unstamped); +}); + +test("a malformed marker fails the render instead of shipping a plan the approver rejects", () => { + // Fail at render time, where the message names the value, rather than several + // steps later inside the approve script where it reads as a hash mismatch. + assert.throws( + () => renderApiDeployment([`--set`, `api.approvalPlanSha256=not-a-sha`]), + /approvalPlanSha256 must be 64 lowercase hex/, + ); +}); + +// The marker key is release-controlled. Both collision cases below are silent +// without the hasKey guard in deployment-api.yaml, and both break the two-pass +// render in a way that only surfaces later, inside the approve script, as an +// unexplained hash mismatch. The tests above use `example.com/team`, an +// ordinary key, so they cannot catch either one. +const podMarkerArg = [`--set`, `pod.annotations.${MARKER.replace(/\./g, "\\.")}=${SAMPLE}`]; + +test("pod.annotations may not smuggle the marker in when api.approvalPlanSha256 is UNSET", () => { + // The dangerous case: this render is the one the release job hashes as + // "unstamped". A marker reaching it means the hash is taken over a document + // that already carries a marker, so the value the approve script recomputes + // from render #2 can never agree with it. + assert.throws( + () => renderApiDeployment(podMarkerArg), + /pod\.annotations must not set paperclip\.blockcast\.net\/approval-plan-sha256/, + ); +}); + +test("pod.annotations may not silently lose to the release value when BOTH are set", () => { + // Previously `set` overwrote the caller's value and rendered successfully, + // reporting nothing. Conflicting intent must be an error, not a winner. + assert.throws( + () => + renderApiDeployment([ + ...podMarkerArg, + `--set`, + `api.approvalPlanSha256=${"b".repeat(64)}`, + ]), + /pod\.annotations must not set paperclip\.blockcast\.net\/approval-plan-sha256/, + ); +}); diff --git a/deploy/helm/paperclip/values.yaml b/deploy/helm/paperclip/values.yaml index 4f8a8a9de2bf..a12df1afd762 100644 --- a/deploy/helm/paperclip/values.yaml +++ b/deploy/helm/paperclip/values.yaml @@ -87,6 +87,16 @@ api: # -- Soft anti-affinity to spread API replicas across nodes. Falls back to # co-located if the cluster has only one schedulable paperclip node. spreadAcrossNodes: true + # -- Approval plan marker (BLO-20733), stamped onto the API pod template as + # `paperclip.blockcast.net/approval-plan-sha256`. 64 lowercase hex, or empty + # to render no annotation (the default -- local and non-Blockcast deploys do + # not go through the admission approval channel). + # + # Set ONLY by the release job, and never by hand: the value is the SHA-256 of + # the canonical rendered Deployment with this annotation removed, so it can + # only be obtained by rendering the chart unstamped first. A hand-set value + # makes `approve-paperclip-api-digest.sh` refuse the plan. + approvalPlanSha256: "" # -- Kubernetes Service exposing the HTTP UI/API. service: diff --git a/scripts/__tests__/pr-verify-lane-outcome.test.mjs b/scripts/__tests__/pr-verify-lane-outcome.test.mjs index 5b099624dcd4..13acb119bdb0 100644 --- a/scripts/__tests__/pr-verify-lane-outcome.test.mjs +++ b/scripts/__tests__/pr-verify-lane-outcome.test.mjs @@ -35,6 +35,7 @@ function runVerifyStep(results) { GENERAL_TESTS_RESULT: results.general_tests ?? "success", WORKTREE_INSTALL_RESULT: results.worktree_install ?? "success", BUILD_RESULT: results.build ?? "success", + HELM_CHART_RESULT: results.helm_chart ?? "success", }; return spawnSync("bash", ["-c", script], { env, encoding: "utf8" }); } @@ -107,3 +108,36 @@ test("verify step exits non-zero for an unrecognized result and treats it as a f assert.match(result.stdout, /::error title=verify: lane failure::/); assert.match(result.stdout, /general_tests/); }); + +// BLO-20733 / Ally review of PR #973: the `Helm chart` lane exists and runs, but +// running is not gating. `verify` is the required context, so a lane it neither +// `needs` nor asserts can go red beside a green required check. Both halves are +// load-bearing and fail independently: +// - absent from `needs` => `needs.helm_chart.result` renders EMPTY, and the +// lane silently never gates (the failure mode being closed here). +// - absent from the script => the result is collected and ignored. +test("verify declares helm_chart as a dependency, so its result is actually populated", () => { + const needsMatch = workflow.match(/\n {2}verify:\n(?: {4}.*\n| *\n)*? {4}needs: \[([^\]]*)\]/); + assert.notEqual(needsMatch, null, "verify job must declare a needs list"); + + const needs = needsMatch[1].split(",").map((lane) => lane.trim()); + assert.ok( + needs.includes("helm_chart"), + `verify.needs must include helm_chart or its result is always empty; got: ${needs.join(", ")}`, + ); +}); + +test("verify step exits non-zero when the Helm chart lane fails", () => { + const result = runVerifyStep({ helm_chart: "failure" }); + assert.notEqual(result.status, 0); + assert.match(result.stdout, /::error title=verify: lane failure::/); + assert.match(result.stdout, /helm_chart/); +}); + +test("verify step reports a cancelled Helm chart lane as cancelled, not as a diff defect", () => { + const result = runVerifyStep({ helm_chart: "cancelled" }); + assert.notEqual(result.status, 0); + assert.match(result.stdout, /::error title=verify: lane cancelled::/); + assert.match(result.stdout, /helm_chart/); + assert.doesNotMatch(result.stdout, /::error title=verify: lane failure::/); +});