From 281552a64f96f39302e3a396ab85880dacf21869 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sat, 2 May 2026 09:45:06 +0800 Subject: [PATCH] fix: clean subchart-default nils when parent sets any child value In coalesceValues, childChartMergeTrue() was shadowing the merge variable to true for child chart keys, which caused cleanNilValues(src) to be skipped even during CoalesceValues (merge=false). This let nil values from subchart defaults leak through when the parent chart provided any user value under the subchart's namespace. Fix by using a separate childMerge variable for childChartMergeTrue so the original merge flag controls nil cleaning. Fixes #32093 Signed-off-by: yxxhero --- pkg/chart/common/util/coalesce.go | 8 ++- pkg/chart/common/util/coalesce_test.go | 91 ++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 999eeb208..36fb51f79 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -249,17 +249,19 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]any, prefix st } } else { // If the key is a child chart, coalesce tables with Merge set to true - merge := childChartMergeTrue(c, key, merge) + childMerge := childChartMergeTrue(c, key, merge) // When coalescing, clean nils from chart defaults before merging - // so they don't leak into the result. + // so they don't leak into the result. Use the original merge flag + // (not childMerge) because childMerge is always true for child + // chart keys, which would skip cleaning subchart-default nils. if !merge { cleanNilValues(src) } // Because v has higher precedence than nv, dest values override src // values. - coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), childMerge) } } } else { diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 252ef11ec..cfeafbe8d 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -928,3 +928,94 @@ func TestCoalesceValuesSubchartNilCleanedWhenUserPartiallyOverrides(t *testing.T _, ok = keyMapping["password"] is.False(ok, "Expected keyMapping.password (nil from chart defaults) to be removed even when user partially overrides the map") } + +// TestCoalesceValuesSubchartDefaultNilsCleanedWithUnrelatedUserValues tests that +// nil values in subchart defaults are cleaned even when the parent provides an +// unrelated user value under that subchart's namespace. +// Regression test for issue #32093. +func TestCoalesceValuesSubchartDefaultNilsCleanedWithUnrelatedUserValues(t *testing.T) { + is := assert.New(t) + + subchart := &chart.Chart{ + Metadata: &chart.Metadata{Name: "child"}, + Values: map[string]any{ + "keyMapping": map[string]any{ + "password": nil, + }, + "ingress": map[string]any{ + "configureCertmanager": nil, + }, + }, + } + + parent := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "parent"}, + Values: map[string]any{}, + }, subchart) + + vals := map[string]any{ + "child": map[string]any{ + "someOtherKey": "value", + }, + } + + v, err := CoalesceValues(parent, vals) + is.NoError(err) + + childVals, ok := v["child"].(map[string]any) + is.True(ok, "child values should be a map") + + keyMapping, ok := childVals["keyMapping"].(map[string]any) + is.True(ok, "keyMapping should be a map") + _, ok = keyMapping["password"] + is.False(ok, "Expected keyMapping.password (nil from chart defaults) to be removed when parent sets unrelated child value") + + ingress, ok := childVals["ingress"].(map[string]any) + is.True(ok, "ingress should be a map") + _, ok = ingress["configureCertmanager"] + is.False(ok, "Expected ingress.configureCertmanager (nil from chart defaults) to be removed when parent sets unrelated child value") +} + +// TestCoalesceValuesSubchartDefaultNilsCleanedWithPartialOverrideSameMap tests +// that nil values in subchart defaults are cleaned even when the user overrides +// a sibling key within the same map. +// Regression test for issue #32093. +func TestCoalesceValuesSubchartDefaultNilsCleanedWithPartialOverrideSameMap(t *testing.T) { + is := assert.New(t) + + subchart := &chart.Chart{ + Metadata: &chart.Metadata{Name: "child"}, + Values: map[string]any{ + "keyMapping": map[string]any{ + "password": nil, + "format": "bcrypt", + }, + }, + } + + parent := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "parent"}, + Values: map[string]any{}, + }, subchart) + + vals := map[string]any{ + "child": map[string]any{ + "keyMapping": map[string]any{ + "format": "sha256", + }, + }, + } + + v, err := CoalesceValues(parent, vals) + is.NoError(err) + + childVals, ok := v["child"].(map[string]any) + is.True(ok, "child values should be a map") + + keyMapping, ok := childVals["keyMapping"].(map[string]any) + is.True(ok, "keyMapping should be a map") + + is.Equal("sha256", keyMapping["format"], "User override should be preserved") + _, ok = keyMapping["password"] + is.False(ok, "Expected keyMapping.password (nil from chart defaults) to be removed when user overrides sibling key in same map") +}