diff --git a/README.md b/README.md index a90d997..c029f19 100644 --- a/README.md +++ b/README.md @@ -66,14 +66,15 @@ $ go run main.go approvals/kir_test.TestKind.Job.input.yaml | xargs docker scout ## How `kir` treats each document -A manifest stream usually mixes workloads with other objects. `kir` handles each by kind: +A manifest stream usually mixes workloads with other objects. `kir` handles each by what it contains, not by its kind: | Document | Result | | --- | --- | -| A workload — `Pod`, `Deployment`, `DaemonSet`, `ReplicaSet`, `StatefulSet`, `Job`, `CronJob` | its images are printed to stdout | -| A valid object with no images — `Service`, `ConfigMap`, `Secret`, … | skipped silently (exit 0) | +| Anything containing a `PodSpec` — `Pod`, `Deployment`, …, `CronJob`, and custom resources like an Argo `Rollout` | its images are printed to stdout | +| A valid object with no `PodSpec` — `Service`, `ConfigMap`, `Secret`, … | skipped silently (exit 0) | | Malformed or unreadable input | reported on stderr, non-zero exit | | A workload whose image value isn't a valid image reference | that image is reported on stderr with a non-zero exit; the document's other images are still printed | -| An unrecognized custom resource (CRD) | skipped for now — see [#75](https://github.com/MPV/kir/issues/75) | + +There is no list of supported kinds. A document yields images if it holds something shaped like a `PodSpec` — matched by decoding it against the Kubernetes API types — so a custom resource that embeds one works without `kir` knowing anything about it. So stdout carries only images and stderr stays quiet for normal input. See [ADR 0007](docs/adr/0007-document-classification.md) for the rationale. diff --git a/approvals/kir_test.TestCustomResource.exitcode.approved.txt b/approvals/kir_test.TestCustomResource.exitcode.approved.txt new file mode 100644 index 0000000..573541a --- /dev/null +++ b/approvals/kir_test.TestCustomResource.exitcode.approved.txt @@ -0,0 +1 @@ +0 diff --git a/approvals/kir_test.TestCustomResource.input.yaml b/approvals/kir_test.TestCustomResource.input.yaml new file mode 100644 index 0000000..0942193 --- /dev/null +++ b/approvals/kir_test.TestCustomResource.input.yaml @@ -0,0 +1,18 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: my-rollout +spec: + replicas: 3 + strategy: + canary: + steps: + - setWeight: 20 + template: + spec: + containers: + - name: app + image: my-registry/app:1.4.2 + initContainers: + - name: setup + image: busybox:1.36 diff --git a/approvals/kir_test.TestCustomResource.stderr.approved.txt b/approvals/kir_test.TestCustomResource.stderr.approved.txt new file mode 100644 index 0000000..e69de29 diff --git a/approvals/kir_test.TestCustomResource.stdout.approved.txt b/approvals/kir_test.TestCustomResource.stdout.approved.txt new file mode 100644 index 0000000..0815858 --- /dev/null +++ b/approvals/kir_test.TestCustomResource.stdout.approved.txt @@ -0,0 +1,2 @@ +my-registry/app:1.4.2 +busybox:1.36 diff --git a/approvals/kir_test.TestFailure.BadYAML.stderr.approved.txt b/approvals/kir_test.TestFailure.BadYAML.stderr.approved.txt index 49591ae..d43cd99 100644 --- a/approvals/kir_test.TestFailure.BadYAML.stderr.approved.txt +++ b/approvals/kir_test.TestFailure.BadYAML.stderr.approved.txt @@ -1 +1 @@ -error: yaml: line 9: did not find expected ',' or ']' +error: error converting YAML to JSON: yaml: line 9: did not find expected ',' or ']' diff --git a/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt b/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt index 49591ae..d43cd99 100644 --- a/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt +++ b/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt @@ -1 +1 @@ -error: yaml: line 9: did not find expected ',' or ']' +error: error converting YAML to JSON: yaml: line 9: did not find expected ',' or ']' diff --git a/approvals/kir_test.TestKind.PodTemplate.exitcode.approved.txt b/approvals/kir_test.TestKind.PodTemplate.exitcode.approved.txt new file mode 100644 index 0000000..573541a --- /dev/null +++ b/approvals/kir_test.TestKind.PodTemplate.exitcode.approved.txt @@ -0,0 +1 @@ +0 diff --git a/approvals/kir_test.TestKind.PodTemplate.input.yaml b/approvals/kir_test.TestKind.PodTemplate.input.yaml new file mode 100644 index 0000000..09b29a7 --- /dev/null +++ b/approvals/kir_test.TestKind.PodTemplate.input.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: PodTemplate +metadata: + name: tmpl +template: + spec: + containers: + - name: worker + image: worker:3.1 diff --git a/approvals/kir_test.TestKind.PodTemplate.stderr.approved.txt b/approvals/kir_test.TestKind.PodTemplate.stderr.approved.txt new file mode 100644 index 0000000..e69de29 diff --git a/approvals/kir_test.TestKind.PodTemplate.stdout.approved.txt b/approvals/kir_test.TestKind.PodTemplate.stdout.approved.txt new file mode 100644 index 0000000..94a335b --- /dev/null +++ b/approvals/kir_test.TestKind.PodTemplate.stdout.approved.txt @@ -0,0 +1 @@ +worker:3.1 diff --git a/approvals/kir_test.TestKind.ReplicationController.exitcode.approved.txt b/approvals/kir_test.TestKind.ReplicationController.exitcode.approved.txt new file mode 100644 index 0000000..573541a --- /dev/null +++ b/approvals/kir_test.TestKind.ReplicationController.exitcode.approved.txt @@ -0,0 +1 @@ +0 diff --git a/approvals/kir_test.TestKind.ReplicationController.input.yaml b/approvals/kir_test.TestKind.ReplicationController.input.yaml new file mode 100644 index 0000000..b24833e --- /dev/null +++ b/approvals/kir_test.TestKind.ReplicationController.input.yaml @@ -0,0 +1,11 @@ +apiVersion: v1 +kind: ReplicationController +metadata: + name: legacy +spec: + replicas: 2 + template: + spec: + containers: + - name: web + image: nginx:1.27 diff --git a/approvals/kir_test.TestKind.ReplicationController.stderr.approved.txt b/approvals/kir_test.TestKind.ReplicationController.stderr.approved.txt new file mode 100644 index 0000000..e69de29 diff --git a/approvals/kir_test.TestKind.ReplicationController.stdout.approved.txt b/approvals/kir_test.TestKind.ReplicationController.stdout.approved.txt new file mode 100644 index 0000000..20a9c2f --- /dev/null +++ b/approvals/kir_test.TestKind.ReplicationController.stdout.approved.txt @@ -0,0 +1 @@ +nginx:1.27 diff --git a/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.exitcode.approved.txt b/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.exitcode.approved.txt new file mode 100644 index 0000000..573541a --- /dev/null +++ b/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.exitcode.approved.txt @@ -0,0 +1 @@ +0 diff --git a/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.input.yaml b/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.input.yaml new file mode 100644 index 0000000..f66e324 --- /dev/null +++ b/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.input.yaml @@ -0,0 +1,9 @@ +apiVersion: logistics.example.com/v1 +kind: ShippingManifest +metadata: + name: not-a-pod +spec: + containers: + - name: cargo-hold-1 + capacity: 40ft + image: photo-of-container.jpg diff --git a/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.stderr.approved.txt b/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.stderr.approved.txt new file mode 100644 index 0000000..e69de29 diff --git a/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.stdout.approved.txt b/approvals/kir_test.TestSkipsNonWorkloads.Lookalike.stdout.approved.txt new file mode 100644 index 0000000..e69de29 diff --git a/approvals/kir_test.go b/approvals/kir_test.go index 04effb3..06fb405 100644 --- a/approvals/kir_test.go +++ b/approvals/kir_test.go @@ -42,7 +42,9 @@ func newlineTerminated(s string) string { } func TestKind(t *testing.T) { - kinds := []string{"Pod", "CronJob", "DaemonSet", "Deployment", "Job", "ReplicaSet", "StatefulSet"} + // PodTemplate and ReplicationController are built-in kinds carrying a + // PodSpec that the previous fixed kind list omitted. + kinds := []string{"Pod", "CronJob", "DaemonSet", "Deployment", "Job", "PodTemplate", "ReplicaSet", "ReplicationController", "StatefulSet"} for _, kind := range kinds { t.Run(kind, func(t *testing.T) { @@ -51,11 +53,23 @@ func TestKind(t *testing.T) { } } -// A non-workload kind (Service) is skipped: no images, no error, exit 0. +// A non-workload document is skipped: no images, no error, exit 0. Service is +// a built-in without a PodSpec; Lookalike is a custom resource with a field +// named containers holding something that is not a container, which pins the +// cost of matching on shape — a name alone must not be enough to match. func TestSkipsNonWorkloads(t *testing.T) { - t.Run("Service", func(t *testing.T) { - verify(t, []string{"kir_test.TestSkipsNonWorkloads.Service.input.yaml"}, nil) - }) + for _, name := range []string{"Service", "Lookalike"} { + t.Run(name, func(t *testing.T) { + verify(t, []string{"kir_test.TestSkipsNonWorkloads." + name + ".input.yaml"}, nil) + }) + } +} + +// The reach structural discovery buys: a custom resource the Kubernetes scheme +// cannot decode, whose embedded PodSpec is found anyway. Nothing in kir names +// the Rollout kind. +func TestCustomResource(t *testing.T) { + verify(t, []string{"kir_test.TestCustomResource.input.yaml"}, nil) } func TestMultiple(t *testing.T) { diff --git a/docs/adr/0009-podspec-discovery.md b/docs/adr/0009-podspec-discovery.md new file mode 100644 index 0000000..763086d --- /dev/null +++ b/docs/adr/0009-podspec-discovery.md @@ -0,0 +1,58 @@ +# 9. Find PodSpecs structurally, validated against the Kubernetes Go types + +- Status: **proposed** — one of four candidate answers to #26, supersedes [0001](0001-typed-kubernetes-decoding.md) if accepted +- Date: 2026-08-09 + +## Context + +[ADR 0001](0001-typed-kubernetes-decoding.md) decodes each document with the +typed client-go scheme and reads the PodSpec through a type switch over seven +hardcoded kinds. #26 asks for the PodSpec to be *found* rather than looked up, +so that custom resources embedding one work too. + +## Decision + +Stop decoding into typed Kubernetes objects. Decode each document into plain Go +values and walk it, testing each node for PodSpec shape. + +The shape test is the load-bearing part, and it is not a field-name heuristic: a +candidate's `containers` / `initContainers` / `ephemeralContainers` are decoded +into the real `corev1` types with unknown fields rejected. The Kubernetes Go +types are the schema. A node matches when Kubernetes itself would call it a +PodSpec. + +Documents are still required to carry a `kind`, which keeps kir aimed at +manifests rather than at any YAML containing something image-like. + +## Consequences + +Custom resources work — an Argo `Rollout` yields its images with nothing in kir +naming that kind — and so do the built-ins the old list omitted +(`ReplicationController`, `PodTemplate`). `List` stops being a special case: its +items are just more nodes, so the kind allow-list and the unstructured item +handling both go. + +Dropping typed decoding drops `k8s.io/client-go` entirely: the binary goes from +27.2 MB to 12.4 MB. `k8s.io/api` stays, as the schema. + +The costs, and they are real: + +- **Slower**, since every candidate node is decode-tested: 105 ms → 133 ms over + 1000 documents (~28 µs per document). Acceptable for a tool that shells out to + an image scanner afterwards. +- **Precision now rests on the strict decode.** A field named `containers` + holding anything else is rejected, and there is a golden fixture + (`TestSkipsNonWorkloads.Lookalike`) to keep it that way. But a custom resource + that inlines a PodSpec *alongside* its own fields would fail the strict decode + and be missed — the failure mode moves from "kind not listed" to "shape not + matched". +- **Bound to the vendored `k8s.io/api`**: a container field newer than the + vendored version fails the strict decode. Bumping the dependency is the fix, + and a stale bump is now a correctness issue rather than only a hygiene one. + In practice the binding is loose — the v0.32.3 → v0.36.3 bump moved no + goldens and needed no code change — but it is the thing to watch. + +Alternatives considered: reflecting over the typed scheme (option A) keeps +perfect precision but cannot see custom resources at all; a CUE schema (option +C) buys a user-editable schema for a large dependency; configurable per-kind +paths (option D) keep precision but push the work onto users. diff --git a/docs/adr/README.md b/docs/adr/README.md index 6dd9e53..78704fd 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -18,3 +18,4 @@ date the decision was actually made. | [0006](0006-conventional-commits-and-releases.md) | Automate releases from Conventional Commits | 2026-08-06 | | [0007](0007-document-classification.md) | How kir classifies each document (workload / image-less / unprocessable) | 2026-08-08 | | [0008](0008-best-effort-processing-and-exit-codes.md) | Best-effort processing; failures surface via the exit code | 2026-08-09 | +| [0009](0009-podspec-discovery.md) | Find PodSpecs structurally, validated against the Kubernetes Go types (proposed — #26) | 2026-08-09 | diff --git a/go.mod b/go.mod index bf15914..6b1d62a 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/distribution/reference v0.6.0 k8s.io/api v0.36.3 k8s.io/apimachinery v0.36.3 - k8s.io/client-go v0.36.3 + sigs.k8s.io/yaml v1.6.0 ) require ( @@ -29,5 +29,4 @@ require ( sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.3.3 // indirect - sigs.k8s.io/yaml v1.6.0 // indirect ) diff --git a/go.sum b/go.sum index c7f6c0f..8ce4518 100644 --- a/go.sum +++ b/go.sum @@ -60,8 +60,6 @@ k8s.io/api v0.36.3 h1:NxB+05W2UGqXWFXcLO0RB5cnqnUPP5v5sVlaOH0Iz4w= k8s.io/api v0.36.3/go.mod h1:JzLQKqRHC5+I8RVj/lS3lCg0mg6nWI9Fo/Sk3ElxHzg= k8s.io/apimachinery v0.36.3 h1:PkzMRBRG8joFD8EhCuQAtNPvJlxb82FwplP26HIzvAM= k8s.io/apimachinery v0.36.3/go.mod h1:cTSjBWgPe/6CQyBKzY/hDIRWCQQQeK0mfLbml0UYFHE= -k8s.io/client-go v0.36.3 h1:M4JdVzXxYcZk4fGpfDdYnxSwhLKWCFoQsHW6t+z8Hfg= -k8s.io/client-go v0.36.3/go.mod h1:gcPwr0c87vjjG6HB6pWEqOeuYVoXSsREjzux2j6GF30= k8s.io/klog/v2 v2.140.0 h1:Tf+J3AH7xnUzZyVVXhTgGhEKnFqye14aadWv7bzXdzc= k8s.io/klog/v2 v2.140.0/go.mod h1:o+/RWfJ6PwpnFn7OyAG3QnO47BFsymfEfrz6XyYSSp0= k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a h1:xCeOEAOoGYl2jnJoHkC3hkbPJgdATINPMAxaynU2Ovg= diff --git a/k8s/k8s.go b/k8s/k8s.go index 1e88e2f..6b8235a 100644 --- a/k8s/k8s.go +++ b/k8s/k8s.go @@ -1,54 +1,114 @@ +// Package k8s finds container images in Kubernetes manifests that have been +// decoded into plain Go values. package k8s import ( - "fmt" + "encoding/json" + "maps" + "slices" - appsv1 "k8s.io/api/apps/v1" - batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/yaml" ) -// GetPodSpec extracts the PodSpec from a Kubernetes object -func GetPodSpec(obj any) (*corev1.PodSpec, error) { - switch resource := obj.(type) { - case *corev1.Pod: - return &resource.Spec, nil - case *appsv1.Deployment: - return &resource.Spec.Template.Spec, nil - case *appsv1.DaemonSet: - return &resource.Spec.Template.Spec, nil - case *appsv1.ReplicaSet: - return &resource.Spec.Template.Spec, nil - case *appsv1.StatefulSet: - return &resource.Spec.Template.Spec, nil - case *batchv1.Job: - return &resource.Spec.Template.Spec, nil - case *batchv1.CronJob: - return &resource.Spec.JobTemplate.Spec.Template.Spec, nil - default: - return nil, fmt.Errorf("object does not have a PodSpec") +// containerFields are the PodSpec fields that carry images, listed in the order +// kir reports them. +var containerFields = []string{"containers", "initContainers", "ephemeralContainers"} + +// maxDepth bounds the walk. Real manifests nest a handful of levels; the limit +// only guards against absurd input. +const maxDepth = 100 + +// FindImages returns the images of every PodSpec-shaped node reachable from +// doc, a manifest decoded into plain Go values (maps, slices, scalars). +// +// Nothing here knows what a Deployment is. The walk descends until it meets a +// node shaped like a PodSpec, which is why a custom resource that embeds one is +// understood on the same footing as a built-in workload — and why a List needs +// no special case, its items being just more nodes. +func FindImages(doc any) []string { + var images []string + find(doc, &images, 0) + return images +} + +func find(node any, images *[]string, depth int) { + if depth > maxDepth { + return + } + + switch n := node.(type) { + case map[string]any: + if found, ok := podSpecImages(n); ok { + *images = append(*images, found...) + return // a PodSpec does not contain another PodSpec + } + // Sorted, so output order depends on the manifest rather than on Go's + // randomised map iteration. + for _, key := range slices.Sorted(maps.Keys(n)) { + find(n[key], images, depth+1) + } + case []any: + for _, item := range n { + find(item, images, depth+1) + } } } -func GetContainerImages(containers []corev1.Container) []string { +// podSpecImages reports whether node is PodSpec-shaped, and if so its images. +// +// The test is not "has a field called containers" but "does that field decode +// into the real corev1 type, rejecting unknown fields". The Kubernetes Go types +// are the schema, so a custom resource embedding a genuine PodSpec matches, +// while a lookalike — a field named containers holding something else — does +// not. +func podSpecImages(node map[string]any) ([]string, bool) { var images []string - for _, container := range containers { - images = append(images, container.Image) + matched := false + + for _, field := range containerFields { + value, ok := node[field] + if !ok { + continue + } + containers, err := decodeContainers(field, value) + if err != nil || len(containers) == 0 { + continue + } + matched = true + for _, container := range containers { + if container.Image != "" { + images = append(images, container.Image) + } + } } - return images + + return images, matched } -func GetContainersFromObject(obj any) ([]corev1.Container, error) { - podSpec, err := GetPodSpec(obj) +// decodeContainers strictly decodes one container list into its corev1 type. +// An error means "not that type", which is the signal the walk needs. +func decodeContainers(field string, value any) ([]corev1.Container, error) { + data, err := json.Marshal(value) if err != nil { return nil, err } + if field == "ephemeralContainers" { + var ephemeral []corev1.EphemeralContainer + if err := yaml.UnmarshalStrict(data, &ephemeral); err != nil { + return nil, err + } + containers := make([]corev1.Container, 0, len(ephemeral)) + for _, ec := range ephemeral { + containers = append(containers, corev1.Container(ec.EphemeralContainerCommon)) + } + return containers, nil + } + var containers []corev1.Container - containers = append(containers, podSpec.Containers...) - containers = append(containers, podSpec.InitContainers...) - for _, ec := range podSpec.EphemeralContainers { - containers = append(containers, corev1.Container(ec.EphemeralContainerCommon)) + if err := yaml.UnmarshalStrict(data, &containers); err != nil { + return nil, err } return containers, nil } diff --git a/k8s/k8s_test.go b/k8s/k8s_test.go index a7c5422..69b3dde 100644 --- a/k8s/k8s_test.go +++ b/k8s/k8s_test.go @@ -1,275 +1,249 @@ package k8s import ( + "slices" "testing" - appsv1 "k8s.io/api/apps/v1" - batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/yaml" ) -// Test that GetPodSpec works for the kinds that have a PodSpec: -func TestGetPodSpec(t *testing.T) { - tests := []struct { - name string - obj any - wantErr bool - }{ - {"Pod", &corev1.Pod{}, false}, - {"Deployment", &appsv1.Deployment{}, false}, - {"DaemonSet", &appsv1.DaemonSet{}, false}, - {"ReplicaSet", &appsv1.ReplicaSet{}, false}, - {"StatefulSet", &appsv1.StatefulSet{}, false}, - {"Job", &batchv1.Job{}, false}, - {"CronJob", &batchv1.CronJob{}, false}, - {"Invalid", "invalid", true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := GetPodSpec(tt.obj) - if (err != nil) != tt.wantErr { - t.Errorf("GetPodSpec() error = %v, wantErr %v", err, tt.wantErr) - } - }) +// decode turns a manifest fragment into the plain Go values FindImages walks. +func decode(t *testing.T, manifest string) any { + t.Helper() + var doc any + if err := yaml.Unmarshal([]byte(manifest), &doc); err != nil { + t.Fatalf("decoding fixture: %v", err) } + return doc } -// Test that GetPodSpec returns the correct PodSpec: -func TestGetPodSpecPod(t *testing.T) { - pod := &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "test-container", - Image: "test-image", - }, - }, - }, - } - - spec, err := GetPodSpec(pod) - if err != nil { - t.Fatalf("GetPodSpec() error = %v", err) - } - - if len(spec.Containers) != 1 { - t.Fatalf("expected 1 container, got %d", len(spec.Containers)) - } - - if spec.Containers[0].Image != "test-image" { - t.Errorf("expected image %q, got %q", "test-image", spec.Containers[0].Image) - } -} - -// Test that GetPodSpec returns the correct PodSpec for all supported kinds: -func TestGetPodSpecSupported(t *testing.T) { - commonPodSpec := corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "test-container", - Image: "test-image", - }, - }, - } - +// The kinds kir has always supported are found without being named: each is +// just a PodSpec at a different depth. +func TestFindImagesWorkloadKinds(t *testing.T) { tests := []struct { - name string - obj any + name string + manifest string + want []string }{ - {"Deployment", &appsv1.Deployment{ - Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: commonPodSpec, - }, - }, - }}, - {"DaemonSet", &appsv1.DaemonSet{ - Spec: appsv1.DaemonSetSpec{ - Template: corev1.PodTemplateSpec{ - Spec: commonPodSpec, - }, - }, - }}, - {"ReplicaSet", &appsv1.ReplicaSet{ - Spec: appsv1.ReplicaSetSpec{ - Template: corev1.PodTemplateSpec{ - Spec: commonPodSpec, - }, - }, - }}, - {"StatefulSet", &appsv1.StatefulSet{ - Spec: appsv1.StatefulSetSpec{ - Template: corev1.PodTemplateSpec{ - Spec: commonPodSpec, - }, - }, - }}, - {"Job", &batchv1.Job{ - Spec: batchv1.JobSpec{ - Template: corev1.PodTemplateSpec{ - Spec: commonPodSpec, - }, - }, - }}, - {"CronJob", &batchv1.CronJob{ - Spec: batchv1.CronJobSpec{ - JobTemplate: batchv1.JobTemplateSpec{ - Spec: batchv1.JobSpec{ - Template: corev1.PodTemplateSpec{ - Spec: commonPodSpec, - }, - }, - }, - }, - }}, + { + name: "Pod (PodSpec directly on spec)", + manifest: ` +kind: Pod +spec: + containers: + - name: app + image: app:1 +`, + want: []string{"app:1"}, + }, + { + name: "Deployment (PodSpec under a template)", + manifest: ` +kind: Deployment +spec: + template: + spec: + containers: + - name: app + image: app:2 +`, + want: []string{"app:2"}, + }, + { + name: "CronJob (PodSpec four levels down)", + manifest: ` +kind: CronJob +spec: + jobTemplate: + spec: + template: + spec: + containers: + - name: app + image: app:3 +`, + want: []string{"app:3"}, + }, + { + name: "List (every item contributes)", + manifest: ` +kind: List +items: +- kind: Pod + spec: + containers: + - name: a + image: a:1 +- kind: Service + spec: + ports: + - port: 80 +- kind: Pod + spec: + containers: + - name: b + image: b:1 +`, + want: []string{"a:1", "b:1"}, + }, + { + name: "all three container fields, in report order", + manifest: ` +kind: Pod +spec: + containers: + - name: app + image: app:1 + initContainers: + - name: init + image: init:1 + ephemeralContainers: + - name: debugger + image: debug:1 + targetContainerName: app +`, + want: []string{"app:1", "init:1", "debug:1"}, + }, + { + name: "no PodSpec anywhere", + manifest: ` +kind: Service +spec: + ports: + - port: 80 +`, + want: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - spec, err := GetPodSpec(tt.obj) - if err != nil { - t.Fatalf("GetPodSpec() error = %v", err) - } - - if len(spec.Containers) != 1 { - t.Fatalf("expected 1 container, got %d", len(spec.Containers)) - } - - if spec.Containers[0].Image != "test-image" { - t.Errorf("expected image %q, got %q", "test-image", spec.Containers[0].Image) + got := FindImages(decode(t, tt.manifest)) + if !slices.Equal(got, tt.want) { + t.Errorf("FindImages() = %v, want %v", got, tt.want) } }) } } -// Test that GetPodSpec fails for an object that does not have a PodSpec: -func TestGetPodSpecInvalid(t *testing.T) { - _, err := GetPodSpec("invalid") - - // Assert correct error message ()"object does not have a PodSpec"): - if err == nil || err.Error() != "object does not have a PodSpec" { - t.Fatalf("GetPodSpec() error = %v, want %q", err, "object does not have a PodSpec") +// The point of structural discovery: a custom resource the Kubernetes scheme +// cannot decode is understood, because its PodSpec is a PodSpec. +func TestFindImagesCustomResource(t *testing.T) { + rollout := ` +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +spec: + strategy: + canary: + steps: + - setWeight: 20 + template: + spec: + containers: + - name: app + image: app:1.4.2 +` + + want := []string{"app:1.4.2"} + if got := FindImages(decode(t, rollout)); !slices.Equal(got, want) { + t.Errorf("FindImages() = %v, want %v", got, want) } } -func TestGetContainerImages(t *testing.T) { - containers := []corev1.Container{ - {Name: "container1", Image: "image1"}, - {Name: "container2", Image: "image2"}, - } - - expected := []string{"image1", "image2"} - images := GetContainerImages(containers) - - if len(images) != len(expected) { - t.Fatalf("expected %d images, got %d", len(expected), len(images)) - } - - for i, img := range images { - if img != expected[i] { - t.Errorf("expected image %q, got %q", expected[i], img) - } - } -} - -func TestGetContainersFromObject(t *testing.T) { +// The counterweight: matching on shape must not mean matching on a field name. +// Decoding into corev1.Container is what separates a PodSpec from a lookalike. +func TestFindImagesRejectsLookalikes(t *testing.T) { tests := []struct { - name string - obj any - want []corev1.Container - wantErr bool + name string + manifest string }{ { - name: "Pod", - obj: &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: "container1", Image: "image1"}, - }, - InitContainers: []corev1.Container{ - {Name: "init-container1", Image: "init-image1"}, - }, - }, - }, - want: []corev1.Container{ - {Name: "container1", Image: "image1"}, - {Name: "init-container1", Image: "init-image1"}, - }, - wantErr: false, + name: "containers of another kind entirely", + manifest: ` +kind: ShippingManifest +spec: + containers: + - name: cargo-hold-1 + capacity: 40ft + image: photo-of-container.jpg +`, }, { - name: "Deployment", - obj: &appsv1.Deployment{ - Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: "container1", Image: "image1"}, - }, - InitContainers: []corev1.Container{ - {Name: "init-container1", Image: "init-image1"}, - }, - }, - }, - }, - }, - want: []corev1.Container{ - {Name: "container1", Image: "image1"}, - {Name: "init-container1", Image: "init-image1"}, - }, - wantErr: false, + name: "containers holding strings", + manifest: ` +kind: Warehouse +spec: + containers: + - CONTAINER-A + - CONTAINER-B +`, }, { - name: "Pod with ephemeral container", - obj: &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: "container1", Image: "image1"}, - }, - InitContainers: []corev1.Container{ - {Name: "init-container1", Image: "init-image1"}, - }, - EphemeralContainers: []corev1.EphemeralContainer{ - { - EphemeralContainerCommon: corev1.EphemeralContainerCommon{ - Name: "debugger", - Image: "ephemeral-image1", - }, - }, - }, - }, - }, - want: []corev1.Container{ - {Name: "container1", Image: "image1"}, - {Name: "init-container1", Image: "init-image1"}, - {Name: "debugger", Image: "ephemeral-image1"}, - }, - wantErr: false, + name: "a manifest embedded as a string", + manifest: ` +kind: ConfigMap +data: + pod.yaml: | + kind: Pod + spec: + containers: + - name: inner + image: inner:1 +`, }, { - name: "Invalid", - obj: "invalid", - want: nil, - wantErr: true, + name: "container status, which reports images but is not a PodSpec", + manifest: ` +kind: Pod +status: + containerStatuses: + - name: app + image: app:1 + imageID: docker-pullable://app@sha256:abc +`, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := GetContainersFromObject(tt.obj) - if (err != nil) != tt.wantErr { - t.Errorf("GetContainersFromObject() error = %v, wantErr %v", err, tt.wantErr) - return - } - if len(got) != len(tt.want) { - t.Errorf("expected %d containers, got %d", len(tt.want), len(got)) - } - for i, container := range got { - if container.Name != tt.want[i].Name || container.Image != tt.want[i].Image { - t.Errorf("expected container %v, got %v", tt.want[i], container) - } + if got := FindImages(decode(t, tt.manifest)); len(got) != 0 { + t.Errorf("FindImages() = %v, want no images", got) } }) } } + +// Go randomises map iteration, so the walk sorts keys. Without that the golden +// files would flake whenever a document holds more than one PodSpec. +func TestFindImagesOrderIsStable(t *testing.T) { + manifest := ` +kind: List +items: +- kind: Pod + spec: + containers: + - name: a + image: a:1 +- kind: Pod + spec: + containers: + - name: b + image: b:1 +- kind: Pod + spec: + containers: + - name: c + image: c:1 +` + + doc := decode(t, manifest) + want := FindImages(doc) + if len(want) != 3 { + t.Fatalf("expected 3 images, got %d", len(want)) + } + for range 50 { + if got := FindImages(doc); !slices.Equal(got, want) { + t.Fatalf("FindImages() = %v, want %v — order is not stable", got, want) + } + } +} diff --git a/yamlparser/yamlparser.go b/yamlparser/yamlparser.go index 91a060e..4333dce 100644 --- a/yamlparser/yamlparser.go +++ b/yamlparser/yamlparser.go @@ -5,24 +5,17 @@ import ( "errors" "fmt" "io" - "slices" "github.com/mpv/kir/k8s" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" utilyaml "k8s.io/apimachinery/pkg/util/yaml" - "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/yaml" ) -var supportedKinds = []string{"Pod", "Deployment", "DaemonSet", "ReplicaSet", "StatefulSet", "Job", "CronJob"} - // ProcessReader reads a (possibly multi-document) YAML stream and returns the -// container images of every supported workload it contains. Documents are -// separated using the Kubernetes YAML reader, which correctly handles leading -// and trailing "---" separators, separators followed by trailing whitespace, -// CRLF line endings, and a final document without a trailing newline. +// container images of every workload it contains. Documents are separated using +// the Kubernetes YAML reader, which correctly handles leading and trailing +// "---" separators, separators followed by trailing whitespace, CRLF line +// endings, and a final document without a trailing newline. // // A document that cannot be processed does not discard the stream. Its failure // is collected, the documents after it are still read, and whatever images were @@ -55,72 +48,31 @@ func ProcessReader(r io.Reader) ([]string, error) { return images, errors.Join(errs...) } +// ProcessData returns the images in a single manifest document. +// +// The document is decoded into plain Go values rather than into a typed +// Kubernetes object, and the PodSpec is then found structurally. Nothing +// consults a list of kinds: a document yields images if it contains something +// shaped like a PodSpec, whether it is a Deployment, a CronJob, a List, or a +// custom resource the Kubernetes scheme has never heard of. func ProcessData(data []byte) ([]string, error) { - // Decode the YAML file into a Kubernetes object - decode := serializer.NewCodecFactory(scheme.Scheme).UniversalDeserializer().Decode - obj, gvk, err := decode(data, nil, nil) - if err != nil { - // Kinds that aren't registered in the scheme (CRDs and other custom - // resources) are skipped rather than failing the whole stream. Some of - // them may embed a PodSpec we could inspect; surfacing those ("seen but - // not detected") is tracked in #75. For now they are skipped silently, - // like any other non-workload document — see - // docs/adr/0007-document-classification.md. - if runtime.IsNotRegisteredError(err) { - return nil, nil - } + var doc map[string]any + if err := yaml.Unmarshal(data, &doc); err != nil { return nil, err } - var images []string - - // Check if the object has containers - if containers, err := k8s.GetContainersFromObject(obj); err == nil { - images = append(images, k8s.GetContainerImages(containers)...) - return images, nil + // Everything kir accepts is a Kubernetes object, and every Kubernetes + // object has a kind. Requiring it keeps kir pointed at manifests instead of + // mining arbitrary YAML (a Helm values.yaml, say) for anything image-like, + // and it keeps an empty or malformed document an error rather than a silent + // no-op — the unprocessable tier of + // docs/adr/0007-document-classification.md, which ADR 0008 surfaces as a + // non-zero exit. + if _, ok := doc["kind"]; !ok { + return nil, fmt.Errorf("Object 'Kind' is missing in %q", data) } - // Handle List type separately - if gvk.Kind == "List" { - list, ok := obj.(*corev1.List) - if !ok { - return nil, fmt.Errorf("not a List") - } - for _, item := range list.Items { - var unstructuredObj unstructured.Unstructured - if err := unstructuredObj.UnmarshalJSON(item.Raw); err != nil { - return nil, fmt.Errorf("error unmarshaling item: %v", err) - } - imgs, err := processUnstructured(unstructuredObj) - if err != nil { - return nil, fmt.Errorf("error processing unstructured item: %v", err) - } - images = append(images, imgs...) - } - return images, nil - } - - // Any other kind (Service, ConfigMap, ...) is a valid object with no images - // to report, not an error; skip it silently so a single non-workload - // document does not discard images from the rest of the stream. See - // docs/adr/0007-document-classification.md. - return nil, nil -} - -func processUnstructured(item unstructured.Unstructured) ([]string, error) { - itemData, err := item.MarshalJSON() - if err != nil { - return nil, fmt.Errorf("error marshaling item: %v", err) - } - gvk := item.GroupVersionKind() - if slices.Contains(supportedKinds, gvk.Kind) { - images, err := ProcessData(itemData) - if err != nil { - return nil, fmt.Errorf("error processing data: %v", err) - } - return images, nil - } - // Non-workload items inside a List are skipped, mirroring how top-level - // non-workload documents are handled. - return nil, nil + // A document with no PodSpec in it — a Service, a ConfigMap — simply has no + // images to report, which is not an error. See ADR 0007. + return k8s.FindImages(doc), nil }