Uh oh!
There was an error while loading. Please reload this page.
fix(chart): every pod spec now states automountServiceAccountToken, and a guard keeps it that way (backend#2345) - #794
Conversation
…nd a guard keeps it that way (backend#2345) Seven of the thirteen pod-bearing templates named no automount key, so they inherited the Kubernetes default of true. The telemetry Collector was one (backend#2344); this is the rest of the class, plus the check that closes it. Six decisions, each read off the chart rather than assumed: true resource-monitor, jobs-manager, auto-upgrade, image-refresh -- each SA is bound to a Role or ClusterRole with real verbs; polling nodes, submitting Jobs, running helm upgrade and patching Deployments is what these workloads are for. The value was already the default, so nothing changes at runtime; it is now a decision rather than an inheritance. false mysql, and both device-plugin DaemonSets -- these name NO ServiceAccount, so they were mounting the namespace default SA token. This is the only behavioural change in the diff: a database, two busybox init containers, and two upstream plugins that speak to the kubelet over the device-plugin socket, none of which call the API server. The guard renders the charts and reads the pod specs back out, so a template added later is checked without touching it. It holds no list of workloads and no list of expected values. Three fail-closed paths: a template no value combination reaches is UNREACHED and fails rather than passing as nothing to check; zero pod specs found is a finding; and a pod spec that refuses the token while the chart binds its SA to a Role is a contradiction. That last check is one-directional on purpose. The mirror rule has a real counter-example -- ingestor post-install-job sets true with no binding, correctly, because it presents the token to jobs-manager to be TokenReviewed rather than to the API server. Bound and needs-its-token are not the same predicate, and a guard assuming they were would need an exception list. Mutation-proven, anchor asserted each time: strip the key from a template -> offender named; drop a render combination -> gpu-device-plugin reported UNREACHED; render nothing -> broken-guard finding; flip jobs-manager to false -> contradiction. Per-vendor unit assertions too, so a device-plugin fix applied to nvidia and not amd reddens. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e514882 to
f755652Compare
saqlainsyed007
left a comment
There was a problem hiding this comment.
Follow-up to #793 that closes the class (backend#2345), and the guard is genuinely the point. Reviewed all six template edits and the new drift check against the code.
The six decisions are each correct and justified from the chart's own RBAC.true where the ServiceAccount is bound to real permissions the workload exists to use — jobs-manager (creates/deletes Jobs, reads logs, TokenReviews), resource-monitor (polls pods/nodes/metrics), auto-upgrade (helm upgrade over *), image-refresh (patches Deployments/DaemonSets); false where no ServiceAccount is named and no API calls happen — both GPU device-plugins (talk to the kubelet over the device-plugin socket) and mysql (a DB + busybox init containers). The false ones were previously mounting the namespace default token by omission.
The guard clears the house bar on every axis:
- Presence, not a value. It requires every rendered pod spec to state the key in one direction or the other — because the defect was that nobody chose, not that anyone chose wrong.
- Derived, not restated. Pod specs are read out of rendered manifests, and the template denominator is grepped from the files, so a template added tomorrow is checked tomorrow — no hand-list to go stale.
- One value check, in the only direction with no exceptions. A pod spec whose SA is the subject of a rendered RoleBinding/ClusterRoleBinding may not set
false(the chart would grant permissions the pod can't reach); the bound-SA set is derived from the rendered bindings. The mirror is deliberately not enforced, correctly —post-install-jobsetstruewith no binding because it presents its token for TokenReview, so "bound" and "needs its token" aren't the same predicate, and a guard assuming they were would need the exception list this file exists to avoid. - Fails closed twice. Zero pod specs found is an error, not agreement; and any pod-bearing template that no value combination in
render_all()reaches is reported UNREACHED and fails — the matrix-guard blind spot handled — plus a template rendering conflicting values across combinations is itself a finding.
The helm-unittest additions pin direction per GPU vendor and for mysql (equal on the key + notExists serviceAccountName), mutation-proof against a flip back. Chart bumped 1.9.62→1.9.63, guard registered in DRIFT_GUARDS. Green, no open threads. Approving.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Aug 23, 2026
/fr-pass |
Closes backend#2345. Follow-up to backend#2344 / #793, which fixed the telemetry Collector instance; this closes the class.
Why this is not just six more lines
#2344 described the Collector as the only silent pod spec. It wasn't — 7 of 13 were silent. The defect was never that someone chose wrong; it was that nobody chose, and nothing asked. Fixing the seventh instance leaves the eighth free to happen, so the guard is the point of this PR and the six edits are what let it be armed green.
The six decisions, each read off the chart
resource-monitor-daemonsettrueresource-monitor-rbac.yamlbinds its SA to a ClusterRole over pods, nodes,nodes/status,metrics.k8s.io— polling those is the jobjobs-manager-deploymenttruerbac.yaml: creates/deletes training Jobs, reads pod logs, issues TokenReviewsauto-upgrade-cronjobtrueauto-upgrade-rbac.yaml:*/*/*in-namespace plus cluster-scoped storage and RBAC — it runshelm upgradeimage-refresh-cronjobtrueimage-refresh-rbac.yaml: patches Deployments and DaemonSetsmysql-deploymentfalsegpu-device-plugin(×2)falseThe four
trues were already the effective value, so nothing changes at runtime — they become decisions instead of inheritances.The one behavioural change is the three
falses:mysql-clientand both device-plugin DaemonSets stop mounting the namespace'sdefaultSA token. Checked before deciding — a database plus two busybox init containers make no API calls, and the upstream NVIDIA/AMD plugins talk to the kubelet over the device-plugin socket mounted in the same pod spec. Worth a reviewer's eye since the device plugins are third-party.The guard —
scripts/tests/automount-token-explicit.sh, added toDRIFT_GUARDSRenders the charts and reads the pod specs back out of the manifests, attributing each to its template via helm's
# Source:line. It holds no list of workloads and no list of expected values — a template added tomorrow is checked tomorrow.Three fail-closed paths, because the obvious way to write this guard is the way that lies:
falsewhile the chart binds its SA to a Role/ClusterRole — the workload cannot use permissions it was granted. This is what stops atruefrom being flipped back and staying green.The mirror rule is deliberately not enforced
bound to a Role → must be trueholds with no exceptions here. The reverse —true → must be bound— does not:ingestor/templates/post-install-job.yamlsetstruewith no binding, correctly, because it presents its token to jobs-manager to be TokenReviewed, never to the API server. "Bound" and "needs its token" are different predicates, and a guard that assumed otherwise would need an exception list — the restating this guard exists to avoid. Written down in the header rather than left for the next person to rediscover.Values in the
falsedirection that RBAC can't speak to are pinned by unit tests instead (mysql_test.yaml,gpu_device_plugin_test.yaml), asserted per vendor because the GPU file holds two separate pod specs.Verification
Guard output, so the decisions are reviewable at a glance:
Mutations — anchor asserted applied each time, then restored
mysql-deploymentgpu-device-plugin.yamlreported UNREACHEDrender_all()emit nothingjobs-managertrue->falsemysqlfalse->trueExpected: false / Actual: true)1 failed, 13 passed— the per-vendor split earns its keepNote on the base
The first commit here is #793's, cherry-picked, so the guard could be armed green rather than landed red (CLAUDE.md rule 4) — the Collector is one of the seven and its fix is still in review. Merge #793 first; this then rebases to the single commit that matters. If you'd rather not carry the duplicate, say so and I'll rebase once #793 lands.
Note
Medium Risk
Security-hardening of ServiceAccount token mounts, including a real behavior change for MySQL and third-party GPU device plugins. The four
trueworkloads are documentation-only at runtime; a wrongfalseon an API-using pod would break upgrades, image refresh, or job submission, which the new guard is meant to catch.Overview
Makes
automountServiceAccountTokenan explicit decision on every remaining silent pod spec, and adds a drift guard so a new template cannot inherit Kubernetes’ defaulttrueagain.Runtime change: MySQL and both GPU device-plugin DaemonSets now set
false(they name no SA and never call the API). jobs-manager, resource-monitor, auto-upgrade, and image-refresh settrue— already the effective default, now stated.The new
automount-token-explicit.shguard (wired intoDRIFT_GUARDS) renders the charts, requires the key on every pod spec, fails on unreached templates or empty renders, and rejects a bound SA that setsfalse. Unit tests pin thefalsedirection for MySQL and each GPU vendor. Chart version 1.9.62 → 1.9.63.Reviewed by Cursor Bugbot for commit f755652. Bugbot is set up for automated code reviews on this repo. Configure here.