Uh oh!
There was an error while loading. Please reload this page.
feat(telemetry): the edge Collector, Class A only, shipping inert (backend#1906) - #779
Conversation
…ckend#1906) RFC-BACKEND-1872 D6/D7. An OpenTelemetry Collector DaemonSet that reads Class A container stdout with `filelog` and forwards to the backend's ingest endpoint. SHIPS DISABLED, and that is the load-bearing decision rather than caution. The exporter authenticates with a token this chart does not create: jobs-manager writes it into a Secret and does not do so yet. Enabling it first would put a DaemonSet on every customer node spooling to disk with nothing it can deliver -- filling toward the 1 GiB cap per node for no benefit, which is exactly the "telemetry must never be the reason a node fills" risk D7 exists to bound. Same posture as `egressProxy.routeWorkloads`. A DAEMONSET, because the kubelet writes container stdout per NODE. A Deployment would see only the node it landed on and report healthy while collecting a fraction of the fleet -- the shape of silence this epic exists to remove. CLASS A ONLY (D12). The include globs name the four control-plane containers this chart owns; a bare wildcard would sweep in training and ingestion pods, which are Class B and gated on backend#1908. Today's bounding is SECRET redaction, not CONTENT redaction, and raw customer cell values have already reached central telemetry once (backend#1879), so the narrow scope is the protection. THE REDACTION FLOOR IS RELOCATED, not re-invented -- D6 precondition 2. Reading stdout through `filelog` BYPASSES controller.py's app-side handler, so without it this change would silently REMOVE a protection. All six `_LOG_REDACTIONS` patterns are carried (client-runtime@45006ec), translated Python -> RE2 deliberately: `\1` becomes `$1`, `(?i)`/`(?m)` kept, and no pattern uses lookaround or in-pattern backreferences, which RE2 rejects -- checked, because a silently-invalid regex in OTTL is a processor that starts and scrubs nothing. All six were compiled as RE2 and run against sample secrets; the `SharedAccessKeyName` carve-out controller.py documents survives. THIS IS A CROSS-REPO SECOND COPY THAT CANNOT BE MACHINE-CHECKED FROM HERE, and the comment says so rather than implying coverage: the producer is Python in another repo, Helm cannot import it, and no test in this repo can detect drift. D7's bounds, all three: byte cap 1 GiB via `sizer: bytes`; `max_elapsed_time: 0` so the cap is the ONLY bound; disk-backed `file_storage` on a hostPath so the queue survives the pod restart that is the common case during an outage. `block_on_overflow: false` is drop-NEWEST per D7's 2026-08-20 amendment (rfcs#36) -- `exporterhelper` sheds at the entrance and has no evict-oldest option -- which is why the Collector's own metrics are exposed: the drop count is the only signal separating a quiet edge from a shedding one. `scheme: Token`, NOT the extension's default `Bearer`. The edge holds a DRF user token and the endpoint accepts it via `TokenAuthentication` under `IsAuthenticatedEdge`; `Bearer` is the other credential type and 401s silently. The token is read through `bearertokenauth`'s `filename`, which watches the projected file (credentialsfile.ValueResolver + WithOnChange) -- verified in the extension's source, because the README does not mention reloading and #1906's "a projected update needs no restart" rests on it. `logs_endpoint`, not `endpoint`: otlphttp appends /v1/logs to the latter and the ingest boundary is a versioned path of ours. One defect caught by rendering rather than reading: Helm parses 1073741824 as a float64 and emitted `queue_size: 1.073741824e+09`, a YAML float where the Collector wants an integer -- the cap would not have been the number in values.yaml. `| int64` fixes it and a test asserts no scientific notation. Verified: helm lint clean; the WHOLE chart suite 510/510 across 34 suites; the new suite 19/19; `chart-version-guard.bats` 23/23 after bumping version and appVersion together to 1.9.51 (the guard requires a bump when chart content changes). 15 mutations, all killed -- enabling by default, widening filelog to a wildcard, dropping redaction from the pipeline, losing a redaction pattern, losing `int64`, reverting `sizer`, adding a second time bound, falling back to memory, reverting to `Bearer`, using `endpoint`, mounting host logs writable, making the Secret required, moving the queue to an emptyDir, removing the config checksum, and granting a ClusterRole. NOT in scope: retiring the two App Insights entries from the squid ACL. That cuts client-runtime's live AzureLogHandler path and belongs to #1910's cutover, not here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Caught by the Fleet auto-upgrade E2E, and it was far worse than the feature not
working: it broke the UPGRADE, on every existing customer.
Error: UPGRADE FAILED: template: telemetry-collector-rbac.yaml:1:14:
<.Values.telemetryCollector.enabled>: nil pointer evaluating interface {}.enabled
`helm upgrade --reuse-values` replays the values STORED WITH THE PREVIOUS RELEASE
and does not merge values.yaml defaults. Every release made before this block
existed therefore has no `telemetryCollector` key at all, so the bare gate is a
nil-pointer dereference and the whole release fails.
THE CHART ALREADY DOCUMENTS THIS TRAP AND I DID NOT FOLLOW IT.
resource-monitor-daemonset.yaml says of `images.resourceMonitor`: "older releases
that upgrade via `helm upgrade --reuse-values` won't have the block in their
stored values, so read it defensively (nested `default dict` tolerates a missing
`images` map AND a missing `resourceMonitor` entry). `dig` is not usable here --
it rejects chartutil.Values." Same idiom now applies here.
Every read goes through `{{- $tc := default (dict) .Values.telemetryCollector -}}`
with nested maps defaulted the same way, and each defaulted value repeats the
values.yaml default so a partial stored map cannot render a half-configured
Collector. An old release upgrading this way gets NO Collector, which is right
twice over: it matches `enabled: false`, and a feature must never arrive on a
cluster via a values map the operator never saw.
The regression test sets `telemetryCollector: null` -- ABSENT, not `false`, because
absent is the case that broke and false is a key that exists (already covered).
Mutation-proved: reverting the gate reproduces the E2E's exact error and the test
errors rather than passing.
Chart bumped to 1.9.52 (the version guard requires it on chart content change).
Whole chart suite 511/511 across 34 suites; the new suite 20/20.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…t could not start Two Bugbot Highs on #779, both correct, and both the same class: a Collector that starts, reports healthy, and ships nothing. 1. EVERY FILELOG GLOB MATCHED NOTHING. `classAContainers` listed `tracebloc-jobs-manager`, `egress-proxy` and `requests-proxy` -- WORKLOAD names. `filelog` matches the kubelet's on-disk path, which carries the CONTAINER name, and the real ones are `api` (jobs-manager), `pods-monitor-container` (Dockerfile.controller, i.e. D6's `controller.py`) and `squid`. Worse, every path was scoped to `.Release.Namespace` while resource-monitor is a DaemonSet in `nodeAgents.namespace` -- so a whole Class A component was unreachable even had its name been right. All four globs targeted nothing. `proxy` (requests-proxy) is deliberately NOT added: it runs the jobs-manager image but is not in D6's Class A list, and admitting a container because it shares an image is how a class boundary stops meaning anything. 2. THE IMAGE PREDATED ITS OWN CONFIG. Pinned 0.109.0, while the pipeline sets `block_on_overflow` (upstream 2025-03, ~0.122+) and `sizer: bytes` on a persistent queue (~0.130). The Collector REJECTS unknown configuration keys, so this is not a cap silently unapplied -- it is a container that does not start, on every node. Now 0.159.0. The mistake worth naming: I verified `sizer` and `block_on_overflow` against CURRENT upstream docs and then pinned an image from before they existed. Verifying a feature is not verifying the version that has it. WHAT KEEPS BOTH FIXED. A new derived guard, `scripts/tests/collector-class-a-agreement.sh`, wired into DRIFT_GUARDS (7 now). It holds NO list of names: it renders the chart, reads container names out of every workload, reads the globs out of the Collector's own ConfigMap, and compares. A hand-written expectation would have agreed with whichever side it was copied from -- which is exactly how the original passed review, and why the helm-unittest assertions could not catch it either (they asserted the same wrong names). It is a cross-DOCUMENT agreement, so helm-unittest cannot express it: that plugin asserts within one template at a time. Hence a shell test. It fails closed -- zero globs or zero containers is a finding, since two empty sets compare equal. Mutation-proved against BOTH findings: restoring the old names, and re-scoping node-agents to the release namespace, each reddens it with the offending pair named. The image floor is asserted as a FLOOR in the helm suite, not an exact tag -- an exact pin there would be a second place to update on every bump and would pass by agreeing with itself. shellcheck earned its place too: it caught `render | python3 - <<'PY'`, where the heredoc overrides the pipe so the comparison read an empty document set. It failed CLOSED ("found 0 Collector ConfigMaps") rather than passing, which is the design working, but SC2259 named the cause directly. Chart 1.9.53. Whole chart suite 512/512 across 34 suites; the Collector suite 21/21; `make drift` green on all 7 guards; manifest up to date. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…r, namespace gate Bugbot's second round on #779. All three verified against the rendered chart before fixing. 1. HIGH — `optional: true` DOES NOT MEAN "buffer, don't fail". It stops the kubelet failing the MOUNT; it does not make the file appear, and `bearertokenauth` needs `filename` to resolve. Whether a missing file aborts the extension's Start is version-dependent and lives in core's `credentialsfile.ValueResolver`, not the extension — I could not establish it for 0.159.0 from that tag or from `main`. So this does not BET on the answer. A `lookup`-guarded pre-flight (the same idiom as resource-monitor's metrics-server probe: empty during `helm template`, so offline rendering is unblocked) refuses the RELEASE with a message naming the missing Secret and namespace. Either upstream behaviour is then fine, because the case is unreachable — which beats reading the source correctly and depending on it. It is also the right direction for a fail-soft component: "the install said no, here is the Secret it wants" is actionable in a way that CrashLoopBackOff on 200 nodes is not. 2. MEDIUM — `global.imageRegistry` was ignored. Squid and the other third-party images use #585 precedence: global mirror, then per-image registry, then docker.io. Passing `image.registry` alone meant a mirrored or air-gapped fleet would ImagePullBackOff the Collector while everything else pulled fine. Both ends of the chain are now asserted — a precedence chain tested at one end only is half-tested. 3. MEDIUM — the Collector is a SECOND TENANT of the node-agents namespace, and the gates had not caught up. `node-agents-namespace.yaml` and the mirrored pull Secret in `docker-registry-secret.yaml` were still gated on `resourceMonitor != false`, so `resourceMonitor: false` + `telemetryCollector.enabled: true` produced a DaemonSet targeting a namespace this chart never created, with no pull Secret. Every dependent resource now shares the feature's gate. Proven both ways: with the fix, 4 resources land in that namespace; reverting the pull-Secret gate drops it to 3. FOUND WHILE FIXING, AND MINE: `telemetryCollector` was the ONLY chart-owned top-level key of 36 absent from `values.schema.json` (`global` is a Helm built-in). So the whole block was unvalidated — a typo in `classAContainers` would have been silently accepted and the Collector would have collected nothing, which is finding #1 of the first round arriving by a different route. Added, with `authScheme` as a CLOSED enum so a typo cannot pick a third value, and `type: [object, null]` so `--reuse-values` from a release predating the block still validates. Tests: 25 in the Collector suite. The three fixes are mutation-proved, including a vacuity check — widening the namespace gate to `true` also reddens, so the positive test is not passing for free. Chart 1.9.54. Whole suite 516/516 across 34 suites; `make drift` green on 7 guards; lint clean; manifest current. A process note for my own future reference: three of my debugging renders came back empty and I briefly read that as the fix not working. All three were invalid test inputs the schema was correctly rejecting — with `2>/dev/null` hiding the reason. Don't suppress stderr while diagnosing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
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 3152dda. Configure here.
aptracebloc
left a comment
There was a problem hiding this comment.
Reviewed. Adds an OpenTelemetry Collector as a per-node DaemonSet in the nodeAgents namespace, reading the four Class-A control-plane containers' stdout off /var/log/pods and forwarding to the backend's versioned ingest path over otlphttp with a Token-scheme DRF token. Relocates controller.py's six secret-redaction patterns (Python→RE2) as a pre-export redaction floor, bounds a 1 GiB disk-backed queue, and ships disabled; Class-A-only scoping keeps Class-B customer training data from leaving the cluster.
The three Bugbot findings from the prior pass are all verified genuinely fixed in the templated code at HEAD 3152dda (not just marked resolved):
- crashloop: a
lookup-based install-time gate nowfails the release with an actionable message if the token Secret is absent, so no DaemonSet is created and the crashloop is unreachable — correct fail-soft posture, andlookupstays empty underhelm templateso offline renders still work. - mirror registry: the image now resolves
global.imageRegistryvia the same precedence as the chart's other self-registry images; mutation-proof tests pin both the mirror and the fallback. - namespace/pull-secret gate: both are now created under the widened
(or (ne resourceMonitor false) $tc.enabled)gate.
CI fully green (40+ checks), all 5 Bugbot threads resolved, MERGEABLE. Two non-blocking test-coverage follow-ups (not merge blockers): the pull-secret gate isn't mutation-pinned (the namespace gate is), and the pre-flight fail is inherently un-pinnable by helm-unittest since lookup returns empty there.
— drafted with Claude Code
Two conflicts, both from develop moving while this PR was in review. client/Chart.yaml — develop reached 1.9.51 on its own while this branch had run to 1.9.54 across four review rounds. Resolved to 1.9.52: one patch above develop, version and appVersion equal, which is what the version guard checks. The absolute number going DOWN is fine and the guard does not care — what it compares is this branch against develop. Makefile DRIFT_GUARDS — a UNION, not a side. develop's #764 added `k3s-components-agreement.sh` and this branch added `collector-class-a-agreement.sh`; both are real guards and taking either side would have silently DELETED the other one. Resolved to 8 guards with develop's ordering preserved, and asserted both names are present rather than eyeballing the line: a conflict resolution that quietly drops a check is indistinguishable from a clean merge afterwards. Verified after resolving: `make drift` green on all 8 (both new guards run), whole chart suite 516/516 across 34 suites, manifest current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ays offline-safe @aptracebloc's two non-blocking follow-ups on #779. Taken now rather than deferred because the conflict-resolution push dismissed the approval anyway, so they cost no extra review round. 1. THE PULL-SECRET GATE WAS NOT MUTATION-PINNED while the namespace gate was. I had verified it by hand — render diff, 4 node-agents resources with the fix and 3 without — and a by-hand check leaves nothing behind. That was the gap, not the fix. Now pinned in BOTH directions: narrowing the gate back reddens the positive test, and widening it to `true` reddens the negative one, so neither passes for free. It needs real registry values, which is worth recording: `dockerRegistry.server` is `format: uri` and `email` is required by its `allOf`, so a partial set is rejected by values.schema.json before any template renders — and helm-unittest surfaces that as a plugin ERROR rather than a template failure, which reads exactly like a code defect. It cost me three confused renders earlier, all of them my own invalid inputs with stderr suppressed. 2. THE PRE-FLIGHT `fail` IS UN-PINNABLE and he is right that it is: `lookup` returns empty under `helm template`, so the guard never fires in a unit test. But the INVERSE is testable and is the failure that would actually hurt — a pre-flight that fired offline would break every `helm template`, every CI render and every `--dry-run` in the fleet. That property is now asserted directly instead of resting on the guard's own comment. Chart 1.9.53. Whole chart suite 519/519 across 34 suites; the Collector suite 28/28; `make drift` green on all 8 guards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
@aptracebloc — heads up that your approval was dismissed, and by me: resolving the merge conflict required a push, and this branch dismisses stale reviews on new commits. Nothing about the review was wrong; it needs re-approving once CI settles. Sorry for the round-trip. Since it cost no extra review cycle, I took both of your non-blocking follow-ups: The pull-secret gate is now mutation-pinned. You were right that the namespace gate was and this one wasn't — I had verified it by hand (render diff: 4 node-agents resources with the fix, 3 without) and a by-hand check leaves nothing behind. That was the gap, not the fix. Pinned in both directions now: narrowing the gate reddens the positive test, widening it to The pre-flight On the conflict itself, one thing worth your eye: the Chart is 1.9.53 (one patch above develop's 1.9.51 after the merge). 519/519 chart tests, |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ft SCC (backend#1906) Both of Bugbot's findings on #779. Both verified against upstream before fixing — they are real, and the second one is the more dangerous of the two. 1. THE DISK-BACKED QUEUE WAS UNWRITABLE. `runAsNonRoot: false` PERMITS root; it does not SELECT it. The pinned otelcol-contrib image declares `USER 10001` (`ARG USER_UID=10001` in the upstream Dockerfile), and the kubelet creates a DirectoryOrCreate hostPath root-owned 0755 — so the Collector ran as 10001 and could not write /var/lib/tracebloc/<release>/telemetry. D7's whole point is a queue that survives a restart; it would have failed the moment the Collector was enabled, which is exactly the kind of defect that hides behind a chart that renders clean. Fixed by pinning `runAsUser: 0`. Root rather than an init-container chown for a reason beyond one fewer container: the READ side needs it too — container log files under /var/log/pods are root-owned and not world-readable on every runtime, so a non-root collector may not be able to read the logs it exists to read. fluent-bit and the cloudwatch-agent already on these clusters run as root for the same reason. It is a NARROW root, and that is the trade being made rather than glossed: all capabilities dropped, no privilege escalation, read-only root filesystem, and the only writable paths are the queue and /tmp. 2. ON OPENSHIFT THE COLLECTOR HAD NO SCC AT ALL. Chart-managed SCCs gated on `resourceMonitor` alone and bound only that ServiceAccount, so a second hostPath DaemonSet with its own SA was refused at admission. A SEPARATE SCC, not a widening: resource-monitor's declares MustRunAsNonRoot, which is correct for it — read-only mounts, no write anywhere — and loosening it to RunAsAny to serve the Collector would hand a broader run-as rule to a workload that does not need one. WHY A NEW DRIFT GUARD. This is the second cross-document gap on this PR to get through a green 519-test suite (the first was the Class A globs), because the defect is the ABSENCE of a relationship between two documents and helm-unittest asserts within one template at a time. scripts/tests/openshift-scc-coverage.sh renders the chart with OpenShift on and compares the hostPath workloads against the SCCs and their `users:` lists — holding no list of its own, deriving both sides, failing closed when either side is empty. It checks run-as compatibility too, and that is the point rather than a flourish: adding the Collector's SA to resource-monitor's existing SCC would satisfy "is it covered" while still failing at admission, since that SCC refuses root. A guard that checked only membership would have gone green on the wrong fix. MUTATION-PROVEN, anchors asserted applied in every case: guard M1 run-as -> MustRunAsNonRoot KILLED (root refused by ...) M2 allowHostDirVolumePlugin off KILLED (no SCC) M3 users emptied KILLED (no SCC) M4 template deleted KILLED — reproduces the original defect unittests M1 drop `runAsUser: 0` 1 failed <- the shipped regression M2 SCC -> MustRunAsNonRoot 1 failed M3 gate on openshift only 2 failed M4 gate on Collector only 1 failed M5 users emptied 1 failed No survivors; green again on restore. 525/525 chart tests, 9/9 drift guards, shellcheck -S warning clean. `scripts/manifest.sha256` deliberately NOT regenerated: it covers only the sub-scripts install.sh fetches and hash-verifies, and a CI guard under scripts/tests/ is not one — checked rather than assumed, after #775. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
…(backend#1906) Bugbot's find, and correct. `file_storage` backed only the exporter queue. The `filelog` receiver set no `storage`, so its read offsets lived in memory — and `start_at` governs where a NEWLY DISCOVERED file is read from, not where a known one resumes. With nothing persisted, every file looks new after a restart, `end` wins, and everything written while the Collector was down is skipped. Silently: no error, no gap in any metric. D7's disk queue covers records already ingested. Nothing covered the read side, and restart-during-a-backend-outage is precisely the path that needs both. THE COMMENT THAT SAT THERE ASSERTED THE OPPOSITE — that `start_at: end` was what stopped re-reads across restarts. It is why this was not caught in review: a wrong comment is worse than none, because it answers the question before anyone asks it (backend#1729 rule 7). Corrected rather than deleted, since the reason `end` is still right needs saying: with offsets persisted it applies only to files with no stored position, so a fresh install does not ingest the whole existing backlog while a known file resumes exactly where it stopped. The receiver shares the exporter's extension. `file_storage` keys entries by component so they cannot collide, and offsets cost kilobytes against the queue's cap. It writes to the same hostPath — which is only writable because the previous commit pinned `runAsUser: 0`, so the two findings are more connected than they looked. WHY A GUARD AND NOT A helm-unittest ASSERTION. The Collector's config is a YAML document embedded in a string inside the ConfigMap. helm-unittest can only regex that string, and a regex for `storage: file_storage` matches the EXPORTER's queue setting just as happily as the receiver's — passing while the receiver has none, which is the exact defect. Parsing is the only way to say where the key is, so scripts/tests/collector-offsets-persisted.sh parses it and checks the named extension is both declared and enabled in `service.extensions`: a storage extension that is configured but not switched on is silently ignored. Mutation-proven, anchors asserted applied: filelog `storage` removed KILLED — reproduces the original defect points at an undeclared extension KILLED declared but absent from service.exts KILLED No survivors; green on restore. 525/525 chart tests, 10/10 drift guards, shellcheck -S warning clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
The telemetry work is careful and the D7 implementation matches the RFC as amended — I reviewed rfcs#36 this morning and this lands block_on_overflow: false with the drop-count-as-only-signal consequence stated, which is exactly Consequence 2 of that amendment. But Bugbot's High is one instance of a three-instance gap, so requesting changes.
The class: everything reaching into node-agents must be gated on both tenants
This PR correctly widened two gates to (resourceMonitor || telemetryCollector.enabled) — the namespace itself and the mirrored pull Secret. Three more still gate on resourceMonitor alone while granting something in nodeAgents.namespace.name. I verified each directly rather than trusting a count:
| file | gate | what it grants in node-agents |
|---|---|---|
auto-upgrade-rbac.yaml:173 | and (ne .Values.resourceMonitor false) (ne …) | Role :189 + RoleBinding :205 ← Bugbot's finding |
image-refresh-rbac.yaml:60 | if ne .Values.resourceMonitor false | Role :86 + RoleBinding :105 ← not flagged |
secrets.yaml:112 | and (ne .Values.resourceMonitor false) (ne …) | Secret :121 ← not flagged |
So with resourceMonitor: false and telemetryCollector.enabled: true — the exact configuration this PR exists to make possible — the namespace is created and the Collector lands in it, while:
- auto-upgrade has no Role there, so the next
helm upgrade --atomic --wait403s and rolls back. Bugbot is right that this is backend#953's failure mode, and--atomicmakes it a rollback rather than a partial apply, which is the expensive version. - image-refresh has no Role there, so pull-secret refresh and rollout for the Collector DaemonSet fail — silently until an image needs re-pulling.
- whatever
secrets.yaml:112mirrors is absent from the namespace entirely.
auto-upgrade-rbac.yaml:179 makes the first one unambiguous, because it already says the release-namespace Role "does NOT reach" the node-agents namespace — so that Role is the only thing granting access there, and its condition is now wrong.
Fix it once, not three times
Three more edits leaves the same trap for the next tenant of that namespace. _helpers.tpl already carries nodeAgents.namespace.name logic, so it's the natural home for one predicate:
{{/* True when anything the chart owns lands in the node-agents namespace. */}}
{{- define "client.nodeAgentsInUse" -}}
{{- $tc := default (dict) .Values.telemetryCollector -}}
{{- if or (ne .Values.resourceMonitor false) $tc.enabled }}true{{ end -}}
{{- end -}}
then five call sites read if and (include "client.nodeAgentsInUse" .) (ne .Values.nodeAgents.namespace.name .Release.Namespace). That's the same "one parser, not N readers" shape this fleet keeps converging on, and it means adding a third tenant is one line rather than an audit.
Whichever way you go, carry the nil-guard.node-agents-namespace.yaml:34 gets this right with $tcNs := default (dict) .Values.telemetryCollector, and it's load-bearing: a bare .Values.telemetryCollector.enabled crashes templating with "nil pointer evaluating interface {}.enabled" for anyone running helm upgrade --reuse-values from a chart that predates the key — which is a very common operator habit and would fail before any resource lands. A helper centralises that guard instead of repeating it five times.
The rest is good, and two things are better than good
Shipping inert is the right call and the reasoning is the right reasoning."Enabling the Collector before that Secret exists would put a DaemonSet on every customer node spooling to disk with nothing it can deliver, filling toward the 1 GiB cap per node for no benefit — precisely the 'telemetry must never be the reason a node fills' risk D7 exists to bound." That's the failure mode reasoned from the decision rather than discovered later.
The redaction-drift caveat is the most honest thing in the PR. Carrying all six _LOG_REDACTIONS patterns because filelog bypasses controller.py's handler is the non-obvious half — without it the move removes a protection that exists today. And stating plainly that it's a cross-repo second copy that Helm cannot machine-check, in the file rather than only in the PR, is what makes it a known residual instead of a latent surprise. Compiling all six as RE2 and running them against sample secrets rather than assuming the Python→RE2 translation held is the verification I'd have asked for — "a silently-invalid regex in OTTL is a processor that starts and scrubs nothing" is exactly the failure that would otherwise look like success.
Worth noting Cursor Bugbot reports neutral here with a High open beneath it — fourth PR today with that combination.
… copies (backend#1906) @saadqbal's review of #779, and he is right that Bugbot's High was one instance of a class. Five templates put something in `nodeAgents.namespace` and each held its own copy of "is resource-monitor on"; this PR widened two of them for the Collector and left the rest, so `resourceMonitor: false` + `telemetryCollector.enabled: true` — the configuration the Collector exists to enable — created the namespace, landed the DaemonSet, and left the RBAC that manages it behind. `tracebloc.nodeAgentsInUse` is now the single predicate, and all five call sites read it. The nil-guard lives there too rather than five times over. I VERIFIED EACH INSTANCE RATHER THAN TAKING THE COUNT, and the count was wrong in both directions: * THERE IS A FOURTH the review did not list — `rbac.yaml:183`, jobs-manager's node-agents Role — found by diffing the two renders rather than by reading gates. * TWO OF THE THREE MUST NOT BE WIDENED, and widening them on request would have made this worse: - `secrets.yaml:112` mirrors CLIENT_ID/CLIENT_PASSWORD there so the resource-monitor DaemonSet can read them via secretKeyRef. The Collector authenticates with its own telemetry token and never reads them, so widening this would copy CUSTOMER CREDENTIALS into a namespace for a workload that has no use for them. Left gated on resource-monitor. - `rbac.yaml:183` grants daemonsets get/list/watch so jobs-manager can read resource-monitor's version for the heartbeat inventory. Read-only, about a specific workload, and nothing asks it for the Collector's version — so it is correctly absent. backend#2274 WILL need it, when jobs-manager starts writing the Collector's token Secret there; that is a Secret write, and it belongs to that ticket's chart half. So two gates widened (auto-upgrade, image-refresh — both mutate DaemonSets in that namespace, and the Collector is one), two deliberately not. GUARDED ONCE TOO, because "fix it once" is only half of it. Three careful readings produced three different counts, which is the argument for a machine check. scripts/tests/node-agents-tenancy.sh renders the chart twice and asserts that the Roles which MUTATE DaemonSets in that namespace do not depend on WHICH DaemonSet is there. Deliberately narrower than "the two renders must match": the mirrored Secret and the read-only jobs-manager Role SHOULD differ, and a stricter check would have to be wrong about them. Writing it caught a defect in itself worth recording: matching the literals `apps`/`daemonsets` missed auto-upgrade's Role, which is `apiGroups: ["*"], resources: ["*"], verbs: ["*"]` — the very Role Bugbot flagged. A guard that cannot see the finding it was written for is worse than none. Wildcards count now. Mutation-proven, anchors asserted applied: auto-upgrade gate reverted to resourceMonitor guard KILLED + 1 unittest failed image-refresh gate reverted guard KILLED the predicate hardcoded to `true` (over-gating) 7 unittests failed the nil-guard removed from the predicate 1 failed, 1 errored That last row SURVIVED at first and the reason matters: Go's `or` short-circuits, so my null test set `resourceMonitor: true`, the second operand was never evaluated, and the test passed happily with the guard DELETED. It sets `resourceMonitor: false` now — which is the whole test. Without the mutation it would have shipped asserting nothing, which is this epic's own dominant defect class appearing in the fix for it. The over-gating direction is covered by helm-unittest's negatives rather than by the guard, and that division is stated in both places instead of left implicit. 529/529 chart tests, 11/11 drift guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
Fixed in 4743b96, and centralised the way you asked — Confirmed, with the render rather than by reading gates: Two corrections to the table, in both directions. There is a fourth you didn't list: And two of the three must NOT be widened. I started to and stopped:
So: auto-upgrade and image-refresh widened (both mutate DaemonSets there, and the Collector is one), the other two deliberately left. Guarded, because "fix it once" is only half of it. Three careful readings produced three different counts — that is the argument for a machine check rather than a fourth careful reading. Writing it caught a defect in the guard itself, which I'd rather record than quietly fix: matching the literals Mutation-proven, anchors asserted applied: auto-upgrade gate reverted → guard KILLED + 1 unit test; image-refresh reverted → guard KILLED; predicate hardcoded That last row survived at first, and it is the most useful thing in this round. Go's On your last point — 529/529 chart tests, 11/11 drift guards. Re-requesting review. |
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
One of my three was a false positive, and you were right to leave it. Correcting that first.
secrets.yaml — my finding was wrong
I flagged secrets.yaml:112 as a third instance of the node-agents gate gap. It isn't. That block mirrors CLIENT_ID/CLIENT_PASSWORDfor the resource-monitor DaemonSet specifically — the comment says so, and the Secret is labelled app: {{ include "tracebloc.resourceMonitorName" . }}. The Collector uses a different secret entirely (telemetry-collector-daemonset.yaml:181, tracebloc-telemetry-token). So gating it on resourceMonitor alone is correct and widening it would mirror a credential the Collector has no use for.
I reached that finding by pattern-matching — gated on resourceMonitor, targets node-agents, therefore needs widening — and never checked what the object was for. That's confirming the half that supports the finding and skipping the half that would refute it, which is the exact failure I've flagged in other people's reasoning twice this week. Two of three, not three.
The two real ones are fixed, and via the helper
tracebloc.nodeAgentsInUse now exists at _helpers.tpl:875, with auto-upgrade-rbac and image-refresh-rbac both reading through it. That's the shape worth having: the next tenant of that namespace changes one definition, not an audit.
Bugbot's new Medium is real, and the premise is in the helper it cites
The node-agents glob is:
/var/log/pods/<nodeAgents.namespace.name>_*/<container>/*.log
and _helpers.tpl documents the design it collides with:
tracebloc.telemetryCollectorName… Same shape astracebloc.resourceMonitorName: release-scoped, so two releases on one cluster do not collide in the shared node-agents namespace.
So two releases sharing that namespace is an anticipated shape, not a hypothetical — and release A's Collector matches release B's tracebloc-resource-monitor pod, shipping B's Class A logs under A's ingest token.
The precise reason it's easy to miss is worth stating in the fix: the release-namespace glob three lines above uses the same bare * for the pod segment and is perfectly safe, because there the namespace segment carries the release identity. In the node-agents glob the namespace is shared by design, so the pod name is the only discriminator — and it's the segment that got wildcarded. Identical printf, opposite safety.
Scoping the pod segment to the release-scoped DaemonSet name closes it:
{{- printf "%s/%s_%s-*/%s/*.log" $logs $.Values.nodeAgents.namespace.name (include "tracebloc.resourceMonitorName" $) . | quote }}
Worth a test asserting the node-agents include contains a release-scoped prefix rather than a bare _*, alongside the existing no-wildcard-include assertion — the two are the same property ("widening this is not a config tweak: it changes what leaves a customer's cluster") applied to different segments of the same path.
And this is the fix-for-a-fix worth noticing. The node-agents glob exists because an earlier Bugbot round found the Collector "healthy and blind to a whole Class A component" — scoping every path to .Release.Namespace missed the DaemonSet. Correcting that blindness over-corrected into matching other releases' components. Not a criticism; just the sort of thing that argues for the test, because the next person widening a glob here will be fixing a blind spot too.
The offset-persistence comment is a good catch of the same kind, incidentally — "the comment that used to sit here asserted the opposite, which is worse than no comment: it described a persistence this config did not have."
LukasWodka
commented
Aug 21, 2026
Filed the Bugbot-gating point as tracebloc/backend#2284 — measured rather than asserted: |
… (backend#1906) Bugbot's find, and it is a cross-tenant leak rather than a noisy metric. The kubelet's log directory is `<ns>_<podName>_<uid>`. A bare pod wildcard is fine in the release namespace — that namespace belongs to one release — but `nodeAgents.namespace` is SHAREABLE ON PURPOSE, which is exactly why the workloads in it are release-scoped. Container names are not: resource-monitor's container is `tracebloc-resource-monitor` in every release. So the node-agents glob matched EVERY release's resource-monitor in a shared namespace, and one edge's Collector would have read another edge's Class A logs and shipped them under its own ingest token. Both Collectors look healthy throughout. The pod portion is now scoped to the release. DaemonSet pods are `<daemonsetName>-<hash>` and the DaemonSet name is release-scoped, so the release prefix is what separates them. THE RESIDUAL IS STATED, NOT PAPERED OVER. The wildcard crosses `-`, so a release named `edge` and one named `edge-2` sharing this namespace would still cross-match. Closing that needs the workload name rather than the release name, and this list cannot give it: it is a list of CONTAINER names, and container-to-workload is not 1:1 in general. Two releases whose names are prefixes of one another, in one shared namespace, is what remains; the ordinary case is closed. GUARDED, and the guard now checks the property rather than the shape. Updating collector-class-a-agreement.sh's parse for the new glob would have been enough to make it pass, which is the trap — so it asserts the invariant instead: a glob into a namespace that is NOT the release namespace may not use a bare pod wildcard. Both sides derived from the render, including which namespace counts as "the release namespace", read off the Deployments rather than written down. Mutation-proven, anchor asserted applied: reverting the glob to a bare pod wildcard → guard KILLED, naming the namespace and the leak. The unit test pins the release prefix too, so dropping it reddens both tiers. A Go template comment ends at the first `star slash`, so spelling the glob out inline inside the explanatory comment truncated it and broke the render — one debug cycle, and worth the note in the file since the next person to document a glob there will hit it. 529/529 chart tests, 11/11 drift guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
Thanks for the correction on Bugbot's cross-release glob is fixed in accf218 (reply on the thread). That one is the same shape as your review's finding, one layer down: the node-agents namespace is shared on purpose, and I'd scoped the workloads to the release but not the log globs that read them. Current state: 529/529 chart tests, 11/11 drift guards, all threads resolved. Also filed the Bugbot-gating point as tracebloc/backend#2284 — measured: |
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…nd (backend#1906) Both Bugbot's, both real, and both the same shape as everything else on this PR: the DaemonSet is Ready, the metrics look quiet, and no records exist. 1. `DirectoryOrCreate` ON A PATH WE DO NOT OWN. `/var/log/pods` is the KUBELET'S directory — we read it. With `DirectoryOrCreate`, a wrong or missing `hostLogsPath` made the kubelet helpfully create an empty directory, and the Collector then started, reported healthy, and matched no files forever. That is the Class A glob guard's failure mode arriving by a different route, and the guard cannot see it: the globs are correct, the directory is just empty. `Directory` fails the pod instead, which is the right direction for a path we expect to already exist — resource-monitor's /proc and /sys have always used it. THE QUEUE KEEPS `DirectoryOrCreate`, and the asymmetry is the design rather than an oversight: that path is ours and does not exist on a node's first install, so `Directory` there would refuse to start the Collector on every fresh node. Both are asserted in one test, because a well-meaning "make these consistent" edit breaks exactly one of the two and the tests should say which. 2. `seLinuxContext: MustRunAs` CANNOT READ CONTAINER LOGS. I copied that from resource-monitor's SCC, where it is correct — that workload reads /proc and /sys. Container logs under /var/log/pods are labelled `container_log_t` with per-container MCS categories, and a namespace MCS context cannot read them. So on OpenShift the pods would be admitted, run as root, and take a permission error on every include glob. `RunAsAny`, the same widening already applied to `runAsUser` on this SCC and for the same reason — resource-monitor's own SCC is untouched and keeps both tighter policies. That copy-from-the-sibling error is worth naming, because it is the second time on this PR: the sibling SCC is the right template to start from and the wrong one to finish with, since every field on it was chosen for a workload that only reads kernel pseudo-filesystems read-only. Mutation-proven, anchors asserted applied: host-logs back to DirectoryOrCreate 1 failed queue changed to Directory 1 failed SELinux back to MustRunAs 1 failed 531/531 chart tests, 11/11 drift guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
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 452d147. Configure here.
saadqbal
left a comment
There was a problem hiding this comment.
Approving. All checks green, zero open threads, MERGEABLE. Everything from three rounds is closed, and the glob fix is better than what I proposed.
The node-agents glob is now release-scoped, and more generally than I suggested. I'd said to thread include "tracebloc.resourceMonitorName" into the pod segment. You used $.Release.Name:
/var/log/pods/<nodeAgents.namespace.name>_<Release.Name>-*/<container>/*.log
That's the better call. Since every node-agent resource name in that namespace is release-scoped by construction — which is the design tracebloc.telemetryCollectorName's own comment states — prefixing with the release name discriminates correctly for resource-monitorand for whatever release-scoped node-agent this chart grows next, without threading a specific DaemonSet name through the template. My version would have needed editing the day a second Class A node-agent appeared; this one doesn't.
And you correctly left the release-namespace glob alone.:67 still uses a bare * for the pod segment, which is right — there the namespace carries release identity, so there's nothing to disambiguate. Not over-applying the fix is the part that shows the reason landed rather than the pattern.
Both RBAC gates still read through tracebloc.nodeAgentsInUse, so the two real findings from my first pass stay closed at the source rather than in two places.
And secrets.yaml is unchanged, which is correct — my flagging it was the error. Restating that here so the record is unambiguous: that block mirrors CLIENT_ID/CLIENT_PASSWORD for the resource-monitor DaemonSet specifically, the Collector uses tracebloc-telemetry-token instead, and widening the gate would have mirrored a credential the Collector has no use for. I pattern-matched from "gated on resourceMonitor, targets node-agents" without checking what the object was for. Two of my three were real; that one wasn't.
Net across the rounds: the RBAC gap was real and is now closed by a helper rather than three edits, the cross-release log-mixing glob was real and is now closed more generally than proposed, and the third thing I raised wasn't a thing. The Collector ships inert with the token secret it depends on still unwritten, so the remaining precondition is the client-runtime jobs-manager half — which the body correctly says this PR doesn't close.
Uh oh!
There was an error while loading. Please reload this page.
| # Same shape as the runAsUser widening on this SCC, and the same trade: the | ||
| # narrowing that makes it defensible is elsewhere — all capabilities dropped, no | ||
| # privilege escalation, a read-only root filesystem, and only two host paths. | ||
| type: RunAsAny |
There was a problem hiding this comment.
seLinuxContext: RunAsAny is necessary but, as templated, not sufficient — on an Enforcing OpenShift node the Collector will still be admitted and blind.
RunAsAny only permits an unconfined context; it does not select one. Unlike the runAsUser widening three lines up — where the SCC widens AND the pod pins runAsUser: 0 — the DaemonSet's pod securityContext sets no seLinuxOptions, and this SCC sets allowPrivilegedContainer: false. So CRI-O still assigns the container a default container_t domain with a per-container MCS level. By the exact mechanism this comment describes (container_log_t with per-container MCS categories), that context cannot dominate another pod's categories, so the include globs still take EACCES — the same "healthy, ships zero records" failure the change means to close.
Reading container_log_t under /var/log/pods requires the pod to actively assume spc_t (SystemHigh), which is why OpenShift's own node log collectors (Vector / Fluentd / fluent-bit) run privileged or with seLinuxOptions.type: spc_t. The runAsUser half works precisely because the pod chooses the value; the SELinux half widens the SCC but the pod never chooses the context, so nothing changes at runtime. Suggest adding seLinuxOptions: { type: spc_t } to the DaemonSet pod securityContext (this SCC's RunAsAny already admits it) and pinning it in the OpenShift test — the new test asserts only seLinuxContext.type: RunAsAny, which restates the template rather than proving a read can occur.
Inert-by-default and OpenShift-gated, so not a blocker for what ships — flagging before the Collector is ever enabled on OpenShift.
— drafted with Claude Code
…n (backend#2274) (#784) The write side landed in client-runtime#368 and this is the half it was waiting for: it needs `telemetryCollector.tokenSecret`, which only existed once #779 merged. Two pieces — RBAC, and the coordinates. THE RBAC IS NOT JUST PLUMBING, AND IT IS NOT 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 is not covered either way: the missing verb is missing in ANY namespace. The Role name is release-scoped, so it cannot collide when the namespaces do coincide. TWO RULES, because `resourceNames` IS NOT HONOURED FOR `create` — the API server cannot match a name that does not 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 same reason as image-refresh-rbac.yaml's collection-verb split. THE COORDINATES GO TO THE `api` CONTAINER ONLY, and that is a real trap rather than a detail: `pods-monitor-container` has a BYTE-IDENTICAL `env:` opening and does not run jobs_manager.py. My first patch attempt asserted a unique match, found two, and refused — which is the only reason it did not 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. FOUR DOCUMENTS NOW DESCRIBE ONE CREDENTIAL, so they get a guard rather than review attention: jobs-manager's env (the writer), the Collector's volume (the reader), `bearertokenauth.filename` (which encodes the key), and the Role + RoleBinding. Any one disagreeing produces the same symptom — nothing — because the Collector mounts the Secret `optional: true` on purpose, so a wrong name, namespace or 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-proven, anchors asserted applied: Role resourceNames renamed KILLED writer env NAME changed KILLED writer env KEY changed KILLED writer env NAMESPACE changed KILLED RoleBinding bound to the wrong SA 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 Tests: 537 passed, 537 total chart tests, 12/12 drift guards, make check green. STILL NOT DONE. 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. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…stead (backend#2274) @saadqbal, review of #787. He is right, and right about why it is not a nitpick: this PR exists to fix a comment that was NEARLY right, and its replacement enumerated the wrong set. Same class of error, one round later, in the one file where the comment IS the deliverable. WHAT WAS WRONG. The list named four `tracebloc.nodeAgentsInUse` consumers. It included secrets.yaml, which does not gate on the helper, and omitted auto-upgrade-rbac.yaml, which does — so "four" was right only because the two errors cancelled. 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 ONE CORRECTION BACK, since precision is this PR's whole subject: he wrote that secrets.yaml "contains no `nodeAgents` reference of any kind". It has two — the gate at :112 and the namespace at :121. His conclusion is exactly right (it does not gate on the HELPER, it gates on `resourceMonitor`, which is the deliberate #779 decision not to mirror customer credentials into that namespace for a workload that never reads them); the supporting detail is not. THE COMMENT NO LONGER ENUMERATES ANYTHING. It says "every other consumer of the helper gates on it; this one does not, because …" — which a reader can reproduce with one grep, cannot rot, and loses nothing. The rbac.yaml reference is anchored on the branch and the rule's `resources:` line rather than on `:62`/`:145`: those were the resources lines while the claim was about verbs (`:68`/`:151`), and cross-file line numbers drift the first time that file gains a rule. AND THE PROPERTY IS NOW GUARDED, because his deeper point is the right one — three hand-maintained lists have gone stale in this area in a week, and rule 1 is derive, never restate. 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 is #779's original finding stated as an invariant, and a new template that forgets its gate fails it without anyone having to notice it was added. It deliberately does NOT check the gate EXPRESSION. Two spellings are both correct — `nodeAgentsInUse`, and a bare `telemetryCollector.enabled` which implies it — so asserting one would flag correct code. Asserting the outcome cannot. It holds no list of templates, gates or helpers, reads even the namespace NAME out of the render, is scoped to `namespace.create: true` (an operator who pre-creates the namespace is legitimately out of scope, said rather than mis-asserted), and fails closed if no combination populates the namespace at all — an inert chart would otherwise satisfy the implication vacuously. Mutation-proven, anchors asserted applied: the token Role loses its gate KILLED — names the Role and the absent namespace the namespace stops being created KILLED Both reproduce the #779 class from opposite directions. 537/537 chart tests, 13/13 drift guards, make check green. Unrelated, and not mine to fix: #765 is @saqlainsyed007's and still bumps 1.9.58 -> 1.9.59, which develop now holds — it needs 1.9.61 once this lands. He said he would note it there; flagging only, since I do not touch teammates' PRs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal's review of #787. He diagnosed the release-namespace bug independently and identically before 5a34fc8 landed; three things he added on top are in here. 1. A SET, NOT `break` ON THE FIRST MATCH. Document order decided which namespace got checked, and a set is honest about there being possibly more than one — today there is exactly one, and a third appearing should be checked rather than shadowed. Every non-release namespace is now checked independently. 2. THE MESSAGE NO LONGER PRESCRIBES A FIX, and this is the part of his review that matters most. It used to say "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. A false positive that arrives 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 is more than one correct spelling anyway. 3. HIS MUTATION, WHICH IS THE ONE THAT MATTERED. The guard was built for #779's DaemonSet-in-an-uncreated-namespace, so that failure is its natural test, and he was right that a run failing on everything cannot 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'] which is #779's original finding, reproduced. AND ONE THE GUARD CAUGHT ON ITSELF. The vacuity check grepped `resources=N` out of the formatted summary line, so reformatting that line for (1) made it report "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. 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". It failed closed while broken, which is the design working. Mutation-proven, anchors asserted applied, all under KUBECONFIG=/nonexistent: Collector DaemonSet gate removed KILLED (#779's original failure) token Role gate removed KILLED namespace not created KILLED No survivors; green on restore. 537/537 chart tests, 13/13 drift guards, make check green — all with no kubeconfig, so none of it depends on ambient kube state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…backend#2274) (#787) * docs(telemetry): the missing verb is in BOTH secrets rules, not one (backend#2274) @saadqbal's two non-blocking asks from the review of #784, taken as a follow-up rather than a push onto that PR: branch protection dismisses stale reviews on new commits, so pushing a comment fix would have re-rolled an approved, green PR for a documentation change. 1. "THE CLUSTER-WIDE RULE" WAS SINGULAR AND WRONG. Verified before writing it down: `rbac.yaml:62` (the ClusterRole, `clusterScope: true`) and `rbac.yaml:145` (the namespaced Role, the `clusterScope: false` branch) BOTH grant `secrets: ["create", "get"]` with no `patch`. So the create succeeds and every refresh 403s on either install shape. The fix does not 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 is not there, which is the specific cost of a comment that is nearly right. 2. THE GATE'S OMISSION READ AS AN OVERSIGHT, so it now says why it is not. Four siblings — docker-registry-secret.yaml, image-refresh-rbac.yaml, node-agents-namespace.yaml, secrets.yaml — gate on `tracebloc.nodeAgentsInUse`, and this template does not. That helper is `or (ne .Values.resourceMonitor false) $tc.enabled`, so `telemetryCollector.enabled` IMPLIES 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, AND PROVEN RATHER THAN ASSERTED. I claimed that on client#775 and was wrong, because telemetry.sh is hash-pinned and install.sh aborts on a manifest mismatch. Chart templates are not in the manifest (`gen-manifest.sh --check` is clean, unchanged), but the render is the thing that matters, so: * rendering the same tree twice differs by 2 lines — POD_TOKEN_SIGNING_SECRET, which is 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 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. 537/537 chart tests, 12/12 drift guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(telemetry): stop enumerating the list, and guard the property instead (backend#2274) @saadqbal, review of #787. He is right, and right about why it is not a nitpick: this PR exists to fix a comment that was NEARLY right, and its replacement enumerated the wrong set. Same class of error, one round later, in the one file where the comment IS the deliverable. WHAT WAS WRONG. The list named four `tracebloc.nodeAgentsInUse` consumers. It included secrets.yaml, which does not gate on the helper, and omitted auto-upgrade-rbac.yaml, which does — so "four" was right only because the two errors cancelled. 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 ONE CORRECTION BACK, since precision is this PR's whole subject: he wrote that secrets.yaml "contains no `nodeAgents` reference of any kind". It has two — the gate at :112 and the namespace at :121. His conclusion is exactly right (it does not gate on the HELPER, it gates on `resourceMonitor`, which is the deliberate #779 decision not to mirror customer credentials into that namespace for a workload that never reads them); the supporting detail is not. THE COMMENT NO LONGER ENUMERATES ANYTHING. It says "every other consumer of the helper gates on it; this one does not, because …" — which a reader can reproduce with one grep, cannot rot, and loses nothing. The rbac.yaml reference is anchored on the branch and the rule's `resources:` line rather than on `:62`/`:145`: those were the resources lines while the claim was about verbs (`:68`/`:151`), and cross-file line numbers drift the first time that file gains a rule. AND THE PROPERTY IS NOW GUARDED, because his deeper point is the right one — three hand-maintained lists have gone stale in this area in a week, and rule 1 is derive, never restate. 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 is #779's original finding stated as an invariant, and a new template that forgets its gate fails it without anyone having to notice it was added. It deliberately does NOT check the gate EXPRESSION. Two spellings are both correct — `nodeAgentsInUse`, and a bare `telemetryCollector.enabled` which implies it — so asserting one would flag correct code. Asserting the outcome cannot. It holds no list of templates, gates or helpers, reads even the namespace NAME out of the render, is scoped to `namespace.create: true` (an operator who pre-creates the namespace is legitimately out of scope, said rather than mis-asserted), and fails closed if no combination populates the namespace at all — an inert chart would otherwise satisfy the implication vacuously. Mutation-proven, anchors asserted applied: the token Role loses its gate KILLED — names the Role and the absent namespace the namespace stops being created KILLED Both reproduce the #779 class from opposite directions. 537/537 chart tests, 13/13 drift guards, make check green. Unrelated, and not mine to fix: #765 is @saqlainsyed007's and still bumps 1.9.58 -> 1.9.59, which develop now holds — it needs 1.9.61 once this lands. He said he would note it there; flagging only, since I do not touch teammates' PRs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(test): the new guard depended on the developer's kube context (backend#2274) `Source-of-truth drift` went red on #787's first run, and the guard I had just added to prevent hardcoded values from rotting was itself hardcoding one. `helm template` with no `--namespace` takes the release namespace from the CALLER'S KUBECONFIG CONTEXT. On the laptop this was written on that is `tracebloc`; on a runner with no kubeconfig it is `default`. The comparator found the node-agents namespace by excluding the literal `"tracebloc"` — so in CI it concluded 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 is 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 in cannot silently match. FIXED IN node-agents-tenancy.sh TOO, which carried the identical literal and 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 verdict is "which namespace is the node-agents one", so a wrong answer there produces a comparison between the wrong two sets rather than an error. REPRODUCED BEFORE FIXING, AND THE FIX PROVEN AGAINST THE SAME CONDITION — which is the step I skipped and which would have caught this before pushing: 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. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(guard): a set of namespaces, and no prescribed fix (backend#2274) @saadqbal's review of #787. He diagnosed the release-namespace bug independently and identically before 5a34fc8 landed; three things he added on top are in here. 1. A SET, NOT `break` ON THE FIRST MATCH. Document order decided which namespace got checked, and a set is honest about there being possibly more than one — today there is exactly one, and a third appearing should be checked rather than shadowed. Every non-release namespace is now checked independently. 2. THE MESSAGE NO LONGER PRESCRIBES A FIX, and this is the part of his review that matters most. It used to say "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. A false positive that arrives 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 is more than one correct spelling anyway. 3. HIS MUTATION, WHICH IS THE ONE THAT MATTERED. The guard was built for #779's DaemonSet-in-an-uncreated-namespace, so that failure is its natural test, and he was right that a run failing on everything cannot 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'] which is #779's original finding, reproduced. AND ONE THE GUARD CAUGHT ON ITSELF. The vacuity check grepped `resources=N` out of the formatted summary line, so reformatting that line for (1) made it report "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. 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". It failed closed while broken, which is the design working. Mutation-proven, anchors asserted applied, all under KUBECONFIG=/nonexistent: Collector DaemonSet gate removed KILLED (#779's original failure) token Role gate removed KILLED namespace not created KILLED No survivors; green on restore. 537/537 chart tests, 13/13 drift guards, make check green — all with no kubeconfig, so none of it depends on ambient kube state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 22, 2026
/fr-pass |
Implements the bulk of backend#1906 — RFC-BACKEND-1872 D6 + D7. An OpenTelemetry Collector DaemonSet that reads Class A container stdout with
filelogand forwards to the backend's ingest endpoint.It ships disabled, and that's the decision to check first
The exporter authenticates with a token this chart does not create — jobs-manager writes it into a Secret and doesn't do so yet. Enabling the Collector before that Secret exists would put a DaemonSet on every customer node spooling to disk with nothing it can deliver, filling toward the 1 GiB cap per node for no benefit. That is precisely the "telemetry must never be the reason a node fills" risk D7 exists to bound. Same posture as
egressProxy.routeWorkloads: ships inert, flipped per fleet once the prerequisite lands.So this PR does not close #1906. The jobs-manager half is client-runtime work and needs its own ticket.
Class A only, and the narrow scope is the protection
The
fileloginclude globs name the four control-plane containers this chart owns. A bare wildcard would sweep in training and ingestion pods — Class B, gated on backend#1908. Today's bounding is secret redaction, not content redaction, and raw customer cell values have already reached central telemetry once (backend#1879). A test asserts no wildcard include exists, because widening this is not a config tweak: it changes what leaves a customer's cluster.The redaction floor is relocated, not re-invented
D6 precondition 2. Reading stdout through
filelogbypassescontroller.py's app-side handler, so without this the move would silently remove a protection that exists today.All six
_LOG_REDACTIONSpatterns are carried (client-runtime@45006ec), translated Python → RE2 deliberately:\1→$1,(?i)/(?m)kept, and no pattern uses lookaround or in-pattern backreferences, which RE2 rejects. Checked rather than assumed — a silently-invalid regex in OTTL is a processor that starts and scrubs nothing. I compiled all six as RE2 and ran them against sample secrets; every one redacts, and theSharedAccessKeyNamecarve-outcontroller.pydocuments survives.D7's bounds, all three
sizer: bytes— expressible as bytes at all only because that sizer existsmax_elapsed_time: 0. A second time-based bound silently overrides the cap, because a batch past it is dropped rather than requeuedfile_storageon a hostPath, not an emptyDir — the restart is the common case during an outageblock_on_overflow: falseis drop-newest, per D7's 2026-08-20 amendment (rfcs#36):exporterhelpersheds at the entrance and has no evict-oldest option. That's why the Collector's own metrics are exposed on 8888 — under drop-newest the drop count (otelcol_exporter_enqueue_failed_log_records) is the only signal separating a quiet edge from a shedding one.Two things verified rather than assumed
scheme: Token, not the extension's defaultBearer. The edge holds a DRF user token (jobs_manager.pysendsAuthorization: Token <key>) and the ingest endpoint accepts it viaTokenAuthenticationunderIsAuthenticatedEdge.Beareris the other credential type (ClientAccessToken) and would 401 silently.The token file is watched. #1906's rationale says "a projected update needs no restart", which depends on
bearertokenauthreloading. The README doesn't mention reloading — so I read the extension's source: it builds acredentialsfile.ValueResolverwithWithOnChange→updateAuthorizationValues(). It does reload. The README's silence wasn't evidence either way.Also
logs_endpoint, notendpoint—otlphttpappends/v1/logsto the latter, and the ingest boundary is a versioned path of ours.A defect rendering caught that reading wouldn't
Helm parses
1073741824from values.yaml as a float64 and emittedqueue_size: 1.073741824e+09— a YAML float where the Collector wants an integer. The Collector would have started and the cap would not have been the number written in values.yaml.| int64fixes it, and a test asserts no scientific notation appears.Test plan
helm lintchart-version-guard.batsversionandappVersionto1.9.51(the guard requires a bump when chart content changes)SharedAccessKeyNamepreserved15 mutations, all killed: enabling by default · widening
filelogto a wildcard · dropping redaction from the pipeline · losing a redaction pattern · losingint64· revertingsizer· adding a second time bound · falling back to memory · reverting toBearer· usingendpoint· mounting host logs writable · making the Secret required · moving the queue to an emptyDir · removing the config checksum · granting a ClusterRole.One test-quality note: three of my assertions initially failed for reasons unrelated to the code — helm-unittest renders
.Release.Namespaceas the literalNAMESPACEplaceholder unless you set it, and\*in its regex patterns doesn't behave like Go's. Fixed by setting the namespace explicitly (which makes the assertion test the real scoping) and using[*]. Worth naming because an assertion that fails for the wrong reason is one you're tempted to weaken.Deliberately not in scope
Retiring the two App Insights entries from the squid ACL. That cuts client-runtime's live
AzureLogHandlerpath, and the RFC is explicit that App Insights cannot be switched off before edges have another route. It belongs to #1910's cutover.RBAC: none. It reads files off the node and never calls the Kubernetes API, so it gets a ServiceAccount and nothing else — a Role here would be privilege this workload has no use for, on a DaemonSet running on every node of a customer's cluster.
🤖 Generated with Claude Code
Note
High Risk
Introduces a per-node DaemonSet that runs as root with hostPath log and queue mounts, plus a new OpenShift SCC (RunAsAny). It also ships log redaction and tenant-scoped collection that, if mis-gated, can leak Class A logs across releases.
Overview
Adds an opt-in OpenTelemetry Collector DaemonSet (contrib
0.159.0) that reads Class A container stdout viafilelogand posts JSON totelemetry/v1/records/. It ships disabled: the ingest token Secret is not created by this chart, and--reuse-valuesupgrades from older releases nil-guard so the feature cannot appear by accident.The pipeline is filelog → secret redaction (six
controller.pypatterns, RE2) → batch → otlphttp withTokenauth, a 1 GiB disk queue, persisted read offsets, and retry-forever so the byte cap is the only bound. Collection is scoped to named control-plane containers; node-agents globs are release-prefixed to avoid cross-tenant ingest in the shared namespace.tracebloc.nodeAgentsInUsenow gates the node-agents namespace, mirrored pull Secret, and auto-upgrade/image-refresh Roles so a Collector-only install still gets those resources. OpenShift gets a separate SCC (RunAsAny, hostPath) rather than widening resource-monitor’s. Chart version 1.9.58. Drift guards cover Class A glob agreement, SCC coverage, offset persistence, and tenancy.Reviewed by Cursor Bugbot for commit 452d147. Bugbot is set up for automated code reviews on this repo. Configure here.