From 576b2e6c22c2ff212dc60e22dbbcebda3a9515df Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 09:32:29 +0000 Subject: [PATCH] fix: keep the images found around a document that fails ProcessReader returned nil on the first document it could not process, so one unparseable object discarded every image already found in that stream. A malformed document at the end of a file cost the whole file, and because `kubectl get pod -A -o yaml | kir -` arrives as a single stream, one bad object in a cluster dump left an error and no images as the only visible result. Failures are now collected per document: the documents after a bad one are still read, every failure is reported rather than only the first, and the images found are returned alongside. cmd prints them even when an input reported an error, and unwraps the joined error so stderr stays one failure per line. ADR 0008 already promised kir prints every image it finds and surfaces failures through the exit code; it said so about inputs, and this makes it true of the documents inside one. The ADR now states that explicitly. --- ...ailure.PartialStream.exitcode.approved.txt | 1 + ..._test.TestFailure.PartialStream.input.yaml | 27 +++++++++++ ...tFailure.PartialStream.stderr.approved.txt | 1 + ...tFailure.PartialStream.stdout.approved.txt | 2 + approvals/kir_test.go | 9 ++++ cmd/cmd.go | 35 +++++++++++---- ...8-best-effort-processing-and-exit-codes.md | 9 ++++ yamlparser/yamlparser.go | 19 ++++++-- yamlparser/yamlparser_test.go | 45 +++++++++++++++++++ 9 files changed, 137 insertions(+), 11 deletions(-) create mode 100644 approvals/kir_test.TestFailure.PartialStream.exitcode.approved.txt create mode 100644 approvals/kir_test.TestFailure.PartialStream.input.yaml create mode 100644 approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt create mode 100644 approvals/kir_test.TestFailure.PartialStream.stdout.approved.txt diff --git a/approvals/kir_test.TestFailure.PartialStream.exitcode.approved.txt b/approvals/kir_test.TestFailure.PartialStream.exitcode.approved.txt new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/approvals/kir_test.TestFailure.PartialStream.exitcode.approved.txt @@ -0,0 +1 @@ +1 diff --git a/approvals/kir_test.TestFailure.PartialStream.input.yaml b/approvals/kir_test.TestFailure.PartialStream.input.yaml new file mode 100644 index 0000000..d49184a --- /dev/null +++ b/approvals/kir_test.TestFailure.PartialStream.input.yaml @@ -0,0 +1,27 @@ +apiVersion: v1 +kind: Pod +metadata: + name: before +spec: + containers: + - name: c + image: before-the-break:1.0 +--- +apiVersion: v1 +kind: Pod +metadata: + name: broken +spec: + containers: + - name: c + image: nginx + ports: [8080 +--- +apiVersion: v1 +kind: Pod +metadata: + name: after +spec: + containers: + - name: c + image: after-the-break:1.0 diff --git a/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt b/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt new file mode 100644 index 0000000..49591ae --- /dev/null +++ b/approvals/kir_test.TestFailure.PartialStream.stderr.approved.txt @@ -0,0 +1 @@ +error: yaml: line 9: did not find expected ',' or ']' diff --git a/approvals/kir_test.TestFailure.PartialStream.stdout.approved.txt b/approvals/kir_test.TestFailure.PartialStream.stdout.approved.txt new file mode 100644 index 0000000..e1194ea --- /dev/null +++ b/approvals/kir_test.TestFailure.PartialStream.stdout.approved.txt @@ -0,0 +1,2 @@ +before-the-break:1.0 +after-the-break:1.0 diff --git a/approvals/kir_test.go b/approvals/kir_test.go index ce2fe2f..04effb3 100644 --- a/approvals/kir_test.go +++ b/approvals/kir_test.go @@ -85,6 +85,15 @@ func TestFailure(t *testing.T) { t.Run("BadYAML", func(t *testing.T) { verify(t, []string{"kir_test.TestFailure.BadYAML.input.yaml"}, nil) }) + + // One unparseable document does not cost the images in the documents around + // it: the fixture's first and third workloads are still reported, the bad + // middle one is named on stderr, and the exit code is non-zero (ADR 0008). + // BadYAML above cannot pin this — its whole file is unparseable, so it + // passes whether or not partial results survive. + t.Run("PartialStream", func(t *testing.T) { + verify(t, []string{"kir_test.TestFailure.PartialStream.input.yaml"}, nil) + }) } // TestCLI covers behaviour that only exists at the CLI boundary — stdin wiring, diff --git a/cmd/cmd.go b/cmd/cmd.go index 77a5472..9a9b34d 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -44,11 +44,11 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { stdin = strings.NewReader("") } images, err := processor.ProcessStdin(stdin) - if err != nil { - logger.Printf("error: %v", err) + failures := logErrors(logger, err) + printImages(stdout, images) + if failures > 0 { return 1 } - printImages(stdout, images) return 0 case "--version", "-v": fmt.Fprintf(stdout, "kir %s (commit %s, built %s)\n", version, commit, date) @@ -64,11 +64,10 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { failures := 0 for _, filePath := range files { images, err := processor.ProcessFile(filePath) - if err != nil { - logger.Printf("error: %v", err) - failures++ - continue - } + // Not `continue`: a file that failed on one document may still have + // yielded images from the others, and dropping them would defeat the + // point of reporting the failure. + failures += logErrors(logger, err) printImages(stdout, images) } if failures > 0 { @@ -77,6 +76,26 @@ func Run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return 0 } +// logErrors writes one "error:" line per failure and returns how many it wrote. +// +// A single input can fail on more than one document, and ProcessReader packs +// those into one joined error. Unwrapping it here keeps stderr to one failure +// per line, which is what anything reading that stream expects. +func logErrors(logger *log.Logger, err error) int { + if err == nil { + return 0 + } + if joined, ok := err.(interface{ Unwrap() []error }); ok { + count := 0 + for _, e := range joined.Unwrap() { + count += logErrors(logger, e) + } + return count + } + logger.Printf("error: %v", err) + return 1 +} + func printImages(w io.Writer, images []string) { for _, image := range images { fmt.Fprintln(w, image) diff --git a/docs/adr/0008-best-effort-processing-and-exit-codes.md b/docs/adr/0008-best-effort-processing-and-exit-codes.md index 6132b64..4092a88 100644 --- a/docs/adr/0008-best-effort-processing-and-exit-codes.md +++ b/docs/adr/0008-best-effort-processing-and-exit-codes.md @@ -12,6 +12,15 @@ An argument that matches no files (a typo, or a glob with no hits) is itself such a failure — reported on stderr with a non-zero exit, not a silent no-op, so a mistyped path can't masquerade as "no images found". +The same rule applies **within** an input, not only between them. A stream is a +batch of documents, so one document `kir` cannot process is reported, counted +against the exit code, and skipped — the documents around it are still read and +their images still printed. Every failure in the stream is reported, not just the +first. This matters most where a whole cluster arrives as one input +(`kubectl get pod -A -o yaml | kir -`): treating a single unparseable object as +fatal to the stream would discard every image beside it and leave "no images +found" as the only visible result. + This lets a pipeline (`kir manifests/* | xargs grype`) trust the exit code — zero means every input was understood, non-zero means at least one wasn't — without losing the images `kir` did find. Which inputs count as a failure vs. a diff --git a/yamlparser/yamlparser.go b/yamlparser/yamlparser.go index 8405ad2..91a060e 100644 --- a/yamlparser/yamlparser.go +++ b/yamlparser/yamlparser.go @@ -2,6 +2,7 @@ package yamlparser import ( "bufio" + "errors" "fmt" "io" "slices" @@ -22,8 +23,16 @@ var supportedKinds = []string{"Pod", "Deployment", "DaemonSet", "ReplicaSet", "S // 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 +// found are returned alongside the joined errors. ADR 0008 has kir print every +// image it finds and surface failures through the exit code; that has to hold +// within a stream as well as between inputs, or one unparseable document in a +// cluster dump costs every image around it. func ProcessReader(r io.Reader) ([]string, error) { var images []string + var errs []error reader := utilyaml.NewYAMLReader(bufio.NewReader(r)) for { doc, err := reader.Read() @@ -31,15 +40,19 @@ func ProcessReader(r io.Reader) ([]string, error) { break } if err != nil { - return nil, fmt.Errorf("error reading YAML document: %v", err) + // The stream can no longer be split into documents, so there is + // nothing further to read — but what was already found still counts. + errs = append(errs, fmt.Errorf("error reading YAML document: %v", err)) + break } imgs, err := ProcessData(doc) if err != nil { - return nil, err + errs = append(errs, err) + continue } images = append(images, imgs...) } - return images, nil + return images, errors.Join(errs...) } func ProcessData(data []byte) ([]string, error) { diff --git a/yamlparser/yamlparser_test.go b/yamlparser/yamlparser_test.go index aa11435..53ff24d 100644 --- a/yamlparser/yamlparser_test.go +++ b/yamlparser/yamlparser_test.go @@ -1,6 +1,9 @@ package yamlparser import ( + "errors" + "slices" + "strings" "testing" ) @@ -68,3 +71,45 @@ spec: } } } + +// A document that cannot be processed must not discard the images found in the +// documents around it. Before this was fixed, one unparseable document anywhere +// in a stream returned no images at all — so a single bad object in a +// `kubectl get -A -o yaml` dump silently cost every image in it. +func TestProcessReaderKeepsImagesAroundABadDocument(t *testing.T) { + stream := strings.Join([]string{ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - {name: c, image: before-the-break}\n", + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - {name: c, image: nginx, ports: [8080}\n", + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - {name: c, image: after-the-break}\n", + }, "---\n") + + images, err := ProcessReader(strings.NewReader(stream)) + + if err == nil { + t.Error("ProcessReader() error = nil, want the bad document reported") + } + want := []string{"before-the-break", "after-the-break"} + if !slices.Equal(images, want) { + t.Errorf("ProcessReader() images = %v, want %v", images, want) + } +} + +// Every failure in a stream is reported, not just the first, so the exit code +// and stderr account for all of them. +func TestProcessReaderReportsEveryBadDocument(t *testing.T) { + bad := "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - {name: c, image: nginx, ports: [8080}\n" + stream := strings.Join([]string{bad, bad}, "---\n") + + images, err := ProcessReader(strings.NewReader(stream)) + + if len(images) != 0 { + t.Errorf("ProcessReader() images = %v, want none", images) + } + var joined interface{ Unwrap() []error } + if !errors.As(err, &joined) { + t.Fatalf("ProcessReader() error = %v, want a joined error covering both documents", err) + } + if got := len(joined.Unwrap()); got != 2 { + t.Errorf("ProcessReader() reported %d failures, want 2", got) + } +}