From c30930114aa418ffac89617db1c2f633572c2692 Mon Sep 17 00:00:00 2001 From: Dmitry Ukov Date: Wed, 22 Apr 2020 11:50:12 +0400 Subject: [PATCH] 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 --- .../plugin/replacement/v1alpha1/errors.go | 93 ++++ .../replacement/v1alpha1/transformer.go | 154 +++---- .../replacement/v1alpha1/transformer_test.go | 412 +++++++++++++++++- .../plugin/replacement/v1alpha1/types.go | 3 - 4 files changed, 565 insertions(+), 97 deletions(-) create mode 100644 pkg/document/plugin/replacement/v1alpha1/errors.go diff --git a/pkg/document/plugin/replacement/v1alpha1/errors.go b/pkg/document/plugin/replacement/v1alpha1/errors.go new file mode 100644 index 00000000..296d9db9 --- /dev/null +++ b/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) +} diff --git a/pkg/document/plugin/replacement/v1alpha1/transformer.go b/pkg/document/plugin/replacement/v1alpha1/transformer.go index c32d2515..15aba543 100644 --- a/pkg/document/plugin/replacement/v1alpha1/transformer.go +++ b/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 { - 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") + if r.Source.ObjRef != nil && r.Source.Value != "" { + return ErrBadConfiguration{Msg: "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 != nil { + if err = substitute(m, r.Target, replacement); 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, err error) { - substringPattern = "" +func extractSubstringPattern(path string) (extractedPath string, substringPattern string) { groups := substringPatternRegex.FindStringSubmatch(path) - if groups != nil { - path = groups[1] - substringPattern = groups[2] + if len(groups) != 3 { + return path, "" } - 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) { - case string: - default: - return nil, fmt.Errorf("pattern-based substitution can only be applied with string replacement values") + repl, ok := replacement.(string) + if !ok { + return nil, ErrPatternSubstring{Msg: "pattern-based substitution can only be applied with string replacement values"} } - switch target.(type) { - case string: - default: - return nil, fmt.Errorf("pattern-based substitution can only be applied to string target fields") + tgt, ok := target.(string) + if !ok { + return nil, ErrPatternSubstring{Msg: "pattern-based substitution can only be applied to string target fields"} } p := regexp.MustCompile(substringPattern) - if !p.MatchString(target.(string)) { - return nil, fmt.Errorf("pattern %s not found in target value %s", substringPattern, target.(string)) + if !p.MatchString(tgt) { + 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) - if err != nil { - return err - } + path, substringPattern := extractSubstringPattern(path) 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 { - item := typedV[i] + for _, item := range typedV { 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:] - switch typedV := v.(type) { - case nil: - fmt.Printf( - "nil value at `%s` ignored in mutation attempt", - strings.Join(pathToField, ".")) + if isArray { + return updateField(v, pathToField, replacement) + } + return updateField(v, pathToField[1:], replacement) +} + +func updateSliceField(m []interface{}, pathToField []string, replacement interface{}) error { + if len(pathToField) == 0 { 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] + } + _, key, value, isArray := getFirstPathSegment(pathToField[0]) + + if isArray { + for _, item := range m { 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 { - return updateField(typedItem, newPathToField, replacement) + return updateField(typedItem, pathToField[1:], replacement) } } } - default: - return fmt.Errorf("%#v is not expected to be a primitive type", typedV) } - return nil -} -func updateSliceField(m []interface{}, pathToField []string, replacement interface{}) error { - if len(pathToField) == 0 { - return nil - } index, err := strconv.Atoi(pathToField[0]) if err != nil { return err } - if len(m) > index && index >= 0 { - if len(pathToField) == 1 { - m[index] = replacement - return nil - } else { - return updateField(m[index], pathToField[1:], replacement) - } + + if len(m) < index || index < 0 { + return ErrIndexOutOfBound{Index: index} + } + if len(pathToField) == 1 { + m[index] = replacement + return nil } - return fmt.Errorf("index %v is out of bound", index) + return updateField(m[index], pathToField[1:], replacement) } diff --git a/pkg/document/plugin/replacement/v1alpha1/transformer_test.go b/pkg/document/plugin/replacement/v1alpha1/transformer_test.go index 71c7b7a6..97495032 100644 --- a/pkg/document/plugin/replacement/v1alpha1/transformer_test.go +++ b/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) - require.NoError(t, err) + err = plugin.Run(strings.NewReader(tc.in), buf) + errString := "" + if err != nil { + errString = err.Error() + } + assert.Equal(t, tc.expectedErr, errString) assert.Equal(t, tc.expectedOut, buf.String()) } } diff --git a/pkg/document/plugin/replacement/v1alpha1/types.go b/pkg/document/plugin/replacement/v1alpha1/types.go index 4a7be1dd..f0fb7c7d 100644 --- a/pkg/document/plugin/replacement/v1alpha1/types.go +++ b/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 {