diff --git a/internal/copystructure/copystructure.go b/internal/copystructure/copystructure.go index aa5510298..c55897aaa 100644 --- a/internal/copystructure/copystructure.go +++ b/internal/copystructure/copystructure.go @@ -89,7 +89,15 @@ func copyValue(original reflect.Value) (any, error) { } copied := reflect.MakeSlice(original.Type(), original.Len(), original.Cap()) for i := 0; i < original.Len(); i++ { - val, err := copyValue(original.Index(i)) + elem := original.Index(i) + + // Handle nil values in slices (e.g., interface{} elements that are nil) + if elem.Kind() == reflect.Interface && elem.IsNil() { + copied.Index(i).Set(elem) + continue + } + + val, err := copyValue(elem) if err != nil { return nil, err } diff --git a/internal/copystructure/copystructure_test.go b/internal/copystructure/copystructure_test.go index d1708dc75..b21af6460 100644 --- a/internal/copystructure/copystructure_test.go +++ b/internal/copystructure/copystructure_test.go @@ -113,6 +113,21 @@ func TestCopy_Slice(t *testing.T) { input[0]["key1"] = "modified" assert.Equal(t, "value1", resultSlice[0]["key1"]) }) + + t.Run("slice with nil elements", func(t *testing.T) { + input := []any{ + "value1", + nil, + "value2", + } + result, err := Copy(input) + require.NoError(t, err) + + resultSlice, ok := result.([]any) + require.True(t, ok) + assert.Equal(t, input, resultSlice) + assert.Nil(t, resultSlice[1]) + }) } func TestCopy_Map(t *testing.T) { diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 07794a04a..6c72b3d56 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -302,6 +302,15 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref if dst == nil { return src } + // Track original non-nil src keys before modifying src + // This lets us distinguish between user nullifying a chart default vs + // user setting nil for a key not in chart defaults. + srcOriginalNonNil := make(map[string]bool) + for key, val := range src { + if val != nil { + srcOriginalNonNil[key] = true + } + } for key, val := range dst { if val == nil { src[key] = nil @@ -311,9 +320,13 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref // values. for key, val := range src { fullkey := concatPrefix(prefix, key) - if dv, ok := dst[key]; ok && !merge && dv == nil { + if dv, ok := dst[key]; ok && !merge && dv == nil && srcOriginalNonNil[key] { + // When coalescing (not merging), if dst has nil and src has a non-nil + // value, the user is nullifying a chart default - remove the key. + // But if src also has nil (or key not in src), preserve the nil delete(dst, key) } else if !ok { + // key not in user values, preserve src value (including nil) dst[key] = val } else if istable(val) { if istable(dv) { diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 871bfa8da..4eaf4be2b 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -731,3 +731,37 @@ func TestConcatPrefix(t *testing.T) { assert.Equal(t, "b", concatPrefix("", "b")) assert.Equal(t, "a.b", concatPrefix("a", "b")) } + +// TestCoalesceValuesEmptyMapWithNils tests the full CoalesceValues scenario +// from issue #31643 where chart has data: {} and user provides data: {foo: bar, baz: ~} +func TestCoalesceValuesEmptyMapWithNils(t *testing.T) { + is := assert.New(t) + + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "test"}, + Values: map[string]any{ + "data": map[string]any{}, // empty map in chart defaults + }, + } + + vals := map[string]any{ + "data": map[string]any{ + "foo": "bar", + "baz": nil, // explicit nil from user + }, + } + + v, err := CoalesceValues(c, vals) + is.NoError(err) + + data, ok := v["data"].(map[string]any) + is.True(ok, "data is not a map") + + // "foo" should be preserved + is.Equal("bar", data["foo"]) + + // "baz" should be preserved with nil value since it wasn't in chart defaults + _, ok = data["baz"] + is.True(ok, "Expected data.baz key to be present but it was removed") + is.Nil(data["baz"], "Expected data.baz key to be nil but it is not") +} diff --git a/pkg/kube/resource.go b/pkg/kube/resource.go index d88b171f0..d6d08d589 100644 --- a/pkg/kube/resource.go +++ b/pkg/kube/resource.go @@ -79,7 +79,14 @@ func (r ResourceList) Intersect(rs ResourceList) ResourceList { return r.Filter(rs.Contains) } -// isMatchingInfo returns true if infos match on Name and GroupVersionKind. +// isMatchingInfo returns true if infos match on Name, Namespace, Group and Kind. +// +// IMPORTANT: Version is intentionally excluded from the comparison. Resources +// served by the same CRD at different API versions (e.g. v2beta1 vs v2beta2) +// share the same underlying storage in the Kubernetes API server. Comparing +// the full GroupVersionKind causes Difference() to treat a version change as +// a resource removal + addition, which makes Helm delete the resource it just +// created during upgrades. See https://github.com/helm/helm/issues/31768 func isMatchingInfo(a, b *resource.Info) bool { - return a.Name == b.Name && a.Namespace == b.Namespace && a.Mapping.GroupVersionKind == b.Mapping.GroupVersionKind + return a.Name == b.Name && a.Namespace == b.Namespace && a.Mapping.GroupVersionKind.GroupKind() == b.Mapping.GroupVersionKind.GroupKind() } diff --git a/pkg/kube/resource_test.go b/pkg/kube/resource_test.go index ccc613c1b..41215a878 100644 --- a/pkg/kube/resource_test.go +++ b/pkg/kube/resource_test.go @@ -72,8 +72,8 @@ func TestIsMatchingInfo(t *testing.T) { gvkDiffVersion := schema.GroupVersionKind{Group: "group1", Version: "diff", Kind: "pod"} resourceInfoDiffVersion := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffVersion}} - if isMatchingInfo(&resourceInfo, &resourceInfoDiffVersion) { - t.Error("expected resources not equal") + if !isMatchingInfo(&resourceInfo, &resourceInfoDiffVersion) { + t.Error("expected resources with different versions but same group and kind to be equal") } gvkDiffKind := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "deployment"}