Merge pull request kubernetes-sigs#5196 from ephesused/issue4928-appe…

…nd-honors-key-style fix: patch additions honor source key style
charles-chenzz · Sep 1, 2023 · 169fdd7 · 169fdd7
2 parents 76f8d28 + 78b8139
commit 169fdd7
Show file tree

Hide file tree

Showing 5 changed files with 246 additions and 5 deletions.
diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go
@@ -776,6 +776,133 @@ 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"
+`,
+		},
+
+		"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"
 `,
 		},
 	}

diff --git 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
@@ -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
@@ -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
@@ -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
 }