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,