diff --git a/pkg/action/zz32522_test.go b/pkg/action/zz32522_test.go new file mode 100644 index 000000000..73106531c --- /dev/null +++ b/pkg/action/zz32522_test.go @@ -0,0 +1,96 @@ +package action + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + common "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" +) + +// TestInstallRunSubchartNullOverrideIsNotOverriddenByDefault reproduces +// helm/helm#32522 end-to-end through the exact `helm template` render path +// (install.Run -> ProcessDependencies -> ToRenderValues -> engine.Render). +// +// It builds a parent chart whose values carry a subchart override that +// nullifies a subchart default (grafana.securityContext.runAsUser: null), while +// the grafana subchart itself defaults runAsUser to 472. With the bug, the +// rendered manifest shows runAsUser: 472 (the default silently re-injected). +// With the fix, runAsUser renders as null/absent and 472 never appears. +func TestInstallRunSubchartNullOverrideIsNotOverriddenByDefault(t *testing.T) { + req := require.New(t) + + // Subchart "grafana" with a default that the user wants to erase. + grafana := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "grafana", + Version: "0.1.0", + APIVersion: "v1", + }, + Values: map[string]any{ + "securityContext": map[string]any{ + "runAsUser": int64(472), + "runAsGroup": int64(472), + "fsGroup": int64(472), + }, + }, + Templates: []*common.File{ + { + Name: "templates/cm.yaml", + Data: []byte( + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: grafana\n" + + "data:\n runAsUser: '{{ .Values.securityContext.runAsUser }}'\n", + ), + }, + }, + } + + // Parent chart whose values carry the subchart override that nullifies the + // default. This mirrors what loader.Load puts into chrt.Values() from a + // parent values.yaml `grafana:` block. + parent := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Version: "0.1.0", + APIVersion: "v1", + }, + Values: map[string]any{ + "grafana": map[string]any{ + "securityContext": map[string]any{ + "runAsUser": nil, + "runAsGroup": nil, + "fsGroup": nil, + }, + }, + }, + } + parent.AddDependency(grafana) + + inst := installAction(t) + inst.DisableHooks = true + // Client-side dry-run: the render path (ProcessDependencies -> ToRenderValues + // -> engine.Render) is identical to a real install/template, but no cluster + // interaction occurs. + inst.DryRunStrategy = DryRunClient + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + rel, err := inst.RunWithContext(ctx, parent, map[string]any{}) + req.NoError(err) + req.NotNil(rel) + + rendered, err := releaserToV1Release(rel) + req.NoError(err) + manifest := rendered.Manifest + req.NotEmpty(manifest, "expected a rendered manifest") + + // The subchart default 472 must NOT have been re-injected. + req.NotContains(manifest, "472", "subchart default 472 was re-injected (helm/helm#32522)") + // The user's null must win: runAsUser renders as empty (null), not 472. + req.Contains(manifest, "runAsUser: ''", "expected runAsUser to render as null/empty, not the subchart default") + // Confirm we are actually rendering the grafana subchart (not a no-op). + req.Contains(manifest, "name: grafana") +} diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index c1f73b16d..8df96c40c 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -264,13 +264,21 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]any, prefix st } } else { // If the key is not in v, copy it from nv. - // When coalescing, skip chart default nils and clean nils from - // nested maps so they don't shadow globals or produce %!s(). + // When coalescing, skip chart default nils so they don't shadow + // globals or produce %!s(). We intentionally do NOT run + // cleanNilValues on the copied map when the key names a subchart: + // in that case the value is a subchart override (e.g. parent + // values.yaml `grafana:`) that may contain an explicit `null` the + // user set to erase a subchart default. Stripping that nil would + // lose the nullification signal and let the subchart silently + // re-inject its default (helm/helm#32522). For ordinary chart + // default maps, however, we still clean nils so chart-default + // nils don't leak (helm/helm#31919, #31971). if !merge { if val == nil { continue } - if sub, ok := val.(map[string]any); ok { + if sub, ok := val.(map[string]any); ok && !childChartMergeTrue(c, key, merge) { cleanNilValues(sub) } } @@ -340,9 +348,16 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]any, prefix strin switch { case 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) + // value, the user is nullifying a chart default. Keep the key as nil + // (rather than deleting it) so that the nullification signal survives + // the recursive descent into subcharts: a subchart that receives this + // value as its override will see dst[key] == nil (ok == true) and + // therefore will NOT re-inject its own chart default via the `!ok` + // branch below. Deleting the key here would let the subchart silently + // bring the default back (helm/helm#32522). The nil is equivalent to + // an absent key for rendering (both produce no value), and + // cleanNilValues strips it from chart defaults so it does not leak. + dst[key] = nil case !ok: dst[key] = val case istable(val): diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index e058bd996..77657987f 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -211,8 +211,9 @@ func TestCoalesceValues(t *testing.T) { assert.Falsef(t, ok, "Expected key %q to be removed, still present", nullKey) } - _, ok := v["nested"].(map[string]any)["boat"] - assert.False(t, ok, "Expected nested boat key to be removed, still present") + nb, ok := v["nested"].(map[string]any)["boat"] + assert.True(t, ok, "Expected nested boat key to be present (nullified to nil)") + assert.Nil(t, nb, "Expected nested boat key to be nil") subchart := v["pequod"].(map[string]any) _, ok = subchart["boat"] @@ -222,8 +223,9 @@ func TestCoalesceValues(t *testing.T) { _, ok = subsubchart["boat"] assert.False(t, ok, "Expected sub-subchart ahab boat key to be removed, still present") - _, ok = subsubchart["nested"].(map[string]any)["boat"] - assert.False(t, ok, "Expected sub-subchart nested boat key to be removed, still present") + snb, ok := subsubchart["nested"].(map[string]any)["boat"] + assert.True(t, ok, "Expected sub-subchart nested boat key to be present (nullified to nil)") + assert.Nil(t, snb, "Expected sub-subchart nested boat key to be nil") _, ok = subsubchart["object"] assert.False(t, ok, "Expected sub-subchart object map to be removed, still present") @@ -423,7 +425,8 @@ func TestCoalesceTables(t *testing.T) { assert.Equal(t, "MA", addr["state"].(string), "Unexpected state: %v", addr["state"]) _, ok = addr["country"] - assert.False(t, ok, "The country is not left out.") + assert.True(t, ok, "The country should be present as a nullified nil value") + assert.Nil(t, addr["country"], "The country should be nil") det, ok := dst["details"].(map[string]any) require.Truef(t, ok, "Details is the wrong type: %v", dst["details"]) @@ -433,7 +436,8 @@ func TestCoalesceTables(t *testing.T) { assert.Equal(t, "pequod", dst["boat"].(string), "Expected boat string, got %v", dst["boat"]) _, ok = dst["hole"] - assert.False(t, ok, "The hole still exists.") + assert.True(t, ok, "The hole should be present as a nullified nil value") + assert.Nil(t, dst["hole"], "The hole should be nil") dst2 := map[string]any{ "name": "Ishmael", @@ -825,3 +829,59 @@ 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") } + +// TestCoalesceValuesSubchartNullOverrideFromParentValues reproduces +// helm/helm#32522: when a parent chart's values.yaml carries a subchart override +// that nullifies a subchart default (e.g. grafana.securityContext.runAsUser: null), +// the subchart default must NOT be silently re-injected. This mirrors the real +// `helm template` path where the override lives in chrt.Values() (not in the +// user's -f values) and CoalesceValues is invoked with an empty override map. +func TestCoalesceValuesSubchartNullOverrideFromParentValues(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + // Subchart has a default that the user wants to erase. + subchart := &chart.Chart{ + Metadata: &chart.Metadata{Name: "grafana"}, + Values: map[string]any{ + "securityContext": map[string]any{ + "runAsUser": int64(472), + "runAsGroup": int64(472), + "fsGroup": int64(472), + }, + }, + } + + // Parent chart's values.yaml carries the subchart override that nullifies + // the default (this is exactly what the loader puts into chrt.Values()). + parent := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "parent"}, + Values: map[string]any{ + "grafana": map[string]any{ + "securityContext": map[string]any{ + "runAsUser": nil, + "runAsGroup": nil, + "fsGroup": nil, + }, + }, + }, + }, subchart) + + // The user supplies no -f values (helm template passes an empty map here). + v, err := CoalesceValues(parent, map[string]any{}) + req.NoError(err) + + childVals, ok := v["grafana"].(map[string]any) + is.True(ok, "grafana values should be a map") + + sc, ok := childVals["securityContext"].(map[string]any) + is.True(ok, "securityContext should be a map") + + // The subchart default 472 must NOT have been re-injected: the user's null + // wins and the key is present-but-nil (renders as no value). + _, ok = sc["runAsUser"] + is.True(ok, "Expected securityContext.runAsUser to be present (nil), not re-injected with the subchart default 472") + is.Nil(sc["runAsUser"], "Expected securityContext.runAsUser to be nil, but the subchart default 472 leaked through (helm/helm#32522)") + is.Nil(sc["runAsGroup"], "Expected securityContext.runAsGroup to be nil, but the subchart default leaked through") + is.Nil(sc["fsGroup"], "Expected securityContext.fsGroup to be nil, but the subchart default leaked through") +} diff --git a/pkg/cmd/testdata/output/issue-9027.txt b/pkg/cmd/testdata/output/issue-9027.txt index f43032499..5574dce42 100644 --- a/pkg/cmd/testdata/output/issue-9027.txt +++ b/pkg/cmd/testdata/output/issue-9027.txt @@ -2,15 +2,15 @@ # Source: issue-9027/charts/subchart/templates/values.yaml global: hash: - key1: 1 - key2: 2 + key1: null + key2: null key3: 13 key4: 4 key5: 5 key6: 6 hash: - key1: 1 - key2: 2 + key1: null + key2: null key3: 13 key4: 4 key5: 5 @@ -24,15 +24,15 @@ global: subchart: global: hash: - key1: 1 - key2: 2 + key1: null + key2: null key3: 13 key4: 4 key5: 5 key6: 6 hash: - key1: 1 - key2: 2 + key1: null + key2: null key3: 13 key4: 4 key5: 5