From 281552a64f96f39302e3a396ab85880dacf21869 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sat, 2 May 2026 09:45:06 +0800 Subject: [PATCH 1/2] 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") +} From 33183f6e2d48a0b3491fe3360c46d32ee0d319ec Mon Sep 17 00:00:00 2001 From: yxxhero Date: Mon, 4 May 2026 14:20:59 +0800 Subject: [PATCH 2/2] fix: seed parent.Values via MergeValues in regression tests Populate parent.Values with MergeValues(parent, nil) to simulate the state after processImportValues, so tests exercise the childChartMergeTrue code path that was the actual source of the #32093 regression. Rename partial-override test to avoid duplication with existing TestCoalesceValuesSubchartNilCleanedWhenUserPartiallyOverrides. Signed-off-by: yxxhero --- pkg/chart/common/util/coalesce_test.go | 32 +++++++++++++++++++++----- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index cfeafbe8d..4ea45014f 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -932,7 +932,11 @@ func TestCoalesceValuesSubchartNilCleanedWhenUserPartiallyOverrides(t *testing.T // 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. +// +// This simulates the state after processImportValues (merge=true) populates +// parent.Values with merged child defaults including nils, then CoalesceValues +// is called with user values that touch the child namespace. The bug in #32093 +// was that childChartMergeTrue shadowed the merge flag, skipping cleanNilValues. func TestCoalesceValuesSubchartDefaultNilsCleanedWithUnrelatedUserValues(t *testing.T) { is := assert.New(t) @@ -953,6 +957,12 @@ func TestCoalesceValuesSubchartDefaultNilsCleanedWithUnrelatedUserValues(t *test Values: map[string]any{}, }, subchart) + // Simulate the state after processImportValues has populated parent.Values + // with merged defaults (including nils from child chart defaults). + mergedDefaults, err := MergeValues(parent, nil) + is.NoError(err) + parent.Values = mergedDefaults + vals := map[string]any{ "child": map[string]any{ "someOtherKey": "value", @@ -976,11 +986,15 @@ func TestCoalesceValuesSubchartDefaultNilsCleanedWithUnrelatedUserValues(t *test 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) { +// TestCoalesceValuesSubchartDefaultNilsCleanedWithPartialOverrideSameMapMerged +// tests that nil values in subchart defaults are cleaned even when the user +// overrides a sibling key within the same map, after the parent chart's Values +// have been populated by a prior merge step. +// +// Unlike TestCoalesceValuesSubchartNilCleanedWhenUserPartiallyOverrides which +// tests coalescing from scratch, this test seeds parent.Values via MergeValues +// to exercise the childChartMergeTrue code path described in issue #32093. +func TestCoalesceValuesSubchartDefaultNilsCleanedWithPartialOverrideSameMapMerged(t *testing.T) { is := assert.New(t) subchart := &chart.Chart{ @@ -998,6 +1012,12 @@ func TestCoalesceValuesSubchartDefaultNilsCleanedWithPartialOverrideSameMap(t *t Values: map[string]any{}, }, subchart) + // Simulate the state after processImportValues has populated parent.Values + // with merged defaults (including nils from child chart defaults). + mergedDefaults, err := MergeValues(parent, nil) + is.NoError(err) + parent.Values = mergedDefaults + vals := map[string]any{ "child": map[string]any{ "keyMapping": map[string]any{