From 09a3f5c14ebaa96e6ae6898246544b0cd10d54d7 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Fri, 4 Sep 2026 06:22:30 +0000 Subject: [PATCH 1/3] fix(release): pin the running digest in the approval window (BLO-28483) The approval window is bounded at MAX_APPROVED_DIGESTS and was ordered purely by age. That is backwards under the failure the window exists to cover: a deploy that fails before its rollout lands still consumes a slot permanently, so a run of consecutive failures -- exactly when a rollback is wanted -- is what ages out the digest actually serving traffic. Once it is gone, helm cannot roll back to the running state and the admission policy denies the attempt, turning a transient upgrade failure into a release that cannot self-heal. Pin the running digest immediately behind the one being released, ahead of the age-ordered fill. This reorders eviction only; the bound is unchanged, so the maxApprovedApiDigests CEL variable does not move and the writer and policy stay in lockstep. The cost is one historical slot, which is the right trade: an older digest is a convenience, the running one is the only guaranteed-good rollback target. live_running_digest degrades to empty on every failure path -- unreachable Deployment, tag-pinned image, foreign repository, containers disagreeing -- so it is an availability safeguard, never a new gate that can fail an otherwise valid release. Ring construction moves into build_approval_ring so it can be exercised directly. Tests cover the pin, dedup against a config-only release, the degrade path, and the bound; a CONTROL test asserts the pre-fix ordering loses the running digest in two deploys, so the suite can actually fail. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.sh | 130 ++++++++--- scripts/approve-paperclip-api-digest.test.js | 215 +++++++++++++++++++ 2 files changed, 321 insertions(+), 24 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.sh b/scripts/approve-paperclip-api-digest.sh index 74d6a92b218f..00918fdbf67b 100755 --- a/scripts/approve-paperclip-api-digest.sh +++ b/scripts/approve-paperclip-api-digest.sh @@ -21,15 +21,26 @@ # the SHA-256 of the canonical full Deployment with that annotation removed. # The approval probe and the release must submit the same stamped manifest. # -# The window is a ring of at most MAX_APPROVED_DIGESTS entries, newest first: -# the digest being released plus the most recently approved ones. That keeps an -# immediate rollback available without accepting every historical digest. Rolling -# back past the window is deliberately an explicit act — re-run this script -# naming that digest. +# The window is a ring of at most MAX_APPROVED_DIGESTS entries: the digest being +# released, then the digest the cluster is currently running, then the most +# recently approved ones. That keeps an immediate rollback available without +# accepting every historical digest. Rolling back past the window is deliberately +# an explicit act — re-run this script naming that digest. +# +# The running digest is pinned ahead of the age-ordered fill rather than taking +# its chances in it. A deploy that fails before its rollout lands still consumes +# a slot permanently, so under plain newest-first ordering a run of consecutive +# failures — exactly when a rollback is wanted — is what ages out the digest +# actually serving traffic, and helm can then no longer roll back to it +# (BLO-28483). Pinning reorders eviction only; the bound is unchanged, so +# the maxApprovedApiDigests CEL variable does not move. # # An approval holds an in-flight lock until its rollout actually lands, so two -# releases cannot rotate the ring underneath each other. If a release fails and -# will never complete, retire its lock explicitly: +# releases cannot rotate the ring underneath each other. Retiring that lock — +# whether automatically on abort or explicitly via the escape hatch below — +# deliberately leaves the ring alone, so an abandoned digest keeps its slot until +# it ages out normally. If a release fails and will never complete, retire its +# lock explicitly: # # PAPERCLIP_APPROVAL_ABANDON_IN_FLIGHT=sha256: \ # PAPERCLIP_APPROVAL_ABANDON_IN_FLIGHT_OWNER= \ @@ -433,6 +444,87 @@ live_deployment_completed_digest() { "$ROLLOUT_COMPLETE_JQ" <<<"$live_json" >/dev/null } +# The digest the cluster is actually serving right now, or empty when that cannot +# be established. Only a digest of OUR repository counts: a sidecar's image is not +# a rollback target for this Deployment. A multi-image pod template is likewise +# refused rather than guessed at -- the completion predicate above already requires +# every container to carry the same image, so disagreement means something outside +# this channel's model is going on and pinning would be a guess. +# +# Every failure path returns empty and succeeds. This is an availability +# safeguard, not a gate: not being able to name the live digest must degrade to +# the previous age-ordered behaviour, never fail an otherwise valid release. +live_running_digest() { + local live_json image + live_json="$("${deploy_kubectl[@]}" -n "$DEPLOY_NAMESPACE" \ + get deployment "$DEPLOYMENT" -o json 2>/dev/null)" || return 0 + image="$(jq -r ' + [ .spec.template.spec.containers[]?.image // empty ] as $images + | if ($images | length) > 0 and (($images | unique | length) == 1) + then $images[0] + else "" + end + ' <<<"$live_json" 2>/dev/null)" || return 0 + [[ "$image" == "${IMAGE_REPOSITORY}@sha256:"* ]] || return 0 + printf '%s\n' "${image#*@}" +} + +# Build the approval window, newest-first, from the digest being released, the +# digest currently running, and the existing list on stdin. +# +# A plain prepend-and-truncate evicts by age alone, which is exactly backwards +# under the failure this window exists to cover. Every failed deploy approves a +# digest that never reached the cluster and permanently consumes a slot, so a run +# of consecutive failures -- precisely when a rollback is needed -- is what ages +# the running digest out. Once it is gone, helm cannot roll back to the state +# actually serving traffic, and a transient upgrade failure becomes a wedged +# release that cannot self-heal (BLO-28483). +# +# So the running digest is pinned immediately behind the one being released, +# ahead of the age-ordered fill. This REORDERS eviction; it does not widen the +# window. The total stays bounded by $3, so the maxApprovedApiDigests CEL +# variable in paperclip/paperclip-public-tools.yaml is untouched and cannot +# drift. The cost is one historical slot, which is the correct trade: an older +# digest is a convenience, the running one is the only guaranteed-good rollback +# target. +# +# Extracted verbatim and exercised by scripts/approve-paperclip-api-digest.test.js, +# so a rewrite fails that test rather than silently reverting the guarantee. +build_approval_ring() { + local new_digest="$1" live_digest="$2" max="$3" + local -a ring=("$new_digest") + local entry + + # Pin only a well-formed, distinct digest, and only when there is a slot for it + # after the released digest. A config-only release reusing the running digest + # lands in the "not distinct" branch and needs no pin -- it is already slot 1. + if (( max >= 2 )) \ + && [[ "$live_digest" =~ ^sha256:[0-9a-f]{64}$ && "$live_digest" != "$new_digest" ]]; then + ring+=("$live_digest") + else + live_digest="" + fi + + # Anything malformed already in the list is discarded rather than carried + # forward -- the policy would ignore it anyway, and leaving it in place would + # consume a slot in the window. Entries already placed above are dropped here so + # they cannot appear twice. + while IFS= read -r entry; do + (( ${#ring[@]} < max )) || break + [[ -n "$entry" ]] || continue + ring+=("$entry") + done < <( + sed $'s/^\r*//; s/\r*$//' \ + | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ + | grep -E '^sha256:[0-9a-f]{64}$' \ + | grep -Fxv "$new_digest" \ + | { if [[ -n "$live_digest" ]]; then grep -Fxv "$live_digest"; else cat; fi } \ + || true + ) + + printf '%s\n' "${ring[@]}" +} + MAX_ROTATE_ATTEMPTS="${PAPERCLIP_APPROVAL_ROTATE_ATTEMPTS:-5}" replace_err="$(mktemp "${TMPDIR:-/tmp}/paperclip-approve-err.XXXXXX")" nonce_err="$(mktemp "${TMPDIR:-/tmp}/paperclip-approve-nonce.XXXXXX")" @@ -548,26 +640,16 @@ for attempt in $(seq 1 "$MAX_ROTATE_ATTEMPTS"); do current_raw="$(jq -r --arg key "$DATA_KEY" '.data[$key] // ""' <<<"$current_json")" - # Keep only well-formed digests, drop the one being approved wherever it already - # sits, then prepend it. Anything malformed already in the list is discarded here - # rather than carried forward — the policy would ignore it anyway, and leaving it - # in place would consume a slot in the window. - mapfile -t existing < <( + # Read the running digest fresh on every rotation attempt: a 409 sends us back + # through here, and a rollout that landed in the meantime changes what the + # rollback target is. + live_digest="$(live_running_digest)" + + mapfile -t approved < <( printf '%s\n' "$current_raw" \ - | sed $'s/^\r*//; s/\r*$//' \ - | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ - | grep -E '^sha256:[0-9a-f]{64}$' \ - | grep -Fxv "$DIGEST" \ - || true + | build_approval_ring "$DIGEST" "$live_digest" "$MAX_APPROVED_DIGESTS" ) - approved=("$DIGEST") - for entry in "${existing[@]:-}"; do - [[ -n "$entry" ]] || continue - (( ${#approved[@]} < MAX_APPROVED_DIGESTS )) || break - approved+=("$entry") - done - payload=$(printf '%s\n' "${approved[@]}") # An exact retry takes ownership with a resourceVersion-guarded write while diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index 25d1121f02ae..1807d13ed749 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -263,3 +263,218 @@ test("valid knobs pass validation and the script proceeds to the deploy credenti assert.match(result.stderr, /PAPERCLIP_DEPLOY_KUBECONFIG must name the deploy credential/); assert.doesNotMatch(result.stderr, /is not a positive integer/); }); + +// --- Approval window eviction order (BLO-28483) --------------------------- +// +// The window is bounded and was ordered purely by age, which is backwards under +// the failure it exists to cover: a deploy that fails before its rollout lands +// still consumes a slot forever, so a run of consecutive failures ages out the +// digest actually serving traffic. helm then cannot roll back to the running +// state and a transient upgrade failure becomes a permanently wedged release. +// The fix pins the running digest behind the one being released. These tests +// hold that guarantee, and the control below proves they can actually fail. + +const RING_FUNCTION_NAME = "build_approval_ring"; +const ringSource = extractShellFunction(RING_FUNCTION_NAME); + +// Read the bound out of the script for the same reason the knobs are: a test +// asserting against a hard-coded 3 would go quietly green if the constant and +// the CEL variable it must match were ever moved together. +function shellReadonly(name) { + const m = script.match(new RegExp(`^readonly ${name}=(\\d+)$`, "m")); + assert.ok(m, `could not read ${name} out of ${scriptPath}`); + return Number(m[1]); +} + +const MAX_APPROVED_DIGESTS = shellReadonly("MAX_APPROVED_DIGESTS"); + +// Distinct, well-formed, lowercase-hex digests keyed by a short label. +function digest(label) { + const hex = label.toString(16).padStart(2, "0"); + return `sha256:${hex.repeat(32).slice(0, 64)}`; +} + +// Runs the shipping ring builder. `liveDigest` of "" is the pre-fix behaviour: +// the running digest could not be established, so ordering falls back to age. +function ringFor(newDigest, liveDigest, existing, max = MAX_APPROVED_DIGESTS) { + const harness = [ + "set -euo pipefail", + ringSource, + `printf '%s' "$1" | ${RING_FUNCTION_NAME} "$2" "$3" "$4"`, + ].join("\n"); + const result = spawnSync( + "bash", + ["-c", harness, "harness", existing.join("\n"), newDigest, liveDigest, String(max)], + { encoding: "utf8" }, + ); + assert.equal(result.status, 0, `ring harness failed: ${result.stderr}`); + return result.stdout.trim().split("\n").filter(Boolean); +} + +test("the digest being released is first and the running digest is pinned right behind it", () => { + const live = digest(0xaa); + const ring = ringFor(digest(0x11), live, [digest(0x22), live, digest(0x33)]); + assert.equal(ring[0], digest(0x11)); + assert.equal(ring[1], live); + assert.ok(ring.length <= MAX_APPROVED_DIGESTS); +}); + +test("the running digest survives an unbounded run of deploys that never land", () => { + const live = digest(0xaa); + // The live ring as it stood on 2026-09-04: a dead slot holding a digest that + // was approved and never applied, the running digest, and one older entry. + let ring = [digest(0x6c), live, digest(0x68)]; + for (let i = 0; i < 25; i += 1) { + ring = ringFor(digest(0x10 + i), live, ring); + assert.ok( + ring.includes(live), + `the running digest was evicted after ${i + 1} consecutive deploys — rollback is now impossible`, + ); + assert.ok(ring.length <= MAX_APPROVED_DIGESTS, `window grew to ${ring.length}`); + } +}); + +test("CONTROL: without the pin the running digest is evicted in two deploys", () => { + // Guards the test above from going hollow. This is the pre-fix ordering, and + // it reproduces the exact arithmetic BLO-28483 was filed on: with one slot + // already consumed by a digest that never ran, the running digest is two + // failed deploys away from eviction. If this ever passes, the pin has stopped + // being load-bearing and the regression test above proves nothing. + const live = digest(0xaa); + let ring = [digest(0x6c), live, digest(0x68)]; + ring = ringFor(digest(0x10), "", ring); + assert.ok(ring.includes(live), "still present after one deploy"); + ring = ringFor(digest(0x11), "", ring); + assert.ok(!ring.includes(live), "pre-fix ordering must evict the running digest on the second deploy"); +}); + +test("a config-only release reusing the running digest does not duplicate it", () => { + const live = digest(0xaa); + const ring = ringFor(live, live, [digest(0x22), digest(0x33)]); + assert.equal(ring[0], live); + assert.equal(ring.filter((entry) => entry === live).length, 1); + assert.ok(ring.length <= MAX_APPROVED_DIGESTS); +}); + +test("a running digest already in the window is pinned rather than duplicated", () => { + const live = digest(0xaa); + const ring = ringFor(digest(0x11), live, [live, digest(0x22)]); + assert.equal(ring.filter((entry) => entry === live).length, 1); + assert.equal(ring[1], live); +}); + +test("an unestablished running digest degrades to newest-first, never to a failure", () => { + const ring = ringFor(digest(0x11), "", [digest(0x22), digest(0x33), digest(0x44)]); + assert.deepEqual(ring, [digest(0x11), digest(0x22), digest(0x33)]); +}); + +test("a malformed running digest is ignored rather than pinned", () => { + const ring = ringFor(digest(0x11), "not-a-digest", [digest(0x22), digest(0x33)]); + assert.deepEqual(ring, [digest(0x11), digest(0x22), digest(0x33)]); +}); + +test("malformed entries are discarded rather than consuming a slot", () => { + const live = digest(0xaa); + const ring = ringFor(digest(0x11), live, ["", " ", "sha256:nope", "garbage", live, digest(0x22)]); + assert.deepEqual(ring, [digest(0x11), live, digest(0x22)]); + for (const entry of ring) { + assert.match(entry, /^sha256:[0-9a-f]{64}$/); + } +}); + +test("the window never exceeds the bound the admission policy enforces", () => { + const live = digest(0xaa); + const crowded = Array.from({ length: 12 }, (_, index) => digest(0x30 + index)); + const ring = ringFor(digest(0x11), live, [...crowded, live]); + assert.equal(ring.length, MAX_APPROVED_DIGESTS); +}); + +test("an empty window yields just the released digest and the running one", () => { + const live = digest(0xaa); + assert.deepEqual(ringFor(digest(0x11), live, []), [digest(0x11), live]); + assert.deepEqual(ringFor(digest(0x11), "", []), [digest(0x11)]); +}); + +test("a one-slot window still releases, dropping the pin rather than overflowing", () => { + // Defensive: the pin must never be able to push the window past the bound, so + // a hypothetical max of 1 keeps only the digest being released. + assert.deepEqual(ringFor(digest(0x11), digest(0xaa), [digest(0x22)], 1), [digest(0x11)]); +}); + +// The ring builder being correct proves nothing unless the script actually hands +// it the running digest, so the reader on the other side of that seam is +// exercised too — against a stubbed kubectl, since it is the one part that talks +// to a cluster. Its contract is narrow: name the digest when it can be +// established beyond doubt, otherwise say nothing and succeed. It must never +// fail a release to protect a rollback target. + +const READER_FUNCTION_NAME = "live_running_digest"; +const readerSource = extractShellFunction(READER_FUNCTION_NAME); + +function shellAssign(name) { + const m = script.match(new RegExp(`^${name}="([^"]+)"$`, "m")); + assert.ok(m, `could not read ${name} out of ${scriptPath}`); + return m[1]; +} + +const IMAGE_REPOSITORY = shellAssign("IMAGE_REPOSITORY"); + +// `deployment` of null makes the stub exit non-zero, standing in for "no such +// Deployment" or an unreachable apiserver. +function runningDigestFor(deployment) { + const dir = mkdtempSync(path.join(tmpdir(), "paperclip-live-digest-")); + const fixture = path.join(dir, "deployment.json"); + writeFileSync(fixture, deployment === null ? "" : JSON.stringify(deployment)); + const harness = [ + "set -euo pipefail", + `DEPLOY_NAMESPACE=paperclip`, + `DEPLOYMENT=paperclip-api`, + `IMAGE_REPOSITORY=${JSON.stringify(IMAGE_REPOSITORY)}`, + `fake_kubectl() { [[ -s ${JSON.stringify(fixture)} ]] || return 1; cat ${JSON.stringify(fixture)}; }`, + "deploy_kubectl=(fake_kubectl)", + readerSource, + READER_FUNCTION_NAME, + ].join("\n"); + const result = spawnSync("bash", ["-c", harness], { encoding: "utf8" }); + assert.equal(result.status, 0, `reader must always succeed; stderr: ${result.stderr}`); + return result.stdout.trim(); +} + +function deploymentWithImages(...images) { + return { spec: { template: { spec: { containers: images.map((image) => ({ image })) } } } }; +} + +test("the running digest is read off a single-container Deployment", () => { + const live = digest(0xaa); + assert.equal(runningDigestFor(deploymentWithImages(`${IMAGE_REPOSITORY}@${live}`)), live); +}); + +test("identical images across containers still name the running digest", () => { + const live = digest(0xaa); + const image = `${IMAGE_REPOSITORY}@${live}`; + assert.equal(runningDigestFor(deploymentWithImages(image, image)), live); +}); + +test("an unreachable or absent Deployment yields no digest and still succeeds", () => { + assert.equal(runningDigestFor(null), ""); +}); + +test("a tag-pinned image is not mistaken for a digest", () => { + assert.equal(runningDigestFor(deploymentWithImages(`${IMAGE_REPOSITORY}:latest`)), ""); +}); + +test("an image from another repository is never pinned", () => { + const live = digest(0xaa); + assert.equal(runningDigestFor(deploymentWithImages(`ghcr.io/someone/else@${live}`)), ""); +}); + +test("containers disagreeing on their image yield no digest rather than a guess", () => { + const images = [`${IMAGE_REPOSITORY}@${digest(0xaa)}`, `${IMAGE_REPOSITORY}@${digest(0xbb)}`]; + assert.equal(runningDigestFor(deploymentWithImages(...images)), ""); +}); + +test("a Deployment with no containers yields no digest", () => { + assert.equal(runningDigestFor({ spec: { template: { spec: {} } } }), ""); +}); + + From 4280df35cad7663c4ba045705ef20d08ec7c4ee7 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Fri, 4 Sep 2026 11:46:42 +0000 Subject: [PATCH 2/3] fix(release): pin the digest that is SERVING, not the one in spec.template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #1639 found the pin believing the pod template alone. That records what was asked for, not what is running: nothing reverts spec.template after a failed rollout, so a digest applied by a deploy that never became ready sits there indefinitely and would be pinned as the rollback target. The reserved slot is then held permanently by an image that never carried traffic while the last healthy digest ages out — the BLO-28483 wedge reached by a different route, and the neighbouring case to the observed 6c45e9e3 incident (approved but never applied, so the spec was never patched). Gate the pin on the rollout having landed, using the health conditions this file already carries in ROLLOUT_COMPLETE_JQ. They are lifted into a sibling ROLLOUT_SERVING_JQ rather than shared textually: the completion predicate also proves plan identity and generation advance, which are lock concerns and not "is this serving". A drift test asserts every serving condition still appears verbatim in the completion predicate, so the two cannot diverge silently. This only tightens evidence, so it costs no availability — the reader's every failure path already returns empty and degrades to the previous age-ordered behaviour. A rollout in flight, or one that never landed, now simply goes unpinned instead of being pinned wrongly. Also: report the window that was READ BACK rather than the one just built. On the exact-retry path the replacement only re-owns the lock and never rewrites .data, so the built ring is not what the cluster holds. Cosmetic while the window was plain age-ordered; misleading now that an operator may read "the rollback target is pinned" off a list that was never persisted. Tests: the fixture could not express rollout status, so every reader case exercised a Deployment where "applied" and "serving" were indistinguishable — which is why 28 tests were green over the defect. It now carries status, with cases for applied-never-ready, in-flight, unobserved-generation and scaled-to- zero. All four fail without the gate. The healthy default omits unavailableReplicas because that is the shape the apiserver actually returns. Verified: 34/34 pass; the four new cases fail with the gate neutered and the drift test fails when either predicate is mutated; the shipping reader driven against the live paperclip-api Deployment names the digest it is serving (a477bcab), so the tighter gate still fires in production; and the ring simulated forward from the actual cluster ring keeps a477bcab through four consecutive failed deploys while 6c45e9e3 drains. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.sh | 53 ++++++- scripts/approve-paperclip-api-digest.test.js | 139 ++++++++++++++++++- 2 files changed, 188 insertions(+), 4 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.sh b/scripts/approve-paperclip-api-digest.sh index 00918fdbf67b..35c7c6a82a44 100755 --- a/scripts/approve-paperclip-api-digest.sh +++ b/scripts/approve-paperclip-api-digest.sh @@ -35,6 +35,11 @@ # (BLO-28483). Pinning reorders eviction only; the bound is unchanged, so # the maxApprovedApiDigests CEL variable does not move. # +# "Currently running" means a rollout that has actually landed and is serving, +# not merely one that was written to the pod template — a digest applied by a +# failed deploy stays in spec.template forever, and pinning that would burn the +# reserved slot on an image which never carried traffic. +# # An approval holds an in-flight lock until its rollout actually lands, so two # releases cannot rotate the ring underneath each other. Retiring that lock — # whether automatically on abort or explicitly via the escape hatch below — @@ -328,6 +333,28 @@ advanced # END ROLLOUT_COMPLETE_JQ ROLLOUT_JQ +# The health half of ROLLOUT_COMPLETE_JQ above: the controller has observed the +# current generation, and every replica is updated to the current pod template, +# ready, and available, with none unavailable. It deliberately carries none of +# that predicate's lock-identity clauses — the rollout marker, the generation +# nonce, the expected image — because it answers a different question: not "did +# MY plan's rollout land?" but "is the template that is written also the one +# serving traffic?". +# +# The condition lines are kept byte-identical to their counterparts above, and +# scripts/approve-paperclip-api-digest.test.js fails if the two drift apart, so +# "this rollout has landed" has one definition in this file rather than two. +read -r -d '' ROLLOUT_SERVING_JQ <<'SERVING_JQ' || true +# BEGIN ROLLOUT_SERVING_JQ +(.spec.replicas // 1) > 0 and +(.status.observedGeneration // 0) >= (.metadata.generation // 1) and +(.status.updatedReplicas // 0) == (.spec.replicas // 1) and +(.status.readyReplicas // 0) == (.spec.replicas // 1) and +(.status.availableReplicas // 0) == (.spec.replicas // 1) and +(.status.unavailableReplicas // 0) == 0 +# END ROLLOUT_SERVING_JQ +SERVING_JQ + # The rollout nonce is read fresh inside the rotation loop, immediately before # the write that stores it — not once up front. A retry can lose a race to a # rollout that lands between attempts, and recording the pre-retry generation @@ -451,13 +478,24 @@ live_deployment_completed_digest() { # every container to carry the same image, so disagreement means something outside # this channel's model is going on and pinning would be a guess. # +# The pod template alone is NOT sufficient evidence, because it records what was +# asked for rather than what is running. Nothing reverts spec.template after a +# failed rollout, so a digest that was applied and never became ready sits there +# indefinitely -- and pinning that would hold a slot for a digest which never +# served traffic while the last healthy one aged out, reaching the very wedge +# BLO-28483 exists to prevent by a different route. So the spec is believed only +# once ROLLOUT_SERVING_JQ confirms the rollout of that spec has fully landed. +# # Every failure path returns empty and succeeds. This is an availability # safeguard, not a gate: not being able to name the live digest must degrade to # the previous age-ordered behaviour, never fail an otherwise valid release. +# Tightening the evidence therefore costs no availability -- a rollout in flight, +# or one that never landed, simply goes unpinned. live_running_digest() { local live_json image live_json="$("${deploy_kubectl[@]}" -n "$DEPLOY_NAMESPACE" \ get deployment "$DEPLOYMENT" -o json 2>/dev/null)" || return 0 + jq -e "$ROLLOUT_SERVING_JQ" <<<"$live_json" >/dev/null 2>&1 || return 0 image="$(jq -r ' [ .spec.template.spec.containers[]?.image // empty ] as $images | if ($images | length) > 0 and (($images | unique | length) == 1) @@ -739,8 +777,6 @@ if [[ -z "$rotated" ]]; then fi echo "Approving ${DIGEST} for harbor.blockcast.net/paperclip/paperclip" -echo "Approval window (newest first, max ${MAX_APPROVED_DIGESTS}):" -printf ' - %s\n' "${approved[@]}" # Read back rather than trusting the replace exit code. The digest and its # transaction lock must be one observed resource version before any probe. @@ -765,6 +801,19 @@ if (( verify_count > MAX_APPROVED_DIGESTS )); then exit 1 fi +# Report the window that was READ BACK, not the one just built. On the exact-retry +# path the replacement only re-owns the lock and never rewrites .data, so the +# locally-built ring is not what the cluster holds. That gap was cosmetic while +# the window was a plain age-ordered list; now that a slot is reserved for the +# running digest, an operator reading "the rollback target is pinned" off a list +# that was never persisted would be misled at exactly the wrong moment. +echo "Approval window (newest first, max ${MAX_APPROVED_DIGESTS}), as persisted:" +printf '%s\n' "$verify_raw" \ + | sed $'s/^\r*//; s/\r*$//' \ + | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ + | grep -E '^sha256:[0-9a-f]{64}$' \ + | sed 's/^/ - /' || true + if ! jq -e \ --arg digest_key "$LOCK_DIGEST_ANNOTATION" \ --arg digest "$DIGEST" \ diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index 1807d13ed749..f1ea8f44b36c 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -419,6 +419,18 @@ function shellAssign(name) { const IMAGE_REPOSITORY = shellAssign("IMAGE_REPOSITORY"); +// The reader's health gate lives in a jq block, not in the function body, so it +// has to be lifted out of the script and injected alongside it — same reason as +// everything else here: a rewrite of the predicate must fail this test rather +// than leave it asserting against a copy that no longer ships. +function extractJqBlock(name) { + const m = script.match(new RegExp(`^# BEGIN ${name}$\\n([\\s\\S]*?)^# END ${name}$`, "m")); + assert.ok(m, `could not read the ${name} jq block out of ${scriptPath}`); + return m[1]; +} + +const SERVING_JQ = extractJqBlock("ROLLOUT_SERVING_JQ"); + // `deployment` of null makes the stub exit non-zero, standing in for "no such // Deployment" or an unreachable apiserver. function runningDigestFor(deployment) { @@ -430,6 +442,9 @@ function runningDigestFor(deployment) { `DEPLOY_NAMESPACE=paperclip`, `DEPLOYMENT=paperclip-api`, `IMAGE_REPOSITORY=${JSON.stringify(IMAGE_REPOSITORY)}`, + "read -r -d '' ROLLOUT_SERVING_JQ <<'SERVING_JQ' || true", + SERVING_JQ.replace(/\n$/, ""), + "SERVING_JQ", `fake_kubectl() { [[ -s ${JSON.stringify(fixture)} ]] || return 1; cat ${JSON.stringify(fixture)}; }`, "deploy_kubectl=(fake_kubectl)", readerSource, @@ -440,8 +455,35 @@ function runningDigestFor(deployment) { return result.stdout.trim(); } +// A Deployment whose rollout has fully landed. Fixtures default to that shape so +// a case which only cares about the image does not have to restate six status +// fields, and `status` overrides merge over the healthy defaults. +// +// `unavailableReplicas` is deliberately ABSENT from the healthy default, because +// that is the shape the apiserver actually returns: the live paperclip-api +// Deployment omits the field entirely while healthy (observed 2026-09-04 at +// generation 560, replicas 2/2/2). The predicate's `// 0` default is what makes +// that read as "none unavailable" rather than as missing data, so the common +// fixture exercises that path rather than a shape production never produces. +function deploymentWith({ images = [], replicas = 2, generation = 7, status = {} } = {}) { + return { + metadata: { generation }, + spec: { + replicas, + template: { spec: { containers: images.map((image) => ({ image })) } }, + }, + status: { + observedGeneration: generation, + updatedReplicas: replicas, + readyReplicas: replicas, + availableReplicas: replicas, + ...status, + }, + }; +} + function deploymentWithImages(...images) { - return { spec: { template: { spec: { containers: images.map((image) => ({ image })) } } } }; + return deploymentWith({ images }); } test("the running digest is read off a single-container Deployment", () => { @@ -474,7 +516,100 @@ test("containers disagreeing on their image yield no digest rather than a guess" }); test("a Deployment with no containers yields no digest", () => { - assert.equal(runningDigestFor({ spec: { template: { spec: {} } } }), ""); + // Built healthy on purpose: with no status the health gate would refuse this + // anyway, and the test would pass without ever reaching the container check. + assert.equal(runningDigestFor(deploymentWith({ images: [] })), ""); +}); + +// The pod template records what was ASKED for. Nothing reverts spec.template +// after a failed rollout, so believing it on its own pins a digest that never +// carried traffic -- burning the reserved slot and letting the last healthy +// digest age out, which is the BLO-28483 wedge reached from the other side. +// These cases are the reason the reader gates on rollout status at all. + +test("a rollout applied but never made ready is not pinned as the running digest", () => { + const applied = digest(0xbb); + assert.equal( + runningDigestFor( + deploymentWith({ + images: [`${IMAGE_REPOSITORY}@${applied}`], + status: { readyReplicas: 0, availableReplicas: 0, unavailableReplicas: 2 }, + }), + ), + "", + ); +}); + +test("a rollout still in flight yields no digest rather than a half-rolled one", () => { + // maxSurge brings a new pod up before the old one leaves, so the template + // already names the new digest while the old one is still serving. Neither is + // unambiguously the running digest, so name neither. + assert.equal( + runningDigestFor( + deploymentWith({ + images: [`${IMAGE_REPOSITORY}@${digest(0xcc)}`], + status: { updatedReplicas: 1, readyReplicas: 1, availableReplicas: 1 }, + }), + ), + "", + ); +}); + +test("a pod template the controller has not observed yet is not believed", () => { + // spec.template was just patched, so status still describes the previous one. + assert.equal( + runningDigestFor( + deploymentWith({ + images: [`${IMAGE_REPOSITORY}@${digest(0xdd)}`], + generation: 8, + status: { observedGeneration: 7 }, + }), + ), + "", + ); +}); + +test("a Deployment scaled to zero has nothing serving and yields no digest", () => { + assert.equal( + runningDigestFor(deploymentWith({ images: [`${IMAGE_REPOSITORY}@${digest(0xee)}`], replicas: 0 })), + "", + ); +}); + +test("a landed rollout that omits unavailableReplicas is still read as serving", () => { + // The shape production actually returns; see deploymentWith. + const live = digest(0xaa); + const deployment = deploymentWith({ images: [`${IMAGE_REPOSITORY}@${live}`] }); + assert.ok( + !Object.hasOwn(deployment.status, "unavailableReplicas"), + "this test is only meaningful while the healthy fixture omits unavailableReplicas", + ); + assert.equal(runningDigestFor(deployment), live); +}); + +// The reader's health gate and the in-flight lock's completion predicate must +// agree on what "this rollout has landed" means. They are separate jq programs +// because they answer different questions -- the lock also proves plan identity +// and generation advance -- so nothing but this test stops one from being +// tightened while the other silently keeps the old reading. +test("the serving predicate does not drift from the completion predicate", () => { + const complete = extractJqBlock("ROLLOUT_COMPLETE_JQ"); + const conditions = SERVING_JQ.split("\n") + .map((line) => line.trim()) + .filter((line) => line && !line.startsWith("#")) + .map((line) => line.replace(/ and$/, "")); + + assert.ok( + conditions.length >= 6, + `expected the serving predicate to carry the rollout-health conditions, got ${conditions.length}`, + ); + for (const condition of conditions) { + assert.ok( + complete.includes(condition), + `ROLLOUT_SERVING_JQ requires \`${condition}\` but ROLLOUT_COMPLETE_JQ no longer does — ` + + "the two definitions of a landed rollout have drifted apart", + ); + } }); From 462da2d6e410f482ca3df52d61edc3cfcd23b929 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Fri, 4 Sep 2026 12:52:46 +0000 Subject: [PATCH 3/3] test(release): make the predicate drift guard bidirectional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's review of 4280df35c mutation-tested the drift guard in all four directions and found one open: a health condition ADDED to ROLLOUT_COMPLETE_JQ alone left the suite green at 34/34. The guard asserted `serving subset of complete` only, so the reader could silently become the weaker of the two definitions and pin a digest the lock's own predicate would not call landed -- the failure the gate was added to close, reintroduced by drift rather than by code. The comment at :345 claimed the test "fails if the two drift apart", which read as bidirectional and was the half that was not true. Compare the two blocks for set equality over the health half instead. LOCK_IDENTITY_CONDITIONS names the four clauses the completion predicate carries deliberately -- expected image, its structural precondition, the rollout marker, the generation advance -- and everything else must appear in both. A new clause therefore forces an explicit classification rather than defaulting to unguarded, and a stale entry in that list fails too, so the exemption cannot outlive the clause it describes. jqConditions() drops the `def advanced: ... ;` prologue: its body is control flow, not conditions, and parsing it as conditions is what made the first attempt at this fail on `def advanced:`. Mutation-tested all six directions at this head (baseline 35/35): serving: drop a condition -> fail 1 serving: weaken in place -> fail 1 serving: ADD a condition only -> fail 4 complete: weaken in place -> fail 1 complete: ADD a condition only -> fail 1 (was: pass 34) complete: drop a lock-only clause-> fail 1 (stale-allowlist guard) Also narrows the script comment to state the guarantee it now has. fix(release): report the window contents when the read-back guards fail Moving the window print after the read-back was correct, but it left it below both `exit 1` guards, so the over-bound path printed a count and no list -- the one failure where the contents are the actionable part, since trimming requires knowing what is in there. Normalise the read-back once into verify_digests and report it from both failure paths through a shared format_digest_list, which marks an empty window explicitly so the "did not persist" report cannot render as a blank line. The count, the absence test, and every operator-facing report now read one list rather than each deriving its own view of verify_raw. Driven through all three paths against the extracted guards: over-bound prints 4 entries and exits 1, absent prints "(none)" and exits 1, the success path is unchanged and exits 0. Refs BLO-28483 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.sh | 46 ++++++--- scripts/approve-paperclip-api-digest.test.js | 102 +++++++++++++++++-- 2 files changed, 126 insertions(+), 22 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.sh b/scripts/approve-paperclip-api-digest.sh index 35c7c6a82a44..ad3c363cb5a3 100755 --- a/scripts/approve-paperclip-api-digest.sh +++ b/scripts/approve-paperclip-api-digest.sh @@ -342,8 +342,10 @@ ROLLOUT_JQ # serving traffic?". # # The condition lines are kept byte-identical to their counterparts above, and -# scripts/approve-paperclip-api-digest.test.js fails if the two drift apart, so -# "this rollout has landed" has one definition in this file rather than two. +# scripts/approve-paperclip-api-digest.test.js compares the two blocks for set +# equality over the health half -- in both directions, so neither predicate can +# gain or lose a health condition without the other -- so "this rollout has +# landed" has one definition in this file rather than two. read -r -d '' ROLLOUT_SERVING_JQ <<'SERVING_JQ' || true # BEGIN ROLLOUT_SERVING_JQ (.spec.replicas // 1) > 0 and @@ -563,6 +565,20 @@ build_approval_ring() { printf '%s\n' "${ring[@]}" } +# Render an approval window for operator output: one indented entry per line, or +# an explicit marker when empty so a failure report never renders as a silent +# blank line. Used by the read-back guards as well as the success path, because +# on the failure paths the contents are the actionable part -- a bare count says +# the window is wrong without saying what is in it to trim. +format_digest_list() { + local list="$1" + if [[ -z "$list" ]]; then + echo " (none)" + return 0 + fi + printf '%s\n' "$list" | sed 's/^/ - /' +} + MAX_ROTATE_ATTEMPTS="${PAPERCLIP_APPROVAL_ROTATE_ATTEMPTS:-5}" replace_err="$(mktemp "${TMPDIR:-/tmp}/paperclip-approve-err.XXXXXX")" nonce_err="$(mktemp "${TMPDIR:-/tmp}/paperclip-approve-nonce.XXXXXX")" @@ -782,22 +798,28 @@ echo "Approving ${DIGEST} for harbor.blockcast.net/paperclip/paperclip" # transaction lock must be one observed resource version before any probe. verify_json="$(kubectl -n "$NAMESPACE" get configmap "$CONFIGMAP" -o json)" verify_raw="$(jq -r --arg key "$DATA_KEY" '.data[$key] // ""' <<<"$verify_json")" -verify_count=$(printf '%s\n' "$verify_raw" \ +# Normalise once. The count, the absence test, and every operator-facing report +# below all read this same list, so they cannot disagree about what the cluster +# holds -- previously each derived its own view from verify_raw. +verify_digests="$(printf '%s\n' "$verify_raw" \ | sed $'s/^\r*//; s/\r*$//' \ | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ - | grep -Ec '^sha256:[0-9a-f]{64}$' || true) + | grep -E '^sha256:[0-9a-f]{64}$' \ + || true)" +verify_count=$(printf '%s' "$verify_digests" | grep -c . || true) -if ! printf '%s\n' "$verify_raw" \ - | sed $'s/^\r*//; s/\r*$//' \ - | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ - | grep -Fxq "$DIGEST"; then +if ! printf '%s\n' "$verify_digests" | grep -Fxq "$DIGEST"; then echo "approval did not persist: ${DIGEST} is absent from ${NAMESPACE}/${CONFIGMAP}" >&2 + echo "the window holds ${verify_count} entries, as persisted:" >&2 + format_digest_list "$verify_digests" >&2 exit 1 fi if (( verify_count > MAX_APPROVED_DIGESTS )); then echo "approval window is ${verify_count} entries, over the ${MAX_APPROVED_DIGESTS} the policy accepts;" >&2 - echo "the admission policy will now deny every rollout until this is trimmed" >&2 + echo "the admission policy will now deny every rollout until this is trimmed." >&2 + echo "The window, as persisted:" >&2 + format_digest_list "$verify_digests" >&2 exit 1 fi @@ -808,11 +830,7 @@ fi # running digest, an operator reading "the rollback target is pinned" off a list # that was never persisted would be misled at exactly the wrong moment. echo "Approval window (newest first, max ${MAX_APPROVED_DIGESTS}), as persisted:" -printf '%s\n' "$verify_raw" \ - | sed $'s/^\r*//; s/\r*$//' \ - | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ - | grep -E '^sha256:[0-9a-f]{64}$' \ - | sed 's/^/ - /' || true +format_digest_list "$verify_digests" if ! jq -e \ --arg digest_key "$LOCK_DIGEST_ANNOTATION" \ diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index f1ea8f44b36c..721c7ea57788 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -587,29 +587,115 @@ test("a landed rollout that omits unavailableReplicas is still read as serving", assert.equal(runningDigestFor(deployment), live); }); +// Split a jq predicate block into the condition lines of its top-level +// conjunction. ROLLOUT_COMPLETE_JQ opens with a `def advanced: … ;` helper whose +// body is control flow rather than conditions; jq definitions end in `;` and +// condition lines never do, so the conjunction is everything after the last one. +function jqConditions(block) { + const lines = block + .split("\n") + .map((line) => line.trim()) + .filter((line) => line && !line.startsWith("#")); + const endOfDefs = lines.reduce((at, line, index) => (line.endsWith(";") ? index : at), -1); + return lines.slice(endOfDefs + 1).map((line) => line.replace(/ and$/, "")); +} + +// The clauses ROLLOUT_COMPLETE_JQ carries that ROLLOUT_SERVING_JQ deliberately +// does not. Each establishes that MY plan's rollout landed, not that whatever +// template is written is the one serving: the expected image and its structural +// precondition, the rollout marker, and the generation advance. Everything else +// in the completion predicate is a rollout-health condition, and the drift test +// below requires it to appear in the serving predicate too. +const LOCK_IDENTITY_CONDITIONS = [ + '(.spec.template.spec.containers | type == "array" and length > 0)', + "(.spec.template.spec.containers | all(.image == $image))", + '(.spec.template.metadata.annotations[$marker_key] // "") == $marker', + "advanced", +]; + // The reader's health gate and the in-flight lock's completion predicate must // agree on what "this rollout has landed" means. They are separate jq programs // because they answer different questions -- the lock also proves plan identity // and generation advance -- so nothing but this test stops one from being // tightened while the other silently keeps the old reading. +// +// The comparison is set equality over the health half, in BOTH directions. The +// reverse direction is the load-bearing one: a health condition added to the +// completion predicate alone would silently make the reader the weaker of the +// two definitions, so it would pin a digest the lock itself would not call +// landed -- the exact failure this gate was introduced to close, reintroduced +// by drift rather than by code. test("the serving predicate does not drift from the completion predicate", () => { - const complete = extractJqBlock("ROLLOUT_COMPLETE_JQ"); - const conditions = SERVING_JQ.split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")) - .map((line) => line.replace(/ and$/, "")); + const serving = jqConditions(SERVING_JQ); + const complete = jqConditions(extractJqBlock("ROLLOUT_COMPLETE_JQ")); assert.ok( - conditions.length >= 6, - `expected the serving predicate to carry the rollout-health conditions, got ${conditions.length}`, + serving.length >= 6, + `expected the serving predicate to carry the rollout-health conditions, got ${serving.length}`, ); - for (const condition of conditions) { + + // Keep the classification honest: a lock-only clause that no longer exists + // must not sit here silently exempting a condition name from the comparison. + for (const condition of LOCK_IDENTITY_CONDITIONS) { assert.ok( complete.includes(condition), + `LOCK_IDENTITY_CONDITIONS in this test lists \`${condition}\` as lock-only, but ` + + "ROLLOUT_COMPLETE_JQ no longer carries it — update the classification", + ); + } + + const completeHealth = complete.filter((condition) => !LOCK_IDENTITY_CONDITIONS.includes(condition)); + + for (const condition of serving) { + assert.ok( + completeHealth.includes(condition), `ROLLOUT_SERVING_JQ requires \`${condition}\` but ROLLOUT_COMPLETE_JQ no longer does — ` + "the two definitions of a landed rollout have drifted apart", ); } + + for (const condition of completeHealth) { + assert.ok( + serving.includes(condition), + `ROLLOUT_COMPLETE_JQ requires \`${condition}\` but ROLLOUT_SERVING_JQ does not — ` + + "the reader would pin a digest the lock's own predicate would not call landed. " + + "Add it to ROLLOUT_SERVING_JQ, or to LOCK_IDENTITY_CONDITIONS in this test if it " + + "proves plan identity rather than rollout health", + ); + } +}); + +const FORMATTER_FUNCTION_NAME = "format_digest_list"; +const formatterSource = extractShellFunction(FORMATTER_FUNCTION_NAME); + +// The list reaches the formatter exactly as it does in the script: through a +// command substitution (which strips trailing newlines) and then double-quoted, +// so a multi-line window is one argument rather than several. +function formatDigestList(list) { + const dir = mkdtempSync(path.join(tmpdir(), "paperclip-digest-list-")); + const fixture = path.join(dir, "window.txt"); + writeFileSync(fixture, list); + const harness = [ + "set -euo pipefail", + formatterSource, + `window="$(cat ${JSON.stringify(fixture)})"`, + `${FORMATTER_FUNCTION_NAME} "$window"`, + ].join("\n"); + const result = spawnSync("bash", ["-c", harness], { encoding: "utf8" }); + assert.equal(result.status, 0, `formatter must always succeed; stderr: ${result.stderr}`); + return result.stdout; +} + +// The read-back guards print this on their failure paths, where the contents are +// the actionable part. An empty window must not render as a blank line, or the +// "did not persist" report would say nothing at all about what the cluster holds. +test("the digest-list formatter indents each entry and marks an empty window", () => { + const a = digest(0xaa); + const b = digest(0xbb); + + assert.equal(formatDigestList(`${a}\n${b}`), ` - ${a}\n - ${b}\n`); + assert.equal(formatDigestList(a), ` - ${a}\n`); + assert.equal(formatDigestList(""), " (none)\n"); });