diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 85904124b..de1e00f19 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -621,6 +621,14 @@ func (u *Upgrade) reuseValues(chart *chartv2.Chart, current *release.Release, ne return nil, fmt.Errorf("failed to rebuild old values: %w", err) } + // A `--set key=null` cancels key's value from current.Config below, + // but that alone only stops CoalesceTables from copying it over. + // oldVals becomes chart.Values, which the final render-time coalesce + // falls back to for any key newVals doesn't have, so without pruning + // the same key here it would resurface from the release being + // reused instead of the chart's own default. + pruneNilOverrides(oldVals, newVals) + newVals = util.CoalesceTables(newVals, current.Config) chart.Values = oldVals @@ -644,6 +652,23 @@ func (u *Upgrade) reuseValues(chart *chartv2.Chart, current *release.Release, ne return newVals, nil } +// pruneNilOverrides deletes from dst every key that is explicitly nil in +// newVals, recursing into nested tables so an explicit `--set key=null` +// removes the key from dst at whatever depth it appears. +func pruneNilOverrides(dst, newVals map[string]any) { + for key, val := range newVals { + if val == nil { + delete(dst, key) + continue + } + if sub, ok := val.(map[string]any); ok { + if dsub, ok := dst[key].(map[string]any); ok { + pruneNilOverrides(dsub, sub) + } + } + } +} + func validateManifest(c kube.Interface, manifest []byte, openAPIValidation bool) error { _, err := c.Build(bytes.NewReader(manifest), openAPIValidation) return err diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index 53419b6a8..4d5773afd 100644 --- a/pkg/action/upgrade_test.go +++ b/pkg/action/upgrade_test.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/cli-runtime/pkg/resource" + chartcommon "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/kube" kubefake "helm.sh/helm/v4/pkg/kube/fake" @@ -332,6 +333,56 @@ func TestUpgradeRelease_ReuseValues(t *testing.T) { } is.Equal(expectedValues, updatedRes.Config) }) + + t.Run("nulling a previously set nested value with reuse-values takes effect on the first upgrade", func(t *testing.T) { + is := assert.New(t) + req := require.New(t) + upAction := upgradeAction(t) + + imageTemplate := &chartcommon.File{ + Name: "templates/image", + ModTime: time.Now(), + Data: []byte(`image: {{ .Values.services.myservice.image | default "chart-default" }} +tag: {{ .Values.services.myservice.tag }}`), + } + ch := buildChartWithTemplates([]*chartcommon.File{imageTemplate}) + + rel := releaseStub() + rel.Name = "nuketown" + rel.Info.Status = common.StatusDeployed + rel.Chart = ch + rel.Config = map[string]any{ + "services": map[string]any{ + "myservice": map[string]any{ + "image": "old-image", + }, + }, + } + req.NoError(upAction.cfg.Releases.Create(rel)) + + upAction.ReuseValues = true + resi, err := upAction.Run(rel.Name, ch, map[string]any{ + "services": map[string]any{ + "myservice": map[string]any{ + "image": nil, + "tag": "1.0.0", + }, + }, + }) + req.NoError(err) + res, err := releaserToV1Release(resi) + req.NoError(err) + + services, ok := res.Config["services"].(map[string]any) + req.True(ok, "expected services key in persisted config") + myservice, ok := services["myservice"].(map[string]any) + req.True(ok, "expected services.myservice key in persisted config") + is.NotContains(myservice, "image", "explicit null should remove the nested key from the persisted config, at whatever depth it appears") + is.Equal("1.0.0", myservice["tag"]) + + is.Contains(res.Manifest, "image: chart-default", "nulling a reused nested value should fall back to the chart default on the first upgrade, not the second") + is.Contains(res.Manifest, "tag: 1.0.0") + }) } func TestUpgradeRelease_ResetThenReuseValues(t *testing.T) {