feat(telemetry): the chart wires jobs-manager to the Collector's token (backend#2274)

[LukasWodka]

Closes tracebloc/backend#2274. Parent epic: tracebloc/backend#1872.

The write side landed in client-runtime#368; this is the half it was waiting for. It needs telemetryCollector.tokenSecret, which only existed once #779 merged.

⚠️ The RBAC gap isn't in the ticket

jobs-manager's cluster-wide rule in rbac.yaml grants secrets: [create, get] and not patch. So on the chart as it stood, the create would succeed and every refresh would 403.

That failure needs no code change to appear. bearertokenauth watches the projected file, so a rotated token is picked up without restarting the Collector — but only if something rewrites the Secret. Without patch, the Collector works until the first token rotation, then 401s on every export and buffers to disk until the cap. I found it reading the rule while writing the reader and recorded it on the ticket then; this closes it.

Gated on the Collector alone, deliberately unlike its sibling node-agents Roles. Those carry a second condition — that nodeAgents.namespace differs from the release namespace — because the release-namespace grant already covers them when the two coincide. This one isn't covered either way: the missing verb is missing in any namespace. The Role name is release-scoped, so it can't collide when the namespaces do coincide.

Two rules, because resourceNames is not honoured for create — the API server can't match a name that doesn't exist yet. So create is namespace-scoped and get/patch are pinned to the one Secret, which is the half that would otherwise let jobs-manager rewrite anything in the namespace. Same shape and reason as image-refresh-rbac.yaml's collection-verb split.

The coordinates go to one container, and that's a real trap

pods-monitor-container has a byte-identical env: opening and doesn't run jobs_manager.py. My first patch asserted a unique match, found two, and refused — which is the only reason it didn't land on the wrong container, where it would have set three variables on a process that never reads them while the Collector still got no token. Both containers are asserted.

jobs-manager holds no defaults for the name, namespace or key: the chart declares them and passes them here, so the two sides can't disagree.

Four documents, one credential — so it gets a guard

The writer (jobs-manager's env), the reader (the Collector's volume), the key (bearertokenauth.filename), and the Role + RoleBinding. Any one disagreeing produces the same symptom: nothing. The Collector mounts the Secret optional: true on purpose — a missing token must buffer, not CrashLoopBackOff on every customer node — so a wrong name, a wrong namespace, a wrong key, and a Role bound to the wrong ServiceAccount are all indistinguishable from "not deployed yet".

scripts/tests/telemetry-token-agreement.sh compares all four out of one render and writes none of them down. It derives the key the way the Collector actually resolves it — basename(bearertokenauth.filename), since the volume has no items and Secret keys project as files named by key — and checks the mount directory matches, so a filename pointing at a path nothing projects to is caught too.

Mutation	Result
Role resourceNames renamed	KILLED
writer env NAME changed	KILLED
writer env KEY changed	KILLED
writer env NAMESPACE changed	KILLED
RoleBinding bound to the wrong ServiceAccount	KILLED
the name-scoped rule dropped	KILLED
the Role template deleted	KILLED (fails closed)
env wired to the wrong container	1 unit test failed
the enabled gate removed	2 unit tests failed

Still not done when this merges

Per the ticket, done is a record arriving at /tracebloc/edge/telemetry from a real edge — which now needs someone to flip telemetryCollector.enabled=true on a fleet and confirm. A Secret that exists proves nothing, because the Collector's absent-token behaviour is to buffer quietly.

Test plan

• helm unittest . — 537 passed
• make drift — 12/12 guards green (incl. the new one)
• make check — parse, shellcheck -S warning, helm-lint, helm-vocab
• Mutation matrix above; every anchor asserted applied
• Renders inert with the Collector off (no RBAC, no env) and survives telemetryCollector: null
• Enable on a fleet and confirm a record arrives

🤖 Generated with Claude Code

---

Note

Medium Risk
Adds a new Role that can create secrets in the node-agents namespace and patch one named token Secret, plus env that points jobs-manager at that credential. Gated on telemetryCollector.enabled and name-scoped for get/patch, but it is still RBAC and secret-handling.

Overview
When telemetryCollector.enabled is true, jobs-manager now receives TELEMETRY_TOKEN_SECRET_{NAMESPACE,NAME,KEY} so it can write the Collector's ingest token. The vars go only on the api container (not pods-monitor). They are omitted when the Collector is off.

A new Role/RoleBinding in the node-agents namespace fills the gap that cluster-wide rbac.yaml grants secrets: [create, get] but not patch. Without that, create would succeed and every token refresh would 403, leaving the Collector 401ing after the first rotation. create is namespace-scoped; get/patch are pinned to the one Secret.

A fail-closed drift guard (telemetry-token-agreement.sh) checks that writer env, Collector volume, bearertokenauth key, and RoleBinding all name the same Secret. Chart version is 1.9.59.

^{Reviewed by Cursor Bugbot for commit 654aa87. 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 654aa87. Configure here.}

[aptracebloc]

👋 @LukasWodka — Arturo (@aptracebloc) is signing off for the day, so he won't pick this up in the next review passes. @saadqbal is already a co-reviewer here and can carry it; if you'd rather not wait on Arturo's eyes specifically, feel free to route it to another available reviewer so it keeps moving. Arturo will re-engage when he's back.

— drafted with Claude Code

[saadqbal]

The RBAC gap is the reason to read this PR, and finding it by reading the rule while writing the reader — rather than by watching a customer edge stop exporting after the first token rotation — is the good version of this. Confirmed it's real and confirmed it needed no code change to appear.

Four things I checked rather than took on trust:

1. The missing verb — real, and in two rules, not one.rbac.yaml:68 (the ClusterRole) and rbac.yaml:151 (the namespaced Role used when clusterScope: false) both grant secrets: ["create", "get"] with no patch. The description says "jobs-manager's cluster-wide rule" in the singular; there are two, and both are affected. It doesn't change the fix — a Role in the node-agents namespace grants patch there regardless of which of those two a given install renders — but the clusterScope: false path would have hit exactly the same 403, so the fix is broader than described rather than narrower.

2. patch is the verb the writer actually uses. This was my main worry: patch and update are separate RBAC verbs, and a writer calling replace_namespaced_secret (PUT → update) would 403 identically with the grant as written, reproducing the exact symptom this PR fixes. It doesn't — jobs_manager.py::sync_telemetry_token_secret is create-then-patch-on-409 and its docstring records why: "patch rather than replace so the object's metadata survives". Both sides chose patch deliberately and each says so. That's the two-sided agreement this PR is claiming, and it holds.

3. The cross-namespace binding is right. Role and RoleBinding in nodeAgents.namespace.name, subject the jobs-manager ServiceAccount in .Release.Namespace. A RoleBinding grants within its own namespace to a subject from anywhere, so this is the correct shape and the comment naming which object lives where is worth having.

4. The gate looks weaker than every sibling's, and provably isn't. This was the one I expected to be a finding: auto-upgrade-rbac.yaml, docker-registry-secret.yaml, image-refresh-rbac.yaml and node-agents-namespace.yaml all gate on include "tracebloc.nodeAgentsInUse" . before creating anything in that namespace, and this template gates only on $tcTok.enabled — which would put a Role in a namespace nothing created. But the helper is

{{- if or (ne .Values.resourceMonitor false) $tc.enabled }}true{{ end -}}

so telemetryCollector.enabledimpliesnodeAgentsInUse, and the extra condition would be dead. telemetry-collector-daemonset.yaml gates identically ({{- if $tc.enabled }}, namespace at :70), so this is consistent with its own consumer rather than looser than it. Worth one line in the comment saying that, since the omission reads as an oversight against four siblings that all do it the other way.

On the parts I'd normally push back on and won't

The create rule really is namespace-wide — jobs-manager can now create any Secret in the node-agents namespace. That's a genuine widening, and it's also the minimum Kubernetes permits: resourceNames is not honoured for create because the API server cannot match a name that does not exist yet. The half that could be pinned is pinned, which is the half that matters — without the split, patch on the whole collection would let jobs-manager rewrite any Secret in that namespace, including ones it doesn't own. Right call, and the same shape as image-refresh-rbac.yaml.

nodeAgents.namespace.name unguarded — no template in the chart guards it with default (dict), so this matches an established convention rather than departing from one. Not flagging it; adding a guard here alone would be the inconsistent choice.

The two-container trap is the finding I'd have missed.pods-monitor-container having a byte-identical env: opening while not running jobs_manager.py is exactly the kind of thing that lands silently — three variables set on a process that never reads them, the Collector still tokenless, and every symptom identical to "not deployed yet". That the first patch asserted a unique match and refused is why it didn't land there, and asserting both containers is the right permanent answer.

The four-document guard is the right response to "any one disagreeing produces the same symptom: nothing", and deriving the key as basename(bearertokenauth.filename) — the way the Collector actually resolves it, since the volume has no items — is what makes it a check rather than a restatement. The mutation table covers the renames in both directions and the fail-closed case.

Approving.

[saadqbal]

Coordination note, unrelated to the review above: this and #765 both bump 1.9.58 → 1.9.59. Whichever lands second will conflict on Chart.yaml and need a re-bump to 1.9.60 — both are approved now, so it's worth knowing before one of you hits it rather than after. #765 has been open longer, so it's the likelier first.

[LukasWodka]

Merged. Both of your non-blocking asks are in #787 rather than pushed here — a comment fix onto an approved PR would have tripped the stale-review dismissal and re-rolled your review for documentation.

Your correction stands and I verified it directly:rbac.yaml:62 (ClusterRole, clusterScope: true) and rbac.yaml:145 (namespaced Role, clusterScope: false) both read verbs: ["create", "get"]. My description said "cluster-wide rule" in the singular, and you're right that the clusterScope: false path would have hit the identical 403 — the comment is now explicit about both, and about why the fix is the same either way.

The gate rationale is in there too, in the form you asked for: one line saying telemetryCollector.enabled implies nodeAgentsInUse so the extra condition would be dead, and that this matches its own consumer's gate rather than being looser than its four siblings.

On the create widening — agreed it's a genuine widening and worth stating plainly rather than burying, which is why the split is in the template comment. It is the minimum Kubernetes permits, and the half that can be pinned is pinned.

One thing I'd flag back: gen-manifest.sh --check passes on #787 because chart templates aren't in the hash manifest — but I measured the render rather than trusting that, after getting "comment-only" wrong on #775. Same tree twice differs by 2 lines (POD_TOKEN_SIGNING_SECRET, generated per render); before/after with the version pinned equal differs by 0 beyond that. So the checksum/config churn in the raw diff is downstream of the mandatory version bump, not the comment.

[LukasWodka]

/fr-pass