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

Jump to bottom

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 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 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 @@ -191,6 +191,9 @@ func copyValueToTarget(target *yaml.RNode, value *yaml.RNode, selector *types.Ta
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)
}
targetFields = append(targetFields, createdField)
} else {
// may return multiple fields, always wrapped in a sequence node
Expand All @@ -202,6 +205,9 @@ func copyValueToTarget(target *yaml.RNode, value *yaml.RNode, selector *types.Ta
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 {
Expand Down
19 changes: 0 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
101 changes: 101 additions & 0 deletions api/krusty/replacementtransformer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package krusty_test
import (
"testing"

"github.com/stretchr/testify/require"
kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
)

Expand Down Expand Up @@ -546,3 +547,103 @@ metadata:
name: red-dc6gc5btkc
`)
}

func TestIssue4761_path_not_in_target_with_create_true(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have these two tests as tests for the replacements filter in api/filters/replacement/replacement_test.go instead of in krusty? The error is thrown by the filter, so I think it makes more sense to have it as a unit test for the filter, keeping the test closer to the logic that it is testing.

The tests here in api/krusty/replacementtransformer_test.go are intended to cover specifically integration test cases that we wouldn't be able to test in the filter unit tests, such as base/overlay structures, name references, interactions with other fields, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will move them!

th := kusttest_test.MakeEnhancedHarness(t)
defer th.Reset()

th.WriteF("resources.yaml", `
---
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.*'
`)

th.WriteK(".", `
resources:
- resources.yaml

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
`)

err := th.RunWithErr(".", th.MakeDefaultOptions())
require.EqualError(t, err, "unable to find or create field spec.configPatches.2.match.proxy.proxyVersion in replacement target")
}

func TestIssue4761_path_not_in_target_with_create_false(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t)
defer th.Reset()

th.WriteF("resources.yaml", `
---
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.*'
`)

th.WriteK(".", `
resources:
- resources.yaml

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
`)

err := th.RunWithErr(".", th.MakeDefaultOptions())
require.EqualError(t, err, "unable to find field spec.configPatches.0.match.proxy.proxyVersion in replacement target")
}