diff --git a/pkg/kube/client.go b/pkg/kube/client.go index c955e8875..dc4268d81 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,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 { diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index ed871c05a..71f64a861 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" @@ -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")