Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1
27 changes: 27 additions & 0 deletions approvals/kir_test.TestFailure.PartialStream.input.yaml
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error: yaml: line 9: did not find expected ',' or ']'
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
before-the-break:1.0
after-the-break:1.0
9 changes: 9 additions & 0 deletions approvals/kir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
35 changes: 27 additions & 8 deletions cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions docs/adr/0008-best-effort-processing-and-exit-codes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 16 additions & 3 deletions yamlparser/yamlparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package yamlparser

import (
"bufio"
"errors"
"fmt"
"io"
"slices"
Expand All @@ -22,24 +23,36 @@ 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()
if err == io.EOF {
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) {
Expand Down
45 changes: 45 additions & 0 deletions yamlparser/yamlparser_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package yamlparser

import (
"errors"
"slices"
"strings"
"testing"
)

Expand Down Expand Up @@ -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)
}
}