Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacements should throw errors on invalid targets #4789

Merged
merged 3 commits into from
Dec 6, 2022
Merged

Replacements should throw errors on invalid targets #4789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ebc6fb
Replacements should throw errors on invalid targets
KnVerey Sep 9, 2022
1a42e2b
Refactor to satisfy complexity linter
KnVerey Nov 24, 2022
9dc0830
Move new tests to filter suite
KnVerey Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 51 additions & 40 deletions api/filters/replacement/replacement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
99 changes: 80 additions & 19 deletions api/filters/replacement/replacement_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,11 +1198,6 @@ apiVersion: apps/v1
kind: Deployment
metadata:
name: deploy1
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: deploy2
`,
replacements: `replacements:
- source:
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was testing that when given a fieldspec that we cannot satisfy, we do nothing. This PR proposes that we should be throwing an error in this exact case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't recall if this was intentional or an oversight when this was introduced. But I agree with you that it is odd behavior to silently do nothing, and that it is more intuitive to throw an error instead.

`,
expected: `apiVersion: v1
kind: Pod
Expand All @@ -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": {
Expand Down Expand Up @@ -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 {
Expand Down