From 691b7d1df33d9ead7c37dde763b2a4200e24bada Mon Sep 17 00:00:00 2001 From: Ed Overton Date: Thu, 8 Jun 2023 17:21:46 -0400 Subject: [PATCH 1/3] fix: patch additions honor source key style When a patch appends a new node, it should honor the key style from the patch. Prior to this commit, no style was applied, leading to problems when the key value could be interpreted as a different type based on its content. For example, "9110" needs quoting to ensure it is seen as a string in yaml. --- .../patchstrategicmerge_test.go | 97 +++++++++++++++++++ kyaml/yaml/fns.go | 5 + kyaml/yaml/merge2/map_test.go | 41 ++++++++ kyaml/yaml/merge3/map_test.go | 57 +++++++++++ kyaml/yaml/walk/map.go | 21 +++- 5 files changed, 216 insertions(+), 5 deletions(-) diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go index 5bc15daa8b..52a9ebd704 100644 --- a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go +++ b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go @@ -776,6 +776,103 @@ spec: autoscaling: true deepgram-api: some: value +`, + }, + + // Issue #4928 + "support numeric keys": { + input: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "foobar" +`, + patch: yaml.MustParse(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "barfoo" + "9110": "foo-foo" +`), + expected: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "barfoo" + "9110": "foo-foo" +`, + }, + + "honor different key style one": { + input: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + '6443': "foobar" +`, + patch: yaml.MustParse(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "barfoo" + 9110: "foo-foo" +`), + expected: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + '6443': "barfoo" + 9110: "foo-foo" +`, + }, + + "honor different key style two": { + input: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "foobar" +`, + patch: yaml.MustParse(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "barfoo" + '9110': "foo-foo" +`), + expected: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "barfoo" + '9110': "foo-foo" `, }, } diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go index ae63d258b8..24f37c052a 100644 --- a/kyaml/yaml/fns.go +++ b/kyaml/yaml/fns.go @@ -690,6 +690,10 @@ type FieldSetter struct { // when setting it. Otherwise, if an existing node is found, the style is // retained. OverrideStyle bool `yaml:"overrideStyle,omitempty"` + + // AppendKeyStyle defines the style of the key when no existing node is + // found, and a new node is appended. + AppendKeyStyle Style `yaml:"appendKeyStyle,omitempty"` } func (s FieldSetter) Filter(rn *RNode) (*RNode, error) { @@ -747,6 +751,7 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) { &yaml.Node{ Kind: yaml.ScalarNode, Value: s.Name, + Style: s.AppendKeyStyle, HeadComment: s.Comments.HeadComment, LineComment: s.Comments.LineComment, FootComment: s.Comments.FootComment, diff --git a/kyaml/yaml/merge2/map_test.go b/kyaml/yaml/merge2/map_test.go index 783e81b737..7a38758a0a 100644 --- a/kyaml/yaml/merge2/map_test.go +++ b/kyaml/yaml/merge2/map_test.go @@ -417,6 +417,47 @@ spec: protocol: TCP - port: 30901 targetPort: 30901 +`, + mergeOptions: yaml.MergeOptions{ + ListIncreaseDirection: yaml.MergeOptionsListAppend, + }, + }, + + // + // Test Case + // + {description: `Verify key style behavior`, + source: ` +kind: Deployment +spec: + source-and-dest: source-and-dest + source-only: source-only + "source-only-key-double-quoted": source-only + source-and-dest-key-style-diff-1: source-and-dest + 'source-and-dest-key-style-diff-2': source-and-dest + "source-and-dest-key-style-diff-3": source-and-dest +`, + dest: ` +kind: Deployment +spec: + source-and-dest: source-and-dest + 'source-and-dest-key-style-diff-1': source-and-dest + "source-and-dest-key-style-diff-2": source-and-dest + source-and-dest-key-style-diff-3: source-and-dest + dest-only: dest-only + 'dest-only-key-single-quoted': dest-only +`, + expected: ` +kind: Deployment +spec: + source-and-dest: source-and-dest + 'source-and-dest-key-style-diff-1': source-and-dest + "source-and-dest-key-style-diff-2": source-and-dest + source-and-dest-key-style-diff-3: source-and-dest + dest-only: dest-only + 'dest-only-key-single-quoted': dest-only + source-only: source-only + "source-only-key-double-quoted": source-only `, mergeOptions: yaml.MergeOptions{ ListIncreaseDirection: yaml.MergeOptionsListAppend, diff --git a/kyaml/yaml/merge3/map_test.go b/kyaml/yaml/merge3/map_test.go index 4875ae76c1..17eada9b0d 100644 --- a/kyaml/yaml/merge3/map_test.go +++ b/kyaml/yaml/merge3/map_test.go @@ -306,4 +306,61 @@ metadata: name: foo annotations: {} `}, + + // + // Test Case + // + { + description: `Verify key style behavior`, + origin: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + unchanged: origin + unchanged-varying-key-style: origin + deleted-in-update: origin + deleted-in-local: origin +`, + update: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + unchanged: origin + 'unchanged-varying-key-style': origin + deleted-in-local: origin + 'added-in-update': 'update' + 'added-in-update-and-local-same-value': 'update-and-local' + 'added-in-update-and-local-diff-value': 'update' +`, + local: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + unchanged: origin + "unchanged-varying-key-style": origin + deleted-in-update: origin + "added-in-local": "local" + "added-in-update-and-local-same-value": "update-and-local" + "added-in-update-and-local-diff-value": "local" +`, + expected: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +data: + unchanged: origin + "unchanged-varying-key-style": origin + "added-in-local": "local" + "added-in-update-and-local-same-value": "update-and-local" + "added-in-update-and-local-diff-value": "update" + 'added-in-update': 'update' +`, + }, } diff --git a/kyaml/yaml/walk/map.go b/kyaml/yaml/walk/map.go index 998af6d32f..afeec0a5a7 100644 --- a/kyaml/yaml/walk/map.go +++ b/kyaml/yaml/walk/map.go @@ -58,7 +58,7 @@ func (l Walker) walkMap() (*yaml.RNode, error) { if l.Schema != nil { s = l.Schema.Field(key) } - fv, commentSch := l.fieldValue(key) + fv, commentSch, keyStyles := l.fieldValue(key) if commentSch != nil { s = commentSch } @@ -90,7 +90,13 @@ func (l Walker) walkMap() (*yaml.RNode, error) { } // this handles empty and non-empty values - _, err = dest.Pipe(yaml.FieldSetter{Name: key, Comments: comments, Value: val}) + fieldSetter := yaml.FieldSetter{ + Name: key, + Comments: comments, + AppendKeyStyle: keyStyles[val], + Value: val, + } + _, err = dest.Pipe(fieldSetter) if err != nil { return nil, err } @@ -153,10 +159,12 @@ func (l Walker) fieldNames() []string { return result } -// fieldValue returns a slice containing each source's value for fieldName -func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSchema) { +// fieldValue returns a slice containing each source's value for fieldName, the +// schema, and a map of each source's value to the style for the source's key. +func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSchema, map[*yaml.RNode]yaml.Style) { var fields []*yaml.RNode var sch *openapi.ResourceSchema + keyStyles := make(map[*yaml.RNode]yaml.Style, len(l.Sources)) for i := range l.Sources { if l.Sources[i] == nil { fields = append(fields, nil) @@ -165,9 +173,12 @@ func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSc field := l.Sources[i].Field(fieldName) f, s := l.valueIfPresent(field) fields = append(fields, f) + if field != nil && field.Key != nil && field.Key.YNode() != nil { + keyStyles[f] = field.Key.YNode().Style + } if sch == nil && !s.IsMissingOrNull() { sch = s } } - return fields, sch + return fields, sch, keyStyles } From c76fd5eb852076fcbd249cb54633b867840af128 Mon Sep 17 00:00:00 2001 From: Ed Overton Date: Tue, 13 Jun 2023 14:50:22 -0400 Subject: [PATCH 2/3] test: update psm key style test --- api/filters/patchstrategicmerge/patchstrategicmerge_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go index 52a9ebd704..044a5a86e2 100644 --- a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go +++ b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go @@ -852,7 +852,7 @@ metadata: name: blabla namespace: blabla-ns data: - "6443": "foobar" + "6443": "key-double-quoted" `, patch: yaml.MustParse(` apiVersion: v1 @@ -861,7 +861,7 @@ metadata: name: blabla namespace: blabla-ns data: - "6443": "barfoo" + 6443: "key as int" '9110': "foo-foo" `), expected: ` @@ -871,7 +871,7 @@ metadata: name: blabla namespace: blabla-ns data: - "6443": "barfoo" + "6443": "key as int" '9110': "foo-foo" `, }, From 096b2c4435c12f127d0d8f64b814f0bf43b6b4c4 Mon Sep 17 00:00:00 2001 From: Ed Overton Date: Thu, 15 Jun 2023 16:12:05 -0400 Subject: [PATCH 3/3] test: add psm test for different key types --- .../patchstrategicmerge_test.go | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go index 044a5a86e2..0bcd951c0e 100644 --- a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go +++ b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go @@ -852,7 +852,7 @@ metadata: name: blabla namespace: blabla-ns data: - "6443": "key-double-quoted" + "6443": "foobar" `, patch: yaml.MustParse(` apiVersion: v1 @@ -861,7 +861,7 @@ metadata: name: blabla namespace: blabla-ns data: - 6443: "key as int" + "6443": "barfoo" '9110': "foo-foo" `), expected: ` @@ -871,8 +871,38 @@ metadata: name: blabla namespace: blabla-ns data: - "6443": "key as int" + "6443": "barfoo" '9110': "foo-foo" +`, + }, + + "different key types": { + input: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "key-string-double-quoted" +`, + patch: yaml.MustParse(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + 6443: "key-int" +`), + expected: ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: blabla + namespace: blabla-ns +data: + "6443": "key-int" `, }, }