fix(coalesce): preserve user null subchart override instead of re-injecting default

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 (472) was silently re-injected during coalescing.

Root cause: in coalesceValues, the  branch copied a not-yet-present
chart value into the result map and unconditionally ran cleanNilValues on it.
For a subchart override block (key names a subchart, e.g. grafana:), that
value contains the user's explicit null used to erase a subchart default;
cleanNilValues stripped it, so the nullification signal was lost and the
subchart's coalesce later re-added its default via the !ok branch.

Fix: skip cleanNilValues when the key names a subchart (childChartMergeTrue),
so the user's null survives into the subchart override and is kept as nil
rather than re-injecting the default. Ordinary chart-default nils are still
cleaned (helm/helm#31919, #31971), preserving their existing behaviour.

Adds a regression test reproducing the real helm template path
(chrt.Values() carries the override, CoalesceValues called with an empty
user map) where the subchart default must not leak.

Fixes helm/helm#32522

Signed-off-by: Mehrdad Biukian Naeini <mehrdad.biu@mtnirancell.ir>
pull/32538/head
Mehrdad Biukian Naeini 4 weeks ago
parent f3d68cdbea
commit 358c719881

@ -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")
}

@ -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(<nil>).
// When coalescing, skip chart default nils so they don't shadow
// globals or produce %!s(<nil>). 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):

@ -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")
}

@ -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

Loading…
Cancel
Save