From 1c9f24adf0b037a8c8945b0acc4df4d28dc822a6 Mon Sep 17 00:00:00 2001
From: Kunal Jain <qlapon@gmail.com>
Date: Mon, 20 Apr 2026 21:29:32 -0700
Subject: [PATCH] fix: separate list traversal from deduplication in SSA dedup
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Rename nameKeyedFields -> dedupFields and remove container lists
(containers, initContainers, ephemeralContainers). Container lists are
now always traversed to reach nested env/volumes, but are never
deduplicated themselves — duplicate container names are invalid and
must not be silently dropped with last-value-wins semantics.

Add a test case asserting duplicate container names are preserved.

Addresses Copilot review comment on PR #32061.

Signed-off-by: Kunal Jain <qlapon@gmail.com>
---
 pkg/kube/client.go      | 51 ++++++++++++++++++++---------------------
 pkg/kube/client_test.go | 19 +++++++++++++++
 2 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/pkg/kube/client.go b/pkg/kube/client.go
index 4cbfa947e..dc4268d81 100644
--- a/pkg/kube/client.go
+++ b/pkg/kube/client.go
@@ -1299,24 +1299,25 @@ func copyRequestStreamToWriter(request *rest.Request, podName, containerName str
 	return nil
 }
 
-// nameKeyedFields is the set of Kubernetes list fields that are keyed by the
-// "name" field under server-side apply merge semantics. Only these fields are
-// candidates for deduplication; other lists that incidentally contain a "name"
-// field (e.g. volumeMounts, which is keyed by mountPath) must not be touched.
-var nameKeyedFields = map[string]bool{
-	"containers":          true,
-	"initContainers":      true,
-	"ephemeralContainers": true,
-	"env":                 true,
-	"volumes":             true,
-	"imagePullSecrets":    true,
+// 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 that are keyed by "name" under server-side apply merge semantics
-// (see nameKeyedFields). When duplicates are found the last occurrence wins,
-// matching Kubernetes client-side-apply semantics. Returns true if any
-// deduplication occurred.
+// 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 {
@@ -1326,10 +1327,7 @@ func deduplicateListMaps(obj map[string]interface{}) bool {
 				deduped = true
 			}
 		case []interface{}:
-			if !nameKeyedFields[key] {
-				continue
-			}
-			newList, changed := processNamedList(v)
+			newList, changed := processListItems(v, dedupFields[key])
 			if changed {
 				obj[key] = newList
 				deduped = true
@@ -1339,14 +1337,15 @@ func deduplicateListMaps(obj map[string]interface{}) bool {
 	return deduped
 }
 
-// processNamedList recurses into list items and deduplicates the list if every
-// item is a map with a "name" key. The last occurrence of each name is kept and
-// the relative order of surviving items is preserved. Returns the (possibly
-// modified) list and whether any change was made.
-func processNamedList(list []interface{}) ([]interface{}, bool) {
+// 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
 
-	// Recurse into each map item first.
+	// 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) {
@@ -1356,7 +1355,7 @@ func processNamedList(list []interface{}) ([]interface{}, bool) {
 		}
 	}
 
-	if len(list) < 2 {
+	if !dedup || len(list) < 2 {
 		return list, changed
 	}
 
diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go
index 3d92eeed8..64dda324f 100644
--- a/pkg/kube/client_test.go
+++ b/pkg/kube/client_test.go
@@ -2053,6 +2053,25 @@ func TestDeduplicateListMaps(t *testing.T) {
 			},
 			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 {