From 5b5d82d74edebf875b6059d6e4e55701cb39b706 Mon Sep 17 00:00:00 2001 From: Kunal Jain Date: Mon, 20 Apr 2026 14:18:07 -0700 Subject: [PATCH] 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")