From 2a16af80bf47964e367240833298c1d8dfbd61a2 Mon Sep 17 00:00:00 2001 From: monopole Date: Sat, 16 Jan 2021 14:06:34 -0800 Subject: [PATCH] Simplify, document and add more tests to var replacement. --- api/filters/refvar/doc.go | 2 +- api/filters/refvar/expand.go | 80 +++++++++----- api/filters/refvar/expand_test.go | 103 +++++++++++------- api/filters/refvar/refvar.go | 10 +- api/filters/refvar/refvar_test.go | 30 ++--- api/go.sum | 2 - api/internal/accumulator/refvartransformer.go | 9 +- 7 files changed, 142 insertions(+), 94 deletions(-) diff --git a/api/filters/refvar/doc.go b/api/filters/refvar/doc.go index b4330f3e9..ab3a01d54 100644 --- a/api/filters/refvar/doc.go +++ b/api/filters/refvar/doc.go @@ -1,3 +1,3 @@ // Package refvar contains a kio.Filter implementation of the kustomize -// refvar transformer. +// refvar transformer (find and replace $(FOO) style variables in strings). package refvar diff --git a/api/filters/refvar/expand.go b/api/filters/refvar/expand.go index 4cf5877bb..3bcbd7a53 100644 --- a/api/filters/refvar/expand.go +++ b/api/filters/refvar/expand.go @@ -1,12 +1,12 @@ // Copyright 2019 The Kubernetes Authors. // SPDX-License-Identifier: Apache-2.0 -// Package expansion provides functions find and replace $(FOO) style variables in strings. package refvar import ( - "bytes" "fmt" + "log" + "strings" ) const ( @@ -17,38 +17,64 @@ const ( // syntaxWrap returns the input string wrapped by the expansion syntax. func syntaxWrap(input string) string { - return string(operator) + string(referenceOpener) + input + string(referenceCloser) + var sb strings.Builder + sb.WriteByte(operator) + sb.WriteByte(referenceOpener) + sb.WriteString(input) + sb.WriteByte(referenceCloser) + return sb.String() } -// MappingFuncFor returns a mapping function for use with Expand that -// implements the expansion semantics defined in the expansion spec; it -// returns the input string wrapped in the expansion syntax if no mapping -// for the input is found. -func MappingFuncFor( - counts map[string]int, - context ...map[string]interface{}) func(string) interface{} { - return func(input string) interface{} { - for _, vars := range context { - val, ok := vars[input] - if ok { - counts[input]++ - switch typedV := val.(type) { - case string, int64, float64, bool: - return typedV - default: - return syntaxWrap(input) - } +// MappingFunc maps a string to anything. +type MappingFunc func(string) interface{} + +// MakePrimitiveReplacer returns a MappingFunc that uses a map to do +// replacements, and a histogram to count map hits. +// +// Func behavior: +// +// If the input key is NOT found in the map, the key is wrapped up as +// as a variable declaration string and returned, e.g. key FOO becomes $(FOO). +// This string is presumably put back where it was found, and might get replaced +// later. +// +// If the key is found in the map, the value is returned if it is a primitive +// type (string, bool, number), and the hit is counted. +// +// If it's not a primitive type (e.g. a map, struct, func, etc.) then this +// function doesn't know what to do with it and it returns the key wrapped up +// again as if it had not been replaced. This should probably be an error. +func MakePrimitiveReplacer( + counts map[string]int, someMap map[string]interface{}) MappingFunc { + return func(key string) interface{} { + if value, ok := someMap[key]; ok { + switch typedV := value.(type) { + case string, int, int32, int64, float32, float64, bool: + counts[key]++ + return typedV + default: + // If the value is some complicated type (e.g. a map or struct), + // this function doesn't know how to jam it into a string, + // so just pretend it was a cache miss. + // Likely this should be an error instead of a silent failure, + // since the programmer passed an impossible value. + log.Printf( + "MakePrimitiveReplacer: bad replacement type=%T val=%v", + typedV, typedV) + return syntaxWrap(key) } } - return syntaxWrap(input) + // If unable to return the mapped variable, return it + // as it was found, and a later mapping might be able to + // replace it. + return syntaxWrap(key) } } -// Expand replaces variable references in the input string according to -// the expansion spec using the given mapping function to resolve the -// values of variables. -func Expand(input string, mapping func(string) interface{}) interface{} { - var buf bytes.Buffer +// DoReplacements replaces variable references in the input string +// using the mapping function. +func DoReplacements(input string, mapping MappingFunc) interface{} { + var buf strings.Builder checkpoint := 0 for cursor := 0; cursor < len(input); cursor++ { if input[cursor] == operator && cursor+1 < len(input) { diff --git a/api/filters/refvar/expand_test.go b/api/filters/refvar/expand_test.go index 858507f0f..badba341c 100644 --- a/api/filters/refvar/expand_test.go +++ b/api/filters/refvar/expand_test.go @@ -7,6 +7,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" . "sigs.k8s.io/kustomize/api/filters/refvar" ) @@ -15,6 +16,48 @@ type expected struct { edited string } +func TestPrimitiveReplacer(t *testing.T) { + varCounts := make(map[string]int) + f := MakePrimitiveReplacer( + varCounts, + map[string]interface{}{ + "FOO": "bar", + "ZOO": "$(FOO)-1", + "BLU": "$(ZOO)-2", + "EIGHT": 8, + "PI": 3.14159, + "ZINT": "$(INT)", + "BOOL": "true", + "HUGENUMBER": int64(9223372036854775807), + "CRAZYMAP": map[string]int{"crazy": 200}, + "ZBOOL": "$(BOOL)", + }) + assert.Equal(t, "$()", f("")) + assert.Equal(t, "$( )", f(" ")) + assert.Equal(t, "$(florida)", f("florida")) + assert.Equal(t, "$(0)", f("0")) + assert.Equal(t, "bar", f("FOO")) + assert.Equal(t, "bar", f("FOO")) + assert.Equal(t, "bar", f("FOO")) + assert.Equal(t, 8, f("EIGHT")) + assert.Equal(t, 8, f("EIGHT")) + assert.Equal(t, 3.14159, f("PI")) + assert.Equal(t, "true", f("BOOL")) + assert.Equal(t, int64(9223372036854775807), f("HUGENUMBER")) + assert.Equal(t, "$(FOO)-1", f("ZOO")) + assert.Equal(t, "$(CRAZYMAP)", f("CRAZYMAP")) + assert.Equal(t, + map[string]int{ + "FOO": 3, + "EIGHT": 2, + "BOOL": 1, + "PI": 1, + "ZOO": 1, + "HUGENUMBER": 1, + }, + varCounts) +} + func TestMapReference(t *testing.T) { type env struct { Name string @@ -51,7 +94,7 @@ func TestMapReference(t *testing.T) { }, } - declaredEnv := map[string]interface{}{ + varMap := map[string]interface{}{ "FOO": "bar", "ZOO": "$(FOO)-1", "BLU": "$(ZOO)-2", @@ -61,11 +104,11 @@ func TestMapReference(t *testing.T) { "ZBOOL": "$(BOOL)", } - counts := make(map[string]int) - mapping := MappingFuncFor(counts, declaredEnv) - + varCounts := make(map[string]int) for _, env := range envs { - declaredEnv[env.Name] = Expand(fmt.Sprintf("%v", env.Value), mapping) + varMap[env.Name] = DoReplacements( + fmt.Sprintf("%v", env.Value), + MakePrimitiveReplacer(varCounts, varMap)) } expectedEnv := map[string]expected{ @@ -79,45 +122,20 @@ func TestMapReference(t *testing.T) { } for k, v := range expectedEnv { - if e, a := v, declaredEnv[k]; e.edited != a || e.count != counts[k] { + if e, a := v, varMap[k]; e.edited != a || e.count != varCounts[k] { t.Errorf("Expected %v count=%d, got %v count=%d", - e.edited, e.count, a, counts[k]) + e.edited, e.count, a, varCounts[k]) } else { - delete(declaredEnv, k) + delete(varMap, k) } } - if len(declaredEnv) != 0 { - t.Errorf("Unexpected keys in declared env: %v", declaredEnv) + if len(varMap) != 0 { + t.Errorf("Unexpected keys in declared env: %v", varMap) } } func TestMapping(t *testing.T) { - context := map[string]interface{}{ - "VAR_A": "A", - "VAR_B": "B", - "VAR_C": "C", - "VAR_REF": "$(VAR_A)", - "VAR_EMPTY": "", - } - doExpansionTest(t, context) -} - -func TestMappingDual(t *testing.T) { - context := map[string]interface{}{ - "VAR_A": "A", - "VAR_EMPTY": "", - } - context2 := map[string]interface{}{ - "VAR_B": "B", - "VAR_C": "C", - "VAR_REF": "$(VAR_A)", - } - - doExpansionTest(t, context, context2) -} - -func doExpansionTest(t *testing.T, context ...map[string]interface{}) { cases := []struct { name string input string @@ -333,11 +351,17 @@ func doExpansionTest(t *testing.T, context ...map[string]interface{}) { expected: "\n", }, } - for _, tc := range cases { counts := make(map[string]int) - mapping := MappingFuncFor(counts, context...) - expanded := Expand(fmt.Sprintf("%v", tc.input), mapping) + expanded := DoReplacements( + fmt.Sprintf("%v", tc.input), + MakePrimitiveReplacer(counts, map[string]interface{}{ + "VAR_A": "A", + "VAR_B": "B", + "VAR_C": "C", + "VAR_REF": "$(VAR_A)", + "VAR_EMPTY": "", + })) if e, a := tc.expected, expanded; e != a { t.Errorf("%v: expected %q, got %q", tc.name, e, a) } @@ -347,8 +371,7 @@ func doExpansionTest(t *testing.T, context ...map[string]interface{}) { } if len(tc.counts) > 0 { for k, expectedCount := range tc.counts { - c, ok := counts[k] - if ok { + if c, ok := counts[k]; ok { if c != expectedCount { t.Errorf( "%v: k=%s, expected count %d, got %d", diff --git a/api/filters/refvar/refvar.go b/api/filters/refvar/refvar.go index e7dbca22a..a2b6fa949 100644 --- a/api/filters/refvar/refvar.go +++ b/api/filters/refvar/refvar.go @@ -13,8 +13,8 @@ import ( // Filter updates $(VAR) style variables with values. // The fieldSpecs are the places to look for occurrences of $(VAR). type Filter struct { - MappingFunc func(string) interface{} `json:"mappingFunc,omitempty" yaml:"mappingFunc,omitempty"` - FieldSpec types.FieldSpec `json:"fieldSpec,omitempty" yaml:"fieldSpec,omitempty"` + MappingFunc MappingFunc `json:"mappingFunc,omitempty" yaml:"mappingFunc,omitempty"` + FieldSpec types.FieldSpec `json:"fieldSpec,omitempty" yaml:"fieldSpec,omitempty"` } func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) { @@ -67,7 +67,7 @@ func (f Filter) setScalar(node *yaml.RNode) error { if !yaml.IsYNodeString(node.YNode()) { return nil } - v := Expand(node.YNode().Value, f.MappingFunc) + v := DoReplacements(node.YNode().Value, f.MappingFunc) updateNodeValue(node.YNode(), v) return nil } @@ -83,7 +83,7 @@ func (f Filter) setMap(node *yaml.RNode) error { if !yaml.IsYNodeString(contents[i+1]) { continue } - newValue := Expand(contents[i+1].Value, f.MappingFunc) + newValue := DoReplacements(contents[i+1].Value, f.MappingFunc) updateNodeValue(contents[i+1], newValue) } return nil @@ -94,7 +94,7 @@ func (f Filter) setSeq(node *yaml.RNode) error { if !yaml.IsYNodeString(item) { return fmt.Errorf("invalid value type expect a string") } - newValue := Expand(item.Value, f.MappingFunc) + newValue := DoReplacements(item.Value, f.MappingFunc) updateNodeValue(item, newValue) } return nil diff --git a/api/filters/refvar/refvar_test.go b/api/filters/refvar/refvar_test.go index 120ffab1a..cdaf95278 100644 --- a/api/filters/refvar/refvar_test.go +++ b/api/filters/refvar/refvar_test.go @@ -11,8 +11,12 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) +var makeMf = func(theMap map[string]interface{}) MappingFunc { + ignored := make(map[string]int) + return MakePrimitiveReplacer(ignored, theMap) +} + func TestFilter(t *testing.T) { - replacementCounts := make(map[string]int) testCases := map[string]struct { input string @@ -35,7 +39,7 @@ metadata: spec: replicas: 5`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "VAR": int64(5), }), FieldSpec: types.FieldSpec{Path: "spec/replicas"}, @@ -57,7 +61,7 @@ metadata: spec: replicas: 1`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "VAR": int64(5), }), FieldSpec: types.FieldSpec{Path: "spec/replicas"}, @@ -79,7 +83,7 @@ metadata: spec: replicas: 1`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "VAR": int64(5), }), FieldSpec: types.FieldSpec{Path: "a/b/c"}, @@ -111,7 +115,7 @@ data: - false - 1.23`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "FOO": "foo", "BAR": "bar", "BOOL": false, @@ -142,7 +146,7 @@ data: BAZ: $(BAZ) PLUS: foo+bar`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "FOO": "foo", "BAR": "bar", }), @@ -181,7 +185,7 @@ data: SLICE: - $(FOO)`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "FOO": "foo", "BAR": "bar", }), @@ -204,8 +208,10 @@ metadata: data: FOO: null`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{}), - FieldSpec: types.FieldSpec{Path: "data/FOO"}, + MappingFunc: makeMf(map[string]interface{}{ + // no replacements! + }), + FieldSpec: types.FieldSpec{Path: "data/FOO"}, }, }, } @@ -223,8 +229,6 @@ data: } func TestFilterUnhappy(t *testing.T) { - replacementCounts := make(map[string]int) - testCases := map[string]struct { input string expectedError string @@ -250,7 +254,7 @@ data: - false ' at path 'data/slice': invalid value type expect a string`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "VAR": int64(5), }), FieldSpec: types.FieldSpec{Path: "data/slice"}, @@ -274,7 +278,7 @@ data: 1: str ' at path 'data': invalid map key: value='1', tag='` + yaml.NodeTagInt + `'`, filter: Filter{ - MappingFunc: MappingFuncFor(replacementCounts, map[string]interface{}{ + MappingFunc: makeMf(map[string]interface{}{ "VAR": int64(5), }), FieldSpec: types.FieldSpec{Path: "data"}, diff --git a/api/go.sum b/api/go.sum index 094a2fda9..226e3a721 100644 --- a/api/go.sum +++ b/api/go.sum @@ -599,8 +599,6 @@ mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b h1:DxJ5nJdkhDlLok9K6qO+5290kphD mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b/go.mod h1:2odslEg/xrtNQqCYg2/jCoyKnw3vv5biOc3JnIcYfL4= mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f h1:Cq7MalBHYACRd6EesksG1Q8EoIAKOsiZviGKbOLIej4= mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f/go.mod h1:4G1h5nDURzA3bwVMZIVpwbkw+04kSxk3rAtzlimaUJw= -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/structured-merge-diff v0.0.0-20190525122527-15d366b2352e/go.mod h1:wWxsB5ozmmv/SG7nM11ayaAW51xMvak/t1r0CSlcokI= sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= sigs.k8s.io/yaml v1.2.0 h1:kr/MCeFWJWTwyaHoR9c8EjH9OumOmoF9YGiZd7lFm/Q= diff --git a/api/internal/accumulator/refvartransformer.go b/api/internal/accumulator/refvartransformer.go index 891dc370d..a02edc4fb 100644 --- a/api/internal/accumulator/refvartransformer.go +++ b/api/internal/accumulator/refvartransformer.go @@ -13,7 +13,6 @@ type refVarTransformer struct { varMap map[string]interface{} replacementCounts map[string]int fieldSpecs []types.FieldSpec - mappingFunc func(string) interface{} } // newRefVarTransformer returns a new refVarTransformer @@ -32,8 +31,7 @@ func newRefVarTransformer( func (rv *refVarTransformer) UnusedVars() []string { var unused []string for k := range rv.varMap { - _, ok := rv.replacementCounts[k] - if !ok { + if _, ok := rv.replacementCounts[k]; !ok { unused = append(unused, k) } } @@ -43,12 +41,11 @@ func (rv *refVarTransformer) UnusedVars() []string { // Transform replaces $(VAR) style variables with values. func (rv *refVarTransformer) Transform(m resmap.ResMap) error { rv.replacementCounts = make(map[string]int) - rv.mappingFunc = refvar.MappingFuncFor( - rv.replacementCounts, rv.varMap) + mf := refvar.MakePrimitiveReplacer(rv.replacementCounts, rv.varMap) for _, res := range m.Resources() { for _, fieldSpec := range rv.fieldSpecs { err := res.ApplyFilter(refvar.Filter{ - MappingFunc: rv.mappingFunc, + MappingFunc: mf, FieldSpec: fieldSpec, }) if err != nil {