Merge 358c719881 into 62c7e845d1

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

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(<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):

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

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