Skip to content

feat(rbac): let jobs-manager read daemonsets for the heartbeat version inventory - #750

Merged
saadqbal merged 6 commits into
developfrom
feat/152-rbac-daemonsets-read
Aug 19, 2026
Merged

feat(rbac): let jobs-manager read daemonsets for the heartbeat version inventory#750
saadqbal merged 6 commits into
developfrom
feat/152-rbac-daemonsets-read

Conversation

@saadqbal

@saadqbalsaadqbal commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Companion to tracebloc/client-runtime#348 (client-runtime#152). Refs client-runtime#152.

Why

client-runtime#152 adds a version inventory to the edge-device heartbeat, assembled from the configured images on the chart's own workloads. The jobs-manager Role already grants read on deployments, which covers jobs-manager, pods-monitor, requests-proxy, mysql-client and egress-proxy.

resource-monitor is a DaemonSet — the one workload that isn't a Deployment — and daemonsets was granted nowhere in this Role.

Without the grant the list 403s. The runtime degrades rather than failing (it lists the two kinds separately for exactly this reason), so the symptom isn't an error anyone would chase: resource-monitor is reported as null forever, which reads as "not deployed".

What

apps: ["deployments"]apps: ["deployments", "daemonsets"], read-only (get, list, watch), in both RBAC branches — an edge on clusterScope: false needs it just as much. Namespace-scoped in the Role branch.

Two helm unit tests assert both branches. The two rule lists are maintained separately in this template and have drifted before, so the assertion is per-branch rather than shared.

Verification

  • helm lint ./client — clean
  • helm unittest ./client469 passed, 31 suites (467 + the 2 new)
  • scripts/chart-version-guard.shclient chart content changed and client/Chart.yaml 'version:' was bumped. ✓

Note on the version bump

Bumped 1.9.45 → 1.9.46. #749 (docs, also open) bumps to the same version — whichever merges second needs a trivial bump to 1.9.47.

🤖 Generated with Claude Code


Note

Medium Risk
Adds namespace-scoped RBAC and a load-bearing env contract for heartbeat inventory; misconfiguration could cause 403s or silent null resource-monitor reporting, but grants are read-only and gated on layout flags.

Overview
Enables jobs-manager to include resource-monitor in the edge heartbeat version inventory (companion to client-runtime changes). The main jobs-manager ClusterRole/Role now grants read-only access to daemonsets as well as deployments.

Because resource-monitor is a DaemonSet in nodeAgents.namespace, split-namespace installs (clusterScope: false) get an extra Role/RoleBinding in that namespace (via tracebloc.rbacNodeAgentsName), mirroring image-refresh RBAC; the pair is skipped when the node-agents namespace is the release namespace to avoid name collisions.

The jobs-manager api container receives NODE_AGENTS_NAMESPACE (the configured namespace when resource monitor is on, empty when resourceMonitor: false so the runtime does not list a disabled workload). Chart version 1.9.47; helm unit tests cover RBAC branches and the env wiring.

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

…n inventory
client-runtime#152 adds a version inventory to the edge-device heartbeat,
assembled from the configured images on the chart's own workloads. The
jobs-manager Role already grants read on `deployments`, which covers
jobs-manager, pods-monitor, requests-proxy, mysql-client and egress-proxy.
resource-monitor is a DaemonSet — the one workload that is not a Deployment —
and `daemonsets` was granted nowhere in this Role.
Without it the list 403s. The runtime degrades rather than failing (it lists
the two kinds separately for exactly this reason), so the symptom is not an
error anyone would chase: resource-monitor is simply reported as null forever,
which reads as "not deployed".
Read-only, and namespace-scoped in the Role branch. Granted in BOTH branches —
an edge on clusterScope: false needs it just as much — and asserted in both by
the helm unit tests, since the two rule lists are maintained separately and
have drifted before.
Refs tracebloc/client-runtime#152
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 686537f. Configure here.

Comment threadclient/templates/rbac.yaml
saadqbaland others added 3 commits August 18, 2026 18:54
…pace
Review finding on PR #750. The `daemonsets` grant I added to the
release-namespace Role does not reach resource-monitor: the chart puts that
DaemonSet in `nodeAgents.namespace.name` (default tracebloc-node-agents),
split out deliberately so it can run under PSA `privileged` without widening
the release namespace. It is the ONE chart workload outside the release
namespace — every other one the inventory reads is a Deployment there.
So on a `clusterScope: false` edge (including OpenShift) the list would still
have failed, and the heartbeat inventory would still report resource-monitor
as null. The ClusterRole branch already covers it cluster-wide; this adds the
second Role/RoleBinding pair in the node-agents namespace for the namespaced
branch — the same split image-refresh already uses for this exact boundary.
Also wires NODE_AGENTS_NAMESPACE into the jobs-manager `api` container:
without it the runtime searches the wrong namespace and RBAC alone would not
have helped. Only `api` gets it — pods-monitor does not read it.
The pair renders only when there is something to read: resourceMonitor
enabled, and the node-agents namespace actually distinct from the release
namespace (where the existing Role already grants it, and where a second pair
would collide on name). The namespace lookup is `dig`-guarded so a
`--reuse-values` upgrade from a release predating `nodeAgents` cannot
nil-pointer this always-rendered template.
helm unittest: 472 passed (was 469). helm lint clean.
Refs tracebloc/client-runtime#152
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding on client-runtime#348. `resourceMonitor: false` is a documented
value, and it gates BOTH the node-agents namespace and the new Role/RoleBinding
pair — but this env was set unconditionally. jobs-manager would then list a
namespace that does not exist, with no Role authorizing it: a 403 on every
60s heartbeat, forever, about a workload the operator deliberately turned off.
The empty value is load-bearing rather than a no-op. The runtime distinguishes
UNSET ("chart predates this env — use the built-in default") from
SET-BUT-EMPTY ("there is no node-agents namespace"), so emptying it is what
actually suppresses the search; omitting the env would fall back to the
default and list it anyway.
Two chart tests, one per branch. The lookup keeps the `dig` guard.
helm unittest: 474 passed (was 472). helm lint clean.
Refs tracebloc/client-runtime#152
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both PRs bumped to 1.9.46. #749 (docs) is approved and merges first, so this
one takes the next version.
ORDERING: this now REQUIRES #749 to merge first. If #750 lands first, #749
would carry 1.9.46 against a develop already on 1.9.47 — a version regression
the chart-version-guard would not catch (it only checks that a bump happened,
not that it moves forward).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
ContributorAuthor

Bumped to 1.9.47 (was 1.9.46) so this no longer collides with #749.

⚠️This creates a merge-order dependency: #749 must merge first. If #750 lands first, #749 would carry 1.9.46 against a develop already on 1.9.47 — a version regression, which chart-version-guard would not catch: it only checks that a bump happened, not that it moves forward.

#749 merged first and took 1.9.46, exactly the ordering this branch's version
bump assumed. Conflict was Chart.yaml only; resolved to 1.9.47, which is now a
forward bump over develop rather than a collision.
helm unittest 474 passed, helm lint clean on the merged tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@aptraceblocaptracebloc 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.

Many long comments, and the issue references seem unnecessary to me. Can we tighten this?

Comment threadclient/tests/rbac_test.yaml Outdated
50 lines of comment removed. Kept the load-bearing "why" in each place — the
DaemonSet living outside the release namespace, and the empty
NODE_AGENTS_NAMESPACE being deliberate rather than a no-op — and dropped the
narration around it. Issue references removed from templates and tests,
including the `(client-runtime#152)` suffixes on test names.
474 helm unit tests still pass; lint clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@saadqbal
saadqbal merged commit 8de5d64 into developAug 19, 2026
23 of 24 checks passed
@saadqbal
saadqbal deleted the feat/152-rbac-daemonsets-read branch August 19, 2026 08:35
LukasWodka added a commit that referenced this pull request Aug 19, 2026
… 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.
LukasWodka added a commit that referenced this pull request Aug 19, 2026
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
LukasWodka added a commit that referenced this pull request Aug 19, 2026
…ods (backend#2119) (#751)
* fix(chart): inject the release-scoped pull-secret name for training pods (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.
* chore(chart): bump 1.9.46 -> 1.9.47 for the pull-secret injection (backend#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.
* fix(chart): support a pre-made pull Secret, and stop env shadowing the 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.
* fix(chart): RBAC names the chart's own Secret, and 1.9.47 was already 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.
* test(chart): arm the pull-secret tests, and make a missing tool fail 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>
* test(chart): anchor the schema refusal on text every helm version prints
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>
* test(chart): helm-unittest coverage for the injected pull-secret name (#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>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor

/fr-pass

Best-effort functional review passed (triage: merged clean, non-interactive; behavioral evidence limited while e2e journey is red — backend#2206). Advancing to Ready for prod.

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.

3 participants

@saadqbal@LukasWodka@aptracebloc