Merge 1c9f24adf0 into 0752c1f5b5

pkg/kube/client.go

@@ -1217,6 +1217,19 @@ func patchResourceServerSide(target *resource.Info, dryRun bool, forceConflicts
 		WithFieldManager(getManagedFieldsManager()).
 		WithFieldValidation(string(fieldValidationDirective))
 
+	// Deduplicate list-map entries (e.g. env vars) before server-side apply.
+	// Server-side apply rejects manifests with duplicate keys in list-maps, but
+	// Kubernetes client-side apply previously allowed them (last value wins).
+	// We preserve that behavior by deduplicating here, keeping the last occurrence.
+	if u, ok := target.Object.(*unstructured.Unstructured); ok {
+		if deduplicateListMaps(u.Object) {
+			slog.Warn("deduplicated list-map entries in manifest; please remove duplicates from the chart",
+				slog.String("name", target.Name),
+				slog.String("namespace", target.Namespace),
+				slog.String("gvk", target.Mapping.GroupVersionKind.String()))
+		}
+	}
+
 	// Send the full object to be applied on the server side.
 	data, err := runtime.Encode(unstructured.UnstructuredJSONScheme, target.Object)
 	if err != nil {
@@ -1286,6 +1299,102 @@ func copyRequestStreamToWriter(request *rest.Request, podName, containerName str
 	return nil
 }
 
+// dedupFields is the set of Kubernetes list fields that should be deduplicated
+// by "name" under server-side apply merge semantics. Container lists
+// (containers, initContainers, ephemeralContainers) are intentionally excluded:
+// duplicate container names are invalid and must not be silently dropped.
+// Other lists that happen to carry a "name" field (e.g. volumeMounts, keyed by
+// mountPath) are also excluded. All list fields are still traversed so that
+// nested dedupFields (e.g. env inside a container) are processed.
+var dedupFields = map[string]bool{
+	"env":              true,
+	"volumes":          true,
+	"imagePullSecrets": true,
+}
+
+// deduplicateListMaps walks an unstructured Kubernetes object and deduplicates
+// list fields in dedupFields under server-side apply merge semantics. When
+// duplicates are found the last occurrence wins, matching Kubernetes
+// client-side-apply semantics. All lists are traversed (to reach nested fields)
+// but only lists whose key is in dedupFields are themselves deduplicated.
+// Returns true if any deduplication occurred.
+func deduplicateListMaps(obj map[string]interface{}) bool {
+	deduped := false
+	for key, val := range obj {
+		switch v := val.(type) {
+		case map[string]interface{}:
+			if deduplicateListMaps(v) {
+				deduped = true
+			}
+		case []interface{}:
+			newList, changed := processListItems(v, dedupFields[key])
+			if changed {
+				obj[key] = newList
+				deduped = true
+			}
+		}
+	}
+	return deduped
+}
+
+// processListItems recurses into list items and, when dedup is true,
+// deduplicates the list if every item is a map with a non-empty string "name".
+// The last occurrence of each name is kept and relative order of surviving
+// items is preserved. Returns the (possibly modified) list and whether any
+// change was made.
+func processListItems(list []interface{}, dedup bool) ([]interface{}, bool) {
+	changed := false
+
+	// Always recurse into map items to reach nested dedupFields.
+	for i, item := range list {
+		if m, ok := item.(map[string]interface{}); ok {
+			if deduplicateListMaps(m) {
+				list[i] = m
+				changed = true
+			}
+		}
+	}
+
+	if !dedup || len(list) < 2 {
+		return list, changed
+	}
+
+	// Only deduplicate if every item is a map with a non-empty string "name".
+	for _, item := range list {
+		m, ok := item.(map[string]interface{})
+		if !ok {
+			return list, changed
+		}
+		nameVal, isString := m["name"].(string)
+		if !isString || nameVal == "" {
+			return list, changed
+		}
+	}
+
+	// Build deduplicated list keeping the last occurrence of each name.
+	seen := make(map[string]bool)
+	result := make([]interface{}, 0, len(list))
+	hasDups := false
+	for i := len(list) - 1; i >= 0; i-- {
+		m := list[i].(map[string]interface{})
+		name, _ := m["name"].(string)
+		if !seen[name] {
+			seen[name] = true
+			result = append(result, list[i])
+		} else {
+			hasDups = true
+		}
+	}
+	if !hasDups {
+		return list, changed
+	}
+	// Reverse to restore the original relative order of surviving items.
+	for i, j := 0, len(result)-1; i < j; i, j = i+1, j-1 {
+		result[i], result[j] = result[j], result[i]
+	}
+	return result, true
+}
+
 // scrubValidationError removes kubectl info from the message.
 func scrubValidationError(err error) error {
 	if err == nil {

pkg/kube/client_test.go

@@ -19,6 +19,7 @@ package kube
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"errors"
 	"io"
 	"net/http"
@@ -1854,12 +1855,235 @@ func TestPatchResourceServerSide(t *testing.T) {
 	}
 }
 
+// TestPatchResourceServerSideDedupEnvVars verifies that duplicate env var
+// entries are removed from the PATCH body before server-side apply is sent,
+// matching Kubernetes "last value wins" client-side-apply semantics.
+func TestPatchResourceServerSideDedupEnvVars(t *testing.T) {
+	pod := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Pod",
+			"metadata": map[string]interface{}{
+				"name":      "whale",
+				"namespace": "default",
+			},
+			"spec": map[string]interface{}{
+				"containers": []interface{}{
+					map[string]interface{}{
+						"name":  "app",
+						"image": "nginx",
+						"env": []interface{}{
+							map[string]interface{}{"name": "FOO", "value": "first"},
+							map[string]interface{}{"name": "BAR", "value": "keep"},
+							map[string]interface{}{"name": "FOO", "value": "last"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	respBody, err := runtime.Encode(unstructured.UnstructuredJSONScheme, pod)
+	require.NoError(t, err)
+
+	var capturedBody []byte
+	restClient := &fake.RESTClient{
+		NegotiatedSerializer: unstructuredSerializer,
+		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+			capturedBody, _ = io.ReadAll(req.Body)
+			header := http.Header{}
+			header.Set("Content-Type", runtime.ContentTypeJSON)
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(bytes.NewReader(respBody)),
+				Header:     header,
+			}, nil
+		}),
+	}
+
+	info := &resource.Info{
+		Client:    restClient,
+		Namespace: "default",
+		Name:      "whale",
+		Object:    pod,
+		Mapping: &meta.RESTMapping{
+			Resource: schema.GroupVersionResource{
+				Version:  "v1",
+				Resource: "pods",
+			},
+			GroupVersionKind: schema.GroupVersionKind{
+				Version: "v1",
+				Kind:    "Pod",
+			},
+			Scope: meta.RESTScopeNamespace,
+		},
+	}
+
+	require.NoError(t, patchResourceServerSide(info, false, false, FieldValidationDirectiveStrict))
+	require.NotNil(t, capturedBody, "expected a PATCH request to be sent")
+
+	var patchedObj map[string]interface{}
+	require.NoError(t, json.Unmarshal(capturedBody, &patchedObj))
+
+	spec, _ := patchedObj["spec"].(map[string]interface{})
+	containers, _ := spec["containers"].([]interface{})
+	require.Len(t, containers, 1)
+	env, _ := containers[0].(map[string]interface{})["env"].([]interface{})
+	require.Len(t, env, 2, "FOO should be deduplicated to one entry; BAR should remain")
+	assert.Equal(t, "BAR", env[0].(map[string]interface{})["name"])
+	assert.Equal(t, "FOO", env[1].(map[string]interface{})["name"])
+	assert.Equal(t, "last", env[1].(map[string]interface{})["value"], "last occurrence of FOO wins")
+}
+
 func TestDetermineFieldValidationDirective(t *testing.T) {
 
 	assert.Equal(t, FieldValidationDirectiveIgnore, determineFieldValidationDirective(false))
 	assert.Equal(t, FieldValidationDirectiveStrict, determineFieldValidationDirective(true))
 }
 
+func TestDeduplicateListMaps(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    map[string]interface{}
+		expected map[string]interface{}
+		changed  bool
+	}{
+		{
+			name: "no duplicates",
+			input: map[string]interface{}{
+				"env": []interface{}{
+					map[string]interface{}{"name": "FOO", "value": "1"},
+					map[string]interface{}{"name": "BAR", "value": "2"},
+				},
+			},
+			expected: map[string]interface{}{
+				"env": []interface{}{
+					map[string]interface{}{"name": "FOO", "value": "1"},
+					map[string]interface{}{"name": "BAR", "value": "2"},
+				},
+			},
+			changed: false,
+		},
+		{
+			name: "duplicate env var keeps last value",
+			input: map[string]interface{}{
+				"env": []interface{}{
+					map[string]interface{}{"name": "FOO", "value": "first"},
+					map[string]interface{}{"name": "BAR", "value": "2"},
+					map[string]interface{}{"name": "FOO", "value": "last"},
+				},
+			},
+			expected: map[string]interface{}{
+				"env": []interface{}{
+					map[string]interface{}{"name": "BAR", "value": "2"},
+					map[string]interface{}{"name": "FOO", "value": "last"},
+				},
+			},
+			changed: true,
+		},
+		{
+			name: "nested container env dedup",
+			input: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"containers": []interface{}{
+						map[string]interface{}{
+							"name": "app",
+							"env": []interface{}{
+								map[string]interface{}{"name": "X", "value": "a"},
+								map[string]interface{}{"name": "X", "value": "b"},
+							},
+						},
+					},
+				},
+			},
+			expected: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"containers": []interface{}{
+						map[string]interface{}{
+							"name": "app",
+							"env": []interface{}{
+								map[string]interface{}{"name": "X", "value": "b"},
+							},
+						},
+					},
+				},
+			},
+			changed: true,
+		},
+		{
+			name: "non-named list not deduplicated",
+			input: map[string]interface{}{
+				"args": []interface{}{"--flag=a", "--flag=b"},
+			},
+			expected: map[string]interface{}{
+				"args": []interface{}{"--flag=a", "--flag=b"},
+			},
+			changed: false,
+		},
+		{
+			// volumeMounts is keyed by mountPath, not name — same name with
+			// different mountPaths must NOT be collapsed.
+			name: "volumeMounts with duplicate names not deduplicated",
+			input: map[string]interface{}{
+				"volumeMounts": []interface{}{
+					map[string]interface{}{"name": "data", "mountPath": "/data"},
+					map[string]interface{}{"name": "data", "mountPath": "/backup"},
+				},
+			},
+			expected: map[string]interface{}{
+				"volumeMounts": []interface{}{
+					map[string]interface{}{"name": "data", "mountPath": "/data"},
+					map[string]interface{}{"name": "data", "mountPath": "/backup"},
+				},
+			},
+			changed: false,
+		},
+		{
+			name: "list with empty name not deduplicated",
+			input: map[string]interface{}{
+				"env": []interface{}{
+					map[string]interface{}{"name": "", "value": "a"},
+					map[string]interface{}{"name": "", "value": "b"},
+				},
+			},
+			expected: map[string]interface{}{
+				"env": []interface{}{
+					map[string]interface{}{"name": "", "value": "a"},
+					map[string]interface{}{"name": "", "value": "b"},
+				},
+			},
+			changed: false,
+		},
+		{
+			// Duplicate container names are an invalid manifest; they must not be
+			// silently dropped. We traverse the list to reach nested env vars but
+			// do not deduplicate the containers list itself.
+			name: "duplicate container names not deduplicated",
+			input: map[string]interface{}{
+				"containers": []interface{}{
+					map[string]interface{}{"name": "app", "image": "nginx:1"},
+					map[string]interface{}{"name": "app", "image": "nginx:2"},
+				},
+			},
+			expected: map[string]interface{}{
+				"containers": []interface{}{
+					map[string]interface{}{"name": "app", "image": "nginx:1"},
+					map[string]interface{}{"name": "app", "image": "nginx:2"},
+				},
+			},
+			changed: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := deduplicateListMaps(tt.input)
+			assert.Equal(t, tt.changed, got)
+			assert.Equal(t, tt.expected, tt.input)
+		})
+	}
+}
+
 func TestClientWaitContextCancellationLegacy(t *testing.T) {
 	podList := newPodList("starfish", "otter")