From dcb26d090191a90e5aff377778c20d18fdb29a1e Mon Sep 17 00:00:00 2001 From: monopole Date: Sun, 31 Jan 2021 19:07:59 -0800 Subject: [PATCH] More fieldspec tests. --- api/filters/fieldspec/fieldspec.go | 53 ++- api/filters/fieldspec/fieldspec_test.go | 398 ++++++++++-------- .../accumulator/namereferencetransformer.go | 11 +- 3 files changed, 256 insertions(+), 206 deletions(-) diff --git a/api/filters/fieldspec/fieldspec.go b/api/filters/fieldspec/fieldspec.go index f7af2bb5b..3811afec9 100644 --- a/api/filters/fieldspec/fieldspec.go +++ b/api/filters/fieldspec/fieldspec.go @@ -4,6 +4,7 @@ package fieldspec import ( + "fmt" "strings" "sigs.k8s.io/kustomize/api/filters/filtersutil" @@ -15,7 +16,16 @@ import ( var _ yaml.Filter = Filter{} -// Filter applies a single fieldSpec to a single object +// Filter possibly mutates its object argument using a FieldSpec. +// If the object matches the FieldSpec, and the node found +// by following the fieldSpec's path is non-null, this filter calls +// the setValue function on the node at the end of the path. +// If any part of the path doesn't exist, the filter returns +// without doing anything and without error, unless it was set +// to create the path. If set to create, it creates a tree of maps +// along the path, and the leaf node gets the setValue called on it. +// Error on GVK mismatch, empty or poorly formed path. +// Filter expect kustomize style paths, not JSON paths. // Filter stores internal state and should not be reused type Filter struct { // FieldSpec contains the path to the value to set. @@ -39,7 +49,8 @@ func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) { return obj, errors.Wrap(err) } fltr.path = utils.PathSplitter(fltr.FieldSpec.Path) - if err := fltr.filter(obj); err != nil { + err := fltr.filter(obj) + if err != nil { s, _ := obj.String() return nil, errors.WrapPrefixf(err, "considering field '%s' of object\n%v", fltr.FieldSpec.Path, s) @@ -47,6 +58,7 @@ func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) { return obj, nil } +// Recursively called. func (fltr Filter) filter(obj *yaml.RNode) error { if len(fltr.path) == 0 { // found the field -- set its value @@ -57,9 +69,9 @@ func (fltr Filter) filter(obj *yaml.RNode) error { } switch obj.YNode().Kind { case yaml.SequenceNode: - return fltr.seq(obj) + return fltr.handleSequence(obj) case yaml.MappingNode: - return fltr.field(obj) + return fltr.handleMap(obj) case yaml.AliasNode: return fltr.filter(yaml.NewRNode(obj.YNode().Alias)) default: @@ -67,17 +79,20 @@ func (fltr Filter) filter(obj *yaml.RNode) error { } } -// field calls filter on the field matching the next path element -func (fltr Filter) field(obj *yaml.RNode) error { +// handleMap calls filter on the map field matching the next path element +func (fltr Filter) handleMap(obj *yaml.RNode) error { fieldName, isSeq := isSequenceField(fltr.path[0]) + if fieldName == "" { + return fmt.Errorf("cannot set or create an empty field name") + } // lookup the field matching the next path element - var lookupField yaml.Filter + var operation yaml.Filter var kind yaml.Kind tag := yaml.NodeTagEmpty switch { case !fltr.FieldSpec.CreateIfNotPresent || fltr.CreateKind == 0 || isSeq: - // dont' create the field if we don't find it - lookupField = yaml.Lookup(fieldName) + // don't create the field if we don't find it + operation = yaml.Lookup(fieldName) if isSeq { // The query path thinks this field should be a sequence; // accept this hint for use later if the tag is NodeTagNull. @@ -85,21 +100,25 @@ func (fltr Filter) field(obj *yaml.RNode) error { } case len(fltr.path) <= 1: // create the field if it is missing: use the provided node kind - lookupField = yaml.LookupCreate(fltr.CreateKind, fieldName) + operation = yaml.LookupCreate(fltr.CreateKind, fieldName) kind = fltr.CreateKind tag = fltr.CreateTag default: // create the field if it is missing: must be a mapping node - lookupField = yaml.LookupCreate(yaml.MappingNode, fieldName) + operation = yaml.LookupCreate(yaml.MappingNode, fieldName) kind = yaml.MappingNode tag = yaml.NodeTagMap } // locate (or maybe create) the field - field, err := obj.Pipe(lookupField) - if err != nil || field == nil { + field, err := obj.Pipe(operation) + if err != nil { return errors.WrapPrefixf(err, "fieldName: %s", fieldName) } + if field == nil { + // No error if field not found. + return nil + } // if the value exists, but is null and kind is set, // then change it to the creation type @@ -117,7 +136,7 @@ func (fltr Filter) field(obj *yaml.RNode) error { } // seq calls filter on all sequence elements -func (fltr Filter) seq(obj *yaml.RNode) error { +func (fltr Filter) handleSequence(obj *yaml.RNode) error { if err := obj.VisitElements(func(node *yaml.RNode) error { // recurse on each element -- re-allocating a Filter is // not strictly required, but is more consistent with field @@ -128,16 +147,14 @@ func (fltr Filter) seq(obj *yaml.RNode) error { return errors.WrapPrefixf(err, "visit traversal on path: %v", fltr.path) } - return nil } // isSequenceField returns true if the path element is for a sequence field. // isSequence also returns the path element with the '[]' suffix trimmed func isSequenceField(name string) (string, bool) { - isSeq := strings.HasSuffix(name, "[]") - name = strings.TrimSuffix(name, "[]") - return name, isSeq + shorter := strings.TrimSuffix(name, "[]") + return shorter, shorter != name } // isMatchGVK returns true if the fs.GVK matches the obj GVK. diff --git a/api/filters/fieldspec/fieldspec_test.go b/api/filters/fieldspec/fieldspec_test.go index 5726e75e9..cefc5b9be 100644 --- a/api/filters/fieldspec/fieldspec_test.go +++ b/api/filters/fieldspec/fieldspec_test.go @@ -15,209 +15,243 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -type TestCase struct { - name string - input string - expected string - filter fieldspec.Filter - fieldSpec string - error string -} - -var tests = []TestCase{ - { - name: "update", - fieldSpec: ` +func TestFilter_Filter(t *testing.T) { + testCases := map[string]struct { + input string + expected string + filter fieldspec.Filter + fieldSpec string + error string + }{ + "path not found": { + fieldSpec: ` path: a/b group: foo kind: Bar `, - input: ` + input: ` +apiVersion: foo +kind: Bar +xxx: +`, + expected: ` +apiVersion: foo +kind: Bar +xxx: +`, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, + }, + "empty path": { + fieldSpec: ` +group: foo +kind: Bar +`, + input: ` +apiVersion: foo +kind: Bar +xxx: +`, + expected: ` +apiVersion: foo +kind: Bar +xxx: +`, + error: `considering field '' of object +apiVersion: foo +kind: Bar +xxx: +: cannot set or create an empty field name`, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, + }, + + "update": { + fieldSpec: ` +path: a/b +group: foo +kind: Bar +`, + input: ` apiVersion: foo/v1beta1 kind: Bar a: b: c `, - expected: ` + expected: ` apiVersion: foo/v1beta1 kind: Bar a: b: e `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - { - name: "update-kind-not-match", - fieldSpec: ` + "update-kind-not-match": { + fieldSpec: ` path: a/b group: foo kind: Bar1 `, - input: ` + input: ` apiVersion: foo/v1beta1 kind: Bar2 a: b: c `, - expected: ` + expected: ` apiVersion: foo/v1beta1 kind: Bar2 a: b: c `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - { - name: "update-group-not-match", - fieldSpec: ` + "update-group-not-match": { + fieldSpec: ` path: a/b group: foo1 kind: Bar `, - input: ` + input: ` apiVersion: foo2/v1beta1 kind: Bar a: b: c `, - expected: ` + expected: ` apiVersion: foo2/v1beta1 kind: Bar a: b: c `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - { - name: "update-version-not-match", - fieldSpec: ` + "update-version-not-match": { + fieldSpec: ` path: a/b group: foo version: v1beta1 kind: Bar `, - input: ` + input: ` apiVersion: foo/v1beta2 kind: Bar a: b: c `, - expected: ` + expected: ` apiVersion: foo/v1beta2 kind: Bar a: b: c `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - { - name: "bad-version", - fieldSpec: ` + "bad-version": { + fieldSpec: ` path: a/b group: foo version: v1beta1 kind: Bar `, - input: ` + input: ` apiVersion: foo/v1beta2/something kind: Bar a: b: c `, - expected: ` + expected: ` apiVersion: foo/v1beta2/something kind: Bar a: b: c `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - { - name: "bad-meta", - fieldSpec: ` + "bad-meta": { + fieldSpec: ` path: a/b group: foo version: v1beta1 kind: Bar `, - input: ` + input: ` a: b: c `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, + error: "missing Resource metadata", }, - error: "missing Resource metadata", - }, - { - name: "miss-match-type", - fieldSpec: ` + "miss-match-type": { + fieldSpec: ` path: a/b/c kind: Bar `, - input: ` + input: ` kind: Bar a: b: a `, - error: `considering field 'a/b/c' of object + error: `considering field 'a/b/c' of object kind: Bar a: b: a : expected sequence or mapping node`, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - { - name: "add", - fieldSpec: ` + "add": { + fieldSpec: ` path: a/b/c/d group: foo create: true kind: Bar `, - input: ` + input: ` apiVersion: foo/v1beta1 kind: Bar a: {} `, - expected: ` + expected: ` apiVersion: foo/v1beta1 kind: Bar a: {b: {c: {d: e}}} `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "update-in-sequence", - fieldSpec: ` + "update-in-sequence": { + fieldSpec: ` path: a/b[]/c/d group: foo kind: Bar `, - input: ` + input: ` apiVersion: foo/v1beta1 kind: Bar a: @@ -225,7 +259,7 @@ a: - c: d: a `, - expected: ` + expected: ` apiVersion: foo/v1beta1 kind: Bar a: @@ -233,244 +267,237 @@ a: - c: d: e `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + }, }, - }, - // Don't create a sequence - { - name: "empty-sequence-no-create", - fieldSpec: ` + // Don't create a sequence + "empty-sequence-no-create": { + fieldSpec: ` path: a/b[]/c/d group: foo create: true kind: Bar `, - input: ` + input: ` apiVersion: foo/v1beta1 kind: Bar a: {} `, - expected: ` + expected: ` apiVersion: foo/v1beta1 kind: Bar a: {} `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + CreateKind: yaml.ScalarNode, + }, }, - }, - // Create a new field for an element in a sequence - { - name: "empty-sequence-create", - fieldSpec: ` + // Create a new field for an element in a sequence + "empty-sequence-create": { + fieldSpec: ` path: a/b[]/c/d group: foo create: true kind: Bar `, - input: ` + input: ` apiVersion: foo/v1beta1 kind: Bar a: b: - c: {} `, - expected: ` + expected: ` apiVersion: foo/v1beta1 kind: Bar a: b: - c: {d: e} `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "group v1", - fieldSpec: ` + "group v1": { + fieldSpec: ` path: a/b group: v1 create: true kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar `, - expected: ` + expected: ` apiVersion: v1 kind: Bar `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "version v1", - fieldSpec: ` + "version v1": { + fieldSpec: ` path: a/b version: v1 create: true kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar `, - expected: ` + expected: ` apiVersion: v1 kind: Bar a: b: e `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("e"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("e"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "successfully set field on array entry no sequence hint", - fieldSpec: ` + + "successfully set field on array entry no sequence hint": { + fieldSpec: ` path: spec/containers/image version: v1 kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar spec: containers: - image: foo `, - expected: ` + expected: ` apiVersion: v1 kind: Bar spec: containers: - image: bar `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("bar"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("bar"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "successfully set field on array entry with sequence hint", - fieldSpec: ` + + "successfully set field on array entry with sequence hint": { + fieldSpec: ` path: spec/containers[]/image version: v1 kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar spec: containers: - image: foo `, - expected: ` + expected: ` apiVersion: v1 kind: Bar spec: containers: - image: bar `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("bar"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("bar"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "failure to set field on array entry with sequence hint in path", - fieldSpec: ` + "failure to set field on array entry with sequence hint in path": { + fieldSpec: ` path: spec/containers[]/image version: v1 kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar spec: containers: `, - expected: ` + expected: ` apiVersion: v1 kind: Bar spec: containers: [] `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("bar"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("bar"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "failure to set field on array entry, no sequence hint in path", - fieldSpec: ` + + "failure to set field on array entry, no sequence hint in path": { + fieldSpec: ` path: spec/containers/image version: v1 kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar spec: containers: `, - expected: ` + expected: ` apiVersion: v1 kind: Bar spec: containers: `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("bar"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("bar"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "filedname with slash '/'", - fieldSpec: ` + "fieldname with slash '/'": { + fieldSpec: ` path: a/b\/c/d version: v1 kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar a: b/c: d: foo `, - expected: ` + expected: ` apiVersion: v1 kind: Bar a: b/c: d: bar `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("bar"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("bar"), + CreateKind: yaml.ScalarNode, + }, }, - }, - { - name: "filedname with multiple '/'", - fieldSpec: ` + "fieldname with multiple '/'": { + fieldSpec: ` path: a/b\/c/d\/e/f version: v1 kind: Bar `, - input: ` + input: ` apiVersion: v1 kind: Bar a: @@ -478,7 +505,7 @@ a: d/e: f: foo `, - expected: ` + expected: ` apiVersion: v1 kind: Bar a: @@ -486,25 +513,24 @@ a: d/e: f: bar `, - filter: fieldspec.Filter{ - SetValue: filtersutil.SetScalar("bar"), - CreateKind: yaml.ScalarNode, + filter: fieldspec.Filter{ + SetValue: filtersutil.SetScalar("bar"), + CreateKind: yaml.ScalarNode, + }, }, - }, -} + } -func TestFilter_Filter(t *testing.T) { - for i := range tests { - test := tests[i] - t.Run(test.name, func(t *testing.T) { - err := yaml.Unmarshal([]byte(test.fieldSpec), &test.filter.FieldSpec) + for n := range testCases { + tc := testCases[n] + t.Run(n, func(t *testing.T) { + err := yaml.Unmarshal([]byte(tc.fieldSpec), &tc.filter.FieldSpec) if !assert.NoError(t, err) { t.FailNow() } out := &bytes.Buffer{} rw := &kio.ByteReadWriter{ - Reader: bytes.NewBufferString(test.input), + Reader: bytes.NewBufferString(tc.input), Writer: out, OmitReaderAnnotations: true, } @@ -512,11 +538,11 @@ func TestFilter_Filter(t *testing.T) { // run the filter err = kio.Pipeline{ Inputs: []kio.Reader{rw}, - Filters: []kio.Filter{kio.FilterAll(test.filter)}, + Filters: []kio.Filter{kio.FilterAll(tc.filter)}, Outputs: []kio.Writer{rw}, }.Execute() - if test.error != "" { - if !assert.EqualError(t, err, test.error) { + if tc.error != "" { + if !assert.EqualError(t, err, tc.error) { t.FailNow() } // stop rest of test @@ -529,7 +555,7 @@ func TestFilter_Filter(t *testing.T) { // check results if !assert.Equal(t, - strings.TrimSpace(test.expected), + strings.TrimSpace(tc.expected), strings.TrimSpace(out.String())) { t.FailNow() } diff --git a/api/internal/accumulator/namereferencetransformer.go b/api/internal/accumulator/namereferencetransformer.go index 324511e88..a94ebcdf0 100644 --- a/api/internal/accumulator/namereferencetransformer.go +++ b/api/internal/accumulator/namereferencetransformer.go @@ -17,6 +17,8 @@ type nameReferenceTransformer struct { backRefs []builtinconfig.NameBackReferences } +const doDebug = false + var _ resmap.Transformer = &nameReferenceTransformer{} type filterMap map[*resource.Resource][]nameref.Filter @@ -47,7 +49,7 @@ func newNameReferenceTransformer( // func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error { fMap := t.determineFilters(m.Resources()) - // debug(fMap) + debug(fMap) for r, fList := range fMap { c := m.SubsetThatCouldBeReferencedByResource(r) for _, f := range fList { @@ -61,8 +63,10 @@ func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error { return nil } -//nolint: deadcode, unused func debug(fMap filterMap) { + if !doDebug { + return + } fmt.Printf("filterMap has %d entries:\n", len(fMap)) rCount := 0 for r, fList := range fMap { @@ -117,6 +121,9 @@ func (t *nameReferenceTransformer) determineFilters( // that might need updating. fMap[res] = append(fMap[res], nameref.Filter{ // Name field to write in the Referrer. + // If the path specified here isn't found in + // the Referrer, nothing happens (no error, + // no field creation). NameFieldToUpdate: referrerSpec, // Specification of object class to read from. // Always read from metadata/name field.