diff --git a/README.md b/README.md index b964068..a90d997 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ A manifest stream usually mixes workloads with other objects. `kir` handles each | 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) | | 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) | 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/cmd/cmd.go b/cmd/cmd.go index 9a9b34d..3dea33e 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/mpv/kir/fileutil" + "github.com/mpv/kir/imageref" "github.com/mpv/kir/processor" ) @@ -45,7 +46,7 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { } images, err := processor.ProcessStdin(stdin) failures := logErrors(logger, err) - printImages(stdout, images) + failures += printImages(stdout, logger, "stdin", images) if failures > 0 { return 1 } @@ -68,7 +69,7 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { // yielded images from the others, and dropping them would defeat the // point of reporting the failure. failures += logErrors(logger, err) - printImages(stdout, images) + failures += printImages(stdout, logger, filePath, images) } if failures > 0 { return 1 @@ -96,8 +97,19 @@ func logErrors(logger *log.Logger, err error) int { return 1 } -func printImages(w io.Writer, images []string) { +// printImages writes every reportable image to w, reports the rest to logger, +// and returns how many it rejected. An unreportable reference gets the same +// treatment as a malformed document (ADR 0008): named on stderr, counted +// against the exit code, and not allowed to discard the images beside it. +func printImages(w io.Writer, logger *log.Logger, source string, images []string) int { + rejected := 0 for _, image := range images { + if err := imageref.Validate(image); err != nil { + logger.Printf("error: %s: %v", source, err) + rejected++ + continue + } fmt.Fprintln(w, image) } + return rejected } diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index 8a0ce2c..a2a3c36 100644 --- a/cmd/cmd_test.go +++ b/cmd/cmd_test.go @@ -95,6 +95,42 @@ func TestRunFileFailure(t *testing.T) { } } +// Which values are unreportable is imageref's business; this pins the CLI +// contract around them. The hostile bytes are written here rather than in an +// approvals fixture, which tooling would normalise (see AGENTS.md). +const hostileImageManifest = ` +apiVersion: v1 +kind: Pod +metadata: + name: hostile +spec: + containers: + - name: reportable + image: registry.k8s.io/nginx-slim:0.8 + - name: forges-a-second-entry + image: "evil\nsecond-line" + - name: repaints-the-terminal + image: "nginx:1.0\x1b[2K\rregistry.io/trusted:safe" +` + +func TestRunRejectsUnreportableImages(t *testing.T) { + var stdout, stderr bytes.Buffer + code := Run([]string{"-"}, strings.NewReader(hostileImageManifest), &stdout, &stderr) + + if code != 1 { + t.Errorf("exit code = %d, want 1", code) + } + if got, want := stdout.String(), "registry.k8s.io/nginx-slim:0.8\n"; got != want { + t.Errorf("stdout = %q, want only the reportable image %q", got, want) + } + if got, want := strings.Count(stderr.String(), "error:"), 2; got != want { + t.Errorf("stderr reported %d errors, want %d:\n%s", got, want, stderr.String()) + } + if strings.ContainsRune(stderr.String(), '\x1b') { + t.Errorf("stderr contains a raw escape byte, want it quoted:\n%q", stderr.String()) + } +} + func TestRunNoArgs(t *testing.T) { var stdout, stderr bytes.Buffer code := Run(nil, nil, &stdout, &stderr) diff --git a/docs/adr/0007-document-classification.md b/docs/adr/0007-document-classification.md index a3336c0..c5ad378 100644 --- a/docs/adr/0007-document-classification.md +++ b/docs/adr/0007-document-classification.md @@ -4,13 +4,14 @@ - Date: 2026-08-08 A manifest stream mixes workloads, image-less objects, custom resources, and the -occasional malformed document. Each falls into one of three tiers: +occasional malformed document. Each falls into one of these tiers: | Tier | Examples | stdout | stderr | exit | |---|---|---|---|---| | Workload (has a PodSpec) | Pod, Deployment, …, CronJob | images | — | 0 | | Known, image-less | Service, ConfigMap, Secret, … | — | — | 0 | | Unprocessable | malformed YAML, unreadable file | — | `error: …` | non-zero (ADR 0008) | +| Workload with an unreportable image | an image value that is not a valid reference | its other images | `error: …` | non-zero | The load-bearing choice: a valid image-less document is **not** an error and **not** a warning — it's expected input with nothing to report, so it's skipped @@ -19,7 +20,24 @@ stdout to images, keeps stderr quiet for normal input, and keeps the exit code trustworthy (an earlier version erred on every Service, which — once failures became non-zero, ADR 0008 — would make `kir manifests/*` exit non-zero). -Unregistered kinds (CRDs) are a fourth case, today handled like image-less — +The last tier classifies an image *value*, because stdout is a contract as much +as a report: one image per line, normally fed straight into another program's +arguments. A line break in a value forges an extra entry in that list, escape +sequences can make a terminal show a registry the scanner was never given, and a +leading dash arrives at the scanner as an option. Each is refused on its own, +leaving the document's other images reported. + +Validity is the canonical parser's verdict +([`distribution/reference`](https://github.com/distribution/reference)), not a +hand-written rule set — the load-bearing choice here. kir accepts exactly what a +registry client would, so anything it refuses was never a pullable image, which +is what makes refusing safe; and it refuses nothing a registry would serve, so it +cannot drop a real image. Hand-written rules failed both ways: an earlier draft +of this let `nginx:`, `{{.Values.image}}` and `$IMAGE` through as images. +Normalisation is not borrowed, only validation — kir reports what the manifest +said, so `nginx` stays `nginx`, not `docker.io/library/nginx`. + +Unregistered kinds (CRDs) are a further case, today handled like image-less — skipped silently. But a CRD may embed a PodSpec (Argo Rollouts, Knative, …), so skipping it silently can drop images (the #49 failure mode). Planned (#75): a `warning:` on stderr, exit 0 — "seen but not detected" — distinct from the silent diff --git a/go.mod b/go.mod index 9784262..bf15914 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.26.0 require ( github.com/approvals/go-approval-tests v1.14.0 + 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 @@ -16,6 +17,7 @@ require ( github.com/kr/text v0.2.0 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect + github.com/opencontainers/go-digest v1.0.0 // indirect github.com/x448/float16 v0.8.4 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect golang.org/x/net v0.49.0 // indirect diff --git a/go.sum b/go.sum index a0ff543..c7f6c0f 100644 --- a/go.sum +++ b/go.sum @@ -5,6 +5,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= +github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5Y4f/wlDRiLyi3E= github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sapM= github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= @@ -24,6 +26,8 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFdJifH4BDsTlE89Zl93FEloxaWZfGcifgq8= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= +github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U= +github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= diff --git a/imageref/imageref.go b/imageref/imageref.go new file mode 100644 index 0000000..fdd0785 --- /dev/null +++ b/imageref/imageref.go @@ -0,0 +1,31 @@ +// Package imageref decides whether a container image reference is safe for kir +// to report. See docs/adr/0007-document-classification.md for why kir refuses +// some values outright. +package imageref + +import ( + // go-digest resolves sha256 only when that hash is linked in, and rejects + // every digest-pinned reference without it. kir links it anyway today via + // client-go, so this changes nothing now; it keeps imageref standing on its + // own for when #26's candidates drop client-go. No test here can pin it — + // the test binary links the hash regardless. + _ "crypto/sha256" + "fmt" + "strconv" + + "github.com/distribution/reference" +) + +// Validate returns an error describing why image cannot be reported, or nil if +// it can. The canonical parser decides, so kir accepts exactly what a registry +// client would. The parsed value is discarded: kir reports what the manifest +// said, so "nginx" stays "nginx" rather than becoming "docker.io/library/nginx". +func Validate(image string) error { + if _, err := reference.ParseNormalizedNamed(image); err != nil { + // Quoted, not interpolated: the parser echoes the offending value back, + // and an escape sequence in it must not reach the terminal this is + // reported on. + return fmt.Errorf("invalid image reference %q: %s", image, strconv.Quote(err.Error())) + } + return nil +} diff --git a/imageref/imageref_test.go b/imageref/imageref_test.go new file mode 100644 index 0000000..5333bff --- /dev/null +++ b/imageref/imageref_test.go @@ -0,0 +1,67 @@ +package imageref + +import ( + "strings" + "testing" +) + +// One case per way a reference can be unreportable, rather than one per hostile +// byte — the parser treats them all the same, so byte variants would pin +// nothing extra. Hostile bytes live here and not in an approvals fixture: a +// checked-in .yaml would have its escapes and trailing whitespace normalised +// (see AGENTS.md). +func TestValidateRejects(t *testing.T) { + for name, image := range map[string]string{ + "empty": "", + "newline forges an entry": "evil\nsecond-line", + "escape sequence spoofs": "nginx:1.0\x1b[2K\rregistry.io/trusted:safe", + "whitespace splits args": "a b c", + "leading dash is a flag": "--platform=linux/amd64", + "empty tag": "nginx:", + "unrendered helm template": "{{.Values.image}}", + "unexpanded variable": "$IMAGE", + } { + t.Run(name, func(t *testing.T) { + if err := Validate(image); err == nil { + t.Errorf("Validate(%q) = nil, want an error", image) + } + }) + } +} + +// Over-rejection is the failure that matters: a dropped image is +// indistinguishable from a manifest that had none. Every shape a registry could +// serve must pass. +func TestValidateAccepts(t *testing.T) { + for _, image := range []string{ + "nginx", + "nginx:1.28", + "registry.k8s.io/nginx-slim:0.8", + "kiwigrid/k8s-sidecar", + "localhost:5000/kir:0.4.3", + "quay.io/org/sub/repo:v1.2.3-rc.1", + "registry.example.com:5000/team/app:2026-08-10_build.7", + // Digest-pinned references are what break when go-digest cannot resolve + // sha256; see the crypto/sha256 import in imageref.go. + "ghcr.io/mpv/kir@sha256:66b7bf84cfc7c1b2d4a7d9848114270ce6049a04d5dab67d767d8ab5c0b3412a", + "nginx:1.0@sha256:76944c9752702d324e06e2a9fa791c38f2b654ee4100c85327729eb3377a4284", + } { + t.Run(image, func(t *testing.T) { + if err := Validate(image); err != nil { + t.Errorf("Validate(%q) = %v, want nil", image, err) + } + }) + } +} + +// The parser echoes the offending value back in its message, so reporting a +// rejection must not pass those bytes through to a terminal. +func TestValidateErrorEscapesTheValue(t *testing.T) { + err := Validate("nginx:1.0\x1b[2K\rregistry.io/trusted:safe") + if err == nil { + t.Fatal("Validate() = nil, want an error") + } + if strings.ContainsAny(err.Error(), "\x1b\r\n") { + t.Errorf("error message carries raw control bytes: %q", err.Error()) + } +}