From 5b5d82d74edebf875b6059d6e4e55701cb39b706 Mon Sep 17 00:00:00 2001 From: Kunal Jain Date: Mon, 20 Apr 2026 14:18:07 -0700 Subject: [PATCH 1/4] fix: Duplicate env vars are now treated as errors in helm v4 Signed-off-by: Kunal Jain --- pkg/kube/client.go | 93 +++++++++++++++++++++++++++++++++++++++++ pkg/kube/client_test.go | 90 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index c955e8875..b5ffb3c7a 100644 --- a/pkg/kube/client.go +++ b/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,86 @@ func copyRequestStreamToWriter(request *rest.Request, podName, containerName str return nil } +// deduplicateListMaps walks an unstructured Kubernetes object and deduplicates +// any list field where every item is a map containing a "name" key (e.g. env +// vars, volumes, containers). When duplicates are found the last occurrence +// wins, matching Kubernetes client-side-apply semantics. 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 := processNamedList(v) + if changed { + obj[key] = newList + deduped = true + } + } + } + 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) { + changed := false + + // Recurse into each map item first. + for i, item := range list { + if m, ok := item.(map[string]interface{}); ok { + if deduplicateListMaps(m) { + list[i] = m + changed = true + } + } + } + + if len(list) < 2 { + return list, changed + } + + // Only deduplicate if every item is a map with a "name" key. + for _, item := range list { + m, ok := item.(map[string]interface{}) + if !ok { + return list, changed + } + if _, hasName := m["name"]; !hasName { + 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 { diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index e98d87520..82d9e0e05 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -1859,6 +1859,96 @@ func TestDetermineFieldValidationDirective(t *testing.T) { 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, + }, + } + + 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") From bc82f2e18b31a45d51c0538452ec76e68ad70ffa Mon Sep 17 00:00:00 2001 From: Kunal Jain Date: Mon, 20 Apr 2026 14:34:17 -0700 Subject: [PATCH 2/4] fix: narrow SSA dedup to name-keyed fields, guard empty name - Add nameKeyedFields allowlist (env, containers, initContainers, ephemeralContainers, volumes, imagePullSecrets) so deduplicateListMaps only touches list fields that are actually keyed by "name" under server-side apply merge semantics; volumeMounts and other lists that incidentally contain a "name" field are no longer collapsed - processNamedList now skips deduplication when any item's "name" is a missing or empty string, preventing unrelated entries from being treated as duplicates - Add regression tests: volumeMounts with duplicate names preserved, env list with empty name not deduplicated Addresses Copilot review comments on PR #32061. Signed-off-by: Kunal Jain --- pkg/kube/client.go | 27 ++++++++++++++++++++++----- pkg/kube/client_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index b5ffb3c7a..4cbfa947e 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -1299,10 +1299,23 @@ 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, +} + // deduplicateListMaps walks an unstructured Kubernetes object and deduplicates -// any list field where every item is a map containing a "name" key (e.g. env -// vars, volumes, containers). When duplicates are found the last occurrence -// wins, matching Kubernetes client-side-apply semantics. Returns true if any +// 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. func deduplicateListMaps(obj map[string]interface{}) bool { deduped := false @@ -1313,6 +1326,9 @@ func deduplicateListMaps(obj map[string]interface{}) bool { deduped = true } case []interface{}: + if !nameKeyedFields[key] { + continue + } newList, changed := processNamedList(v) if changed { obj[key] = newList @@ -1344,13 +1360,14 @@ func processNamedList(list []interface{}) ([]interface{}, bool) { return list, changed } - // Only deduplicate if every item is a map with a "name" key. + // 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 } - if _, hasName := m["name"]; !hasName { + nameVal, isString := m["name"].(string) + if !isString || nameVal == "" { return list, changed } } diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 82d9e0e05..09f302d19 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -1938,6 +1938,40 @@ func TestDeduplicateListMaps(t *testing.T) { }, 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, + }, } for _, tt := range tests { From dc0197c98d03f00667ae80cfc8e65e171af889e8 Mon Sep 17 00:00:00 2001 From: Kunal Jain Date: Mon, 20 Apr 2026 19:12:41 -0700 Subject: [PATCH 3/4] test: add SSA integration test for duplicate env var deduplication Add TestPatchResourceServerSideDedupEnvVars to verify that duplicate env var entries in an unstructured manifest are removed from the PATCH body before server-side apply is sent, with last-value-wins semantics. Addresses Copilot review comment on PR #32061. Signed-off-by: Kunal Jain --- pkg/kube/client_test.go | 81 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 09f302d19..3d92eeed8 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -19,6 +19,7 @@ package kube import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -1853,6 +1854,86 @@ 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)) From 1c9f24adf0b037a8c8945b0acc4df4d28dc822a6 Mon Sep 17 00:00:00 2001 From: Kunal Jain Date: Mon, 20 Apr 2026 21:29:32 -0700 Subject: [PATCH 4/4] 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 --- 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 {