From 51a8e6374300a9b5a486900ab648c6e7b9e88400 Mon Sep 17 00:00:00 2001 From: Konstantins Feofantovs Date: Fri, 28 Aug 2026 18:55:14 +0300 Subject: [PATCH] feat: five runtime knobs are named, kept and shipped (ADR 0047) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0046 dropped `env` whole, and took GOMAXPROCS and GOMEMLIMIT with it — the two settings behind the most common waste in Go on Kubernetes, and stated nowhere else. The module list can show `automaxprocs` is absent; it cannot show what was set instead. - GOMAXPROCS, GOGC, GOMEMLIMIT, GODEBUG, GOTRACEBACK are kept at the same informer transform. Every other variable is still removed before the object is cached, and so are envFrom, args and command. - A name is not a promise about the value: one of the five sourced from a secretKeyRef or configMapKeyRef is dropped anyway. - One read from the container's own limits — the canonical way to tie GOMAXPROCS to a CPU limit — ships as the field path (`resource:limits.cpu`), because the spec holds no value until the kubelet resolves it. - They ship in `workload_metadata` as `runtime_env`. Keeping a field the payloads do not carry would be collection without a consumer, which ADR 0046 refuses. Negative check: keeping the allow-listed names regardless of where the value comes from fails with `kept [GOGC GOTRACEBACK GOMAXPROCS GOMEMLIMIT], want [GOMAXPROCS GOMEMLIMIT]`. Golden updated; §8 stayed at its ceiling by shedding the lines it grew. Co-Authored-By: Claude Opus 5 (1M context) --- docs/adr/0046-the-cache-is-the-source.md | 2 + .../0047-runtime-knobs-are-named-and-kept.md | 92 +++++++++++++++++++ docs/adr/README.md | 1 + docs/security.md | 32 ++++--- internal/collector/cachetransform.go | 37 +++++++- internal/collector/cachetransform_test.go | 55 ++++++++++- internal/collector/pods.go | 39 +++++++- internal/metadata/workload.go | 18 ++-- internal/sink/spool_test.go | 9 ++ .../testdata/workload-metadata.golden.json | 4 + 10 files changed, 262 insertions(+), 27 deletions(-) create mode 100644 docs/adr/0047-runtime-knobs-are-named-and-kept.md diff --git a/docs/adr/0046-the-cache-is-the-source.md b/docs/adr/0046-the-cache-is-the-source.md index d672308..07b1ff8 100644 --- a/docs/adr/0046-the-cache-is-the-source.md +++ b/docs/adr/0046-the-cache-is-the-source.md @@ -6,6 +6,8 @@ Status: Accepted Amends: 0031 §7 +Amended by: 0047 + Moves the drop of `env`, `args` and `command` from the payload seam to the informer's transform, so the values are never held rather than never shipped. No payload changes. diff --git a/docs/adr/0047-runtime-knobs-are-named-and-kept.md b/docs/adr/0047-runtime-knobs-are-named-and-kept.md new file mode 100644 index 0000000..de17dd0 --- /dev/null +++ b/docs/adr/0047-runtime-knobs-are-named-and-kept.md @@ -0,0 +1,92 @@ +# 0047. Five environment variables are named, kept and shipped; every other one is still dropped + +Date: 2026-08-28 + +Status: Accepted + +Amends: 0046 §2 + +Narrows ADR 0046's blanket drop of `env` to a closed list of Go runtime knobs, +and ships them in `workload_metadata`. `envFrom`, `args` and `command` are +unchanged, and so is every variable not on the list. + +## Context + +ADR 0046 removed `env` at the informer transform, which was right for the reason +it gave: environment values are where connection strings and tokens get written +by hand. It removed something else along with them. + +`GOMAXPROCS` left unset under a CPU limit is the most common waste in Go on +Kubernetes: the runtime counts the node's cores, not the cgroup's quota, so a +container limited to half a core starts as many scheduler threads as the machine +has CPUs and spends its quota being descheduled. `GOMEMLIMIT` unset under a +memory limit is the second: the collector has no idea a ceiling exists, so the +container is killed where the garbage collector would have run. Both are stated +entirely in the environment. Neither is visible anywhere else — the module list +can show that `go.uber.org/automaxprocs` is absent, which is evidence that the +default was *not* fixed, but not evidence about what was set instead. + +Both are readable from a single pass over the cluster, so a report that names +them needs no profiling and no accumulated history. + +There is a way to get this wrong that looks reasonable: keep `env` and let the +payload builder pick. That is the shape ADR 0046 exists to refuse — the values +would be held in the cache for the lifetime of the process and dropped at +serialization, which is the outcome CLAUDE.md invariant 4 ranks second. + +## Decision + +**1. A closed list of names, applied at the same transform.** `GOMAXPROCS`, +`GOGC`, `GOMEMLIMIT`, `GODEBUG`, `GOTRACEBACK`. Everything else is removed before +the object is cached, exactly as ADR 0046 has it. The list is in code, not in +configuration: a customer-editable list of variables to collect would be a +setting whose wrong value is a disclosure. + +**2. A name is not a promise about the value's origin.** A variable called +`GOGC` whose value comes from a `secretKeyRef` or a `configMapKeyRef` is a read +of a Secret or a ConfigMap, which the agent does not do (invariant 4, and it +holds no RBAC for either). Those are dropped whatever their name. + +**3. A knob read from the container's own limits is kept as the field path it +reads.** `valueFrom.resourceFieldRef` is the canonical way to set `GOMAXPROCS` +from a CPU limit, and it is the *good* configuration this finding is looking +for. The spec holds no value in that case — the kubelet resolves it at start — +so the payload carries `resource:limits.cpu` rather than a number. Naming the +source answers the question the finding asks: does this knob follow the limit? + +**4. `envFrom` stays dropped entirely.** It imports a Secret or ConfigMap whole. +There is no name to filter on before the import happens, so there is no version +of this decision that applies to it. + +**5. The values ship.** `workload_metadata` records carry `runtime_env`, absent +when nothing on the list is set — which is the state most of these findings are +about. Keeping a field in the cache that no payload carries would be collection +without a consumer, which is the thing this ADR was careful not to do. + +## Consequences + +**Easier.** The first-hour report can state the two findings above with the +configuration in hand rather than inferring it from a missing module. `GODEBUG` +and `GOTRACEBACK` come along because they are the same class of fact and cost +nothing extra: a fleet-wide view of which `GODEBUG` switches are set is +occasionally the whole explanation for a performance question. + +**Harder / given up.** The promise in `docs/security.md` is no longer "no +environment variable is read". It is now a list, and a list is a thing a reader +must check rather than a thing they can take on faith. That is a real reduction +in how easy the guarantee is to verify, bought for a finding we believe is worth +it. The mitigation is that the list is short, is in one function, and is +enumerated in the customer document by name. + +A value on this list is still operator-authored free text. `GODEBUG` in +particular is a comma-separated set of switches, and nothing stops someone +writing something else into it. The agent does not bound or truncate these +values, which is the same treatment the image reference already gets and is +called out for (ADR 0039). + +**Not changed.** `args`, `command` and `envFrom` are dropped as before, and so is +every variable whose name is not one of the five. No new RBAC, no new watch, no +new privilege: this is a narrowing of a filter the agent already runs. + +This ADR records decisions implemented in the same pull request, per the process +in [`README.md`](README.md). diff --git a/docs/adr/README.md b/docs/adr/README.md index c3cea91..89613e0 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -104,3 +104,4 @@ line to paste. - [0044. One fact in one place, and the places have ceilings](0044-prose-has-a-ceiling.md) - [0045. A kubelet read is bounded in time and in size, and one node's silence costs one node](0045-a-kubelet-read-is-bounded.md) - [0046. The informer cache is the source, so the fields the agent promises not to collect are removed there](0046-the-cache-is-the-source.md) +- [0047. Five environment variables are named, kept and shipped; every other one is still dropped](0047-runtime-knobs-are-named-and-kept.md) diff --git a/docs/security.md b/docs/security.md index 4144889..11d8b89 100644 --- a/docs/security.md +++ b/docs/security.md @@ -464,7 +464,7 @@ VCS hostname and the layout of your source tree. Line numbers are kept. ## 8. Data collected and data leaving the cluster -This is usually the most important section for review. Data falls into three +This is usually the most important section for review. Data falls into four classes with different sensitivity and different default policies: | Data class | Examples | Sensitivity | Default policy | @@ -491,7 +491,7 @@ appearing here (ADR 0022). | `pod_disruptions` | Per hour: the pods the *cluster* removed — preempted, evicted under node pressure, drained, or removed via the eviction API — with the node and the instant | **yes** | | `job_runs` | Per hour: each Job that finished — when it started and finished, whether it succeeded, the failure reason, its pod success/failure counts, and its declared `parallelism`/`completions`/`backoffLimit` | **yes** | | `deployment_revisions` | Per flush: each ReplicaSet of a collected Deployment — its revision number, when it was created, its desired/current/ready replica counts, and each container's image reference | **yes** | -| `workload_metadata` | Declared shape per (namespace, workload, container, image digest): image, requests, limits, ports, QoS, replica counts by phase, by node, and by unscheduled reason, and the pod's reduced placement constraints (ADR 0031) | no | +| `workload_metadata` | Declared shape per (namespace, workload, container, image digest): image, requests, limits, ports, the five runtime knobs of the field-minimization list below, QoS, replica counts by phase, by node, and by unscheduled reason, and the pod's reduced placement constraints (ADR 0031) | no | | `workload_policy` | Per collected workload, what bounds it from outside its own spec: disruption budgets covering it, autoscalers driving it, and the volume claims its pods mount (ADR 0032); plus `unavailable_sources`, naming any of those the agent could not read — never granted, or no longer being fed (ADR 0033, ADR 0035) | not directly — but a claim is named, and a StatefulSet's claim is `