Uh oh!
There was an error while loading. Please reload this page.
release-train: develop -> staging - #789
Merged
Merged
Conversation
…ckend#1906) (#779) * feat(telemetry): the edge Collector, Class A only, shipping inert (backend#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> * fix(telemetry): the Collector gate broke `helm upgrade --reuse-values` 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> * fix(telemetry): the Collector collected nothing, and ran an image that 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> * fix(telemetry): three more on the Collector — token pre-flight, mirror, 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> * test(telemetry): pin the pull-secret gate, and that the pre-flight stays 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> * fix(telemetry): select root for the Collector, and give it an OpenShift 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> * fix(telemetry): the filelog receiver kept its read offsets in memory (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> * fix(telemetry): one predicate for the node-agents namespace, not five 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> * fix(telemetry): one edge's Collector could ingest another edge's logs (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> * fix(telemetry): two more ways the Collector could run healthy and blind (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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…d#2268) (#782) * feat(telemetry): the Windows installer emits an outcome event (backend#2268) `scripts/install-k8s.ps1` was 6,646 lines containing zero telemetry — no outcome event, no spool, nothing for any transport to carry. Not "the transport is missing": the EMITTER was missing, while backend#1907 sat closed titled "CLI and installer telemetry" and RFC-BACKEND-1872 listed `client installer` as a single row. Every statement of the form "the installer now emits" was true of one platform and false of the other. A PORT OF telemetry.sh's CONTRACT, not of its lines. Same closed vocabularies, same resource/record split, same attribute discipline, same `--help` latch, same declared-exit-2 handoff, same "a signal on a skipped run is still skipped", same never-create-HOST_DATA_DIR rule. Its comments are not re-argued here; they are implemented, with pointers to the twin. What IS documented is every place the platform forced a real difference, because that is what a reviewer cannot check against the other file. WIRED, because an emitter nothing calls is this epic's dominant defect class. The `finally` at install-k8s.ps1:6638 already described itself as mirroring bash's `install_cleanup`, so it is the emit point. The exit status comes from the classifying sites (PowerShell gives `finally` no access to an exit's code), and the interrupted case is DERIVED from `$script:OutcomeReported` — the installer's own existing Ctrl-C signal — rather than from a second mechanism that could disagree with it. FOUR REAL BUGS, ALL FOUND BY RUNNING THE TESTS RATHER THAN BY READING THE CODE. Recording them because each is a trap the next PowerShell port will hit: 1. `Set-StrictMode -Version Latest` is NOT file-scoped. Dot-sourcing applied it to the CALLER, imposing it on all 6,600 lines of install-k8s.ps1 — code written without it, where reading an absent property returns $null. 560 sibling assertions failed with "The property 'override' cannot be found", nowhere near this file. In the field it would have surfaced as the installer dying somewhere unrelated to telemetry: the observer breaking the thing it observes, the one outcome this feature must never have. Removed. 2. `,` BINDS TIGHTER THAN `+`. `@( 'a:' + $x + '"', 'b:' + $y + '"' )` is not a two-element array — the `,` binds to the adjacent string operands, `+` gets an array, and the whole thing collapses to ONE string joined by $OFS. The record rendered as `{"resource":{"service.name":"installer" "tracebloc...` — not JSON, entirely plausible in a log, and every regex assertion passed. Only the ConvertFrom-Json round-trip caught it. Each element is parenthesised now, and the field count is checked before the record is built. 3. `-match` IS CASE-INSENSITIVE. The key regex `[a-z][a-z0-9_]*` accepted `A.b`, so the closed key vocabulary was not closed. Every shape test is `-cmatch` now, which is what `[[ =~ ]]` does on the bash side. 4. A WINDOWS PATH OPENS WITH A COLON. The bash twin takes `${1%%:*}` for the source file because a POSIX path has no colon; `C:\...\install-k8s.ps1:12` made that return `C`, in no vocabulary, so the location was dropped on every real Windows failure. Splits on the LAST colon now. Also: the shape regexes anchor \A..\z, not ^..$. telemetry.sh moved off `grep` because grep matches a LINE; .NET reintroduces that hole in a subtler spelling — `$` matches before a trailing newline even without Multiline, so "abc\n" passes '^[a-z]+$'. Tested directly, so a tidy-up back to ^..$ reddens. And `Get-Command -Name 'Log'` resolved to /usr/bin/log, the macOS system logger, which the emitter then shelled out to. Every lookup is -CommandType Function. DELIVERY. A hash-pinned fetched sub-script like every other, added to install.ps1's $Files and gen-manifest.sh's WINDOWS_FILES (which the two check against each other). It is the first `scripts/lib/` entry on the Windows side, and Invoke-WebRequest -OutFile does not create directories — so the bootstrap now creates the parent first, as install.sh has always done with `mkdir -p`. Without that the Windows bootstrap would die on its first fetch. No integrity property changes: every file is still verified against the signed manifest. THE DUPLICATION IS GUARDED. A ps1 cannot read bash declarations at runtime, so the closed sets exist twice — the restated-not-derived shape that goes stale silently. telemetry-vocabulary-agreement.sh now parses BOTH twins and compares five vocabularies, holding no list of its own. Not the source vocabulary: bash names its eighteen files and ps1 its three, and those SHOULD differ. Writing that parser surfaced a third failure mode worth naming — the one-line `= @('A','B')` form made the range scan run away and return every quoted string in the file, so a non-empty WRONG answer passed the emptiness check. Bounded now. TESTS. 82 new (75 emitter + 7 wiring), 848 total passing against 766 on develop. Mutation-proven, anchor asserted applied every time: emit call deleted from the finally 1 failed run-started latch deleted 1 failed latch moved AFTER Confirm-Config 1 failed <- order, not presence an exit-2 handoff declaration dropped 1 failed a Step loses its phase letter 1 failed failure status no longer recorded 1 failed (see below) lib dropped from the bootstrap's $Files 1 failed a value added to one twin's vocabulary guard exit 1 a value removed from one twin's vocabulary guard exit 1 the ps1 twin deleted guard exit 2 (fails closed) a declaration reformatted past the parser guard exit 2 (fails closed) The sixth row SURVIVED at first, and mutation testing is the only reason it does not still: the assertion was `'$script:TbExitCode = 1'`, which is a PREFIX of `= 130`, so the interrupted line satisfied it and deleting the failure line changed nothing. It asserts `= 1;` now. A failed install would have reported exit_code 0 — the one wrong answer that looks entirely fine. Two existing source-text guards in install-k8s.Tests.ps1 asserted lines this change edits. Both updated to the new text rather than loosened, and both now also assert the telemetry half, so the wiring cannot be removed silently. NOT IN SCOPE. Delivery — the host transport is backend#2217, and #1906's Collector is a pod that can reach neither of these files. This produces the records. 831 lines of it are the emitter and its tests; the installer diff is 60. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(telemetry): several outcomes reached the emitter with the wrong status (backend#2268) @saqlainsyed007's review of #782, and every one of the five is real. They share a shape worth naming: the emitter was present and the vocabularies faithfully ported, but the STATUS arriving at it did not match what happened — succeeded on failure, succeeded on help, succeeded on skip. For a feature whose only job is to report the truth of a run, that is the worst class of defect it can have, and it is invisible from every angle except reading the record. 1. ERR REPORTED SUCCESS FOR EVERY FAILURE IT HANDLED (High). `Err` is the installer's primary failure helper — it sets `$script:OutcomeReported` and exits 1, and never touched `$script:TbExitCode`. `finally` cannot read an exit's code, so the emitter got 0 and wrote `install.run.succeeded`. Only `Test-InstallSucceeded` and the `catch` set the status; everything routed through `Err` lied. The last-resort `trap` had the identical hole, so a terminating error outside the try lied too. Both set it now. 2. THE LATCH FIRED BEFORE THE TERMINAL FLAGS (High), so `-Help` and `-Diagnose` latched as started, reached the `finally` with code 0, and emitted `install.run.succeeded` for a run that never touched the machine. That is exactly the client#747 bug, and the comment beside the latch said it was there to prevent it while the code did the opposite. The wiring test asserted `$latch -lt $help` and so PINNED the inversion rather than catching it — a test agreeing with the code instead of with the requirement, which is the class this epic keeps finding, appearing in the change that cites it. Both bounds are asserted now (after the dispatch, before Confirm-Config) so neither can drift. 3. THE FAST PATH MAPPED "NOTHING TO DO" TO SUCCEEDED. It exits 0 after the latch without `Set-TelemetryRunSkipped`, so re-runs on an already-healthy machine inflated the success count. `skipped` is a registered verb and the bash twin already reports it from assess.sh's gate — so the two platforms were answering the same question differently. 4. THE CONNECT PHASE DID NOT EXIST. `Wait-ForClientReady` had no `Start-TelemetryPhase -Letter 'f'`, so the readiness wait was timed inside `helm` and a client that never became Ready classified as `helm_install_failed` when helm had in fact succeeded — a FABRICATED helm failure in the exact rate this feature exists to produce. My Step-letters test asserted a..e and read as complete; it now says explicitly that `f` comes from the readiness gate, not a numbered step, and asserts it separately. 5. THE SPOOL BEGAN WITH A UTF-8 BOM. `Add-Content`/`Set-Content -Encoding utf8` writes one on PowerShell 5.1 — the PowerShell a stock Windows install has — so the first JSONL record started EF BB BF and no byte consumer could parse it. The install log already avoids this with `UTF8Encoding($false)`; the spool uses the same idiom through two writers rather than three call sites. Asserted on the BYTES, because every string-level read strips a BOM invisibly and would pass either way. Mutation-proven, each fix reverted individually: Err stops recording the status 1 failed the trap stops recording it 1 failed latch moved back before the flags 1 failed <- the shipped bug fast path stops marking skipped 1 failed connect phase removed 1 failed BOM reintroduced (UTF8Encoding($true)) 2 failed No survivors; green on restore. One existing guard asserted the old `trap` line; updated to the full new line so neither half can be dropped, not loosened. 853 Pester passing (0 failed), manifest regenerated for both changed ps1 files, make check green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(telemetry): the emitter read bash's environment, not the installer's state (backend#2268) Two Bugbot findings with one cause, and it is the only place this port was wrong as a CLASS rather than a line. In bash, a sourced lib and its caller share one variable namespace, so telemetry.sh reads `$CLIENT_STATE` and `$HOST_DATA_DIR` straight out of the environment. install-k8s.ps1 does not work that way: it RESOLVES those into script variables — `$script:ClientState` (set by Wait-ForClientReady) and `$script:HOST_DATA_DIR` (line 661, defaulting to `$env:USERPROFILE\.tracebloc` when the env var is unset, which is the normal case) — and never exports them. Ported literally, the emitter read names that are empty on every ordinary Windows install: * `client_state` was always absent, so a connect-phase failure could never classify as bad_credentials / image_pull_failed / image_pull_untrusted_ca / crash_loop and collapsed to the phase-based `not_ready`. The state-based half of the classifier was dead code in production. * the data-dir spool was never used, so every record went to the one-off fallback file — which backend#2217's transport does not look for. Telemetry that emits correctly and delivers nowhere. Both are silent by construction: §1.2 omits an absent value, so the record stays well-formed. Hence one helper with an explicit precedence — the installer's resolved value, then the environment — rather than two one-line fixes. AND A DERIVED TEST FOR THE CLASS, which is the part that pays. Every name the emitter reads as a script variable must be a name install-k8s.ps1 actually assigns; the names are parsed out of the emitter's own Get-InstallerValue calls, so no list lives in the test. It immediately caught my own over-application: TB_VERSION IS NOT A SCRIPT VARIABLE — IT IS NOTHING AT ALL. Nothing in the ps1 pair set it, by either mechanism, so `service.version` on every Windows record was permanently `0.0.0-unknown` — the field that says WHICH installer failed, on the platform this feature was added for. Bugbot did not flag it and neither did I; the derived test did. install.ps1 now exports the resolved ref exactly as install.sh:247 does, and install-k8s.ps1 derives TB_VERSION from it, mirroring common.sh:1128. install-k8s.ps1 runs as a CHILD process, so the export is inherited — asserted, along with the ordering, because an export after the launch would be silently useless. Mutation-proven, anchors asserted applied: client state back to env-only 3 failed HOST_DATA_DIR back to env-only 1 failed the ref export removed from install.ps1 1 failed The new tests set NO environment variable, which is the point: with env fallback alone they fail. The pre-existing tests all set env vars and so passed either way — they could not have caught this, and adding assertions that distinguish the two sources was the actual work. 860 Pester passing (0 failed), manifest regenerated, make check green, cross-twin vocabulary agreement green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(telemetry): a location parser nothing ever fed (backend#2268) Bugbot's find, and the honest description is dead code that reads as a feature. The emitter carried `Get-TelemetrySourceBasename` / `Get-TelemetrySourceLine` — colon-splitting careful enough for Windows drive letters, with a test asserting `C:\Users\...\install-k8s.ps1:12` parses — and NOTHING in the ps1 pair ever set the value. So every real Windows failure omitted source attribution while the parser and its tests sat there looking finished. Both failure sites now supply it, and they need different mechanisms: * `Show-FatalError` has an ErrorRecord, whose `InvocationInfo` knows exactly where the throw came from. * `Err` is a hand-raised failure with no ErrorRecord, so `$MyInvocation` — which describes the CALL to `Err` — gives the line that actually failed rather than a line inside `Err`. Only the file NAME survives, never the path that reached it, and the emitter closes the basename against its own source vocabulary besides — so a location from anything that is not one of our scripts still drops BOTH halves, which is the existing rule: a line number with no file is a confident wrong answer, not a partial one. Read through the same precedence helper as the rest of the installer's state, so this is the fourth instance of the class the previous commit named — and the third one Bugbot found rather than me. The derived ScriptVar test now covers it for free: `TbErrLoc` is read as a script variable, so install-k8s.ps1 must assign it. Mutation-proven, anchors asserted applied: Err stops recording the location 1 failed Show-FatalError stops recording it 1 failed the emitter reads TB_ERR_LOC only again 1 failed The new test sets no environment variable and asserts the username and drive letter do not reach the record. 866 Pester passing (0 failed), manifest regenerated, make check green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(telemetry): closed-set membership was case-blind too (backend#2268) Bugbot's find, and the same class as this PR's `-match` -> `-cmatch` fix one operator down: `-in` and `-notin` are CASE-INSENSITIVE in PowerShell. So `'STG' -in @('dev','stg','prod')` was True and `STG` reached the record verbatim as `deployment.environment`. A query keyed on `stg` misses that row — a wrong label, which is worse than no record. Same hole on `client_state` via `-notin`. FIXED BY NORMALISING, NOT BY DROPPING, AND THAT DIVERGES FROM BUGBOT'S SUGGESTED REMEDY DELIBERATELY. It proposed matching the bash twin, which drops an unrecognised environment under §3.2. That is right for bash, where `case` is case-sensitive throughout, so `STG` genuinely is not a valid environment there. It is valid here. PowerShell's `switch` is case-insensitive, so install-k8s.ps1's own Get-TraceblocClientEnv/Get-BackendUrl send `CLIENT_ENV=STG` to the STAGING backend — measured, not assumed: stg -> env=stg url=stg-api STG -> env=STG url=stg-api <- a real staging install Staging -> env=stg url=stg-api So a run with `CLIENT_ENV=STG` is a correctly configured staging install that worked. Dropping its record to match the twin would discard telemetry for an install that succeeded — the opposite of what this feature is for. Folding to the canonical spelling first, then requiring an exact match, fixes the wrong-label defect Bugbot identified while keeping the run countable: `STG` is emitted as `stg`, and a genuine non-member like `staging-2` still drops. Alias folding (`Staging` -> `stg`) stays delegated to Get-TraceblocClientEnv, so the emitter holds no second copy of that mapping (backend#1745). FILENAMES ARE THE ONE EXCEPTION and it is now stated rather than inherited from an operator default: a file's case is a filesystem artifact on Windows, not a contract value, so `Install-K8s.PS1` must still be attributed. Folded through the same helper, which also makes the emitted value canonical. There is now a grep test asserting NO bare `-in`/`-notin`/`-match`/`-notmatch` remains in the emitter, comments excluded. This class has been introduced twice in one PR; the third time should fail a test rather than reach review. MY OWN TESTS HIT THE SAME TRAP A THIRD TIME. `Should -Match` in Pester is also case-insensitive, so `Should -Not -Match 'STG'` was satisfied by the correct output `stg` and my first draft failed against a working emitter. Every assertion here now parses the JSON and uses `-BeExactly` instead of a regex, which is both case-exact and a better test. Mutation-proven, anchors asserted applied: membership back to `-in` 1 failed canonicalisation dropped (raw returned) 6 failed the fold removed (strict, loses STG runs) 6 failed 862 Pester passing (0 failed), manifest regenerated, make check green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(telemetry): a seeded default is not an observation (backend#2268) Bugbot's find, and a REGRESSION FROM MY OWN PREVIOUS FIX — worth saying plainly, because the direction it moved in is the interesting part. install-k8s.ps1 SEEDS `$script:ClientState = "starting"` at load (:772), long before anything has diagnosed the client. The bash twin leaves `CLIENT_STATE=""` (summary.sh:29) and fills it only at the readiness gate (:59/:61) for exactly this reason. `Get-TelemetryErrorClass` prefers state over phase, so once the emitter started reading the script variable, EVERY failure in preflight, tools, cluster, register or helm reported `error.type: not_ready` with `client_state: starting` instead of the phase-based class. Two commits ago the state never worked and the attribute was absent. One commit ago it always said `starting`. The second is worse: an absent attribute omits information, a seeded one ASSERTS something false — and it did so on the paths this feature primarily exists to measure, while looking more complete than before. DERIVED, not a new marker variable. Both real writers of ClientState live inside Wait-ForClientReady (:5387/:5388), which is also where phase `f` opens — so "we are in the connect phase" IS "the gate has run", and there is no second thing to keep in step. A failure during connect with the value still `starting` is honestly `not_ready`: we waited, and it did not become ready. THE GUARD I WROTE LAST COMMIT TO CATCH THIS CLASS HAD THE SAME HOLE AS THE CODE. It banned bare `-in`/`-notin`/`-match`/`-notmatch` and said nothing about `-eq`/`-ne`, which are equally case-insensitive — so a mutation swapping `-ceq` for `-eq` on the new gate survived it. A guard against a class that knows only two members of the class is the shape this epic keeps finding, and it was mine. Now it flags the whole family (`eq ne in notin match notmatch contains notcontains like notlike`) when compared against a STRING LITERAL — scoped that way on purpose, because `-eq 0` and `-eq $null` have no case and a blanket ban would be noise that gets switched off. Four existing literal comparisons converted to `-ceq` to satisfy it. A false-positive mutation (numeric `-eq 0`) confirms the scoping holds. Mutation-proven, anchors asserted applied: gate removed, state read unconditionally 5 failed gate on the wrong phase 8 failed gate made case-insensitive (-eq) 1 failed <- survived before the widening one literal comparison reverted to -eq 1 failed numeric -eq introduced 0 failed <- correctly NOT flagged Tests Passed: 880, Failed: 0, Skipped: 13, Inconclusive: 0, NotRun: 0 manifest regenerated, make check green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…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>
…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
ContributorAuthor
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
removed their request for review
August 22, 2026 15:40
6 tasks
#790) Bugbot High on the client#789 promotion, holding `client` out of the 22 Aug staging hop. The concern is real and now fixed. The stated MECHANISM is not what happens, and the real one is worse — so both are recorded here rather than the ticket's version being implemented as written. WHAT THE FINDING SAID: a partial `telemetryCollector` map under `--reuse-values` leaves the lists absent, `include` renders empty, and the DaemonSet stays Ready collecting nothing. WHAT ACTUALLY HAPPENS, constructed rather than reasoned about: helm template … -f <partial map> -> all 4 globs helm template … --set-json '{"enabled":true}' -> all 4 globs Helm coalesces chart defaults at render time, so an ABSENT key comes back from values.yaml. The `--reuse-values` shape does not reproduce this, and the sibling `| default` guards the ticket points at are for scalars, which have no default to coalesce in the same way. THE REACHABLE ROUTE IS AN EXPLICIT NULL, and it is a genuine defect: --set telemetryCollector.classAContainers=null -> include drops to 1 of 4 helm DELETES a key set to null, and a deleted key does NOT coalesce back. The schema's `minItems: 1` rejects an EMPTY list and says nothing about an absent one. AND THE QUIET CASE IS ONE LIST, NOT BOTH — which inverts the severity argument. Nulling BOTH empties the include, and filelog refuses to start on that: loud. Nulling ONE leaves a perfectly valid config with a shorter list, so the Collector starts, reports Ready, and silently omits three of the four Class A containers. That is the healthy-but-blind failure this feature exists to eliminate, and it is the case the ticket's framing does not reach. FIXED IN TWO LAYERS, each driven through the path that reaches it: * values.schema.json marks both lists `required`, so a null — which deletes — fails validation. Verified it does NOT break the three documented paths: a null whole map (the `--reuse-values`-from-an-older-release case the schema's own description calls out), an absent map, and an explicitly disabled Collector all still validate and render. * the template fails closed on each list separately. Not belt-and-braces: helm has `--skip-schema-validation`, so this is the layer that cannot be bypassed, and the guard drives exactly that flag. MY FIRST VERSION OF THE TEMPLATE GUARD MISSED THE CASE THAT MATTERS. It checked only that the combined include list was non-empty, which fires on total blindness — the loud one — and passed the one-list case that renders short and silent. Caught by testing it rather than by reading it; the guard now checks each list. Tests: the shrink cases live in collector-class-a-agreement.sh, which already owns this concern, rather than in a new file. Both schema refusals, both template refusals under `--skip-schema-validation`, and a CONTROL asserting a partial map still coalesces all four globs — the behaviour the finding assumed was broken, now pinned so a change that really did break coalescing is caught. Mutation-proven, anchors asserted applied: schema `required` removed KILLED (refused, but for the wrong reason) per-list template fails removed KILLED (the render succeeded) include reads a non-existent key KILLED (1 glob, want 4) No survivors; green on restore. 538/538 chart tests, 13/13 drift guards, make check green — all under KUBECONFIG=/nonexistent, so none of it depends on ambient kube state. Closestracebloc/backend#2341 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 22, 2026
ContributorAuthor
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
…#792) #790 merged with its four CI failures unfixed — I had diagnosed and fixed them locally but was holding the push for a full bats run, and the merge landed first. So `develop` is red for everyone until this goes in. Entirely my sequencing error: the fix should have been pushed as soon as it was verified, with bats as confirmation rather than a gate on sharing it. Four failures, two causes. 1. THE GUARD ASSERTED HELM 4'S ERROR TEXT, AND CI PINS 3.15.4. `standard-checks.yml` installs helm v3.15.4; this was written against v4.1.1. Two version-specific things, both mine: * the schema refusal reads `missing property 'x'` on helm 4 and `x is required` on helm 3, so all four checks saw the render refused and judged it "refused, but not for the expected reason"; * `--skip-schema-validation` did not exist before helm 3.16, so the two template-layer cases failed on an unknown flag rather than on the chart. Now matched on the PREAMBLE — "don't meet the specifications" — which is identical in both versions and still tells a schema refusal apart from the template one. The `--skip-schema-validation` half is FEATURE-DETECTED and skips loudly where the flag is absent, rather than passing silently: on CI the schema layer is what gets exercised, and the output says so. 2. A PIPE INTO AN EARLY-CLOSING READER, in the guard's own error path. `printf '%s' "$out" | head -2` SIGPIPEs its producer under `set -euo pipefail` and returns 141 — the backend#1778 class this repo has a dedicated scanner for, which is what `quality / pipefail early-close` and bats test 803 were both reporting. Replaced with the documented here-string idiom. The scanner then caught my REPLACEMENT for cause 1 as well: `helm template --help | grep -q --skip-schema-validation` closes the pipe on its first hit. Now captured into a variable and matched with `case`, so there is no pipe at all. Two instances of one class, in a fix for a different class — the gate earned its keep twice in one change. Verified on the post-merge develop: pipefail gate exit 0, its 42 bats tests green, shellcheck -S warning clean, the guard's five cases green, 13/13 drift guards. The other five guards added this session were also written against helm 4, and CI's drift log shows all of them passing on 3.15.4 — only this one carried version-specific assertions. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 22, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f4cccd1. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Aug 22, 2026
ContributorAuthor
/fr-pass |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated promotion by the release train (RFC-0008 D14). Head is the train-managed
release-train/to-stagingbranch (a mirror ofdevelop), so it never collides with a human PR. Merged only when the fr-gate is green.Note
High Risk
Introduces a per-node Collector that runs as root with hostPath log/queue mounts, OpenShift SCC, and token Secret RBAC, plus new installer telemetry. Default-off and fail-soft, but this is security-sensitive node and data-handling surface.
Overview
Adds an opt-in edge OpenTelemetry Collector (chart
1.9.61) that reads Class A control-plane stdout and ships it to the backend ingest path, plus a Windows twin of installer outcome telemetry. Both ship inert by default.Collector. New DaemonSet/ConfigMap/SA in
nodeAgents.namespace: filelog → secret redaction floor → disk-backed 1 GiB queue →otlphttpwith DRFTokenauth. Class A only (release-namespace containers plus release-scoped resource-monitor).telemetryCollector.enableddefaults false; templates nil-guard--reuse-valuesupgrades.tracebloc.nodeAgentsInUsenow gates namespace, pull Secret, auto-upgrade, and image-refresh so Collector-only installs still get RBAC and the namespace. jobs-manager gets token Secret coordinates and a namespaced Role withcreate+ name-scopedget/patch. OpenShift gets a separate SCC (RunAsAny, hostPath) rather than widening resource-monitor’s.Windows installer.
scripts/lib/telemetry.ps1is fetched and signed with the bootstrap;install-k8s.ps1records exit codes, phases, skip/handoff, and emits one fail-soft JSONL event fromfinally. Drift guards cover Class A globs, offset persistence, SCC coverage, tenancy, namespace safety, and token wiring.Reviewed by Cursor Bugbot for commit f4cccd1. Bugbot is set up for automated code reviews on this repo. Configure here.