Merge "Replacement transformer refactoring"

pkg/document/plugin/replacement/v1alpha1/errors.go

@@ -0,0 +1,93 @@
+/*
+ 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
+
+     https://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 v1alpha1
+
+import (
+	"fmt"
+	"strings"
+
+	"sigs.k8s.io/kustomize/api/resource"
+	"sigs.k8s.io/kustomize/api/types"
+)
+
+// ErrTypeMismatch is returned for type conversion errors. This error
+// is raised if JSON path element points to a wrong data structure e.g.
+// JSON path 'a.b[x=y]c' considers that there is a list of maps under key 'b'
+// therefore ErrTypeMismatch will be returned for following structure
+// a:
+//   b:
+//     - 'some string'
+type ErrTypeMismatch struct {
+	Actual      interface{}
+	Expectation string
+}
+
+func (e ErrTypeMismatch) Error() string {
+	return fmt.Sprintf("%#v %s", e.Actual, e.Expectation)
+}
+
+// ErrBadConfiguration returned in case of plugin misconfiguration
+type ErrBadConfiguration struct {
+	Msg string
+}
+
+func (e ErrBadConfiguration) Error() string {
+	return e.Msg
+}
+
+// ErrMultipleResources returned if multiple resources were found
+type ErrMultipleResources struct {
+	ResList []*resource.Resource
+}
+
+func (e ErrMultipleResources) Error() string {
+	return fmt.Sprintf("found more than one resources matching from %v", e.ResList)
+}
+
+// ErrResourceNotFound returned if resource does not exist in resource map
+type ErrResourceNotFound struct {
+	ObjRef *types.Target
+}
+
+func (e ErrResourceNotFound) Error() string {
+	keys := [5]string{"Group:", "Version:", "Kind:", "Name:", "Namespace:"}
+	values := [5]string{e.ObjRef.Group, e.ObjRef.Version, e.ObjRef.Kind, e.ObjRef.Name, e.ObjRef.Namespace}
+
+	var resFilter string
+	for i, key := range keys {
+		if values[i] != "" {
+			resFilter += key + values[i] + " "
+		}
+	}
+	return fmt.Sprintf("failed to find any resources identified by %s", strings.TrimSpace(resFilter))
+}
+
+// ErrPatternSubstring returned in case of issues with sub-string pattern substitution
+type ErrPatternSubstring struct {
+	Msg string
+}
+
+func (e ErrPatternSubstring) Error() string {
+	return e.Msg
+}
+
+// ErrIndexOutOfBound returned if JSON path points to a wrong index of a list
+type ErrIndexOutOfBound struct {
+	Index int
+}
+
+func (e ErrIndexOutOfBound) Error() string {
+	return fmt.Sprintf("index %v is out of bound", e.Index)
+}

pkg/document/plugin/replacement/v1alpha1/transformer.go

@@ -79,35 +79,31 @@ func (p *plugin) Run(in io.Reader, out io.Writer) error {
 	return nil
 }
 
+// Config function reads replacements configuration
 func (p *plugin) Config(
-	_ *resmap.PluginHelpers, c []byte) (err error) {
+	_ *resmap.PluginHelpers, c []byte) error {
 	p.Replacements = []types.Replacement{}
-	err = yaml.Unmarshal(c, p)
+	err := yaml.Unmarshal(c, p)
 	if err != nil {
 		return err
 	}
 	for _, r := range p.Replacements {
 		if r.Source == nil {
-			return fmt.Errorf("`from` must be specified in one replacement")
+			return ErrBadConfiguration{Msg: "`from` must be specified in one replacement"}
 		}
 		if r.Target == nil {
-			return fmt.Errorf("`to` must be specified in one replacement")
+			return ErrBadConfiguration{Msg: "`to` must be specified in one replacement"}
 		}
-		count := 0
+		if r.Source.ObjRef != nil && r.Source.Value != "" {
-		if r.Source.ObjRef != nil {
+			return ErrBadConfiguration{Msg: "only one of fieldref and value is allowed in one replacement"}
-			count += 1
-		}
-		if r.Source.Value != "" {
-			count += 1
-		}
-		if count > 1 {
-			return fmt.Errorf("only one of fieldref and value is allowed in one replacement")
 		}
 	}
 	return nil
 }
 
-func (p *plugin) Transform(m resmap.ResMap) (err error) {
+// Transform resources using configured replacements
+func (p *plugin) Transform(m resmap.ResMap) error {
+	var err error
 	for _, r := range p.Replacements {
 		var replacement interface{}
 		if r.Source.ObjRef != nil {
@@ -119,8 +115,7 @@ func (p *plugin) Transform(m resmap.ResMap) (err error) {
 		if r.Source.Value != "" {
 			replacement = r.Source.Value
 		}
-		err = substitute(m, r.Target, replacement)
+		if err = substitute(m, r.Target, replacement); err != nil {
-		if err != nil {
 			return err
 		}
 	}
@@ -135,16 +130,16 @@ func getReplacement(m resmap.ResMap, objRef *types.Target, fieldRef string) (int
 	}
 	resources, err := m.Select(s)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 	if len(resources) > 1 {
-		return "", fmt.Errorf("found more than one resources matching from %v", resources)
+		return nil, ErrMultipleResources{ResList: resources}
 	}
 	if len(resources) == 0 {
-		return "", fmt.Errorf("failed to find one resource matching from %v", objRef)
+		return nil, ErrResourceNotFound{ObjRef: objRef}
 	}
 	if fieldRef == "" {
-		fieldRef = ".metadata.name"
+		fieldRef = "metadata.name"
 	}
 	return resources[0].GetFieldValue(fieldRef)
 }
@@ -165,7 +160,7 @@ func substitute(m resmap.ResMap, to *types.ReplTarget, replacement interface{})
 	return nil
 }
 
-func getFirstPathSegment(path string) (field string, key string, value string, array bool) {
+func getFirstPathSegment(path string) (field string, key string, value string, isArray bool) {
 	groups := pattern.FindStringSubmatch(path)
 	if len(groups) != 4 {
 		return path, "", "", false
@@ -184,20 +179,17 @@ func updateField(m interface{}, pathToField []string, replacement interface{}) e
 	case []interface{}:
 		return updateSliceField(typedM, pathToField, replacement)
 	default:
-		return fmt.Errorf("%#v is not expected to be a primitive type", typedM)
+		return ErrTypeMismatch{Actual: typedM, Expectation: "is not expected be a primitive type"}
 	}
 }
 
 // Extract the substring pattern (if present) from the target path spec
-//nolint:unparam // TODO (dukov) refactor this or remove
+func extractSubstringPattern(path string) (extractedPath string, substringPattern string) {
-func extractSubstringPattern(path string) (extractedPath string, substringPattern string, err error) {
-	substringPattern = ""
 	groups := substringPatternRegex.FindStringSubmatch(path)
-	if groups != nil {
+	if len(groups) != 3 {
-		path = groups[1]
+		return path, ""
-		substringPattern = groups[2]
 	}
-	return path, substringPattern, nil
+	return groups[1], groups[2]
 }
 
 // apply a substring substitution based on a pattern
@@ -208,116 +200,106 @@ func applySubstringPattern(target interface{}, replacement interface{},
 		return replacement, nil
 	}
 
-	switch replacement.(type) {
+	repl, ok := replacement.(string)
-	case string:
+	if !ok {
-	default:
+		return nil, ErrPatternSubstring{Msg: "pattern-based substitution can only be applied with string replacement values"}
-		return nil, fmt.Errorf("pattern-based substitution can only be applied with string replacement values")
 	}
 
-	switch target.(type) {
+	tgt, ok := target.(string)
-	case string:
+	if !ok {
-	default:
+		return nil, ErrPatternSubstring{Msg: "pattern-based substitution can only be applied to string target fields"}
-		return nil, fmt.Errorf("pattern-based substitution can only be applied to string target fields")
 	}
 
 	p := regexp.MustCompile(substringPattern)
-	if !p.MatchString(target.(string)) {
+	if !p.MatchString(tgt) {
-		return nil, fmt.Errorf("pattern %s not found in target value %s", substringPattern, target.(string))
+		return nil, ErrPatternSubstring{
+			Msg: fmt.Sprintf("pattern '%s' is defined in configuration but was not found in target value %s",
+				substringPattern, tgt),
+		}
 	}
-	return p.ReplaceAllString(target.(string), replacement.(string)), nil
+	return p.ReplaceAllString(tgt, repl), nil
 }
 
-//nolint:gocyclo // TODO (dukov) Refactor this or remove
 func updateMapField(m map[string]interface{}, pathToField []string, replacement interface{}) error {
 	path, key, value, isArray := getFirstPathSegment(pathToField[0])
 
-	path, substringPattern, err := extractSubstringPattern(path)
+	path, substringPattern := extractSubstringPattern(path)
-	if err != nil {
-		return err
-	}
 
 	v, found := m[path]
 	if !found {
-		m[path] = map[string]interface{}{}
+		m[path] = make(map[string]interface{})
 		v = m[path]
 	}
 
+	if v == nil {
+		return ErrTypeMismatch{Actual: v, Expectation: "is not expected be nil"}
+	}
+
 	if len(pathToField) == 1 {
 		if !isArray {
-			replacement, err = applySubstringPattern(m[path], replacement, substringPattern)
+			renderedRepl, err := applySubstringPattern(m[path], replacement, substringPattern)
 			if err != nil {
 				return err
 			}
-			m[path] = replacement
+			m[path] = renderedRepl
 			return nil
 		}
 		switch typedV := v.(type) {
-		case nil:
-			fmt.Printf("nil value at `%s` ignored in mutation attempt", strings.Join(pathToField, "."))
 		case []interface{}:
-			for i := range typedV {
+			for _, item := range typedV {
-				item := typedV[i]
 				typedItem, ok := item.(map[string]interface{})
 				if !ok {
-					return fmt.Errorf("%#v is expected to be %T", item, typedItem)
+					return ErrTypeMismatch{Actual: item, Expectation: fmt.Sprintf("is expected to be %T", typedItem)}
 				}
 				if actualValue, ok := typedItem[key]; ok {
 					if value == actualValue {
+						// TODO (dukov) should not we do 'item = replacement' here?
 						typedItem[key] = value
 					}
 				}
 			}
 		default:
-			return fmt.Errorf("%#v is not expected to be a primitive type", typedV)
+			return ErrTypeMismatch{Actual: typedV, Expectation: "is not expected be a primitive type"}
 		}
 	}
 
-	newPathToField := pathToField[1:]
+	if isArray {
-	switch typedV := v.(type) {
+		return updateField(v, pathToField, replacement)
-	case nil:
-		fmt.Printf(
-			"nil value at `%s` ignored in mutation attempt",
-			strings.Join(pathToField, "."))
-		return nil
-	case map[string]interface{}:
-		return updateField(typedV, newPathToField, replacement)
-	case []interface{}:
-		if !isArray {
-			return updateField(typedV, newPathToField, replacement)
-		}
-		for i := range typedV {
-			item := typedV[i]
-			typedItem, ok := item.(map[string]interface{})
-			if !ok {
-				return fmt.Errorf("%#v is expected to be %T", item, typedItem)
-			}
-			if actualValue, ok := typedItem[key]; ok {
-				if value == actualValue {
-					return updateField(typedItem, newPathToField, replacement)
-				}
-			}
-		}
-	default:
-		return fmt.Errorf("%#v is not expected to be a primitive type", typedV)
 	}
-	return nil
+	return updateField(v, pathToField[1:], replacement)
 }
 
 func updateSliceField(m []interface{}, pathToField []string, replacement interface{}) error {
 	if len(pathToField) == 0 {
 		return nil
 	}
+	_, key, value, isArray := getFirstPathSegment(pathToField[0])
+
+	if isArray {
+		for _, item := range m {
+			typedItem, ok := item.(map[string]interface{})
+			if !ok {
+				return ErrTypeMismatch{Actual: item, Expectation: fmt.Sprintf("is expected to be %T", typedItem)}
+			}
+			if actualValue, ok := typedItem[key]; ok {
+				if value == actualValue {
+					return updateField(typedItem, pathToField[1:], replacement)
+				}
+			}
+		}
+	}
+
 	index, err := strconv.Atoi(pathToField[0])
 	if err != nil {
 		return err
 	}
-	if len(m) > index && index >= 0 {
+
-		if len(pathToField) == 1 {
+	if len(m) < index || index < 0 {
-			m[index] = replacement
+		return ErrIndexOutOfBound{Index: index}
-			return nil
-		} else {
-			return updateField(m[index], pathToField[1:], replacement)
-		}
 	}
-	return fmt.Errorf("index %v is out of bound", index)
+	if len(pathToField) == 1 {
+		m[index] = replacement
+		return nil
+	}
+	return updateField(m[index], pathToField[1:], replacement)
 }

pkg/document/plugin/replacement/v1alpha1/transformer_test.go

@@ -17,7 +17,7 @@ import (
 
 func samplePlugin(t *testing.T) plugtypes.Plugin {
 	plugin, err := replv1alpha1.New(nil, []byte(`
-apiVersion: airshipit.org/v1beta1
+apiVersion: airshipit.org/v1alpha1
 kind: ReplacementTransformer
 metadata:
   name: notImportantHere
@@ -74,10 +74,11 @@ func TestReplacementTransformer(t *testing.T) {
 		cfg         string
 		in          string
 		expectedOut string
+		expectedErr string
 	}{
 		{
 			cfg: `
-apiVersion: airshipit.org/v1beta1
+apiVersion: airshipit.org/v1alpha1
 kind: ReplacementTransformer
 metadata:
   name: notImportantHere
@@ -152,7 +153,7 @@ spec:
 		},
 		{
 			cfg: `
-apiVersion: airshipit.org/v1beta1
+apiVersion: airshipit.org/v1alpha1
 kind: ReplacementTransformer
 metadata:
   name: notImportantHere
@@ -221,7 +222,7 @@ spec:
 		},
 		{
 			cfg: `
-apiVersion: airshipit.org/v1beta1
+apiVersion: airshipit.org/v1alpha1
 kind: ReplacementTransformer
 metadata:
   name: notImportantHere
@@ -320,7 +321,7 @@ metadata:
 		},
 		{
 			cfg: `
-apiVersion: airshipit.org/v1beta1
+apiVersion: airshipit.org/v1alpha1
 kind: ReplacementTransformer
 metadata:
   name: notImportantHere
@@ -390,15 +391,410 @@ spec:
         name: init-alpine
 `,
 		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Pod
+      name: pod2
+    fieldrefs:
+    - spec.non.existent.field`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedOut: `apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - image: busybox
+    name: myapp-container
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod2
+spec:
+  containers:
+  - image: busybox
+    name: myapp-container
+  non:
+    existent:
+      field: pod1
+`,
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+  target:
+    objref:
+      kind: Deployment
+    fieldrefs:
+    - spec.template.spec.containers[name=nginx-latest].image`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedErr: "found more than one resources matching from " +
+				"[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"metadata\":{\"name\":\"pod1\"}," +
+				"\"spec\":{\"containers\":[{\"image\":\"busybox\",\"name\":\"myapp-container\"" +
+				"}]}}{nsfx:false,beh:unspecified} {\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"metadata\":" +
+				"{\"name\":\"pod2\"},\"spec\":{\"containers\":[{\"image\":\"busybox\",\"name\":\"myapp-container\"}]}}" +
+				"{nsfx:false,beh:unspecified}]",
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+      namespace: default
+  target:
+    objref:
+      kind: Deployment
+    fieldrefs:
+    - spec.template.spec.containers[name=nginx-latest].image`,
+			expectedErr: "failed to find any resources identified by Kind:Pod Name:pod1 Namespace:default",
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Pod
+      name: pod2
+    fieldrefs:
+    - labels.somelabel.key1.subkey1`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+labels:
+  somelabel: 'some string value'
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedErr: `"some string value" is not expected be a primitive type`,
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Pod
+      name: pod2
+    fieldrefs:
+    - labels.somelabel[subkey1=val1]`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+labels:
+  somelabel: 'some string value'
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedErr: `"some string value" is not expected be a primitive type`,
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Pod
+      name: pod2
+    fieldrefs:
+    - spec[subkey1=val1]`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+labels:
+  somelabel: 'some string value'
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedErr: "map[string]interface {}{\"containers\":[]interface " +
+				"{}{map[string]interface {}{\"image\":\"busybox\", \"name\":\"myapp-container\"}}} " +
+				"is not expected be a primitive type",
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Pod
+      name: pod2
+    fieldrefs:
+    - spec.containers.10`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+labels:
+  somelabel: 'some string value'
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedErr: "index 10 is out of bound",
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Pod
+      name: pod2
+    fieldrefs:
+    - spec.containers.notInteger.name`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+apiVersion: v1
+kind: Pod
+labels:
+  somelabel: 'some string value'
+metadata:
+  name: pod2
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox`,
+			expectedErr: `strconv.Atoi: parsing "notInteger": invalid syntax`,
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Deployment
+    fieldrefs:
+    - spec.template.spec.containers%TAG%`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+group: apps
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy1
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:TAG
+        name: nginx-latest`,
+			expectedErr: "pattern-based substitution can only be applied to string target fields",
+		},
+		{
+			cfg: `
+apiVersion: airshipit.org/v1alpha1
+kind: ReplacementTransformer
+metadata:
+  name: notImportantHere
+replacements:
+- source:
+    objref:
+      kind: Pod
+      name: pod1
+  target:
+    objref:
+      kind: Deployment
+    fieldrefs:
+    - spec.template.spec.containers[name=nginx-latest].image%TAG%`,
+			in: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod1
+spec:
+  containers:
+  - name: myapp-container
+    image: busybox
+---
+group: apps
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy1
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:latest
+        name: nginx-latest`,
+			expectedErr: "pattern 'TAG' is defined in configuration but was not found in target value nginx:latest",
+		},
 	}
 
 	for _, tc := range testCases {
-		plugun, err := replv1alpha1.New(nil, []byte(tc.cfg))
+		plugin, err := replv1alpha1.New(nil, []byte(tc.cfg))
 		require.NoError(t, err)
 
 		buf := &bytes.Buffer{}
-		err = plugun.Run(strings.NewReader(tc.in), buf)
+		err = plugin.Run(strings.NewReader(tc.in), buf)
-		require.NoError(t, err)
+		errString := ""
+		if err != nil {
+			errString = err.Error()
+		}
+		assert.Equal(t, tc.expectedErr, errString)
 		assert.Equal(t, tc.expectedOut, buf.String())
 	}
 }

pkg/document/plugin/replacement/v1alpha1/types.go

@@ -18,9 +18,6 @@ import (
 	"sigs.k8s.io/kustomize/api/types"
 )
 
-//noinspection GoUnusedGlobalVariable
-var KustomizePlugin plugin
-
 // Find matching image declarations and replace
 // the name, tag and/or digest.
 type plugin struct {