diff --git a/krm/filter.go b/krm/filter.go index 18eea28..1109771 100644 --- a/krm/filter.go +++ b/krm/filter.go @@ -8,9 +8,9 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -func DefaultNodeHandler(_, curr, next string, opts Options) (string, Change, error) { +func DefaultNodeHandler(_, curr, next string, opts Options) (string, Change, bool, error) { if curr == next { - return curr, Change{}, nil + return curr, Change{}, false, nil } rawRef := curr @@ -23,29 +23,29 @@ func DefaultNodeHandler(_, curr, next string, opts Options) (string, Change, err oldRef, err := name.ParseReference(rawRef) if err != nil { - return curr, Change{}, err + return curr, Change{}, false, err } newRef, digest, err := ParseImageRefWithDigest(next) if err != nil { - return curr, Change{}, err + return curr, Change{}, false, err } if oldRef.Context().Name() != newRef.Context().Name() { - return curr, Change{}, nil + return curr, Change{}, false, nil } ok, err := MatchTag(newRef.Identifier(), opts) if err != nil { - return curr, Change{}, err + return curr, Change{}, false, err } if !ok { - return curr, Change{}, nil + return curr, Change{}, false, nil } if _, err := name.ParseReference(next); err != nil { - return curr, Change{}, err + return curr, Change{}, false, err } c := Change{ @@ -56,26 +56,26 @@ func DefaultNodeHandler(_, curr, next string, opts Options) (string, Change, err switch opts.Part { case "": - return next, c, nil + return next, c, true, nil case PartTag: - return newRef.Identifier(), c, nil + return newRef.Identifier(), c, true, nil case PartDigest: - return digest, c, nil + return digest, c, true, nil case PartTagDigest: - return strings.TrimSuffix(fmt.Sprintf("%s@%s", newRef.Identifier(), digest), "@"), c, nil + return strings.TrimSuffix(fmt.Sprintf("%s@%s", newRef.Identifier(), digest), "@"), c, true, nil default: - return curr, Change{}, fmt.Errorf("unknown part: %s", opts.Part) + return curr, Change{}, false, fmt.Errorf("unknown part: %s", opts.Part) } } const CommentPrefix = "# kobold:" -type NodeHandler func(key, currentRef, nextRef string, opts Options) (string, Change, error) +type NodeHandler func(key, currentRef, nextRef string, opts Options) (string, Change, bool, error) type ImageRefUpdateFilter struct { handler NodeHandler @@ -123,13 +123,16 @@ func (i *ImageRefUpdateFilter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error lastChange := Change{} for _, imageRef := range i.imageRefs { - v, change, err := i.handler(mn.Key.YNode().Value, mn.Value.YNode().Value, imageRef, opts) + v, change, changed, err := i.handler(mn.Key.YNode().Value, mn.Value.YNode().Value, imageRef, opts) if err != nil { i.Warnings = append(i.Warnings, fmt.Sprintf("failed to update image ref %q: %v", imageRef, err)) continue } - mn.Value.YNode().Value = v - lastChange = change + + if changed { + mn.Value.YNode().Value = v + lastChange = change + } } if originalValue != mn.Value.YNode().Value { diff --git a/krm/filter_test.go b/krm/filter_test.go index 1c20697..822d895 100644 --- a/krm/filter_test.go +++ b/krm/filter_test.go @@ -354,3 +354,35 @@ func Test_renderer_Render(t *testing.T) { }) } } + +// Test_lastChange_preserved verifies that when a YAML node is evaluated against +// multiple image refs and only one matches, the resulting Change is not +// overwritten by the empty Change from the non-matching refs. +func Test_lastChange_preserved(t *testing.T) { + t.Parallel() + + // The "kube" testdata has a node pinned to "latest". We pass two refs: + // one that matches (nginx:latest) and one that does not (busybox:latest). + // The bug caused the Change from the matching ref to be overwritten by + // the empty Change from the non-matching ref. + _, changes, _, err := testPipe("kube", + "test.azurecr.io/nginx:latest@sha256:82becede498899ec668628e7cb0ad87b6e1c371cb8a1e597d83a47fac21d6af3", + "test.azurecr.io/busybox:latest@sha256:220611111e8c9bbe242e9dc1367c0fa89eef83f26203ee3f7c3764046e02b248", + ) + if err != nil { + t.Fatal(err) + } + + if len(changes) == 0 { + t.Fatal("expected at least one change, got none") + } + + for i, c := range changes { + if c.Description == "" { + t.Errorf("change[%d]: Description is empty (lastChange was overwritten by a non-matching ref)", i) + } + if c.Repo == "" { + t.Errorf("change[%d]: Repo is empty", i) + } + } +}