From aa8dd153a45ba99413e323e2cda1847d51d90f22 Mon Sep 17 00:00:00 2001 From: "allyblockcast[bot]" Date: Sun, 2 Aug 2026 19:16:51 +0000 Subject: [PATCH 1/2] feat(chart): stamp the approval plan marker on the API pod template (BLO-20733) onprem-k8s#1874 at 5c0abef2 already requires this. approve-paperclip-api-digest.sh hard-fails (exit 2) on any planned Deployment whose pod template lacks paperclip.blockcast.net/approval-plan-sha256, records it on the in-flight lock, and ROLLOUT_COMPLETE_JQ requires the live template to carry that exact value before retiring the lock. The live Deployment is applied by helm upgrade, so the chart is the only thing that can put the marker there. The value is the SHA-256 of the canonical rendered Deployment with the annotation removed, so it cannot be computed inside the template. The release job renders unstamped, hashes, then re-renders with --set. That is only sound while stamping perturbs nothing else in the output, which is what the new tests pin (both mutation-checked). Also runs deploy/helm/paperclip/tests/ in CI for the first time: 21 existing assertions about the shipped manifests had never executed in any workflow. BLO-20733 Co-Authored-By: Claude --- .github/workflows/pr.yml | 36 ++++ .../paperclip/templates/deployment-api.yaml | 29 +++- .../tests/approval-plan-marker.test.mjs | 160 ++++++++++++++++++ deploy/helm/paperclip/values.yaml | 10 ++ 4 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 deploy/helm/paperclip/tests/approval-plan-marker.test.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b81e734bd126..d017686b2268 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] diff --git a/deploy/helm/paperclip/templates/deployment-api.yaml b/deploy/helm/paperclip/templates/deployment-api.yaml index 50e8a90c338d..4f4954ceec78 100644 --- a/deploy/helm/paperclip/templates/deployment-api.yaml +++ b/deploy/helm/paperclip/templates/deployment-api.yaml @@ -41,7 +41,34 @@ 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. + */}} + {{- $podAnnotations := deepCopy (.Values.pod.annotations | default dict) }} + {{- 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 "paperclip.blockcast.net/approval-plan-sha256" . }} + {{- 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..96a3762cf3e4 --- /dev/null +++ b/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs @@ -0,0 +1,160 @@ +// 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/, + ); +}); 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: From 907188096d8cc433bf78a5f919adfb0477fc44ea Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Tue, 4 Aug 2026 00:32:59 +0000 Subject: [PATCH 2/2] fix(chart,ci): reject the reserved marker key and make the Helm lane gate verify (BLO-20733) Both Ally Important findings on PR #973 @ 1dd3154b. 1. deployment-api.yaml copied all of pod.annotations into the map it then stamps, so the release-controlled marker key could arrive from chart values. Two silent failure modes, neither covered by the existing tests (they use an ordinary `example.com/team` key): - api.approvalPlanSha256 UNSET: the value passes straight through, so the render the release job treats as "unstamped" already carries a marker. The hash taken from render #1 is then computed over a document containing a marker and can never match what the approve script recomputes from render #2 -- every release dies at "planned Deployment pod template must carry ...". - api.approvalPlanSha256 SET: `set` silently overwrote the caller's value, hiding a conflict rather than reporting it. The key is release-controlled, so reject it outright instead of picking a winner. That is the only behaviour that keeps render #1 genuinely unstamped. 2. helm_chart ran but did not gate. `verify` is the required context and neither listed helm_chart in `needs` nor asserted its result, so a red Helm lane could sit beside a green required check. Both halves matter and fail independently: without the `needs` entry `needs.helm_chart.result` renders empty and the lane silently never gates; without the map entry the result is collected and ignored. Verified locally (helm + kubectl present): - chart marker suite 7/7, full chart suite 28/28, verify lane suite 11/11 - mutation-proven three ways, each restoring to green: remove the hasKey guard -> 2 chart tests red drop helm_chart from verify.needs -> 1 lane test red drop the lane_results entry -> 2 lane tests red Rebased onto master, which had since rewritten the verify step for cancelled-vs-failed lanes (BLO-20867 #964); the new entry follows that shape. Co-Authored-By: Claude --- .github/workflows/pr.yml | 4 ++- .../paperclip/templates/deployment-api.yaml | 17 +++++++++- .../tests/approval-plan-marker.test.mjs | 32 +++++++++++++++++ .../__tests__/pr-verify-lane-outcome.test.mjs | 34 +++++++++++++++++++ 4 files changed, 85 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index d017686b2268..e5346ad304c5 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -387,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 @@ -418,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 4f4954ceec78..6a3ad2414f2e 100644 --- a/deploy/helm/paperclip/templates/deployment-api.yaml +++ b/deploy/helm/paperclip/templates/deployment-api.yaml @@ -60,13 +60,28 @@ spec: 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 "paperclip.blockcast.net/approval-plan-sha256" . }} + {{- $_ := set $podAnnotations $markerKey . }} {{- end }} {{- with $podAnnotations }} annotations: diff --git a/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs b/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs index 96a3762cf3e4..1051a535a365 100644 --- a/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs +++ b/deploy/helm/paperclip/tests/approval-plan-marker.test.mjs @@ -158,3 +158,35 @@ test("a malformed marker fails the render instead of shipping a plan the approve /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/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::/); +});