Browse Source

fix: correctly merge map fields during templating (#5671)

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Gergely Brautigam 4 months ago
parent
commit
f6729a7354
2 changed files with 95 additions and 1 deletions
  1. 26 1
      runtime/template/v2/template.go
  2. 69 0
      runtime/template/v2/template_test.go

+ 26 - 1
runtime/template/v2/template.go

@@ -347,7 +347,32 @@ func applyParsedToPath(parsed any, target string, obj client.Object) error {
 		}
 		}
 
 
 		// once we constructed the entire segment, we finally apply our parsed object
 		// once we constructed the entire segment, we finally apply our parsed object
-		current[parts[len(parts)-1]] = parsed
+		// MERGE the parsed content into existing content instead of replacing it
+		lastPart := parts[len(parts)-1]
+		if existing, exists := current[lastPart]; exists {
+			// if both existing and new values are maps, merge them
+			existingMap, existingOk := existing.(map[string]any)
+			parsedMap, parsedOk := parsed.(map[string]any)
+
+			if existingOk && parsedOk {
+				for k, v := range parsedMap {
+					existingMap[k] = v
+				}
+
+				current[lastPart] = existingMap
+			} else {
+				// existing or parsed value is not a map, replace entirely.
+				// this might break if people are trying to overwrite
+				// fields that aren't supposed to do that. but that's
+				// on the user to keep in mind. If they are trying to
+				// update a number field with a complex value, that's
+				// going to error on update anyway.
+				current[lastPart] = parsed
+			}
+		} else {
+			// field doesn't exist yet, create it
+			current[lastPart] = parsed
+		}
 	}
 	}
 
 
 	// convert back to original object
 	// convert back to original object

+ 69 - 0
runtime/template/v2/template_test.go

@@ -1232,6 +1232,54 @@ webhook_url: {{ .webhook }}
 				assert.False(t, hasDuplicateSlack, "should not have spec.slack.slack (the bug)")
 				assert.False(t, hasDuplicateSlack, "should not have spec.slack.slack (the bug)")
 			},
 			},
 		},
 		},
+		{
+			name:   "nested path merge behavior - preserves existing fields",
+			target: "spec.slack",
+			scope:  esapi.TemplateScopeKeysAndValues,
+			tpl: map[string][]byte{
+				"slack-config": []byte(`
+api_url: {{ .url }}
+`),
+			},
+			data: map[string][]byte{
+				"url": []byte("https://hooks.slack.com/services/NEW"),
+			},
+			verify: func(t *testing.T, obj map[string]interface{}) {
+				specMap := obj["spec"].(map[string]interface{})
+				slackMap := specMap["slack"].(map[string]interface{})
+
+				// Should have the new field from template
+				assert.Equal(t, "https://hooks.slack.com/services/NEW", slackMap["api_url"], "api_url should be set from template")
+
+				// Should PRESERVE existing fields that were not in the template
+				assert.Equal(t, "general", slackMap["channel"], "existing channel field should be preserved")
+				assert.Equal(t, "test-value", slackMap["other_field"], "existing other_field should be preserved")
+			},
+		},
+		{
+			name:   "nested path overwrite field - updates existing value",
+			target: "spec.slack",
+			scope:  esapi.TemplateScopeKeysAndValues,
+			tpl: map[string][]byte{
+				"slack-config": []byte(`
+channel: {{ .new_channel }}
+`),
+			},
+			data: map[string][]byte{
+				"new_channel": []byte("alerts"),
+			},
+			verify: func(t *testing.T, obj map[string]interface{}) {
+				specMap := obj["spec"].(map[string]interface{})
+				slackMap := specMap["slack"].(map[string]interface{})
+
+				// Should update the existing field
+				assert.Equal(t, "alerts", slackMap["channel"], "channel should be updated")
+
+				// Should still preserve other existing fields
+				assert.Equal(t, "https://hooks.slack.com/existing", slackMap["api_url"], "existing api_url should be preserved")
+				assert.Equal(t, "test-value", slackMap["other_field"], "existing other_field should be preserved")
+			},
+		},
 	}
 	}
 
 
 	for _, tt := range tests {
 	for _, tt := range tests {
@@ -1248,6 +1296,27 @@ webhook_url: {{ .webhook }}
 				},
 				},
 			}
 			}
 
 
+			// For merge behavior tests, pre-populate the object with existing content
+			if strings.Contains(tt.name, "merge behavior") {
+				obj.Object["spec"] = map[string]interface{}{
+					"slack": map[string]interface{}{
+						"channel":     "general",
+						"other_field": "test-value",
+					},
+				}
+			}
+
+			// For overwrite field test, pre-populate with different initial values
+			if strings.Contains(tt.name, "overwrite field") {
+				obj.Object["spec"] = map[string]interface{}{
+					"slack": map[string]interface{}{
+						"channel":     "general",
+						"api_url":     "https://hooks.slack.com/existing",
+						"other_field": "test-value",
+					},
+				}
+			}
+
 			err := Execute(tt.tpl, tt.data, tt.scope, tt.target, obj)
 			err := Execute(tt.tpl, tt.data, tt.scope, tt.target, obj)
 
 
 			if tt.wantErr {
 			if tt.wantErr {