From 74b0b3adc61884422152d3630ad7f7d49e7309b7 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Thu, 27 May 2021 12:01:58 -0700 Subject: [PATCH 1/4] test to demonstrate multiple fieldpaths issue in replacements --- api/filters/replacement/replacement_test.go | 78 +++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/api/filters/replacement/replacement_test.go b/api/filters/replacement/replacement_test.go index 450c29c4c..c91a0f437 100644 --- a/api/filters/replacement/replacement_test.go +++ b/api/filters/replacement/replacement_test.go @@ -1338,6 +1338,84 @@ spec: `, expectedErr: "delimiter option can only be used with scalar nodes", }, + // test to demonstrate https://github.com/kubernetes-sigs/kustomize/issues/3927 + // the source field is overwritten when there are multiple fieldPaths + "multiple field paths in target": { + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: source +data: + value: example +--- +apiVersion: kubernetes-client.io/v1 +kind: ExternalSecret +metadata: + name: some-secret +spec: + backendType: secretsManager + data: + - key: some-prefix-replaceme + name: first + version: latest + property: first + - key: some-prefix-replaceme + name: second + version: latest + property: second + - key: some-prefix-replaceme + name: third + version: latest + property: third +`, + replacements: `replacements: +- source: + kind: ConfigMap + version: v1 + name: source + fieldPath: data.value + targets: + - select: + group: kubernetes-client.io + version: v1 + kind: ExternalSecret + name: some-secret + fieldPaths: + - spec.data.0.key + - spec.data.1.key + - spec.data.2.key + options: + delimiter: "-" + index: 2 +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: source +data: + value: some-prefix-some-prefix-some-prefix-example +--- +apiVersion: kubernetes-client.io/v1 +kind: ExternalSecret +metadata: + name: some-secret +spec: + backendType: secretsManager + data: + - key: some-prefix-example + name: first + version: latest + property: first + - key: some-prefix-some-prefix-example + name: second + version: latest + property: second + - key: some-prefix-some-prefix-some-prefix-example + name: third + version: latest + property: third +`, + }, } for tn, tc := range testCases { From 4014440d06011b09bab363ae896ac576e2b67f96 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Tue, 1 Jun 2021 13:18:20 -0700 Subject: [PATCH 2/4] test to demonstrate '.' in list element path issue --- api/filters/replacement/replacement_test.go | 49 +++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/api/filters/replacement/replacement_test.go b/api/filters/replacement/replacement_test.go index c91a0f437..00ec1f430 100644 --- a/api/filters/replacement/replacement_test.go +++ b/api/filters/replacement/replacement_test.go @@ -1338,8 +1338,51 @@ spec: `, expectedErr: "delimiter option can only be used with scalar nodes", }, - // test to demonstrate https://github.com/kubernetes-sigs/kustomize/issues/3927 - // the source field is overwritten when there are multiple fieldPaths + "list index contains '.' character": { + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: source +data: + value: example +--- +apiVersion: kubernetes-client.io/v1 +kind: ExternalSecret +metadata: + name: some-secret +spec: + backendType: secretsManager + data: + - key: some-prefix-replaceme + name: .first + version: latest + property: first + - key: some-prefix-replaceme + name: second + version: latest + property: second +`, + replacements: `replacements: +- source: + kind: ConfigMap + version: v1 + name: source + fieldPath: data.value + targets: + - select: + group: kubernetes-client.io + version: v1 + kind: ExternalSecret + name: some-secret + fieldPaths: + - spec.data.[name=.first].key + - spec.data.[name=second].key + options: + delimiter: "-" + index: 2 +`, + expectedErr: "wrong Node Kind for spec.data expected: MappingNode was SequenceNode: value: {- key: some-prefix-replaceme", + }, "multiple field paths in target": { input: `apiVersion: v1 kind: ConfigMap @@ -1431,7 +1474,7 @@ spec: t.Errorf("unexpected error: %s\n", err.Error()) t.FailNow() } - if !assert.Equal(t, tc.expectedErr, err.Error()) { + if !assert.Contains(t, err.Error(), tc.expectedErr) { t.FailNow() } } From b8ae69b74828366e544d96ae77645d9772aed9b8 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Tue, 1 Jun 2021 13:23:12 -0700 Subject: [PATCH 3/4] copy target rnode in replacements --- api/filters/replacement/replacement.go | 2 ++ api/filters/replacement/replacement_test.go | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/api/filters/replacement/replacement.go b/api/filters/replacement/replacement.go index 75fbb9467..81a2e834d 100644 --- a/api/filters/replacement/replacement.go +++ b/api/filters/replacement/replacement.go @@ -87,6 +87,8 @@ func applyToNode(node *yaml.RNode, value *yaml.RNode, target *types.TargetSelect } func setTargetValue(options *types.FieldOptions, t *yaml.RNode, value *yaml.RNode) error { + value = value.Copy() + if options != nil && options.Delimiter != "" { if t.YNode().Kind != yaml.ScalarNode { diff --git a/api/filters/replacement/replacement_test.go b/api/filters/replacement/replacement_test.go index 00ec1f430..5850accf8 100644 --- a/api/filters/replacement/replacement_test.go +++ b/api/filters/replacement/replacement_test.go @@ -1436,7 +1436,7 @@ kind: ConfigMap metadata: name: source data: - value: some-prefix-some-prefix-some-prefix-example + value: example --- apiVersion: kubernetes-client.io/v1 kind: ExternalSecret @@ -1449,11 +1449,11 @@ spec: name: first version: latest property: first - - key: some-prefix-some-prefix-example + - key: some-prefix-example name: second version: latest property: second - - key: some-prefix-some-prefix-some-prefix-example + - key: some-prefix-example name: third version: latest property: third From 84724a3ebf64a4b977c7344a442c700643509acb Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Tue, 1 Jun 2021 13:39:48 -0700 Subject: [PATCH 4/4] smarter path splitter for replacements --- api/filters/fieldspec/fieldspec.go | 2 +- api/filters/replacement/replacement.go | 8 ++-- api/filters/replacement/replacement_test.go | 23 ++++++++++- api/internal/utils/pathsplitter.go | 40 +++++++++++++++++-- api/internal/utils/pathsplitter_test.go | 43 ++++++++++++++++++++- 5 files changed, 104 insertions(+), 12 deletions(-) diff --git a/api/filters/fieldspec/fieldspec.go b/api/filters/fieldspec/fieldspec.go index cbc8776fe..8739d0733 100644 --- a/api/filters/fieldspec/fieldspec.go +++ b/api/filters/fieldspec/fieldspec.go @@ -49,7 +49,7 @@ func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) { if match := isMatchGVK(fltr.FieldSpec, obj); !match { return obj, nil } - fltr.path = utils.PathSplitter(fltr.FieldSpec.Path) + fltr.path = utils.PathSplitter(fltr.FieldSpec.Path, "/") if err := fltr.filter(obj); err != nil { s, _ := obj.String() return nil, errors.WrapPrefixf(err, diff --git a/api/filters/replacement/replacement.go b/api/filters/replacement/replacement.go index 81a2e834d..35858940d 100644 --- a/api/filters/replacement/replacement.go +++ b/api/filters/replacement/replacement.go @@ -7,6 +7,7 @@ import ( "fmt" "strings" + "sigs.k8s.io/kustomize/api/internal/utils" "sigs.k8s.io/kustomize/api/types" "sigs.k8s.io/kustomize/kyaml/resid" "sigs.k8s.io/kustomize/kyaml/yaml" @@ -66,7 +67,7 @@ func rejectId(rejects []*types.Selector, id *resid.ResId) bool { func applyToNode(node *yaml.RNode, value *yaml.RNode, target *types.TargetSelector) error { for _, fp := range target.FieldPaths { - fieldPath := strings.Split(fp, ".") + fieldPath := utils.SmarterPathSplitter(fp, ".") var t *yaml.RNode var err error if target.Options != nil && target.Options.Create { @@ -88,13 +89,10 @@ func applyToNode(node *yaml.RNode, value *yaml.RNode, target *types.TargetSelect func setTargetValue(options *types.FieldOptions, t *yaml.RNode, value *yaml.RNode) error { value = value.Copy() - if options != nil && options.Delimiter != "" { - if t.YNode().Kind != yaml.ScalarNode { return fmt.Errorf("delimiter option can only be used with scalar nodes") } - tv := strings.Split(t.YNode().Value, options.Delimiter) v := yaml.GetValue(value) // TODO: Add a way to remove an element @@ -121,7 +119,7 @@ func getReplacement(nodes []*yaml.RNode, r *types.Replacement) (*yaml.RNode, err if r.Source.FieldPath == "" { r.Source.FieldPath = types.DefaultReplacementFieldPath } - fieldPath := strings.Split(r.Source.FieldPath, ".") + fieldPath := utils.SmarterPathSplitter(r.Source.FieldPath, ".") rn, err := source.Pipe(yaml.Lookup(fieldPath...)) if err != nil { diff --git a/api/filters/replacement/replacement_test.go b/api/filters/replacement/replacement_test.go index 5850accf8..d32b7dfdc 100644 --- a/api/filters/replacement/replacement_test.go +++ b/api/filters/replacement/replacement_test.go @@ -1381,7 +1381,28 @@ spec: delimiter: "-" index: 2 `, - expectedErr: "wrong Node Kind for spec.data expected: MappingNode was SequenceNode: value: {- key: some-prefix-replaceme", + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: source +data: + value: example +--- +apiVersion: kubernetes-client.io/v1 +kind: ExternalSecret +metadata: + name: some-secret +spec: + backendType: secretsManager + data: + - key: some-prefix-example + name: .first + version: latest + property: first + - key: some-prefix-example + name: second + version: latest + property: second`, }, "multiple field paths in target": { input: `apiVersion: v1 diff --git a/api/internal/utils/pathsplitter.go b/api/internal/utils/pathsplitter.go index c8d2f3342..002f43bb4 100644 --- a/api/internal/utils/pathsplitter.go +++ b/api/internal/utils/pathsplitter.go @@ -5,18 +5,50 @@ package utils import "strings" -// PathSplitter splits a slash delimited string, permitting escaped slashes. -func PathSplitter(path string) []string { - ps := strings.Split(path, "/") +// TODO: Move these to kyaml + +// PathSplitter splits a delimited string, permitting escaped delimiters. +func PathSplitter(path string, delimiter string) []string { + ps := strings.Split(path, delimiter) var res []string res = append(res, ps[0]) for i := 1; i < len(ps); i++ { last := len(res) - 1 if strings.HasSuffix(res[last], `\`) { - res[last] = strings.TrimSuffix(res[last], `\`) + "/" + ps[i] + res[last] = strings.TrimSuffix(res[last], `\`) + delimiter + ps[i] } else { res = append(res, ps[i]) } } return res } + +// SmarterPathSplitter splits a path, retaining bracketed list entry identifiers. +// E.g. [name=com.foo.someapp] survives as one thing after splitting +// "spec.template.spec.containers.[name=com.foo.someapp].image" +// See kyaml/yaml/match.go for use of list entry identifiers. +// This function uses `PathSplitter`, so it respects list entry identifiers +// and escaped delimiters. +func SmarterPathSplitter(path string, delimiter string) []string { + var result []string + split := PathSplitter(path, delimiter) + + for i := 0; i < len(split); i++ { + elem := split[i] + if strings.HasPrefix(elem, "[") && !strings.HasSuffix(elem, "]") { + // continue until we find the matching "]" + bracketed := []string{elem} + for i < len(split)-1 { + i++ + bracketed = append(bracketed, split[i]) + if strings.HasSuffix(split[i], "]") { + break + } + } + result = append(result, strings.Join(bracketed, delimiter)) + } else { + result = append(result, elem) + } + } + return result +} diff --git a/api/internal/utils/pathsplitter_test.go b/api/internal/utils/pathsplitter_test.go index 9b52c3806..5e133f572 100644 --- a/api/internal/utils/pathsplitter_test.go +++ b/api/internal/utils/pathsplitter_test.go @@ -44,6 +44,47 @@ func TestPathSplitter(t *testing.T) { "nginx.ingress.kubernetes.io/auth-secret"}, }, } { - assert.Equal(t, tc.exp, PathSplitter(tc.path)) + assert.Equal(t, tc.exp, PathSplitter(tc.path, "/")) + } +} + +func TestSmarterPathSplitter(t *testing.T) { + testCases := map[string]struct { + input string + expected []string + }{ + "simple": { + input: "spec.replicas", + expected: []string{"spec", "replicas"}, + }, + "sequence": { + input: "spec.data.[name=first].key", + expected: []string{"spec", "data", "[name=first]", "key"}, + }, + "key, value with . prefix": { + input: "spec.data.[.name=.first].key", + expected: []string{"spec", "data", "[.name=.first]", "key"}, + }, + "key, value with . suffix": { + input: "spec.data.[name.=first.].key", + expected: []string{"spec", "data", "[name.=first.]", "key"}, + }, + "multiple '.' in value": { + input: "spec.data.[name=f.i.r.s.t.].key", + expected: []string{"spec", "data", "[name=f.i.r.s.t.]", "key"}, + }, + "with escaped delimiter": { + input: `spec\.replicas`, + expected: []string{`spec.replicas`}, + }, + "unmatched bracket": { + input: "spec.data.[name=f.i.[r.s.t..key", + expected: []string{"spec", "data", "[name=f.i.[r.s.t..key"}, + }, + } + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + assert.Equal(t, tc.expected, SmarterPathSplitter(tc.input, ".")) + }) } }