Replacements should throw errors on invalid targets (kubernetes-sigs#…

…4789) * Replacements should throw errors on invalid targets * Refactor to satisfy complexity linter * Move new tests to filter suite
elisshafer · Dec 8, 2022 · 0a834b2 · 0a834b2
1 parent f396453
commit 0a834b2
Show file tree

Hide file tree

Showing 2 changed files with 131 additions and 59 deletions.
diff --git a/api/filters/replacement/replacement.go b/api/filters/replacement/replacement.go
@@ -4,13 +4,13 @@
 package replacement
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
 	"sigs.k8s.io/kustomize/api/internal/utils"
 	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/kustomize/api/types"
+	"sigs.k8s.io/kustomize/kyaml/errors"
 	"sigs.k8s.io/kustomize/kyaml/resid"
 	kyaml_utils "sigs.k8s.io/kustomize/kyaml/utils"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
@@ -105,7 +105,7 @@ func getRefinedValue(options *types.FieldOptions, rn *yaml.RNode) (*yaml.RNode,
 func applyReplacement(nodes []*yaml.RNode, value *yaml.RNode, targetSelectors []*types.TargetSelector) ([]*yaml.RNode, error) {
 	for _, selector := range targetSelectors {
 		if selector.Select == nil {
-			return nil, errors.New("target must specify resources to select")
+			return nil, errors.Errorf("target must specify resources to select")
 		}
 		if len(selector.FieldPaths) == 0 {
 			selector.FieldPaths = []string{types.DefaultReplacementFieldPath}
@@ -179,37 +179,61 @@ func rejectId(rejects []*types.Selector, id *resid.ResId) bool {
 
 func copyValueToTarget(target *yaml.RNode, value *yaml.RNode, selector *types.TargetSelector) error {
 	for _, fp := range selector.FieldPaths {
-		fieldPath := kyaml_utils.SmarterPathSplitter(fp, ".")
-		create, err := shouldCreateField(selector.Options, fieldPath)
-		if err != nil {
+		mutator := updateMatchingFields
+		if selector.Options != nil && selector.Options.Create {
+			mutator = createOrUpdateOneField
+		}
+		if err := mutator(target, value, selector.Options, fp); err != nil {
 			return err
 		}
+	}
+	return nil
+}
 
-		var targetFields []*yaml.RNode
-		if create {
-			createdField, createErr := target.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
-			if createErr != nil {
-				return fmt.Errorf("error creating replacement node: %w", createErr)
-			}
-			targetFields = append(targetFields, createdField)
-		} else {
-			// may return multiple fields, always wrapped in a sequence node
-			foundFieldSequence, lookupErr := target.Pipe(&yaml.PathMatcher{Path: fieldPath})
-			if lookupErr != nil {
-				return fmt.Errorf("error finding field in replacement target: %w", lookupErr)
-			}
-			targetFields, err = foundFieldSequence.Elements()
-			if err != nil {
-				return fmt.Errorf("error fetching elements in replacement target: %w", err)
-			}
-		}
+// updateMatchingFields updates all fields in target that match the given field path.
+// If the field path does not already exist in the target, an error is returned.
+func updateMatchingFields(target *yaml.RNode, value *yaml.RNode, opts *types.FieldOptions, fp string) error {
+	fieldPath := kyaml_utils.SmarterPathSplitter(fp, ".")
+	// may return multiple fields, always wrapped in a sequence node
+	foundFieldSequence, lookupErr := target.Pipe(&yaml.PathMatcher{Path: fieldPath})
+	if lookupErr != nil {
+		return fmt.Errorf("error finding field in replacement target: %w", lookupErr)
+	}
+	targetFields, err := foundFieldSequence.Elements()
+	if err != nil {
+		return fmt.Errorf("error fetching elements in replacement target: %w", err)
+	}
+	if len(targetFields) == 0 {
+		return errors.Errorf("unable to find field %s in replacement target", fp)
+	}
 
-		for _, t := range targetFields {
-			if err := setFieldValue(selector.Options, t, value); err != nil {
-				return err
-			}
+	for _, t := range targetFields {
+		if err := setFieldValue(opts, t, value); err != nil {
+			return err
 		}
+	}
+	return nil
+}
 
+// createOrUpdateOneField updates the field in the target that matches the given field path.
+// If the field path does not already exist in the target, it is created.
+// Wildcard matching is not supported, nor is creating intermediate list nodes.
+func createOrUpdateOneField(target *yaml.RNode, value *yaml.RNode, opts *types.FieldOptions, fp string) error {
+	fieldPath := kyaml_utils.SmarterPathSplitter(fp, ".")
+	for _, f := range fieldPath {
+		if f == "*" {
+			return fmt.Errorf("cannot support create option in a multi-value target")
+		}
+	}
+	createdField, createErr := target.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
+	if createErr != nil {
+		return fmt.Errorf("error creating replacement node: %w", createErr)
+	}
+	if createdField == nil {
+		return errors.Errorf("unable to find or create field %s in replacement target", fp)
+	}
+	if err := setFieldValue(opts, createdField, value); err != nil {
+		return errors.WrapPrefixf(err, "unable to set field %s in replacement target", fp)
 	}
 	return nil
 }
@@ -243,16 +267,3 @@ func setFieldValue(options *types.FieldOptions, targetField *yaml.RNode, value *
 
 	return nil
 }
-
-func shouldCreateField(options *types.FieldOptions, fieldPath []string) (bool, error) {
-	if options == nil || !options.Create {
-		return false, nil
-	}
-	// create option is not supported in a wildcard matching
-	for _, f := range fieldPath {
-		if f == "*" {
-			return false, fmt.Errorf("cannot support create option in a multi-value target")
-		}
-	}
-	return true, nil
-}
diff --git a/api/filters/replacement/replacement_test.go b/api/filters/replacement/replacement_test.go
@@ -1198,11 +1198,6 @@ apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: deploy1
----
-apiVersion: apps/v1
-kind: Deployment
-metadata:
-  name: deploy2
 `,
 			replacements: `replacements:
 - source:
@@ -1216,15 +1211,6 @@ metadata:
     - spec.template.spec.containers
     options:
       create: true
-- source:
-    kind: Pod
-    name: pod
-    fieldPath: spec.containers
-  targets:
-  - select:
-      name: deploy2
-    fieldPaths:
-    - spec.template.spec.containers
 `,
 			expected: `apiVersion: v1
 kind: Pod
@@ -1245,11 +1231,6 @@ spec:
       containers:
       - image: busybox
         name: myapp-container
----
-apiVersion: apps/v1
-kind: Deployment
-metadata:
-  name: deploy2
 `,
 		},
 		"complex type with delimiter in source": {
@@ -2416,6 +2397,86 @@ spec:
   restartPolicy: new
 `,
 		},
+		"issue4761 - path not in target with create: true": {
+			input: `
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: EnvoyFilter
+metadata:
+  name: request-id
+spec:
+  configPatches:
+    - applyTo: NETWORK_FILTER
+    - applyTo: NETWORK_FILTER
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: istio-version
+  annotations:
+    config.kubernetes.io/local-config: true
+data:
+  ISTIO_REGEX: '^1\.14.*'
+`,
+			replacements: `
+replacements:
+  - source:
+      kind: ConfigMap
+      name: istio-version
+      fieldPath: data.ISTIO_REGEX
+    targets:
+      - select:
+          kind: EnvoyFilter
+        fieldPaths:
+          - spec.configPatches.0.match.proxy.proxyVersion
+          - spec.configPatches.1.match.proxy.proxyVersion
+          - spec.configPatches.2.match.proxy.proxyVersion
+          - spec.configPatches.3.match.proxy.proxyVersion
+        options:
+          create: true
+`,
+			expectedErr: "unable to find or create field spec.configPatches.2.match.proxy.proxyVersion in replacement target",
+		},
+		"issue4761 - path not in target with create: false": {
+			input: `
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: EnvoyFilter
+metadata:
+  name: request-id
+spec:
+  configPatches:
+    - applyTo: NETWORK_FILTER
+    - applyTo: NETWORK_FILTER
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: istio-version
+  annotations:
+    config.kubernetes.io/local-config: true
+data:
+  ISTIO_REGEX: '^1\.14.*'
+`,
+			replacements: `
+replacements:
+  - source:
+      kind: ConfigMap
+      name: istio-version
+      fieldPath: data.ISTIO_REGEX
+    targets:
+      - select:
+          kind: EnvoyFilter
+        fieldPaths:
+          - spec.configPatches.0.match.proxy.proxyVersion
+          - spec.configPatches.1.match.proxy.proxyVersion
+          - spec.configPatches.2.match.proxy.proxyVersion
+          - spec.configPatches.3.match.proxy.proxyVersion
+        options:
+          create: false
+`,
+			expectedErr: "unable to find field spec.configPatches.0.match.proxy.proxyVersion in replacement target",
+		},
 	}
 
 	for tn, tc := range testCases {