docs(telemetry): the missing verb is in BOTH secrets rules, not one (backend#2274)

[LukasWodka]

Follow-up to #784 — @saadqbal's two non-blocking asks from that review. Taken as a separate PR rather than a push onto #784: branch protection dismisses stale reviews on new commits, so a comment fix would have re-rolled an approved, green PR.

1. "The cluster-wide rule" was singular, and wrong

Verified before writing it down: both of jobs-manager's secrets rules lack patch.

rbac.yaml:62 ClusterRole (clusterScope: true) verbs: ["create", "get"]
rbac.yaml:145 namespaced Role (clusterScope: false) verbs: ["create", "get"]

The fix doesn't change — a Role in the node-agents namespace grants patch there whichever branch renders — but the old wording would have sent someone reading the clusterScope: false path looking for a difference that isn't there. That's the specific cost of a comment that's nearly right, and it's the reason this is worth a PR rather than a shrug.

2. The gate's omission read as an oversight, so it now says why it isn't

Four siblings — docker-registry-secret.yaml, image-refresh-rbac.yaml, node-agents-namespace.yaml, secrets.yaml — gate on tracebloc.nodeAgentsInUse, and this template doesn't. Asad checked and it's fine: the helper is or (ne .Values.resourceMonitor false) $tc.enabled, so telemetryCollector.enabledimplies it and the extra condition would be dead. This gates exactly as its own consumer does (telemetry-collector-daemonset.yaml is {{- if $tc.enabled }}) rather than more loosely than its siblings.

Four templates doing it one way and a fifth doing it another is worth one line, whichever way it resolves.

Comment-only — proven, not asserted

I claimed "comment-only, no behaviour change" on #775 and was wrong, because telemetry.sh is hash-pinned and install.sh aborts before the privileged install on a manifest mismatch. Chart templates aren't in the manifest (gen-manifest.sh --check is clean and unchanged), but the render is what actually matters, so I measured it:

• rendering the same tree twice differs by 2 lines — POD_TOKEN_SIGNING_SECRET, generated per render;
• before/after with the version pinned equal differs by 0 lines beyond that.

So the version labels and the two checksum/config annotations visible in the raw diff are downstream of the mandatory Chart.yaml bump (the checksummed ConfigMap carries the chart-version label), and nothing is attributable to the comment.

Test plan

• helm unittest . — 537 passed
• make drift — 12/12 guards green
• gen-manifest.sh --check — unchanged (chart templates aren't hash-pinned)
• Render diff isolated with the version pinned equal — 0 attributable lines
• Both rbac.yaml rules read directly to confirm the claim before writing it

🤖 Generated with Claude Code

---

Note

Low Risk
CI/test and comment changes only; chart templates are unchanged aside from a version bump. No runtime RBAC or install-path behavior is modified.

Overview
Adds a required make drift guard (node-agents-namespace-safety.sh) that renders all four resource-monitor × Collector combinations and fails if anything lands in a non-release namespace the chart does not also create. The check is property-based (from the render), scoped to namespace.create: true, and fails closed if no combo populates that namespace.

Pins helm template --namespace to a synthetic release NS in both this guard and node-agents-tenancy.sh, so kubeconfig context can no longer make CI treat the release namespace as the node-agents one.

Clarifies the telemetry-token-rbac.yaml comment: jobs-manager lacks patch on secrets in both ClusterRole and namespaced Role, and gating on Collector enablement (not nodeAgentsInUse) is intentional. Chart version 1.9.59 → 1.9.60.

^{Reviewed by Cursor Bugbot for commit 056446d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

bugbot run

[cursor]

✅ 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 47775e6. Configure here.}

[saadqbal]

The methodology here is the best part, and I want to credit it before the finding. Proving "comment-only" by render diff — rendering the same tree twice to isolate POD_TOKEN_SIGNING_SECRET as per-render noise, then diffing before/after with the version pinned equal to get 0 attributable lines — is a real proof rather than an assertion, and it correctly identifies the version labels and checksum/config annotations as downstream of the mandatory Chart.yaml bump. Doing that because the claim was wrong on #775 is the right response to having been wrong once.

Taking it as a separate PR rather than pushing onto #784 is also right, and for the reason given: a comment fix would have dismissed a green approval and re-rolled the review.

Both claims I asked about are verified correct.rbac.yaml really does grant secrets: ["create", "get"] with no patch in both branches, and the nodeAgentsInUse reasoning is exactly right — the helper is or (ne .Values.resourceMonitor false) $tc.enabled, so telemetryCollector.enabled implies it and the extra condition would be dead.

Requesting changes on one line, and it isn't the usual comment nitpick

Normally I'd flag this non-blocking. Here I won't, because the comment is the entire deliverable — the PR exists to fix a comment that was nearly right, and its own replacement enumerates the wrong set. Same class of error, one round later.

four siblings — docker-registry-secret.yaml, image-refresh-rbac.yaml, node-agents-namespace.yaml and secrets.yaml — all do gate on it

git grep -l nodeAgentsInUse origin/develop -- client/templates returns five files, one of which is _helpers.tpl (which defines it). The four consumers are:

auto-upgrade-rbac.yaml <- MISSING from the list (gates at :173)
docker-registry-secret.yaml ✓
image-refresh-rbac.yaml ✓
node-agents-namespace.yaml ✓

secrets.yaml does not gate on it at all — it contains no nodeAgents reference of any kind; its ifs are placeholder validation, podTokenSigningSecret and perExperimentDbCreds.

So the list has one wrong entry and one omission, and "four" is right only by coincidence — one in, one out. The omitted file is auto-upgrade-rbac.yaml, which is the one I named explicitly in the #784 review this PR is answering, so it's also the easiest to check.

The cost is the same one this PR articulates so well: someone opening secrets.yaml to see how it gates will find nothing and be left unsure whether the pattern or the comment is wrong. Two-word fix.

And the deeper version, since this is the third hand-maintained list I've seen bite this week

The list is derivable with one grep, and it's the unlisted_namers() shape from .github#295 — where a derived check found two files nobody had listed on its first run. This repo already has the pattern in scripts/tests/telemetry-token-agreement.sh and telemetry-vocabulary-agreement.sh: compare a set out of one render and write none of it down.

The cheap version is to stop enumerating and say "every other consumer of tracebloc.nodeAgentsInUse gates on it; this one doesn't, because …" — which can't go stale and loses nothing a reader needs. The durable version is a guard asserting the enumerated set equals grep -l nodeAgentsInUse client/templates minus _helpers.tpl and minus this file. Your call which; the first is free.

Minor, while you're in there

rbac.yaml:62 and :145 are the resources: lines — the verbs: lines they refer to are :68 and :151. Defensible as "the rule starting at :62", but the claim is specifically about verbs, and cross-file line numbers in a comment drift the first time rbac.yaml gains a rule. Anchoring on the rule's distinguishing text (clusterScope branch + resources: ["configmaps", "secrets"]) would survive that; the line numbers are the part that won't.

Unrelated heads-up

develop is now at 1.9.59 and this takes 1.9.60, so #765 — still open, still bumping 1.9.58 → 1.9.59 — is now conflicting for certain and needs 1.9.61. Flagging here since it's the same author-adjacent queue; I'll note it there too.

[LukasWodka]

Fixed in 169d5b2 — and you're right that it isn't the usual comment nitpick. This PR exists to fix a comment that was nearly right, and its replacement enumerated the wrong set. Same class, one round later, in the one file where the comment is the deliverable.

Verified before fixing:

git grep -l nodeAgentsInUse origin/develop -- client/templates
_helpers.tpl <- defines it
auto-upgrade-rbac.yaml <- omitted from my list
docker-registry-secret.yaml
image-refresh-rbac.yaml
node-agents-namespace.yaml
<- secrets.yaml absent; I had listed it

"Four" was right only because the two errors cancelled. That's a worse failure than being off by one, because the count looks checkable and isn't.

One correction back, since precision is this PR's subject. You wrote that secrets.yaml "contains no nodeAgents reference of any kind" — it has two, the gate at :112 and the namespace at :121. Your conclusion is exactly right: it doesn't gate on the helper, it gates on resourceMonitor, which is the deliberate #779 decision not to mirror CLIENT_ID/CLIENT_PASSWORD into that namespace for a workload that never reads them. The supporting detail is the only part that's off.

Took the free option and the durable one

The comment no longer enumerates anything — it now says "every other consumer of the helper gates on it; this one doesn't, because …". Reproducible with one grep, can't rot, loses nothing.

And the property is guarded, because your deeper point is the right one — three hand-maintained lists have gone stale in this area in a week, and "derive, never restate" is rule 1. scripts/tests/node-agents-namespace-safety.sh asserts the outcome those five gates exist to produce: across all four tenant combinations, if any rendered resource declares the node-agents namespace, the Namespace must render too. That's #779's original finding stated as an invariant, so a new template that forgets its gate fails without anyone having to notice it was added.

 resourceMonitor=true tc=true ns=created resources=14
resourceMonitor=true tc=false ns=created resources=9
resourceMonitor=false tc=true ns=created resources=9
resourceMonitor=false tc=false nothing outside the release namespace

It deliberately does not check the gate expression: nodeAgentsInUse and a bare telemetryCollector.enabled are both correct (the second implies the first), so asserting one spelling would flag correct code. Asserting the outcome can't. It holds no list of templates, gates or helpers, reads even the namespace name out of the render, is scoped to namespace.create: true (a pre-created namespace is legitimately out of scope — stated rather than mis-asserted), and fails closed if no combination populates the namespace, since an inert chart would otherwise satisfy the implication vacuously.

Mutations, anchors asserted applied: the token Role loses its gate → KILLED, naming the Role and the absent namespace; the namespace stops being created → KILLED. Both reproduce the #779 class from opposite directions.

On the line numbers — you're right, :62/:145 are the resources: lines and the verbs are at :68/:151. Now anchored on the branch and the rule's distinguishing text instead, since cross-file line numbers drift the first time rbac.yaml gains a rule.

On #765

Thanks — confirmed it's @saqlainsyed007's and still on 1.9.58 → 1.9.59, so it'll need 1.9.61 once this lands. Flagging only; I don't touch teammates' PRs.

537/537 chart tests, 13/13 drift guards, make check green.

[LukasWodka]

bugbot run

[LukasWodka]

CI was red on my own new guard, fixed in 5a34fc8 — and the failure is worth recording because the guard I added to stop hardcoded values rotting was itself hardcoding one.

helm template with no --namespace takes the release namespace from the caller's kubeconfig context. On this laptop that's tracebloc; on a runner with no kubeconfig it's default. My comparator found the node-agents namespace by excluding the literal "tracebloc", so in CI it decided the release namespace was the node-agents one and reported all 35 release-namespace resources as orphaned.

A guard whose verdict depends on the developer's kube context is worse than no guard: green where it's written, red where it runs. Both helm invocations now pin --namespace, and the comparator receives that value instead of assuming one. The pinned value is deliberately not any real namespace, so a literal creeping back cannot silently match.

Same literal was in node-agents-tenancy.sh, which passed CI only because the chart happens to put no DaemonSet in the release namespace — luck, not design. It would have started lying the first time one appeared, and silently: its job is deciding which namespace is the node-agents one, so a wrong answer there compares the wrong two sets rather than erroring. Fixed too.

Reproduced before fixing, and the fix proven against the same condition — the step I skipped and which would have caught this before I pushed:

KUBECONFIG=/nonexistent old code -> [ERROR] ... render into 'default' ...
KUBECONFIG=/nonexistent new code -> green, all four combinations

make drift is green under that environment too, so the whole tier is now independent of ambient kube state rather than just this guard.

537/537 chart tests, 13/13 drift guards.

[LukasWodka]

bugbot run

[saadqbal]

My finding is closed, and closed the better way. The comment no longer enumerates:

NOT ENUMERATING THOSE CONSUMERS, deliberately. … "Four" was right only by coincidence, one in and one out — the same nearly-right-comment failure this file was edited to fix, one round later. A list a reader can reproduce with git grep -l nodeAgentsInUse client/templates does not need to be written down, and written down it can only rot.

That's the free option I hoped for rather than the version that needs maintaining, and recording how the previous list failed is what stops the next person re-adding it.

Then you went further and built the guard. The design is right and I want to be specific about which parts, because the fix below is small and I don't want it read as "scrap this":

• it asserts the property, not the gates — "if any rendered resource declares the node-agents namespace, the Namespace object must render too" is the invariant; gates are an implementation of it
• it deliberately does not check the gate expression, because nodeAgentsInUse and a bare telemetryCollector.enabled are both correct and asserting one spelling would flag correct code. That's the distinction that makes it a property check
• it fails closed on vacuity — populated == 0 is an error, so an inert chart can't satisfy the implication by having nothing to check
• it scopes out namespace.create: false with the reason stated rather than silently mis-asserting it

The bug: it hardcodes the release namespace, and the render doesn't use it

ifnsandns!="tracebloc":
target=nsbreak

out="$(helm template t "$CHART""${BASE[@]}" …)"# no --namespace

Helm's release namespace with no --namespace is default, not tracebloc. So the exclusion never matches anything, target becomes the first namespace in the render — the release namespace — and the guard then demands a Namespace/default object that no chart should ever create. Hence 35 flagged resources, all of them core chart objects.

And the remediation it prints is the part that worries me more than the red build:

Gate them on tracebloc.nodeAgentsInUse (or on a condition that implies it).

Applied as written to Deployment/t-jobs-manager, Secret/t-secrets, Service/jobs-manager and PersistentVolumeClaim/client-pvc, that makes the entire chart conditional on node agents being in use. A false positive that arrives with confident, specific, harmful advice is worse than one that just fails — someone in a hurry could follow it.

There's an irony worth keeping, because it's the same lesson one level up. The header says:

The namespace NAME comes out of the render, not from this file: it is chart-configurable, and hardcoding it here would be the very thing this guard exists to avoid.

It derived the namespace it was thinking about and restated the one it wasn't.

The fix

Make the release namespace explicit and pass the same value to both sides, so they cannot disagree:

REL_NS=tracebloc
out="$(helm template t "$CHART" --namespace "$REL_NS""${BASE[@]}" \ --set "resourceMonitor=$rm_val" --set "telemetryCollector.enabled=$tc_val"2>/dev/null)"|| …
printed="$(printf '%s'"$out"| python3 "$CMP""$label""$REL_NS")"

label, release_ns=sys.argv[1], sys.argv[2]
targets= {d.get("metadata", {}).get("namespace") fordindocs} - {None, release_ns}

then assert the Namespace-exists property for each member of targets. Two reasons for the set rather than a patch to the existing loop: break on the first match makes document order decide which namespace gets checked, and a set is honest about the possibility of more than one — today there's exactly one, and if a third ever appears you want it checked rather than shadowed.

One thing to add while you're in there

The guard was built for a specific past failure — #779's DaemonSet targeting an uncreated namespace — so that failure is its natural mutation test. Worth confirming it actually reddens: render resourceMonitor=false tc=true with the Collector DaemonSet's gate removed, and the guard must fail. Right now I can't tell from a red run whether it would catch the real thing, because it's failing on everything.

Changes requested still stands — CI is red on this and the fix is in this PR's own new file. Everything from my previous round is closed.

[cursor]

✅ 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 5a34fc8. Configure here.}

[LukasWodka]

All three in 056446d. You diagnosed the release-namespace bug independently and identically before 5a34fc8 landed — the three things you added on top are what this commit is.

1. A set, not break on the first match. You're right that document order was deciding which namespace got checked, and that a set is honest about there being possibly more than one. Every non-release namespace is now checked independently.

2. The message no longer prescribes a fix — this is the part of your review that mattered most. It said "gate them on tracebloc.nodeAgentsInUse", which while the guard was mis-firing on the release namespace advised making the entire chart conditional on node agents being in use. Applied as written to Deployment/t-jobs-manager or PersistentVolumeClaim/client-pvc it would have been actively destructive. I'd have called a false positive a nuisance; you're right that one arriving with confident, specific, harmful advice is worse than one that merely fails, because someone in a hurry can act on it. It now states the violated property and names the namespace, and leaves the gate to the reader — there's more than one correct spelling anyway.

3. Your mutation, which is the one that mattered. You were right that a run failing on everything can't tell you whether it catches the real thing. Removing the Collector DaemonSet's own gate:

[ERROR] resourceMonitor=false tc=false: 1 resource(s) render into
'tracebloc-node-agents' but the chart does not create it:
['DaemonSet/t-telemetry-collector']

That's #779's original finding, reproduced by the guard built for it.

And one the guard caught on itself

Reformatting the summary line for (1) broke the vacuity check, which grepped resources=N out of that formatted string — so it reported "no combination put anything in the namespace" on a perfectly healthy chart. A check coupled to a display string: the same class as everything else this file has found, and it failed closed while broken, which is the design working. The two halves now agree on a POPULATED= marker emitted on every exit path including the early one, so "checked nothing" is distinguishable from "died before printing".

On the irony you flagged

It derived the namespace it was thinking about and restated the one it wasn't.

That's the sharpest summary of the bug and I've kept the substance of it in the file, because the next person writing a render-comparison guard will reach for exactly that shortcut.

Mutations, anchors asserted applied, all under KUBECONFIG=/nonexistent: Collector DaemonSet gate removed → KILLED (#779's original); token Role gate removed → KILLED; namespace not created → KILLED.

537/537 chart tests, 13/13 drift guards, make check green — all with no kubeconfig, so none of it depends on ambient kube state any more.

[LukasWodka]

bugbot run

[cursor]

✅ 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 056446d. Configure here.}

[saadqbal]

Fixed, and the fix is better than the diagnosis I gave you on three counts. Green, no open threads, approving.

Your root cause is sharper than mine, and I was slightly wrong. I said "Helm's release namespace with no --namespace is default". It isn't, in general — it's the namespace from the caller's kubeconfig context, falling back to default only when there's no kubeconfig. That's the whole explanation for the thing my version couldn't account for: why the author wrote != "tracebloc" at all, and why make drift was genuinely green locally and red on the first CI run. The generalisation is the part worth keeping:

A guard whose verdict depends on the developer's kube context is worse than no guard: it is green where it is written and red where it runs.

RELEASE_NS="ship-guard-release-ns" is a hardening I didn't think to ask for — pinning to a value that is deliberately not any real namespace means a literal creeping back into the comparator can't silently match and appear to work. That closes the failure mode rather than the instance.

You found a bug I read past. The old vacuity check was case "$printed" in *"resources="[1-9]*) — scraping a count out of the formatted human summary, so reformatting the display line would have silently disarmed the non-vacuity guard. I looked at that line and accepted it. The POPULATED= marker as an explicit contract between the two halves is right, and so is emitting it on the early-exit path so the caller can distinguish "checked nothing" from "the comparator died before printing". Your note that it's "the same class as everything else this file has caught" is the correct read — a check coupled to a display string.

And the same latent bug was in a sibling guard.node-agents-tenancy.sh carried the identical != "tracebloc" test, and the label on it is the honest one:

This guard passed in CI only because the chart happens to put no DaemonSet in the release namespace — luck, not design.

That's the follow-through that matters more than the original fix: the bug class got hunted in the neighbourhood rather than patched where it was reported.

Both of my structural asks are in — the set difference with the document-order and third-namespace reasons recorded, and the prescriptive remediation replaced by a statement of the property plus the namespace, leaving the gate spelling to the reader. That last one is now consistent with the guard's own founding principle: it declines to assert a gate spelling in the check, so it shouldn't prescribe one in the error either.

One observation, not a request. Pinning the release namespace to an unreal value means the guard never renders the coincide configuration — nodeAgents.namespace.name == .Release.Namespace, where resources legitimately land in the release namespace and no Namespace object is needed. The targets set handles that correctly by construction (empty set → "nothing outside the release namespace" → POPULATED=0), and the default differ case supplies the populated combination the vacuity check needs, so nothing is broken. It's just worth knowing that the branch the sibling Roles carry their second condition for is unexercised here. Fine as scoped — the guard's subject is the differ case — and not worth widening unless that branch ever grows logic of its own.

Where this ended up: a comment correction became a comment that can't rot, then a derived guard, then a fix to a pre-existing guard with the same latent flaw. Worth the three rounds.

[LukasWodka]

/fr-pass