Replacement transformer refactoring

* Apply recommendations from "Effective Go"
* Simplify applySubstringPattern logic
* Reduce complexity of updateMapField
* Switch to typed errors
* Increased test coverage

Change-Id: I8e53a251a43c8f31c286284c77452fbf43ce4e43
This commit is contained in:
Dmitry Ukov 2020-04-22 11:50:12 +04:00
parent 6c716e1a57
commit c30930114a
4 changed files with 569 additions and 101 deletions

View File

@ -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)
}

View File

@ -79,35 +79,31 @@ func (p *plugin) Run(in io.Reader, out io.Writer) error {
return nil return nil
} }
// Config function reads replacements configuration
func (p *plugin) Config( func (p *plugin) Config(
_ *resmap.PluginHelpers, c []byte) (err error) { _ *resmap.PluginHelpers, c []byte) error {
p.Replacements = []types.Replacement{} p.Replacements = []types.Replacement{}
err = yaml.Unmarshal(c, p) err := yaml.Unmarshal(c, p)
if err != nil { if err != nil {
return err return err
} }
for _, r := range p.Replacements { for _, r := range p.Replacements {
if r.Source == nil { 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 { 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 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 { for _, r := range p.Replacements {
var replacement interface{} var replacement interface{}
if r.Source.ObjRef != nil { if r.Source.ObjRef != nil {
@ -119,8 +115,7 @@ func (p *plugin) Transform(m resmap.ResMap) (err error) {
if r.Source.Value != "" { if r.Source.Value != "" {
replacement = 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 return err
} }
} }
@ -135,16 +130,16 @@ func getReplacement(m resmap.ResMap, objRef *types.Target, fieldRef string) (int
} }
resources, err := m.Select(s) resources, err := m.Select(s)
if err != nil { if err != nil {
return "", err return nil, err
} }
if len(resources) > 1 { 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 { if len(resources) == 0 {
return "", fmt.Errorf("failed to find one resource matching from %v", objRef) return nil, ErrResourceNotFound{ObjRef: objRef}
} }
if fieldRef == "" { if fieldRef == "" {
fieldRef = ".metadata.name" fieldRef = "metadata.name"
} }
return resources[0].GetFieldValue(fieldRef) return resources[0].GetFieldValue(fieldRef)
} }
@ -165,7 +160,7 @@ func substitute(m resmap.ResMap, to *types.ReplTarget, replacement interface{})
return nil 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) groups := pattern.FindStringSubmatch(path)
if len(groups) != 4 { if len(groups) != 4 {
return path, "", "", false return path, "", "", false
@ -184,20 +179,17 @@ func updateField(m interface{}, pathToField []string, replacement interface{}) e
case []interface{}: case []interface{}:
return updateSliceField(typedM, pathToField, replacement) return updateSliceField(typedM, pathToField, replacement)
default: 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 // 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) 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 // apply a substring substitution based on a pattern
@ -208,116 +200,106 @@ func applySubstringPattern(target interface{}, replacement interface{},
return replacement, nil 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) 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 { func updateMapField(m map[string]interface{}, pathToField []string, replacement interface{}) error {
path, key, value, isArray := getFirstPathSegment(pathToField[0]) 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] v, found := m[path]
if !found { if !found {
m[path] = map[string]interface{}{} m[path] = make(map[string]interface{})
v = m[path] v = m[path]
} }
if v == nil {
return ErrTypeMismatch{Actual: v, Expectation: "is not expected be nil"}
}
if len(pathToField) == 1 { if len(pathToField) == 1 {
if !isArray { if !isArray {
replacement, err = applySubstringPattern(m[path], replacement, substringPattern) renderedRepl, err := applySubstringPattern(m[path], replacement, substringPattern)
if err != nil { if err != nil {
return err return err
} }
m[path] = replacement m[path] = renderedRepl
return nil return nil
} }
switch typedV := v.(type) { switch typedV := v.(type) {
case nil:
fmt.Printf("nil value at `%s` ignored in mutation attempt", strings.Join(pathToField, "."))
case []interface{}: case []interface{}:
for i := range typedV { for _, item := range typedV {
item := typedV[i]
typedItem, ok := item.(map[string]interface{}) typedItem, ok := item.(map[string]interface{})
if !ok { 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 actualValue, ok := typedItem[key]; ok {
if value == actualValue { if value == actualValue {
// TODO (dukov) should not we do 'item = replacement' here?
typedItem[key] = value typedItem[key] = value
} }
} }
} }
default: 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 { func updateSliceField(m []interface{}, pathToField []string, replacement interface{}) error {
if len(pathToField) == 0 { if len(pathToField) == 0 {
return nil 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]) index, err := strconv.Atoi(pathToField[0])
if err != nil { if err != nil {
return err 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)
} }

View File

@ -17,7 +17,7 @@ import (
func samplePlugin(t *testing.T) plugtypes.Plugin { func samplePlugin(t *testing.T) plugtypes.Plugin {
plugin, err := replv1alpha1.New(nil, []byte(` plugin, err := replv1alpha1.New(nil, []byte(`
apiVersion: airshipit.org/v1beta1 apiVersion: airshipit.org/v1alpha1
kind: ReplacementTransformer kind: ReplacementTransformer
metadata: metadata:
name: notImportantHere name: notImportantHere
@ -74,10 +74,11 @@ func TestReplacementTransformer(t *testing.T) {
cfg string cfg string
in string in string
expectedOut string expectedOut string
expectedErr string
}{ }{
{ {
cfg: ` cfg: `
apiVersion: airshipit.org/v1beta1 apiVersion: airshipit.org/v1alpha1
kind: ReplacementTransformer kind: ReplacementTransformer
metadata: metadata:
name: notImportantHere name: notImportantHere
@ -152,7 +153,7 @@ spec:
}, },
{ {
cfg: ` cfg: `
apiVersion: airshipit.org/v1beta1 apiVersion: airshipit.org/v1alpha1
kind: ReplacementTransformer kind: ReplacementTransformer
metadata: metadata:
name: notImportantHere name: notImportantHere
@ -221,7 +222,7 @@ spec:
}, },
{ {
cfg: ` cfg: `
apiVersion: airshipit.org/v1beta1 apiVersion: airshipit.org/v1alpha1
kind: ReplacementTransformer kind: ReplacementTransformer
metadata: metadata:
name: notImportantHere name: notImportantHere
@ -320,7 +321,7 @@ metadata:
}, },
{ {
cfg: ` cfg: `
apiVersion: airshipit.org/v1beta1 apiVersion: airshipit.org/v1alpha1
kind: ReplacementTransformer kind: ReplacementTransformer
metadata: metadata:
name: notImportantHere name: notImportantHere
@ -390,15 +391,410 @@ spec:
name: init-alpine 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 { for _, tc := range testCases {
plugun, err := replv1alpha1.New(nil, []byte(tc.cfg)) plugin, err := replv1alpha1.New(nil, []byte(tc.cfg))
require.NoError(t, err) require.NoError(t, err)
buf := &bytes.Buffer{} 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()) assert.Equal(t, tc.expectedOut, buf.String())
} }
} }

View File

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