fix(chart): render control-plane images from the last-refreshed digest so helm upgrade preserves the pin - #1013
Conversation
…t so a helm upgrade preserves the pin (client-runtime#199) Control-plane images render as repo:tag (IfNotPresent) when unpinned, and image-refresh keeps them current out-of-band via kubectl set image repo@digest. A helm upgrade --reset-then-reuse-values re-renders the bare tag and drops that pin; on a node whose :tag cache is stale the pod then silently runs an OLD image. New tracebloc.controlPlaneDigest helper seeds the effective digest from the jobs-manager Deployment last-refreshed-<image>-digest annotation via lookup, so a re-render renders repo@digest instead. Operator pins still win; lookup is empty during helm template/diff/first install (-> :tag, correct there). No new RBAC (auto-upgrade SA already reads the Deployment in the release ns). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Validated on a real cluster (the piece that can't be unit-tested)
Before (current develop chart) vs after (this PR), same command:
Each rendered digest matches the Deployment's Remaining caveat unchanged: |
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Changes requested. The mechanism is right and the fallback is genuinely safe — I confirmed helm template with no cluster renders :tag + IfNotPresent at all four sites and exits 0 (aks and bm), so the Helm chart renders gate is fine, and your no-new-RBAC claim checks out: the auto-upgrade Role is * on * in the release namespace. CI is green; the org scan's CI=FAILURE is a superseded-run artifact (a CANCELLED Helm unit tests alongside two SUCCESS runs on the same sha).
Two things I think have to change before this lands, and one merge-order point.
1. BLOCKING — the annotation reaches image: with no validation (_helpers.tpl:532). Every one of the 11 digest keys that can reach an image: field is schema-guarded by ^(sha256:[a-f0-9]{64})?$ — --set images.jobsManager.digest=sha256:deadbeefcafe is rejected outright, I checked. The annotation path has no equivalent, and tracebloc.image drops the tag when a digest is present, so a malformed value renders an unstartable ref that helm cannot detect (the kubelet reports InvalidImageName; the apiserver accepts the spec happily).
This is already live in the repo: scripts/tests/e2e-auto-upgrade.sh:195 writes tracebloc.io/last-refreshed-jobs-manager-digest=sha256:e2e-sentinel, then runs helm upgrade --reuse-values and --reset-then-reuse-values against the working-tree chart — and line 278 asserts the annotation is still there afterwards. With defaults (imageRefresh.enabled: true, docker.io, no operator pin) branch 2 fires and renders docker.io/tracebloc/jobs-manager@sha256:e2e-sentinel onto the init container, api and requests-proxy. That script's only readiness wait is kubectl wait ... nodes, and it never asserts a control-plane image ref — so Fleet auto-upgrade E2E (k3d) passed on this head while doing exactly that. A sha256:[a-f0-9]{64} guard in the helper closes it and is unit-testable with no cluster, which also partly answers your "can't be tested" caveat.
2. BLOCKING — imageRefresh.suspend is never consulted. tracebloc.imageRefreshEnabled gates only on not $ir.enabled, but suspend is a real key (values.yaml:1833, consumed at image-refresh-cronjob.yaml:702). With enabled: true, suspend: true — supported, and the state the local-k3d recipe leaves behind — the CronJob never runs, yet the helper still treats refresh as the live update path and pins the last annotation permanently. It hurts most on the DaemonSet: before this change a newly joined node pulled the current :tag; after it, the node pulls a frozen digest and silently runs a stale build with nothing reporting it. That is the same "silently runs an OLD control-plane image" class as client-runtime#199, arriving from the other direction.
3. MERGE ORDER — B depends on A, it does not merely pair with it. I'd said A and B compose, and in the steady state they do: with recorded == latest, B renders that digest and A's live-spec check reads on-digest and no-ops. But the annotation LAGS the live spec in two reachable states, and in both, B pulls the workload backwards:
- a rollout that times out —
set imagehas moved the spec,set -eexits before the annotate, so spec = NEW and annotation = OLD; helm rollback, which re-applies a stored manifest rather than re-rendering, so it restores the digest that was live at that revision's render time. Answering the rollback question directly: yes, a rendered digest means a rollback can undo a security refresh.
Both are self-healing only because A's live-spec reconcile re-pins within a tick. Without A, recorded == latest no-ops and the stale digest sticks until the next upstream publish. So A first is a correctness precondition, not a preference — worth stating in the header next to the lookup caveats.
Non-blocking, but I'd like them addressed or written down:
4. This narrows #569's offline-restart guarantee, and the body doesn't mention it. Keeping the pull policy is necessary but not sufficient: IfNotPresent only helps when the REFERENCE is cache-satisfiable. A node that pulled :tag -> sha256:OLD satisfies repo@sha256:OLD from cache but cannot satisfy repo@sha256:NEW. Since the first-observation path records latest WITHOUT re-imaging, we can now render a digest the node has never pulled: online that is the fix working, offline it turns "restarts on a stale image" into "cannot start". Same root cause makes a side-loaded k3d image unpullable — k3d image import stores a tag alias with no resolvable digest, and the mirror gate doesn't help because a local k3d install IS docker.io.
5. First-observation roll. The script records without re-imaging precisely because "rewriting repo:tag to repo@digest for identical content would roll every pod on install". This reintroduces that roll through helm: first tick writes the annotations, the next upgrade changes all four pod specs for a zero-content change — recreating jobs-manager (strategy: Recreate on RWO PVCs, the #545 wedge) and every resource-monitor pod on every node. Not hourly, to be fair: auto-upgrade only fires on a chart-version bump. Relatedly, the helper's doc says "the digest image-refresh last APPLIED" — on this path it is what refresh last OBSERVED and deliberately did not apply. Worth correcting; it changes how a reader reasons about the value.
6. A --set env.CLIENT_ENV=stg switch becomes inert on the image: the annotation key isn't env-scoped and the tag is dropped when a digest is present, so the edge renders the other env's digest while every label reads stg. It self-corrects next tick unless the switch leaves the Deployment unsettled — then the settled guard skips forever and each upgrade re-pins the old env's digest.
7. Seven uncached lookups per render (5 in jobs-manager, 1 each in requests-proxy and resource-monitor), and a non-NotFound error RAISES rather than returning empty — failing the whole release, which is what resource-monitor-daemonset.yaml:22-27 documents for backend#2469. The RBAC half is fine as you argued; the transient-apiserver-error half is a new failure mode for a render that used to work offline, and unlike the metrics-server lookup there's no escape hatch equivalent to nodeAgents.metricsServerPreflight: false.
8. Routing the effective digest into controlPlanePullPolicy (:277, :912) is a no-op: wherever the helper can return non-empty, imageRefreshEnabled && docker.io already held, so the policy was already IfNotPresent. Harmless, just two more lookups.
On the Helm 4 server-side-apply question from the last round: I said this would get strictly worse with the chart rendering a digest, and I want to withdraw that. The premise was wrong — repo:tag is itself a rendered value for .image, so helm has always declared and owned that field; that ownership is exactly why the #199 revert happens. This changes the VALUE helm writes, not the ownership set, and adds no manager. On the next upgrade after a kubectl set image, helm reclaims the field and writes what the chart rendered: it does not error and does not preserve the digest. Before, it reclaimed and wrote the wrong value; after, the right one in the steady state. So: neutral on ownership, better on outcome, with the lag window in (3) as the exception. Also relevant for @LukasWodka: the fleet path is Helm 3 today — autoUpgrade pins alpine/helm 3.16.4 and CI's render job pins v3.15.4 — so SSA ownership doesn't reach the fleet until someone bumps that tag to 4.x.
On testing: I mutated the helper to always return "" and 5 tests across the jobs_manager / requests_proxy / resource_monitor suites go red, so the operator-pin branch is genuinely covered (695 pass clean). Branch 2 has nothing that can redden, as you say — but I don't think it needs a new suite in full-seal-e2e. The rig already exists: e2e-auto-upgrade.sh runs on real k3d, already seeds that exact annotation, and already runs both upgrade paths. It needs one assertion on the rendered image ref (and, given finding 1, a real 64-hex digest instead of the sentinel).
What I could NOT verify here, stated plainly: I have no cluster, so branch 2 — the lookup path, the whole point of the PR — was never actually executed by me; every claim about it is read from the template and from the CronJob's annotation writer. No k3d locally, so the side-loaded-digest hazard in (4) is reasoned from the tag-alias behaviour, not reproduced. And no Helm 4 server-side-apply rig, so the ownership analysis above is from managedFields semantics plus the fact that the revert demonstrably happens today (which proves helm reclaims the field rather than erroring) — not from an observed conflict.
2 participants
Refs tracebloc/client-runtime#199
The problem (recap)
Control-plane images render as
repo:tag+IfNotPresentwhen unpinned (pertracebloc.controlPlanePullPolicy, #569/#705 — deliberately, to survive an offline restart).image-refreshkeeps them current out-of-band viakubectl set image repo@digest. Buthelm upgrade --reset-then-reuse-values(the hourlyauto-upgrade) re-renders the bare:tagand drops that pin; on a node whose:taglayer is stale the pod then silently runs an old control-plane image (client-runtime#199: a pre-#416 jobs-manager under a sealed egress netpol on stg/prod). #1008 catches this on the nextimage-refreshtick (≤ ~15 min); this PR removes the revert entirely.The fix (this PR — "B")
New
tracebloc.controlPlaneDigesthelper resolves the effective digest:images.<name>.digest) — wins, unchanged;tracebloc.io/last-refreshed-<image>-digestannotation vialookup;""(bare:tag).Wired into all four control-plane image sites (jobs-manager api + pods-monitor, requests-proxy, resource-monitor). A
helm upgradenow rendersrepo@<current-digest>instead of reverting to:tag.Why it's safe / the caveats to review
backend#2469bootstrap-lockout rule (which is about new reads the SA can't already do).lookupis empty duringhelm template/helm diff/ the FIRST install → helper returns""→:tag. That's correct there (nothing pinned yet; the node's fresh tag pull is the right image), but it means:@digest(verified) and all existing unit tests pass, but the lookup-seed itself needs an e2e / real-cluster test (belongs infull-seal-e2e). Flagging that gap explicitly.helm diffwill show a:tag→@digestchurn that won't actually happen on apply — worth a note for anyone who diffs upgrades.helm lintclean,helm templaterenders (tag when clusterless,@digestwhen pinned), 101/101 unit tests pass, Chart.yaml bumped.Relationship to #1008
Independent, not stacked. #1008 (A) bounds the exposure to one tick and is the safer, self-contained fix; this (B) eliminates the window but has the bigger blast radius (
lookupsemantics above). Ideally land A first, then B after a cluster e2e confirms the seed. Reviewer: Shujaat (owns #569/#705); Asad for the pipeline view.Not changing the pull policy
Always-when-unpinned would re-break #569/#705 (offline restart →ImagePullBackOff).🤖 Generated with Claude Code
Note
Medium Risk
Changes how all four control-plane images render on every
helm upgradevia clusterlookup; wrong or stale annotations could pin an unexpected digest, though operator pins still win and there is no new RBAC.Overview
Adds
tracebloc.controlPlaneDigestso control-plane workloads stop reverting to a floating:tagonhelm upgradeafter image-refresh has pinned them withkubectl set image(client-runtime#199).Resolution order: operator
images.*.digest(unchanged) → when image-refresh is active on docker.io, the digest from the jobs-manager Deployment’stracebloc.io/last-refreshed-<image>-digestannotation vialookup→ otherwise bare tag. That helper is wired into jobs-manager (init + api + pods-monitor), requests-proxy (jobs-manager annotation), and resource-monitor;imagePullPolicyuses the same effective digest astracebloc.controlPlanePullPolicy.Chart version 1.9.108 → 1.9.110.
lookupis empty underhelm template/ first install, so behavior there stays:taguntil a cluster exists and refresh has run.Reviewed by Cursor Bugbot for commit d92bdab. Bugbot is set up for automated code reviews on this repo. Configure here.