From 1ef5011934219ea65d9181da8f85cac0b20600b9 Mon Sep 17 00:00:00 2001 From: Konstantins Feofantovs Date: Thu, 27 Aug 2026 22:33:34 +0300 Subject: [PATCH] fix: env, args and command never enter the informer cache (ADR 0046) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Invariant 4 ranks "not collected" above "collected and dropped", and security.md §8 promised env, args and command were "discarded before anything is stored". They were dropped at the payload seam (ADR 0031 §7) — upstream of it, every Pod, ReplicaSet, Deployment, StatefulSet, DaemonSet, Job and CronJob in the cluster sat in memory exactly as the API server returned it. - One transform on the factory, so it applies to every informer: env, envFrom, args and command go from every container list of every pod spec an object carries, including init and ephemeral containers and every workload template. - managedFields and kubectl.kubernetes.io/last-applied-configuration go from every object of every kind. The annotation is a verbatim copy of the object as applied, so stripping containers alone would have moved the values rather than dropped them. - Not done: informers are not namespace-scoped. The namespace filter supports a deny-list, and the opt-out annotations live on objects the agent would then stop watching. Measured on kind 1.36.1 against /api/v1/pods, which is what a watch delivers and not what `kubectl get -o json` shows: 9 pods, 77,463 B as received, 42,634 B after the transform. managedFields alone was 39% of it. Negative checks: unwiring the transform fails the lister test with the env value printed; removing the CronJob case fails the per-kind test. A 75 s run against the kind cluster reports 0 errors, 9 pods observed, workload_not_cached 0, and all four usage signals — the owner chain resolves through the stripped objects. §8 shrank by the lines it grew, per ADR 0044. Co-Authored-By: Claude Opus 5 (1M context) --- .../0031-placement-constraints-are-reduced.md | 2 +- docs/adr/0046-the-cache-is-the-source.md | 102 ++++++++++ docs/adr/README.md | 1 + docs/security.md | 20 +- internal/collector/cachetransform.go | 65 +++++++ internal/collector/cachetransform_test.go | 179 ++++++++++++++++++ internal/collector/pods.go | 3 +- 7 files changed, 361 insertions(+), 11 deletions(-) create mode 100644 docs/adr/0046-the-cache-is-the-source.md create mode 100644 internal/collector/cachetransform.go create mode 100644 internal/collector/cachetransform_test.go diff --git a/docs/adr/0031-placement-constraints-are-reduced.md b/docs/adr/0031-placement-constraints-are-reduced.md index 0010867..e956d76 100644 --- a/docs/adr/0031-placement-constraints-are-reduced.md +++ b/docs/adr/0031-placement-constraints-are-reduced.md @@ -3,7 +3,7 @@ Date: 2026-08-24 Status: Accepted -Amended by: 0032 +Amended by: 0032, 0046 Adds a `placement` block to the pod scope of `workload_metadata`. No new payload kind, no new informer, no new RBAC. diff --git a/docs/adr/0046-the-cache-is-the-source.md b/docs/adr/0046-the-cache-is-the-source.md new file mode 100644 index 0000000..d672308 --- /dev/null +++ b/docs/adr/0046-the-cache-is-the-source.md @@ -0,0 +1,102 @@ +# 0046. The informer cache is the source, so the fields the agent promises not to collect are removed there + +Date: 2026-08-27 + +Status: Accepted + +Amends: 0031 §7 + +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. + +## Context + +CLAUDE.md invariant 4 orders the outcomes: not collected beats collected and +dropped, which beats collected and not sent. `docs/security.md` §8 states the +promise as "`env`, `args` and `command` are discarded before anything is stored +or transported", and ADR 0031 §7 backs it with a test on the encoded payload +bytes. + +The payload was never the first place the fields existed. The controller built +its factory as `informers.NewSharedInformerFactory(clientset, 0)` — no +transform, no field selector — so every Pod, ReplicaSet, Deployment, +StatefulSet, DaemonSet, Job and CronJob in the cluster sat in memory as the API +server returned it, for the lifetime of the process. Nothing read those fields. +They were simply kept, which is the middle outcome the invariant ranks second. + +Three things ride along with them: + +- `kubectl.kubernetes.io/last-applied-configuration` is a verbatim copy of the + object as applied. Removing `env` from a container while leaving this + annotation moves the values rather than dropping them. +- `managedFields` is read by nothing here, and is larger than it looks: + `kubectl get -o json` has hidden it since 1.21, so the objects an informer + actually receives are not the objects an operator inspects. +- A field selector cannot help. The pod field selectors the API server supports + are `spec.nodeName`, `status.phase` and the name and namespace — none of the + fields at issue. + +Measured on kind (Kubernetes 1.36.1, 9 pods), reading `/api/v1/pods` rather +than through `kubectl`, so the bytes are the ones a watch delivers: + + all pods, as received 77,463 B + without managedFields 46,917 B + without env / args / command 73,180 B + without both 42,634 B + +45% of what the pod cache held was material nothing reads, against a container +limit of 256Mi. + +## Decision + +**1. One transform, registered on the factory, applies to every informer.** +`informers.WithTransform` runs before an object is stored, which for a watch-fed +cache is the earliest point the agent controls. That is what "filter early" +means here; the payload test of ADR 0031 §7 stays where it is, now as the second +of two checks rather than the only one. + +**2. It removes, from every container list of every pod spec an object +carries** — a Pod's own containers, init containers and ephemeral containers, +and the template inside a ReplicaSet, Deployment, StatefulSet, DaemonSet, Job or +CronJob — `env`, `envFrom`, `args` and `command`. + +**3. It removes `managedFields` and the applied-configuration annotation from +every object, whatever its kind**, including the ones with no pod spec at all. +Both are unread, and a kind added to the factory later gets this much without +anyone remembering to ask for it. + +**4. The number of cached objects is not reduced, only their size.** Scoping the +informers to the configured namespaces would cut more, and is not done here: the +namespace filter supports a deny-list as well as an allow-list, and the +annotation opt-outs are properties of objects the agent would then no longer be +watching. A watch topology that depends on the filter's shape is a bigger +decision than this one. + +## Consequences + +**Easier.** The promise in `docs/security.md` §8 is now true of the process and +not only of the payload: a heap dump of the controller does not contain a +customer's environment variables, and neither does a core file, a debugger, or +anything else that reads its memory. The pod cache is 45% smaller on the +measurement above. + +**Harder / given up.** Every cached object is now a *modified* copy of what the +API server holds, so a future feature that needs one of these fields cannot read +it from the lister and will find the absence at runtime rather than in review. +That is the intended direction — a field this agent must not collect should be +inconvenient to reach — but it is a real edge, and the transform is the one +place to look when a field is unexpectedly empty. + +The transform is a type switch, so a workload kind added to the factory without +a case keeps its template's `env`. The test enumerates the kinds and fails on +one that is not stripped; enumeration is what a test can do here, and it does +not know about a kind nobody added to it. + +**Not changed.** No payload, no golden file, no RBAC grant, no chart value, and +no field that was collected before is missing from anything shipped. The two +paths that read pod specs — the payload builders and the profile join — read +fields this transform does not touch. + +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 993d2bc..c3cea91 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -103,3 +103,4 @@ line to paste. - [0043. Both halves of the counter reading come from one observation](0043-the-counter-reading-comes-from-one-observation.md) - [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) diff --git a/docs/security.md b/docs/security.md index d2a0665..4144889 100644 --- a/docs/security.md +++ b/docs/security.md @@ -521,9 +521,14 @@ section describes each in detail. carries a cloud account identifier, a region, your repository taxonomy, and whatever your CI writes into a tag. - From pod specs: `metadata`, `spec.containers[].resources` and `[].ports`, the - nine placement fields below, `ownerReferences`, and `status`. `env`, `args` and - `command` are discarded before anything is stored or transported, because they - frequently contain inline credentials. From `status`: the image digest, the + nine placement fields below, `ownerReferences`, and `status`. `env`, `envFrom`, + `args` and `command` are removed as each object arrives, before it enters the + agent's cache, because they frequently contain inline credentials — so they are + not held in memory either, not merely kept out of payloads + ([ADR 0046](adr/0046-the-cache-is-the-source.md)). The + `kubectl.kubernetes.io/last-applied-configuration` annotation is dropped with + them: it is a verbatim copy of the object as applied, environment included. + From `status`: the image digest, the restart counter, a terminated state's reason and exit code, the reason and transition time of exactly two conditions — `PodScheduled` and `DisruptionTarget` — and `status.reason` when it says `Evicted`. Nothing else. @@ -544,13 +549,10 @@ section describes each in detail. bounded at 128 bytes and lists at fixed counts, and anything past a bound is dropped whole rather than truncated. What was dropped is counted and never named. From `volumes` only `persistentVolumeClaim` names are read - ([ADR 0032](adr/0032-cluster-policy.md)) — a bound claim on a zonal volume pins - its pod to that zone, which no field of the spec states; a Secret, ConfigMap or - projected volume is skipped without its name being touched. + ([ADR 0032](adr/0032-cluster-policy.md)); a Secret, ConfigMap or projected + volume is skipped without its name being touched. - From ReplicaSets: `metadata`, the replica counts, and from `spec.template` - exactly one field per container — the image reference. Not `env`, not `args`, - not `command`, and it is checked against the encoded bytes, so a field added - later that carries the template through fails a test + exactly one field per container — the image reference ([ADR 0030](adr/0030-deployment-revisions.md)). - From Jobs: `metadata`, the terminal condition's type, reason and transition time, the start and completion times, the succeeded and failed counts, and diff --git a/internal/collector/cachetransform.go b/internal/collector/cachetransform.go new file mode 100644 index 0000000..4062c27 --- /dev/null +++ b/internal/collector/cachetransform.go @@ -0,0 +1,65 @@ +package collector + +import ( + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" +) + +// lastAppliedAnnotation holds a verbatim copy of the object as applied, env +// and all, so stripping a container's env without stripping this would move +// the values rather than drop them. +const lastAppliedAnnotation = "kubectl.kubernetes.io/last-applied-configuration" + +// dropUncollectedFields is where "filter early" is enforced for everything the +// controller watches: the informer cache is the source, and a field removed +// here is never held, not merely never shipped (CLAUDE.md invariant 4, +// ADR 0046). +// +// It runs on every object entering every cache, so a kind it does not name +// still loses its managed fields and its applied-configuration copy. +func dropUncollectedFields(obj any) (any, error) { + if m, err := meta.Accessor(obj); err == nil { + m.SetManagedFields(nil) + delete(m.GetAnnotations(), lastAppliedAnnotation) + } + switch o := obj.(type) { + case *corev1.Pod: + dropPodSpecFields(&o.Spec) + case *appsv1.Deployment: + dropPodSpecFields(&o.Spec.Template.Spec) + case *appsv1.StatefulSet: + dropPodSpecFields(&o.Spec.Template.Spec) + case *appsv1.DaemonSet: + dropPodSpecFields(&o.Spec.Template.Spec) + case *appsv1.ReplicaSet: + dropPodSpecFields(&o.Spec.Template.Spec) + case *batchv1.Job: + dropPodSpecFields(&o.Spec.Template.Spec) + case *batchv1.CronJob: + dropPodSpecFields(&o.Spec.JobTemplate.Spec.Template.Spec) + } + return obj, nil +} + +// dropPodSpecFields clears the three fields the agent promises never to +// collect, in every container list a pod spec has. +func dropPodSpecFields(spec *corev1.PodSpec) { + for i := range spec.Containers { + dropContainerFields(&spec.Containers[i]) + } + for i := range spec.InitContainers { + dropContainerFields(&spec.InitContainers[i]) + } + for i := range spec.EphemeralContainers { + dropContainerFields((*corev1.Container)(&spec.EphemeralContainers[i].EphemeralContainerCommon)) + } +} + +func dropContainerFields(c *corev1.Container) { + c.Env = nil + c.EnvFrom = nil + c.Command = nil + c.Args = nil +} diff --git a/internal/collector/cachetransform_test.go b/internal/collector/cachetransform_test.go new file mode 100644 index 0000000..4dc4555 --- /dev/null +++ b/internal/collector/cachetransform_test.go @@ -0,0 +1,179 @@ +package collector + +import ( + "context" + "reflect" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +// secretish is what makes this an invariant and not housekeeping: env values +// are where connection strings and tokens are written by hand. +// #nosec G101 -- a fixture, and the scanner reading it that way is the point +const secretish = "postgres://svc:hunter2@db.internal/prod" + +func loadedContainer(name string) corev1.Container { + return corev1.Container{ + Name: name, + Image: "example.com/app:1.2.3", + Command: []string{"/bin/sh", "-c"}, + Args: []string{"exec /app --token=t0ps3cret"}, + Env: []corev1.EnvVar{{Name: "DATABASE_URL", Value: secretish}}, + EnvFrom: []corev1.EnvFromSource{{ + SecretRef: &corev1.SecretEnvSource{LocalObjectReference: corev1.LocalObjectReference{Name: "db"}}, + }}, + } +} + +func loadedTemplate() corev1.PodTemplateSpec { + return corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{loadedContainer("app")}}, + } +} + +func loadedMeta(name string) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Namespace: "shop", + Name: name, + Annotations: map[string]string{ + lastAppliedAnnotation: `{"spec":{"containers":[{"env":[{"name":"DATABASE_URL","value":"` + secretish + `"}]}]}}`, + CollectAnnotation: "false", + }, + ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "kubectl", Operation: metav1.ManagedFieldsOperationApply}}, + } +} + +// containersIn returns every container an object holds, wherever the API +// buries it: a pod's own three lists, a workload's template, a CronJob's +// template of a template. +func containersIn(obj any) []corev1.Container { + var out []corev1.Container + var walk func(v reflect.Value) + walk = func(v reflect.Value) { + switch v.Kind() { + case reflect.Pointer, reflect.Interface: + if !v.IsNil() { + walk(v.Elem()) + } + case reflect.Slice, reflect.Array: + for i := range v.Len() { + walk(v.Index(i)) + } + case reflect.Struct: + switch c := v.Interface().(type) { + case corev1.Container: + out = append(out, c) + return + case corev1.EphemeralContainer: + out = append(out, corev1.Container(c.EphemeralContainerCommon)) + return + } + for i := range v.NumField() { + if v.Type().Field(i).IsExported() { + walk(v.Field(i)) + } + } + } + } + walk(reflect.ValueOf(obj)) + return out +} + +// "Not collected" beats "collected and dropped": the cache is the source, so +// the fields have to go before the object is stored, not before it is +// serialized (CLAUDE.md invariant 4, ADR 0046). +func TestNoWatchedKindEntersTheCacheCarryingEnvArgsOrCommand(t *testing.T) { + podWithEphemeral := &corev1.Pod{ObjectMeta: loadedMeta("web")} + podWithEphemeral.Spec.Containers = []corev1.Container{loadedContainer("app")} + podWithEphemeral.Spec.InitContainers = []corev1.Container{loadedContainer("init")} + podWithEphemeral.Spec.EphemeralContainers = []corev1.EphemeralContainer{{ + EphemeralContainerCommon: corev1.EphemeralContainerCommon(loadedContainer("debug")), + }} + + objects := []any{ + podWithEphemeral, + &appsv1.Deployment{ObjectMeta: loadedMeta("web"), Spec: appsv1.DeploymentSpec{Template: loadedTemplate()}}, + &appsv1.StatefulSet{ObjectMeta: loadedMeta("db"), Spec: appsv1.StatefulSetSpec{Template: loadedTemplate()}}, + &appsv1.DaemonSet{ObjectMeta: loadedMeta("logs"), Spec: appsv1.DaemonSetSpec{Template: loadedTemplate()}}, + &appsv1.ReplicaSet{ObjectMeta: loadedMeta("web-abc"), Spec: appsv1.ReplicaSetSpec{Template: loadedTemplate()}}, + &batchv1.Job{ObjectMeta: loadedMeta("nightly"), Spec: batchv1.JobSpec{Template: loadedTemplate()}}, + &batchv1.CronJob{ObjectMeta: loadedMeta("nightly"), Spec: batchv1.CronJobSpec{ + JobTemplate: batchv1.JobTemplateSpec{Spec: batchv1.JobSpec{Template: loadedTemplate()}}, + }}, + &corev1.Namespace{ObjectMeta: loadedMeta("shop")}, + } + + for _, obj := range objects { + kind := reflect.TypeOf(obj).Elem().Name() + if len(containersIn(obj)) == 0 && kind != "Namespace" { + t.Fatalf("%s: the fixture carries no containers, so this row proves nothing", kind) + } + + stored, err := dropUncollectedFields(obj) + if err != nil { + t.Fatalf("%s: %v", kind, err) + } + for _, c := range containersIn(stored) { + if c.Env != nil || c.EnvFrom != nil || c.Command != nil || c.Args != nil { + t.Errorf("%s: container %q reached the cache with env=%v envFrom=%v command=%v args=%v", + kind, c.Name, c.Env, c.EnvFrom, c.Command, c.Args) + } + if c.Image == "" || c.Name == "" { + t.Errorf("%s: container %q lost a field the payloads are built from", kind, c.Name) + } + } + + meta := reflect.ValueOf(stored).Elem().FieldByName("ObjectMeta").Interface().(metav1.ObjectMeta) + if _, ok := meta.Annotations[lastAppliedAnnotation]; ok { + t.Errorf("%s: the applied-configuration copy is still there, env and all", kind) + } + if meta.Annotations[CollectAnnotation] != "false" { + t.Errorf("%s: the opt-out annotation was dropped with it; the filter reads that one", kind) + } + if meta.ManagedFields != nil { + t.Errorf("%s: managed fields are cached and nothing reads them", kind) + } + } +} + +// The transform is only worth as much as its wiring: a function that strips +// every field and is registered with nothing passes the test above. +func TestTheStrippedObjectIsWhatTheListersServe(t *testing.T) { + p := pod("web", nil) + p.Spec.Containers[0].Env = []corev1.EnvVar{{Name: "DATABASE_URL", Value: secretish}} + p.Spec.Containers[0].Args = []string{"--token=t0ps3cret"} + p.Annotations = map[string]string{lastAppliedAnnotation: secretish} + + watcher := NewPodWatcher(fake.NewClientset(p), func(PodInfo) {}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { _ = watcher.Run(ctx) }() + + deadline := time.After(5 * time.Second) + for { + cached, err := watcher.podLister.Pods("shop").Get("web") + if err == nil { + if got := cached.Spec.Containers[0]; got.Env != nil || got.Args != nil { + t.Errorf("the cache holds env=%v args=%v for %q", got.Env, got.Args, got.Name) + } + if _, ok := cached.Annotations[lastAppliedAnnotation]; ok { + t.Error("the cache holds the applied-configuration copy") + } + if cached.Spec.Containers[0].Image == "" { + t.Error("the transform is wired but takes too much: the image is gone") + } + return + } + select { + case <-deadline: + t.Fatalf("the pod never reached the cache: %v", err) + case <-time.After(20 * time.Millisecond): + } + } +} diff --git a/internal/collector/pods.go b/internal/collector/pods.go index 00d9a79..af403a0 100644 --- a/internal/collector/pods.go +++ b/internal/collector/pods.go @@ -242,7 +242,8 @@ func NewPodWatcher(clientset kubernetes.Interface, onPod func(PodInfo)) *PodWatc // on its own goroutine and calls ReplicaSets, WorkloadPolicies and // ClusterPolicy while Run is still starting. Assigning the lister fields // inside Run would be a write racing those reads. - factory := informers.NewSharedInformerFactory(clientset, 0) + factory := informers.NewSharedInformerFactoryWithOptions(clientset, 0, + informers.WithTransform(dropUncollectedFields)) w := &PodWatcher{ clientset: clientset, onPod: onPod,