fix(chart): inject the release-scoped pull-secret name for training pods (backend#2119) - #751
Conversation
…ods (backend#2119)
`client-runtime`'s `job.yaml` hardcoded `imagePullSecrets: [{name: regcred}]`.
This chart has never created a Secret by that name — `tracebloc.registrySecretName`
renders `<release>-regcred`, and `docker-registry-secret.yaml` renders only when
`dockerRegistry` is set with `create: true`. So the reference could not resolve on
ANY release: every training pod named a Secret the kubelet could not retrieve,
logged `FailedToRetrieveImagePullSecret`, and fell back to an ANONYMOUS pull of
the ~2.7 GB training image.
On a mirrored install the consequence is sharper than slowness: the training pod
was the one pod in the release NOT using the credential the operator supplied.
The name is release-dependent, so it cannot live as a literal in the runtime repo
— a literal there is wrong for every release, and the one it carried was wrong
for all of them. `IMAGE_PULL_SECRET_NAME` is rendered from the same helper as
every other `imagePullSecrets` block here and gated on the same condition that
CREATES the Secret, so the three cannot disagree.
Absent when no registry is configured: the runtime reads that as "pull publicly"
and sets no `imagePullSecrets` at all, which is the honest declaration of a
public pull rather than a broken reference to a private one.
Skew-safe both ways: a runtime older than backend#2119 ignores the variable, and
a newer one treats its absence as public.
VERIFIED BY RENDERING, not by reading:
with a private registry -> `IMAGE_PULL_SECRET_NAME: "myrel-regcred"`, and the
Secret `myrel-regcred` is present in the same render
without one -> the variable is absent (0 occurrences)
`scripts/manifest.sha256` covers installer scripts only (0 template entries), so
no regeneration is required for a chart-template change.
…ckend#2119) The `chart content ⇒ Chart.yaml version bump` gate is required, and correctly so: a template change that ships without a version bump is invisible to `helm upgrade`, which is exactly the failure mode that would keep every training pod on the broken `regcred` reference after this fix merged. `version` and `appVersion` moved together, matching the existing pairing. Running the guard locally reports `could not determine the PR base SHA — refusing to report N/A without checking`, which is the fail-closed behaviour working; it resolves the base in CI.
|
Findings from a review pass. No verdict attached. 1. Version collision, and both gates are blind to it
If the release train tags v1.9.47 off develop before this merges, this content lands at an already-published version and either ships dark or overwrites the published tarball — exactly the mode (#750 also touched 2. Nothing asserts the new env varNo test anywhere references A future edit to the 3. Nit — comment density18 lines of comment for 2 lines of YAML, restating the PR body, and it renders into the shipped manifest. The why-injected-rather-than-literal sentence earns its place inline; the incident history belongs in the ticket. Verified and cleanThe gating claim holds exactly as stated, and it's worth recording why: Merge order vs the paired — drafted with Claude Code |
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
Rendered both cases and ran the suite on the branch — the mechanism is right. Gate, Secret name and env var all come out of the same two helpers, the Secret lands in .Release.Namespace, and that is where jobs-manager spawns training Jobs (get_current_namespace() reads its own SA namespace), so the name cannot dangle. rbac.yaml already grants secrets: get in both the clusterScope true and false branches, so the runtime's fail-closed 403 branch isn't reachable by upgrading one side. 467 helm-unittest tests pass with the change in place.
Requesting changes on the version only: develop is already at 1.9.47 (#750, 8de5d64), and since both sides made the identical two-line edit git merges this with no conflict and no CI signal — two different chart contents under one version. Merge develop and bump to 1.9.48.
aptracebloc already raised that and the missing helm-unittest coverage for the new var, so I'm not repeating either inline. I do think the test is worth adding — the whole fix is one {{- if }}, and global_image_registry_test.yaml already has the exact two-case shape to copy.
|
Status update on the version collision from my earlier comment — re-checked, and @saadqbal reached the same conclusion independently. Still colliding, still latent. v1.9.47 is not tagged and not released. Newest tag and release is Both gates remain blind, as expected: Still no test referencing Nothing new needed from me; @saadqbal's two inline findings (the hand-made — drafted with Claude Code |
…e name
Both of Asad's findings on client#751, and the rendering test the original
change should have had.
1. THE CONFIG THAT WORKED BY ACCIDENT (`dockerRegistry.create: false` plus a
Secret hand-made in the namespace and literally named `regcred`).
He is right that it worked, and right about why it matters. Worth naming
what it actually was: those installs were HALF authenticated. Every
chart-rendered pod got no imagePullSecrets at all, because
`useImagePullSecrets` was false; only the TRAINING pod authenticated, because
client-runtime's job.yaml carried `regcred` as a literal. So the moment
backend#2119 replaces that literal, they fall to an anonymous pull with
nothing louder than an INFO log -- the same silent downgrade this PR exists
to remove, moved rather than fixed.
A values.yaml note (his first suggestion) documents the breakage without
giving them anywhere to go: their reason for hand-making the Secret is
usually that credentials must not live in values, so "set create: true"
is not an answer. So: `dockerRegistry.existingSecret`, his second. The
helper returns it, every imagePullSecrets block and the injected
IMAGE_PULL_SECRET_NAME read that one helper, and their control-plane pods
become authenticated too -- which they never were.
The trap, and why `createRegistrySecret` is a separate helper: "a pull
Secret exists" and "the chart renders one" had the same answer until now.
Gating the Secret template on the former would have the chart overwrite the
operator's Secret using credentials it does not have -- a supported config
turned into a broken pull on the first upgrade. Mutation-tested.
`create` + `existingSecret` together is refused at both layers: the schema
declares the exclusion (better message, fires before rendering) and the
template `fail`s (the gate, since `--skip-schema-validation` exists). Both
proved to fire.
Documented honestly rather than papered over: with `existingSecret` the
chart cannot mirror the Secret into the node-agents and GPU device-plugin
namespaces, because it does not have its contents. values.yaml names those
namespaces and the conditions under which they apply. The failure if the
operator misses one is ImagePullBackOff -- loud and diagnosable, unlike the
failure this option prevents.
2. THE PASSTHROUGH SHADOW. `IMAGE_PULL_SECRET_NAME` was absent from both
`.Values.env` exclusion lists, so `env.IMAGE_PULL_SECRET_NAME` rendered it
twice with the passthrough copy winning. Excluded now.
His point about the consequence is the part that decided the fix: a slow
pull is recoverable, but client-runtime treats a pull-secret name that does
not resolve as PERMANENT and dead-letters the experiment, so a shadowed
value costs the run.
Excluded from BOTH containers' lists, not just the one where it could do
harm, so a stray value is inert rather than sitting unread in
pods-monitor's environment. Nobody can depend on that: the variable does not
exist before this change. `JOB_IMAGE_HOST` has the same unexcluded shape and
is deliberately LEFT -- it is not a regression, and removing an override
somebody may rely on is a separate decision. It also serves as the live
canary below.
TESTS -- `scripts/tests/chart-pull-secret.bats`, 11 cases against a real
`helm template`. The original fix was verified by rendering once by hand, which
is how a chart with no pull-secret rendering test shipped a reference that
resolved on no release at all.
create -> the injected name IS a Secret present in the same render
(the property that was broken), and one distinct name
across every imagePullSecrets block
existingSecret -> the operator's name everywhere, and ZERO Secrets rendered
public -> no name, no imagePullSecrets, no Secret. Absence is the
declaration of an anonymous pull, so it has to be real
create: false -> same as public, not half-configured
contradiction -> refused by the schema, and by the template when the
schema is skipped
env shadow -> api keeps the computed value; pods-monitor gets nothing
Plus a general "no container renders any env key twice" over every workload,
so the next computed var added without an exclusion is caught here instead of
in review -- with a canary test asserting `JOB_IMAGE_HOST` still renders twice,
because a parser that found no containers and a clean render look identical.
Five mutations, each reddening at its own site: the Secret gate reverted to
`useImagePullSecrets` (1 red), `registrySecretName` ignoring `existingSecret`
(2), the exclusions removed -- Asad's finding, restored (2), and
`useImagePullSecrets` ignoring `existingSecret` (2), the `fail` removed (1).
Env values are read by parsing the rendered YAML per container, not by
grepping: the two containers have separate env lists and a grep cannot tell
them apart -- which is exactly why the first count of this variable read 2 when
the api container had 1.
bats-hygiene, chart-version-guard (23), chart-env-vocabulary (28) and
`helm lint` all green. `scripts/manifest.sha256` covers installer scripts only
(0 entries under scripts/tests), so no regeneration. Chart already bumped to
1.9.47 in this PR.
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… taken Bugbot round two on client#751. Both findings measured, both real. 1. VERSION COLLISION. `develop` is already `1.9.47` — bumped by #750, a different change — and this PR also said 1.9.47. Measured: merge-base 1.9.46 this PR 1.9.47 develop 1.9.47 <- #750, unrelated content newest published tag v1.9.44 Git auto-merges cleanly and the version guard passes, because the guard asks "is there a bump against THIS PR's base", which there is. Two different chart contents would then share one version, and auto-upgrade compares versions — so the training-pod credential fix could land under a version Helm already considers current and never apply. Now 1.9.48, which is unused on develop and untagged. The guard's blind spot is not fixed here — it compares against the base, not against the branch it will merge into, and a same-version collision is invisible to it by construction. Filed separately. 2. RBAC NAMED THE OPERATOR'S SECRET. The auto-upgrade Role in the GPU device-plugin namespace — `kube-system` by default — pins `get`/`update`/`patch`/`delete` to a `resourceNames` list so Helm can reconcile the mirrored pull Secret. Resolving that list through `tracebloc.registrySecretName` meant that with `existingSecret` the auto-upgrade ServiceAccount got read AND DELETE on the operator's own dockerconfigjson in kube-system: rights over a Secret this chart does not own, never asked for, granted as a side effect of naming one. `tracebloc.createdRegistrySecretName` is the chart's own name, independent of `existingSecret`, and RBAC uses that. One more consumer of the split the Secret template already needed — "does the chart own it" is a different question from "what does the release reference". It fixes Bugbot's second half too, which is the direction I would have missed: switching a live release from `create: true` to `existingSecret` orphans the mirrored `<release>-regcred`, and Helm has to be able to delete it. A list that followed `existingSecret` stops naming the orphan, so the delete 403s and stalls every later auto-upgrade tick. Three tests, 12-14, and the parser asserts it found resourceNames at all — without that, a Role-walker that reached nothing looks exactly like a chart with no offending grant. Mutation: point RBAC back at `registrySecretName` and both `existingSecret` cases redden (the grant appears, and the orphan becomes undeletable). `create: true` renders identically either way, which is the third test: splitting the names must not desynchronise the ordinary path. Not changed, and pre-existing rather than introduced here: the same Role holds `create` on secrets in kube-system with no `resourceNames`, because RBAC cannot scope `create` by name. Out of scope for a review round on this PR; noted on the thread. 14/14 in the new file, bats-hygiene green, chart-version-guard 23/23, helm lint clean.
|
bugbot run |
The conflict is the FIX working. Both branches moved Chart.yaml off the merge-base's 1.9.46: develop to 1.9.47 (#750) and this branch to 1.9.48 after Bugbot found that the original 1.9.47 here collided with it. While both sides said 1.9.47 git merged the line silently -- which is exactly how the collision stayed invisible to the version guard (backend#2193). # Conflicts: # client/Chart.yaml
…in CI Bugbot, and it is the sharpest finding on this PR: the fourteen chart-render tests I added in the previous commit NEVER RAN. `scripts/tests/*.bats` is run by exactly one job -- the required `Unit tests` job -- which installs bats and not helm, so every case hit `skip "helm not installed"`. A wrong IMAGE_PULL_SECRET_NAME or Secret name would have shipped green past tests written to catch exactly that. That is this repo's own inert-verification class, in the tests written to close an inert-verification gap. Verified rather than assumed: no `setup-helm` anywhere in standard-checks.yml, and `Unit tests` IS required on develop (`Unit tests`, `Lint`, `quality/*`, `version-bump-gate`, `Source-of-truth drift`). Two fixes, because arming the job alone would leave the trap for the next test: 1. helm installed in the `Unit tests` job, same sha-pinned action and same v3.15.4 pin as helm-ci.yaml and drift-checks.yaml -- and for the reason helm-ci already states: helm is absent from the runner image, and an unpinned one silently changes which assertions run. The job's timeout arithmetic is re-done in its comment rather than left stale: setup-helm is one ~15MB download, and the documented worst case (390s bounded apt + ~155s bats) keeps ~6 min of headroom inside the 15. 2. A MISSING TOOL NOW FAILS IN CI and skips only locally. `require_tool` reads `CI`, which GitHub sets to `true` on every runner. On a laptop without helm a skip is the right answer; in a required gate it is indistinguishable from a pass, and that is the entire defect above. Proved both directions with a PATH that hides helm but keeps bats: 14 reds with CI=true, 14 clean skips without. Also from the same finding: `--skip-schema-validation` landed in helm 3.16 and CI pins v3.15.4, so that one case self-skips below 3.16 -- the same treatment and the same reason as the helper-backstop cases in chart-env-vocabulary.sh, and it names the version it saw. The SCHEMA half of that pair runs on every version, so the contradiction is never unguarded; only the second layer is version-bound. Bumping the CI pin turns it on with no change to the test. bats-hygiene 18/18, chart-pull-secret 14/14 locally on helm 4.1.1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
The previous commit armed these tests in the required `Unit tests` job, and the
first thing that arming did was catch a version-specific string in one of my own
assertions -- which is the arming working, a commit after Bugbot said the tests
would never run.
`[[ "$output" == *"must be false"* ]]` passed on my local helm 4.1.1 and failed
in CI on the v3.15.4 pin. Measured on both, same values, rather than reasoned
about:
3.15.4 - dockerRegistry.create: dockerRegistry.create does not match: false
4.1.1 - at '/dockerRegistry/create': value must be false
So the per-rule wording changes wholesale between schema-validator generations.
Anchored instead on the header line both print -- "meet the specifications of the
schema" -- which is also the more meaningful assertion: it proves the SCHEMA
layer refused rather than the template, and that distinction is the entire point
of the test (its sibling covers the template layer). `create` and `false` are
then matched case-insensitively to pin WHICH rule fired, so an unrelated schema
failure such as a missing required value cannot satisfy it.
Bugbot's conclusion was right and its stated cause was not: 3.15.4 does not
print "Must be false", it prints "does not match: false", so this was never a
case-sensitivity bug. Installed the pinned 3.15.4 locally and read both outputs
rather than taking either the tool's diagnosis or my own guess.
Verified on BOTH versions:
v4.1.1 ok=14 not ok=0 skipped=0
v3.15.4 ok=14 not ok=0 skipped=1 (--skip-schema-validation, as designed)
And the fail-closed property from the previous commit still holds: 14 reds with
CI=true and helm hidden.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ 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 4484ff9. Configure here.
… (#2119) @saadqbal asked for this in his review body and I had only acted on his two line threads -- the ask was in the summary, which is exactly the failure mode of treating "0 unresolved threads" as "review addressed". His words: the whole fix is one `{{- if }}`, and `global_image_registry_test.yaml` already has the two-case shape to copy. Eight cases in that shape, and the two invariants are the ones the original defect broke in each direction: 1. a pull Secret exists -> the var is injected AND names the Secret the pod actually pulls with. Asserted against the pod's own `imagePullSecrets` and against the rendered Secret's `metadata.name`, so the two cannot drift apart -- the broken state was a plausible string that resolved to nothing. 2. no registry configured -> the var is ABSENT, not empty. client-runtime reads absence as "pull publicly", so an empty value would make it set a pull secret nothing created: the same defect inverted. `existingSecret` is covered too, because that is the path where the name is not derivable from the release name at all -- the case a literal in job.yaml could never have got right -- plus the assertion that the chart renders NO Secret there, so it cannot overwrite the operator's own. Mutation-proved, since a green suite that cannot fail is what this coverage is for: | mutation | result | |---|---| | delete the injecting `{{- if }}` block | 2 failed | | render the var unconditionally (empty on a public pull) | 2 failed | | drop the release prefix from the name helper | 3 failed | | gate the Secret template on `useImagePullSecrets` | 1 failed | Full suite 482 passed / 32 suites (467 before this file). helm lint clean, chart-version-guard 23/23, chart-pull-secret 14/14, and the chart stays one patch above develop's 1.9.47. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@saadqbal — you asked for the helm-unittest coverage in your review body, and I had only acted on your two line threads. That is squarely the failure mode of treating "0 unresolved threads" as "review addressed", so thank you for putting it in writing where re-reading caught it. Added in Eight cases in
Mutation-proved, because a green suite that cannot fail is the thing this coverage is for:
Full suite 482 passed / 32 suites — 467 before this file, matching your count. helm lint clean, Everything else from your review is in: the version is Arturo's two follow-ups on the same change are also addressed: the |
|
bugbot run |
There was a problem hiding this comment.
✅ 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 d0cf519. Configure here.
3 participants
Part of backend#2119. Pairs with
client-runtimePR (branchfix/2119-image-pull-secret) — that one is the behaviour change; this one supplies the name it needs.The defect
client-runtime'sjob.yamlhardcoded:This chart has never created a Secret by that name.
tracebloc.registrySecretNamerenders<release>-regcred, anddocker-registry-secret.yamlrenders only whendockerRegistryis set withcreate: true. So the reference could not resolve on any release: every training pod named a Secret the kubelet could not retrieve, loggedFailedToRetrieveImagePullSecret, and fell back to an anonymous pull of the ~2.7 GB training image.Anonymous pulls are rate-limited per source IP, so under concurrency this becomes intermittent image-pull failures that present as random training failures.
On a mirrored install the consequence is sharper than slowness: the training pod was the one pod in the release not using the credential the operator supplied.
Why the name is injected rather than written in the runtime
It is release-dependent. A literal in
client-runtimecannot be correct for every release — and the literal it carried was correct for none.IMAGE_PULL_SECRET_NAMEis rendered from the same helper as every otherimagePullSecretsblock in this chart, and gated on the same condition that creates the Secret, so the three cannot disagree.When no registry is configured the variable is absent, and the runtime reads that as "pull publicly" and sets no
imagePullSecretsat all — the honest declaration of a public pull, rather than a broken reference to a private one.Skew-safe in both directions: a runtime older than backend#2119 ignores the variable; a newer one treats its absence as public.
Verified by rendering, not by reading
dockerRegistry.create=true+ server/user/pass/emailIMAGE_PULL_SECRET_NAME: "myrel-regcred"— andSecret myrel-regcredpresent in the same renderRendering it was worth doing: my first three attempts failed on chart schema validation (
servermust be an absolute URI,emailrequired,clientId/clientPasswordnon-empty,storageClass.provisionerrequired whenstorageClass.create), so an unrendered "looks right" review would have proved nothing.Manifest
scripts/manifest.sha256covers installer scripts only — 0 template entries — so a chart-template change needs no regeneration. Confirmed againstscripts/gen-manifest.sh'sFILESarray rather than assumed.Note
Medium Risk
Changes registry authentication wiring for spawned training pods and RBAC on mirrored pull Secrets in kube-system; mitigated by layered schema/template guards and extensive render tests, but wrong names would still break private-registry training pulls.
Overview
Pairs with client-runtime backend#2119: jobs-manager now gets
IMAGE_PULL_SECRET_NAMEfrom the sametracebloc.registrySecretNamehelper as everyimagePullSecretsblock, only when a registry pull Secret is configured. Training pods can use the real Secret name instead of a hardcodedregcredthat this chart never creates.dockerRegistry.existingSecretis added for bring-your-ownkubernetes.io/dockerconfigjsonSecrets. Helpers split “use a pull secret” vs “chart creates the Secret” so the chart does not overwrite operator Secrets;create: trueandexistingSecrettogether fail at schema and template. Auto-upgrade GPU-namespace RBAC names only the chart-created<release>-regcredviatracebloc.createdRegistrySecretName, not the operator’s Secret name.env.IMAGE_PULL_SECRET_NAMEis excluded from the genericenvpassthrough so it cannot shadow the computed value. Chart 1.9.48; helm unittest andchart-pull-secret.bats(realhelm template) lock the behavior. Unit tests CI installs Helm v3.15.4 so bats cases cannot skip when helm is missing.Reviewed by Cursor Bugbot for commit d0cf519. Bugbot is set up for automated code reviews on this repo. Configure here.