Merge pull request #144 from fluxcd/images-in-templates

Ensure that an unchanged image is not in update result

go.sum

@@ -1617,6 +1617,7 @@ sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.9/go.mod h1:dzAXnQb
 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.14/go.mod h1:LEScyzhFmoF5pso/YSeBstl57mOzx9xlU9n85RGrDQg=
 sigs.k8s.io/controller-runtime v0.8.3 h1:GMHvzjTmaWHQB8HadW+dIvBoJuLvZObYJ5YoZruPRao=
 sigs.k8s.io/controller-runtime v0.8.3/go.mod h1:U/l+DUopBc1ecfRZ5aviA9JDmGFQKvLf5YkZNx2e0sU=
+sigs.k8s.io/kustomize v1.0.11 h1:Yb+6DDt9+aR2AvQApvUaKS/ugteeG4MPyoFeUHiPOjk=
 sigs.k8s.io/kustomize v2.0.3+incompatible h1:JUufWFNlI44MdtnjUqVnvh29rR37PQFzPbLXqhyOyX0=
 sigs.k8s.io/kustomize v2.0.3+incompatible/go.mod h1:MkjgH3RdOWrievjo6c9T245dYlB5QeXV4WCbnt/PEpU=
 sigs.k8s.io/kustomize/kyaml v0.10.16 h1:4rn0PTEK4STOA5kbpz72oGskjpKYlmwru4YRgVQFv+c=

pkg/update/filter.go

@@ -0,0 +1,143 @@
+/*
+Copyright 2020, 2021 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package update
+
+import (
+	"github.com/go-openapi/spec"
+	"sigs.k8s.io/kustomize/kyaml/fieldmeta"
+	"sigs.k8s.io/kustomize/kyaml/openapi"
+	"sigs.k8s.io/kustomize/kyaml/setters2"
+	"sigs.k8s.io/kustomize/kyaml/yaml"
+)
+
+// The implementation of this filter is adapted from
+// [kyaml](https://github.com/kubernetes-sigs/kustomize/blob/kyaml/v0.10.16/kyaml/setters2/set.go),
+// with the following changes:
+//
+// - it calls its callback for each field it sets
+//
+// - it will set all fields referring to a setter present in the
+// schema -- this is behind a flag in the kyaml implementation, but
+// the only desired mode of operation here
+//
+// - substitutions are not supported -- they are not used for image
+// updates
+//
+// - no validation is done on the value being set -- since the schema
+// is constructed here, it's assumed the values will be appropriate
+//
+// - only scalar nodes are considered (i.e., no sequence replacements)
+//
+// - only per-field schema references (those in a comment in the YAML)
+// are considered -- these are the only ones relevant to image updates
+
+type SetAllCallback struct {
+	SettersSchema *spec.Schema
+	Callback      func(setter, oldValue, newValue string)
+}
+
+func (s *SetAllCallback) Filter(object *yaml.RNode) (*yaml.RNode, error) {
+	return object, accept(s, object, "", s.SettersSchema)
+}
+
+// visitor is provided to accept to walk the AST.
+type visitor interface {
+	// visitScalar is called for each scalar field value on a resource
+	// node is the scalar field value
+	// path is the path to the field; path elements are separated by '.'
+	visitScalar(node *yaml.RNode, path string, schema *openapi.ResourceSchema) error
+}
+
+// getSchema returns per-field OpenAPI schema for a particular node.
+func getSchema(r *yaml.RNode, settersSchema *spec.Schema) *openapi.ResourceSchema {
+	// get the override schema if it exists on the field
+	fm := fieldmeta.FieldMeta{SettersSchema: settersSchema}
+	if err := fm.Read(r); err == nil && !fm.IsEmpty() {
+		// per-field schema, this is fine
+		if fm.Schema.Ref.String() != "" {
+			// resolve the reference
+			s, err := openapi.Resolve(&fm.Schema.Ref, settersSchema)
+			if err == nil && s != nil {
+				fm.Schema = *s
+			}
+		}
+		return &openapi.ResourceSchema{Schema: &fm.Schema}
+	}
+	return nil
+}
+
+// accept walks the AST and calls the visitor at each scalar node.
+func accept(v visitor, object *yaml.RNode, p string, settersSchema *spec.Schema) error {
+	switch object.YNode().Kind {
+	case yaml.DocumentNode:
+		// Traverse the child of the document
+		return accept(v, yaml.NewRNode(object.YNode()), p, settersSchema)
+	case yaml.MappingNode:
+		return object.VisitFields(func(node *yaml.MapNode) error {
+			// Traverse each field value
+			return accept(v, node.Value, p+"."+node.Key.YNode().Value, settersSchema)
+		})
+	case yaml.SequenceNode:
+		return object.VisitElements(func(node *yaml.RNode) error {
+			// Traverse each list element
+			return accept(v, node, p, settersSchema)
+		})
+	case yaml.ScalarNode:
+		fieldSchema := getSchema(object, settersSchema)
+		return v.visitScalar(object, p, fieldSchema)
+	}
+	return nil
+}
+
+// set applies the value from ext to field
+func (s *SetAllCallback) set(field *yaml.RNode, ext *setters2.CliExtension, sch *spec.Schema) (bool, error) {
+	// check full setter
+	if ext.Setter == nil {
+		return false, nil
+	}
+
+	// this has a full setter, set its value
+	old := field.YNode().Value
+	field.YNode().Value = ext.Setter.Value
+	s.Callback(ext.Setter.Name, old, ext.Setter.Value)
+
+	// format the node so it is quoted if it is a string. If there is
+	// type information on the setter schema, we use it.
+	if len(sch.Type) > 0 {
+		yaml.FormatNonStringStyle(field.YNode(), *sch)
+	}
+	return true, nil
+}
+
+// visitScalar
+func (s *SetAllCallback) visitScalar(object *yaml.RNode, p string, fieldSchema *openapi.ResourceSchema) error {
+	if fieldSchema == nil {
+		return nil
+	}
+	// get the openAPI for this field describing how to apply the setter
+	ext, err := setters2.GetExtFromSchema(fieldSchema.Schema)
+	if err != nil {
+		return err
+	}
+	if ext == nil {
+		return nil
+	}
+
+	// perform a direct set of the field if it matches
+	_, err = s.set(object, ext, fieldSchema.Schema)
+	return err
+}

pkg/update/setters.go

@@ -88,11 +88,48 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 	// would be interpreted as part of the $ref path.
 
 	var settersSchema spec.Schema
-	var setters []*setters2.Set
-	setterToImage := make(map[string]imageRef)
 
 	// collect setter defs and setters by going through all the image
 	// policies available.
+	result := Result{
+		Files: make(map[string]FileResult),
+	}
+
+	// Compilng the result needs the file, the image ref used, and the
+	// object. Each setter will supply its own name to its callback,
+	// which can be used to look up the image ref; the file and object
+	// we will get from `setAll` which keeps track of those as it
+	// iterates.
+	imageRefs := make(map[string]imageRef)
+	setAllCallback := func(file, setterName string, node *yaml.RNode) {
+		ref, ok := imageRefs[setterName]
+		if !ok {
+			return
+		}
+
+		meta, err := node.GetMeta()
+		if err != nil {
+			return
+		}
+		oid := ObjectIdentifier{meta.GetIdentifier()}
+
+		fileres, ok := result.Files[file]
+		if !ok {
+			fileres = FileResult{
+				Objects: make(map[ObjectIdentifier][]ImageRef),
+			}
+			result.Files[file] = fileres
+		}
+		objres, ok := fileres.Objects[oid]
+		for _, n := range objres {
+			if n == ref {
+				return
+			}
+		}
+		objres = append(objres, ref)
+		fileres.Objects[oid] = objres
+	}
+
 	defs := map[string]spec.Schema{}
 	for _, policy := range policies {
 		if policy.Status.LatestImage == "" {
@@ -119,40 +156,27 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 
 		tag := ref.Identifier()
 		// annoyingly, neither the library imported above, nor an
-		// alternative, I found will yield the original image name;
+		// alternative I found, will yield the original image name;
 		// this is an easy way to get it
 		name := image[:len(tag)+1]
 
 		imageSetter := fmt.Sprintf("%s:%s", policy.GetNamespace(), policy.GetName())
 		defs[fieldmeta.SetterDefinitionPrefix+imageSetter] = setterSchema(imageSetter, policy.Status.LatestImage)
-		setterToImage[imageSetter] = ref
-		setters = append(setters, &setters2.Set{
-			Name:          imageSetter,
-			SettersSchema: &settersSchema,
-		})
+		imageRefs[imageSetter] = ref
 
 		tagSetter := imageSetter + ":tag"
-
 		defs[fieldmeta.SetterDefinitionPrefix+tagSetter] = setterSchema(tagSetter, tag)
-		setterToImage[tagSetter] = ref
-		setters = append(setters, &setters2.Set{
-			Name:          tagSetter,
-			SettersSchema: &settersSchema,
-		})
+		imageRefs[tagSetter] = ref
 
 		// Context().Name() gives the image repository _as supplied_
 		nameSetter := imageSetter + ":name"
-		setterToImage[nameSetter] = ref
 		defs[fieldmeta.SetterDefinitionPrefix+nameSetter] = setterSchema(nameSetter, name)
-		setters = append(setters, &setters2.Set{
-			Name:          nameSetter,
-			SettersSchema: &settersSchema,
-		})
+		imageRefs[nameSetter] = ref
 	}
 
 	settersSchema.Definitions = defs
-	setAll := &setAllRecorder{
-		setters: setters,
+	set := &SetAllCallback{
+		SettersSchema: &settersSchema,
 	}
 
 	// get ready with the reader and writer
@@ -168,7 +192,7 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 		Inputs:  []kio.Reader{reader},
 		Outputs: []kio.Writer{writer},
 		Filters: []kio.Filter{
-			setAll,
+			setAll(set, setAllCallback),
 		},
 	}
 
@@ -177,94 +201,49 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 	if err != nil {
 		return Result{}, err
 	}
-	return setAll.getResult(setterToImage), nil
+	return result, nil
 }
 
-type update struct {
-	file, name string
-	object     *yaml.RNode
-}
-
-type setAllRecorder struct {
-	setters []*setters2.Set
-	updates []update
-}
-
-func (s *setAllRecorder) getResult(nameToImage map[string]imageRef) Result {
-	result := Result{
-		Files: make(map[string]FileResult),
-	}
-updates:
-	for _, update := range s.updates {
-		file, ok := result.Files[update.file]
-		if !ok {
-			file = FileResult{
-				Objects: make(map[ObjectIdentifier][]ImageRef),
-			}
-			result.Files[update.file] = file
-		}
-		objects := file.Objects
-
-		meta, err := update.object.GetMeta()
-		if err != nil {
-			continue updates
-		}
-		id := ObjectIdentifier{meta.GetIdentifier()}
-
-		ref, ok := nameToImage[update.name]
-		if !ok { // this means an update was made that wasn't recorded as being an image
-			continue updates
-		}
-		// if the name and tag of an image are both used, we don't need to record it twice
-		for _, n := range objects[id] {
-			if n == ref {
-				continue updates
-			}
-		}
-		objects[id] = append(objects[id], ref)
-	}
-	return result
-}
-
-// Filter is an implementation of kio.Filter which records each use of
-// a setter at each object in each file, and only includes the files
-// that were updated in the output nodes. The implementation is
-// adapted from
-// https://github.com/kubernetes-sigs/kustomize/blob/kyaml/v0.10.13/kyaml/setters2/set.go#L503
-func (s *setAllRecorder) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
-	filesToUpdate := sets.String{}
-	for i := range nodes {
-		for _, setter := range s.setters {
-			preCount := setter.Count
-			_, err := setter.Filter(nodes[i])
-			if err != nil {
-				return nil, err
-			}
-			if setter.Count > preCount {
+// setAll returns a kio.Filter using the supplied SetAllCallback
+// (dealing with individual nodes), amd calling the given callback
+// whenever a field value is changed, and returning only nodes from
+// files with changed nodes. This is based on
+// [`SetAll`](https://github.com/kubernetes-sigs/kustomize/blob/kyaml/v0.10.16/kyaml/setters2/set.go#L503
+// from kyaml/kio.
+func setAll(filter *SetAllCallback, callback func(file, setterName string, node *yaml.RNode)) kio.Filter {
+	return kio.FilterFunc(
+		func(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
+			filesToUpdate := sets.String{}
+			for i := range nodes {
 				path, _, err := kioutil.GetFileAnnotations(nodes[i])
 				if err != nil {
 					return nil, err
 				}
-				filesToUpdate.Insert(path)
-				s.updates = append(s.updates, update{
-					file:   path,
-					name:   setter.Name,
-					object: nodes[i],
-				})
+
+				filter.Callback = func(setter, oldValue, newValue string) {
+					if newValue != oldValue {
+						callback(path, setter, nodes[i])
+						filesToUpdate.Insert(path)
+					}
+				}
+				_, err = filter.Filter(nodes[i])
+				if err != nil {
+					return nil, err
+				}
 			}
-		}
-	}
-	var nodesInUpdatedFiles []*yaml.RNode
-	for i := range nodes {
-		path, _, err := kioutil.GetFileAnnotations(nodes[i])
-		if err != nil {
-			return nil, err
-		}
-		if filesToUpdate.Has(path) {
-			nodesInUpdatedFiles = append(nodesInUpdatedFiles, nodes[i])
-		}
-	}
-	return nodesInUpdatedFiles, nil
+
+			var nodesInUpdatedFiles []*yaml.RNode
+			for i := range nodes {
+				path, _, err := kioutil.GetFileAnnotations(nodes[i])
+				if err != nil {
+					return nil, err
+				}
+				if filesToUpdate.Has(path) {
+					nodesInUpdatedFiles = append(nodesInUpdatedFiles, nodes[i])
+				}
+			}
+			return nodesInUpdatedFiles, nil
+		})
 }
 
 func setterSchema(name, value string) spec.Schema {

pkg/update/testdata/setters/expected/marked.yaml

@@ -12,3 +12,5 @@ spec:
           containers:
           - name: c
             image: updated:v1.0.1 # {"$imagepolicy": "automation-ns:policy"}
+          - name: d
+            image: image:v1.0.0 # {"$imagepolicy": "automation-ns:unchanged"}

pkg/update/testdata/setters/original/marked.yaml

@@ -12,3 +12,5 @@ spec:
           containers:
           - name: c
             image: image:v1.0.0 # {"$imagepolicy": "automation-ns:policy"}
+          - name: d
+            image: image:v1.0.0 # {"$imagepolicy": "automation-ns:unchanged"}

pkg/update/update_test.go

@@ -38,6 +38,30 @@ func TestUpdate(t *testing.T) {
 }
 
 var _ = Describe("Update image via kyaml setters2", func() {
+
+	var (
+		policies = []imagev1alpha1_reflect.ImagePolicy{
+			imagev1alpha1_reflect.ImagePolicy{
+				ObjectMeta: metav1.ObjectMeta{ // name matches marker used in testdata/setters/{original,expected}
+					Namespace: "automation-ns",
+					Name:      "policy",
+				},
+				Status: imagev1alpha1_reflect.ImagePolicyStatus{
+					LatestImage: "updated:v1.0.1",
+				},
+			},
+			imagev1alpha1_reflect.ImagePolicy{
+				ObjectMeta: metav1.ObjectMeta{ // name matches marker used in testdata/setters/{original,expected}
+					Namespace: "automation-ns",
+					Name:      "unchanged",
+				},
+				Status: imagev1alpha1_reflect.ImagePolicyStatus{
+					LatestImage: "image:v1.0.0",
+				},
+			},
+		}
+	)
+
 	It("updates the image marked with the image policy (setter) ref", func() {
 		tmp, err := ioutil.TempDir("", "gotest")
 		Expect(err).ToNot(HaveOccurred())
@@ -53,6 +77,15 @@ var _ = Describe("Update image via kyaml setters2", func() {
 					LatestImage: "updated:v1.0.1",
 				},
 			},
+			imagev1alpha1_reflect.ImagePolicy{
+				ObjectMeta: metav1.ObjectMeta{ // name matches marker used in testdata/setters/{original,expected}
+					Namespace: "automation-ns",
+					Name:      "unchanged",
+				},
+				Status: imagev1alpha1_reflect.ImagePolicyStatus{
+					LatestImage: "image:v1.0.0",
+				},
+			},
 		}
 
 		_, err = UpdateWithSetters("testdata/setters/original", tmp, policies)
@@ -65,18 +98,6 @@ var _ = Describe("Update image via kyaml setters2", func() {
 		Expect(err).ToNot(HaveOccurred())
 		defer os.RemoveAll(tmp)
 
-		policies := []imagev1alpha1_reflect.ImagePolicy{
-			imagev1alpha1_reflect.ImagePolicy{
-				ObjectMeta: metav1.ObjectMeta{ // name matches marker used in testdata/setters/{original,expected}
-					Namespace: "automation-ns",
-					Name:      "policy",
-				},
-				Status: imagev1alpha1_reflect.ImagePolicyStatus{
-					LatestImage: "updated:v1.0.1",
-				},
-			},
-		}
-
 		result, err := UpdateWithSetters("testdata/setters/original", tmp, policies)
 		Expect(err).ToNot(HaveOccurred())