Skip to content

fix(ci): teach digest-drift to acknowledge the ingestor prod-pin float divergence (backend#2673) - #877

Merged
aptracebloc merged 4 commits into
developfrom
fix/2673-digest-drift-acknowledged
Aug 27, 2026
Merged

fix(ci): teach digest-drift to acknowledge the ingestor prod-pin float divergence (backend#2673)#877
aptracebloc merged 4 commits into
developfrom
fix/2673-digest-drift-acknowledged

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Closes tracebloc/backend#2673

Problem

client's digest-drift.yml (nightly) has had 0 passes since 2026-08-14 — but the red is genuine and permanent: images.ingestor.prodDigest (v0.8.2) is pinned deliberately behind the channelTags.prod: "0.8" float. data-ingestors#468 (first in v0.8.8) drops the edgeuser DB_USER fallback while prod's serviceDbAccountsByEnv.prod is still false, so the float now roams the unsafe set and the pin must stay in {v0.8.0..v0.8.4} (docs/SECURITY.md §4.1.1). The pin and the check are both correct — advancing the pin (Option C) is a security regression and is NOT what this does.

The real defect is signaling: check-digest-drift.sh reds if any pin drifts, so the intentional, permanent ingestor divergence made the job red every night. That (1) trained everyone to ignore it (backend#2386) and (2) masked any new, actionable drift on squid/mysql-client behind the standing red — the exact failure backend#1853 exists to prevent, re-emerging one level up.

Fix — Option A (acknowledged drift)

A pin may now declare an ackDrift: {line, reason} block. check-digest-drift.sh then treats that pin's float-vs-pin divergence as expected — job GREEN, printed ACKNOWLEDGED with the reason and the SECURITY.md §4.1.1 pointer — while still re-verifying, every run, that the pin resolves to a healthy multi-arch index.

  • Acknowledges the class, not a digest → the float may roam any 0.8.x patch without re-alarming.
  • Lapses if channelTags.prod moves off the acknowledged line (boundary is line-specific, must be re-derived).
  • Any other pin drifting, or the acknowledged pin ceasing to resolve / going single-arch, still reds.
  • Declared in the chart behind CODEOWNERS review — lifting it is a reviewed chart change, not a silent script edit. The pin does not move.

Verification

  • scripts/tests/check-digest-drift.bats: 28/28 (9 new ACK cases — green, conditional-red ×2, lapsed, per-pin non-masking, inert-on-agreement, reason-verbatim, summary, unquoted-reason fallback).
  • Against the realclient/values.yaml with a stub mirroring today's registry state: ingestor → ACKNOWLEDGED, squid + mysql-client → ok, exit 0. Squid also drifting → exit 1 with squid's DRIFT visible (masking hole closed). Ingestor pin unresolvable → exit 1.
  • make lint (parse + shellcheck), helm lint --strict (all 4 ci overlays), helm unittest (ingestor/jobs-manager, 84), check-style, check-facts, chart-version-guard (1.9.75→1.9.76 bumped), all green.

Ran /code-review (xhigh) on the diff and fixed all three findings before pushing (SECURITY.md §4.1.1 updated to describe the acknowledgement; pin-unhealthy message no longer misattributes a missing buildx; added the fallback test).

Reviewers

The acknowledgement semantics sit on the ingestor security boundary — @saadqbal is the CODEOWNERS reviewer for values.yaml/values.schema.json/SECURITY.md. cc @saqlainsyed007 as co-owner of the boundary per the issue: please sanity-check the acknowledgement semantics (class-not-digest, line-binding lapse, still-verify-pin-health).


Note

Medium Risk
Changes CI signaling and security-boundary documentation around a deliberate prod ingestor pin, but does not advance the pin or alter runtime image selection; incorrect ack semantics could hide real drift or falsely green an unhealthy pin.

Overview
The nightly digest-drift watch was permanently red because ingestor prodDigest (v0.8.2) is deliberately behind channelTags.prod: "0.8" while prod cannot take the float yet (docs/SECURITY.md §4.1.1). That standing red masked real drift on other pins (squid, mysql-client).

This PR adds chart-declared images.ingestor.ackDrift: { line, reason } (schema + values, CODEOWNERS-reviewed) and extends scripts/check-digest-drift.sh so only that pin’s float-vs-pin gap is ACKNOWLEDGED (green, reason printed) instead of DRIFT. It is not a mute: acknowledgement binds to line (lapses if prod float moves), does not apply to sibling digest: pins, and every run still re-verifies the pin resolves as a healthy linux/amd64 + linux/arm64 index—unresolvable or single-arch still reds.

pin_platforms, deferred prodDigest discovery (order-independent ack binding), and nine new bats cases cover lapse, per-pin masking, and attestation filtering. SECURITY.md and Makefile comments reflect the new behavior. Chart version 1.9.77.

Reviewed by Cursor Bugbot for commit 4c56ea0. Bugbot is set up for automated code reviews on this repo. Configure here.

…t divergence (backend#2673)
client/values.yaml pins images.ingestor.prodDigest (v0.8.2) deliberately BEHIND
the channelTags.prod "0.8" float: data-ingestors#468 (first in v0.8.8) drops the
edgeuser DB_USER fallback while prod's serviceDbAccountsByEnv.prod is still false,
so the float roams the unsafe set and the pin must stay in {v0.8.0..v0.8.4}
(docs/SECURITY.md §4.1.1). The pin and the check are both correct — so the fix is
NOT to advance the pin.
The problem was signaling: check-digest-drift.sh reds if ANY pin drifts, so the
intentional, permanent ingestor divergence made the nightly red every night
(0 passes since 2026-08-14). That trained everyone to ignore it (backend#2386)
AND masked any NEW, actionable drift on squid/mysql-client behind the standing
red — the exact failure backend#1853 exists to prevent, re-emerging one level up.
Option A (acknowledged drift). A pin may now declare an `ackDrift: {line, reason}`
block next to it; check-digest-drift.sh then treats that pin's float-vs-pin
divergence as EXPECTED — job GREEN, printed ACKNOWLEDGED with the reason and the
SECURITY.md §4.1.1 pointer — while STILL re-verifying, every run, that the pin
itself resolves to a healthy multi-arch index. It is a CLASS not a digest (the
float may roam any 0.8.x patch without re-alarming), it LAPSES if the float
changes line, and ANY other pin drifting — or the acknowledged pin ceasing to
resolve or going single-arch — still reds. The pin does not move.
Declared in the chart (behind CODEOWNERS review), not in the script, so lifting
the acknowledgement is a reviewed chart change. When the boundary is resolved
(prod flag flips / a prod-safe 0.8.x is cut), delete the block in the same change
that advances the pin.
- scripts/check-digest-drift.sh: ackDrift discovery + pin_platforms re-verify +
ACKNOWLEDGED / lapsed / unhealthy classification and summary.
- scripts/tests/check-digest-drift.bats: 8 cases (green, conditional-red x2,
lapsed, per-pin non-masking, inert-on-agreement, reason verbatim, summary).
- client/values.yaml + values.schema.json: the ackDrift block and its schema.
- client/Chart.yaml: version/appVersion 1.9.75 -> 1.9.76 (chart content changed).
- Makefile: the digest-drift target comment no longer claims a permanent red.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aptraceblocaptracebloc self-assigned this Aug 27, 2026
aptraceblocand others added 2 commits August 27, 2026 14:35
develop advanced to 1.9.76 (client#875) after this branch bumped to the same
version, so the chart-version-guard saw no bump. Re-bump one patch above the
new develop tip.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka
LukasWodka requested review from saqlainsyed007 and removed request for saadqbalAugust 27, 2026 12:48
Comment threadscripts/check-digest-drift.sh Outdated
Comment threadclient/values.yaml
Comment threadscripts/check-digest-drift.sh Outdated
Comment threadscripts/check-digest-drift.sh
Comment threadscripts/check-digest-drift.sh Outdated

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 8067fc0 (high effort). The design is sound and the security-critical property holds — I verified the important things by reading the parser rather than trusting the prose; the open items are latent hardening, not live defects.

Verified safe (the crux): the acknowledgement can NOT mask unrelated drift. ack_line/ack_reason/in_ack are reset at every top-level and 2-space key (the parser's image-block boundaries), so ingestor's ackDrift never carries into squid/mysql-client/etc. — a drifting sibling image still reddens (exit 1) with the acked ingestor reported ACKNOWLEDGED alongside it. The ack also lapses when the float line: changes and re-verifies the pin still resolves to a healthy multi-arch index. The security boundary (prodDigest still pins 0.8.2 behind the 0.8 float, SECURITY.md §4.1.1) is untouched — no pin moves. This does exactly what #2673 asks.

Worth fixing before this merges — two latent vectors, both on a drift guard whose whole job is not to false-green (inline):

  1. The ack is block-scoped, not bound to the annotated pin: within one image block ack_line/ack_reason attach to every pin the block emits, so a second non-empty pin in images.ingestor (e.g. a populated per-edge digest:) would inherit the ack and its genuine drift would print ACKNOWLEDGED → exit 0. Dormant only because that sibling digest: is "" today (discover_pins skips empty pins); it becomes a silent false-green the moment anyone fills it in.
  2. The ack is emit-order-dependentackDrift: must textually precede prodDigest: or the ack pair is still empty when the pin is emitted → plain DRIFT → spurious permanent red (the exact "train people to ignore it" failure this PR removes). Works today; a values.schema.json reorder or reformat would silently break it.

Both die at once if the ack is bound to the prodDigest leaf rather than the block. Given this is a security-adjacent guard, I'd like 1+2 addressed (or consciously deferred with a tracked follow-up) before I approve — flagging rather than hard-blocking since the current chart is correct.

Lower priority (inline): (3) pin_platforms is buildx-only while resolve_index_digest has a docker manifest inspect fallback — a no-buildx runner spuriously reds the acked pin (fails closed, and CI has buildx); (4) the ack keys on the channel-line string, so a future 0.8.x the float rolls to stays green across a boundary — documented as "acknowledge the class," just confirm that residual is intended; (5) the stub pin_platforms omits the real path's grep -v '^unknown' attestation filter, so the bats seam can't catch an unknown/unknown miscount.

CI is green and mergeable, but I'm holding on my own open threads (above). Address 1+2 (or tell me you're deferring them and why) and I'll approve on the next pass.

…ock (backend#2673)
Addresses @saqlainsyed007 review on client#877 (two latent false-green vectors
on a drift guard whose whole job is not to false-green):
1. The ack was BLOCK-scoped: ack_line/ack_reason attached to every pin the
image block emitted, so a populated sibling `digest:` in images.ingestor
would inherit the ack and its genuine drift would print ACKNOWLEDGED (exit 0).
Dormant only because that digest is \"\" today.
2. It was EMIT-ORDER-dependent: ackDrift: had to textually precede prodDigest:
or the ack pair was still empty at emit -> plain DRIFT -> spurious permanent
red (the exact failure this PR removes). A values.yaml reorder would break it.
Both die by deferring prodDigest emission to the block boundary (flush_prod):
the pin is emitted only once the whole block is read, so (a) the ack binds to
the prodDigest leaf alone and a sibling digest: never carries it, and (b) key
order no longer matters. Block repo/float are also resolved at flush time, so
those are order-independent too.
Also (review #5): pin_platforms now runs the stub path through the SAME
unknown/-drop + dedup as the registry path, so the bats seam exercises the
attestation filter and the reported platform set never shows unknown/unknown.
Tests: +3 regression cases (sibling-digest still reds; ackDrift-after-prodDigest
still acknowledges; attestation filtered) -> 31 total, all green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aptracebloc

Copy link
Copy Markdown
ContributorAuthor

Thanks @saqlainsyed007 — pushed 4c56ea0 addressing the review.

1 + 2 (the two false-green vectors) — fixed together by binding the ack to the prodDigest leaf instead of the image block.discover_pins now defers the prodDigest emission to the block boundary (flush_prod): the pin is emitted only once the whole block has been read, so

  • the ack pair attaches to prodDigestonly — a populated sibling digest: emits inline with an empty ack and its genuine drift still reddens (was: inherited the ack → silent ACKNOWLEDGED); and
  • key order no longer matters — ackDrift:, channelTags: and repository: may sit in any order relative to prodDigest: (was: ackDrift had to textually precede it or the ack silently emptied → spurious permanent red).

5 — fixed:pin_platforms routes the stub and registry paths through the same grep -v '^unknown' | sort -u, so the seam exercises the attestation filter.

3 — deferring with rationale (will add the fallback if you'd prefer):pin_platforms is buildx-only to mirror resolve-ingestor-digest.sh's own platform check; it fails closed and CI has buildx. I broadened the unhealthy message so it no longer misattributes a missing buildx to a GC'd digest.

4 — confirmed intended: keying on the channel line (not a digest) is the "acknowledge the class" design; advancing within the line is guarded by the --write ceiling (backend#1528), CI ingestor-multiarch, and client-runtime's env-contract test. The ack lapses on a line change and re-verifies pin health every run.

Regression tests added for 1/2/5 (31 total, all green). Replied inline and resolved each thread; re-requested your review.

@aptracebloc

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 4c56ea0. Configure here.

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the delta 8067fc02..4c56ea0c — all five things I raised are genuinely resolved (verified in code and by running the suite, 31/31 bats green):

  • Latent 1 (ack leaks to a sibling pin in the same block) — fixed. discover_pins defers prodDigest emission to the block boundary via flush_prod() and attaches the ack pair there and only there; a canonical digest: pin emits inline with an empty ack pair, and an empty ack_line routes straight to report_drift. Regression test 28 (sibling digest: drifts → DRIFT, exit 1, while prodDigest prints ACKNOWLEDGED) passes.
  • Latent 2 (emit-order dependence) — fixed. flush_prod() runs at every 0-/2-space boundary and at END, after ackDrift/channelTags.prod/repository are all read regardless of file order. Test 29 (ackDrift after prodDigest → ACKNOWLEDGED, exit 0) passes.
  • Nit (no docker manifest inspect fallback in pin_platforms) — accepted: kept buildx-only for parity with resolve-ingestor-digest.sh and it fails closed (empty platforms → red); the unhealthy-pin message was broadened so a missing buildx isn't misattributed to a GC'd digest.
  • Nit (ack keyed on line string, not digest) — accepted by design: within-line advances are guarded by the resolve-ingestor ceiling / multiarch CI / env-contract test and documented in SECURITY.md §4.1.1.
  • Nit (stub seam didn't filter unknown/unknown attestations) — fixed: both stub and registry paths now share one normalization tail (grep -v '^unknown'sort -u); test 30 confirms the attestation entry is filtered.

No new defects. This is ready on my side — holding the approve only on CI: the required checks are green but bugbot / review / Cursor Bugbot are still in progress, so the rollup is PENDING. I'll approve on the next pass once they clear.

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refusing Option C outright is the most important line in this PR."Advancing the pin is a security regression and is NOT what this does" — the pin is deliberately behind the float because data-ingestors#468 drops the edgeuser fallback while prod's serviceDbAccountsByEnv.prod is still false. The obvious way to make a red check green would have reintroduced the thing the pin exists to prevent, and saying so at the top stops anyone proposing it later.

And naming the real defect as masking rather than noise is what makes the fix the right size. A permanently-red check doesn't just train people to ignore it — it hides new, actionable drift on squid and mysql-client behind the standing red. "The exact failure backend#1853 exists to prevent, re-emerging one level up" is the correct diagnosis, and it's why the answer is per-pin acknowledgement rather than a broader mute or a longer wait.

The four properties that separate an acknowledgement from a mute are all present and each has its own test, which I checked:

  • it still verifies what it can — an acknowledged pin that stops resolving reds, and one that goes single-arch reds, both asserting status -eq 1and that ACKNOWLEDGED is absent from the output
  • it lapses on its own boundary — moving channelTags.prod to a line the acknowledgement was not reasoned about reds with acknowledgement lapsed
  • it does not mask siblings — an acknowledged ingestor drift with squid also drifting exits 1 with squid's DRIFT visible
  • the summary counts acknowledged distinctly, not as plain agreement — so "clean" and "acknowledged" never collapse into one number, which is the difference between a report and a reassurance

Worth contrasting with the EXEMPT map .github spent #352/#353/#355 burning down. That list had to be pruned by hand, and its staleness was only catchable by a separate stale_exemptions check bolted alongside. This acknowledgement is scoped to a line and invalidates itself when the float moves off it — no second guard needed, and no way for it to quietly outlive its reason. That's a strictly better shape for the same problem, and worth copying next time someone needs an exemption.

Declaring it in the chart behind CODEOWNERS rather than in the script is the other half: lifting it becomes a reviewed chart change instead of a one-line edit nobody sees.

This also closes the loop on client#868 — the staleness watch I approved predicted it would file exactly this on its first real run, naming digest-drift.yml as red on schedule ~13 days unnoticed. Nice to see the prediction and the fix land within a day of each other.

Green, no threads, 28/28 bats with the masking case verified against real values.yaml. 👍

@aptracebloc
aptracebloc merged commit 3e8ea94 into developAug 27, 2026
48 checks passed
@aptracebloc
aptracebloc deleted the fix/2673-digest-drift-acknowledged branch August 27, 2026 14:56
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@aptracebloc@saadqbal@saqlainsyed007@LukasWodka