fix: reuse only user-supplied values when no values flag is given (#464)

cmd/helm.go

@@ -214,7 +214,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 	if d.insecureSkipTLSVerify {
 		flags = append(flags, "--insecure-skip-tls-verify")
 	}
-	// Helm automatically enable --reuse-values when there's no --set, --set-string, --set-json, --set-values, --set-file present.
+	// Helm automatically reuses the user-supplied values of the last release when there's no --set, --set-string, --set-json, --set-values, --set-file present.
 	// Let's simulate that in helm-diff.
 	// See https://medium.com/@kcatstack/understand-helm-upgrade-flags-reset-values-reuse-values-6e58ac8f127e
 	shouldDefaultReusingValues := len(d.values) == 0 && len(d.stringValues) == 0 && len(d.stringLiteralValues) == 0 && len(d.jsonValues) == 0 && len(d.valueFiles) == 0 && len(d.fileValues) == 0
@@ -240,12 +240,23 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 			}
 			err = d.writeExistingValues(tmpfile, false)
 		} else {
-			err = d.writeExistingValues(tmpfile, true)
+			// Only --reuse-values also reuses the values of the chart that the release was installed from.
+			// By default, Helm reuses just the user-supplied values, so that changes to the values.yaml
+			// of the chart take effect. See https://github.com/databus23/helm-diff/issues/464
+			err = d.writeExistingValues(tmpfile, d.reuseValues)
 		}
 		if err != nil {
 			return nil, err
 		}
-		flags = append(flags, "--values", tmpfile.Name())
+		existingValues, err := os.ReadFile(tmpfile.Name())
+		if err != nil {
+			return nil, err
+		}
+		// `helm get values` prints "null" for a release without user-supplied values.
+		// There is nothing to reuse then.
+		if v := bytes.TrimSpace(existingValues); len(v) > 0 && !bytes.Equal(v, []byte("null")) {
+			flags = append(flags, "--values", tmpfile.Name())
+		}
 	}
 	for _, value := range d.values {
 		flags = append(flags, "--set", value)

cmd/main_test.go

@@ -32,13 +32,18 @@ func printFakeHelmVersion() {
 }
 
 // printFakeHelmOutput prints the output for a fake helm invocation.
-// A `helm version` call prints helm version build info; any other
+// A `helm version` call prints helm version build info; a `helm get values`
+// call prints HELM_DIFF_FAKE_VALUES_OUTPUT if that is set; any other
 // invocation prints HELM_DIFF_FAKE_OUTPUT.
 func printFakeHelmOutput() {
 	if len(os.Args) > 1 && os.Args[1] == "version" {
 		printFakeHelmVersion()
 		return
 	}
+	if v, ok := os.LookupEnv("HELM_DIFF_FAKE_VALUES_OUTPUT"); ok && len(os.Args) > 2 && os.Args[1] == "get" && os.Args[2] == "values" {
+		fmt.Print(v)
+		return
+	}
 	fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT"))
 }
 
@@ -129,4 +134,26 @@ func TestFakeHelmVersionOutput(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, "manifest-output", string(out))
 	})
+
+	t.Run("get values prints HELM_DIFF_FAKE_VALUES_OUTPUT when set", func(t *testing.T) {
+		t.Setenv("HELM_DIFF_FAKE_HELM", "1")
+		t.Setenv("HELM_DIFF_FAKE_HELM_MODE", "default")
+		t.Setenv("HELM_DIFF_FAKE_VALUES_OUTPUT", "user: value\n")
+		t.Setenv("HELM_DIFF_FAKE_OUTPUT", "manifest-output")
+
+		out, err := exec.Command(exe, "get", "values", "my-release").CombinedOutput()
+		require.NoError(t, err)
+		require.Equal(t, "user: value\n", string(out))
+	})
+
+	t.Run("get values prints empty output when HELM_DIFF_FAKE_VALUES_OUTPUT is set but empty", func(t *testing.T) {
+		t.Setenv("HELM_DIFF_FAKE_HELM", "1")
+		t.Setenv("HELM_DIFF_FAKE_HELM_MODE", "default")
+		t.Setenv("HELM_DIFF_FAKE_VALUES_OUTPUT", "")
+		t.Setenv("HELM_DIFF_FAKE_OUTPUT", "manifest-output")
+
+		out, err := exec.Command(exe, "get", "values", "my-release").CombinedOutput()
+		require.NoError(t, err)
+		require.Empty(t, out)
+	})
 }

cmd/upgrade_test.go

@@ -401,35 +401,92 @@ data:
 		fakeHelmMode     string
 		args             []string
 		useUpgradeDryRun bool
+		// existingValues is what `helm get values` prints for an existing release.
+		// Empty means that it prints the manifest, which stands in for some values.
+		existingValues string
 		// wantGetValues is the expected `helm get values` call.
 		// Empty means that the values must not be fetched at all.
 		wantGetValues string
+		// wantValues is whether the fetched values must be passed on with --values.
+		wantValues bool
 	}{
 		{
 			name:          "existing release with --reuse-values",
 			fakeHelmMode:  "capture_args",
 			args:          []string{"--reuse-values"},
 			wantGetValues: "get values my-release --output yaml --all --namespace prod-apps\n",
+			wantValues:    true,
 		},
 		{
 			name:          "existing release with --reset-then-reuse-values",
 			fakeHelmMode:  "capture_args",
 			args:          []string{"--reset-then-reuse-values"},
 			wantGetValues: "get values my-release --output yaml --namespace prod-apps\n",
+			wantValues:    true,
 		},
 		{
 			name:          "existing release with --install --reuse-values",
 			fakeHelmMode:  "capture_args",
 			args:          []string{"--install", "--reuse-values"},
 			wantGetValues: "get values my-release --output yaml --all --namespace prod-apps\n",
+			wantValues:    true,
 		},
 		{
-			// Helm enables --reuse-values by default when there is no --set, --set-string,
-			// --set-literal, --set-json, --values or --set-file flag, so the values of an
-			// existing release are fetched even without any of the reuse flags.
+			// Helm reuses the user-supplied values of an existing release when there is no
+			// --set, --set-string, --set-literal, --set-json, --values or --set-file flag.
+			// Unlike with --reuse-values, the values of the chart that the release was installed
+			// from are not reused, so the values are fetched without --all.
+			// See https://github.com/databus23/helm-diff/issues/464
 			name:          "existing release without value flags",
 			fakeHelmMode:  "capture_args",
+			wantGetValues: "get values my-release --output yaml --namespace prod-apps\n",
+			wantValues:    true,
+		},
+		{
+			name:             "existing release without value flags and HELM_DIFF_USE_UPGRADE_DRY_RUN",
+			fakeHelmMode:     "capture_args",
+			useUpgradeDryRun: true,
+			wantGetValues:    "get values my-release --output yaml --namespace prod-apps\n",
+			wantValues:       true,
+		},
+		{
+			// `helm get values` prints "null" for a release without user-supplied values.
+			name:           "existing release without value flags and without user-supplied values",
+			fakeHelmMode:   "capture_args",
+			existingValues: "null\n",
+			wantGetValues:  "get values my-release --output yaml --namespace prod-apps\n",
+		},
+		{
+			// Helm reuses the values of an existing release only when no new values are
+			// given, so with --set there is nothing to fetch and the release values are dropped.
+			name:         "existing release with --set",
+			fakeHelmMode: "capture_args",
+			args:         []string{"--set", "key=val"},
+		},
+		{
+			// --reset-values resets to the values built into the chart, so the values
+			// of the release are not fetched at all.
+			name:         "existing release with --reset-values",
+			fakeHelmMode: "capture_args",
+			args:         []string{"--reset-values"},
+		},
+		{
+			// An explicit --reuse-values also reuses the values of the chart that the
+			// release was installed from, even when --set is given on top.
+			name:          "existing release with --reuse-values and --set",
+			fakeHelmMode:  "capture_args",
+			args:          []string{"--reuse-values", "--set", "key=val"},
 			wantGetValues: "get values my-release --output yaml --all --namespace prod-apps\n",
+			wantValues:    true,
+		},
+		{
+			// `helm get values` prints "null" for a release without user-supplied values,
+			// so --reset-then-reuse-values has nothing to pass on with --values either.
+			name:           "existing release with --reset-then-reuse-values and without user-supplied values",
+			fakeHelmMode:   "capture_args",
+			args:           []string{"--reset-then-reuse-values"},
+			existingValues: "null\n",
+			wantGetValues:  "get values my-release --output yaml --namespace prod-apps\n",
 		},
 		{
 			name:         "unreleased with --install",
@@ -468,6 +525,9 @@ data:
 		t.Run(tc.name, func(t *testing.T) {
 			argsFile := t.TempDir() + "/args"
 			setupFakeHelm(t, tc.fakeHelmMode, manifestYAML, argsFile, "")
+			if tc.existingValues != "" {
+				t.Setenv("HELM_DIFF_FAKE_VALUES_OUTPUT", tc.existingValues)
+			}
 			renderSubcmd := "template"
 			if tc.useUpgradeDryRun {
 				t.Setenv("HELM_DIFF_USE_UPGRADE_DRY_RUN", "true")
@@ -499,6 +559,9 @@ data:
 			} else if !strings.Contains(argsContent, tc.wantGetValues) {
 				t.Errorf("expected 'helm %s', got:\n%s", strings.TrimSpace(tc.wantGetValues), argsContent)
 			}
+			if strings.Contains(argsContent, " --values ") != tc.wantValues {
+				t.Errorf("expected the existing values to be passed with --values: %t, got:\n%s", tc.wantValues, argsContent)
+			}
 		})
 	}
 }

scripts/issues/464.sh

@@ -0,0 +1,130 @@
+#!/usr/bin/env bash
+# Reproduction test for https://github.com/databus23/helm-diff/issues/464
+#
+# Bug: "Diff is incorrect when values in values.yaml change and using local Chart".
+#
+# When no --set/--values flag is given, helm-diff fetched the values of the
+# release with `helm get values --all`, i.e. the fully coalesced values
+# including the values.yaml of the chart the release was installed from,
+# and passed them to the render command as --values. A --values file takes
+# precedence over the new chart's values.yaml, so every change to a chart
+# default was masked and the diff came out empty.
+#
+# Helm's own default only reuses the user-supplied values of the release
+# (current.Config), never the coalesced ones, so chart default changes do
+# apply. `helm upgrade --reuse-values` is the flag that keeps the old chart
+# values.
+#
+# Scenarios (each prints its full diff output to the CI log):
+#   A. chart default change, release without user-supplied values:
+#      the change is visible in the diff
+#   B. same release with --reuse-values: the old chart values are kept,
+#      so the change is not visible (unchanged --reuse-values semantics)
+#   C. chart default change, release with a user-supplied value:
+#      the user value wins, no diff
+#   D. same release with --set: the release values are not reused,
+#      the --set value replaces the user value in the diff
+
+set -euo pipefail
+
+ISSUE=464
+# shellcheck source=lib.sh
+source "$(dirname "$0")/lib.sh"
+
+# expect <scenario> <description> <ERE pattern> <present|absent>
+expect() {
+  local scenario="$1" desc="$2" pattern="$3" want="$4" got
+  local out="$WORK/${scenario// /_}.out"
+  strip_ansi < "$out" > "$WORK/stripped.out"
+  if grep -qE "$pattern" "$WORK/stripped.out"; then
+    got=present
+  else
+    got=absent
+  fi
+  if [ "$got" = "$want" ]; then
+    echo "OK: [$scenario] $desc is $want"
+  else
+    echo "FAIL: [$scenario] expected $desc to be $want (#${ISSUE})"
+    FAIL=1
+  fi
+}
+
+# new_chart <dir> <default> writes a chart with a single ConfigMap whose
+# only value comes from the chart's values.yaml. The ConfigMap is named
+# after the release so the two releases below can coexist in one namespace:
+# helm (3.18+ and v4) refuses to import a resource another release owns.
+new_chart() {
+  mkdir -p "$1/templates"
+  cat > "$1/Chart.yaml" <<'YAML'
+apiVersion: v2
+name: issue-test
+version: 0.1.0
+YAML
+  printf 'key: %s\n' "$2" > "$1/values.yaml"
+  cat > "$1/templates/cm.yaml" <<'YAML'
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cm-{{ .Release.Name }}
+data:
+  key: {{ .Values.key | quote }}
+YAML
+}
+
+kubectl create namespace "$NS" 2>/dev/null || true
+
+new_chart "$WORK/chart" old
+# rel has no user-supplied values, rel2 has one that shadows the default.
+helm upgrade -i rel "$WORK/chart" -n "$NS" >/dev/null
+helm upgrade -i rel2 "$WORK/chart" -n "$NS" --set key=user >/dev/null
+
+# check <description> <got> <want> guards a setup assumption and fails the
+# test on mismatch.
+check() {
+  local desc="$1" got="$2" want="$3"
+  if [ "$got" = "$want" ]; then
+    echo "OK: $desc"
+  else
+    echo "FAIL: $desc, got: $got, want: $want (#${ISSUE})"
+    # shellcheck disable=SC2034 # FAIL is read by finish() in lib.sh
+    FAIL=1
+  fi
+}
+
+# `helm get values` must print exactly what the scenarios below rely on.
+check "rel has no user-supplied values" "$(helm get values rel -n "$NS" --output yaml)" "null"
+check "rel2 has key=user as its only user-supplied value" "$(helm get values rel2 -n "$NS" --output yaml)" "key: user"
+
+# The only change from here on is the chart's default in values.yaml.
+printf 'key: new\n' > "$WORK/chart/values.yaml"
+
+###############################################################################
+# Variant A: the chart default change must be visible in the default diff
+###############################################################################
+run_diff "A default" noassert rel "$WORK/chart" -n "$NS"
+expect "A default" "the new chart default (key: \"new\")" '^\+[[:space:]]*key: "new"' present
+expect "A default" "the old chart default (key: \"old\")" '^-[[:space:]]*key: "old"' present
+
+###############################################################################
+# Variant B: --reuse-values keeps the values of the chart the release was
+# installed from, so the change stays invisible (as in plain Helm)
+###############################################################################
+run_diff "B reuse-values" noassert rel "$WORK/chart" -n "$NS" --reuse-values
+expect "B reuse-values" "no key diff (--reuse-values keeps the old chart values)" '^[+-][[:space:]]*key:' absent
+
+###############################################################################
+# Variant C: a user-supplied value shadows the default, so the default change
+# does not affect the diff
+###############################################################################
+run_diff "C user-value" noassert rel2 "$WORK/chart" -n "$NS"
+expect "C user-value" "no key diff (the user-supplied value wins)" '^[+-][[:space:]]*key:' absent
+
+###############################################################################
+# Variant D: with --set, Helm does not reuse the values of the release,
+# so the --set value replaces the user value
+###############################################################################
+run_diff "D set-override" noassert rel2 "$WORK/chart" -n "$NS" --set key=override
+expect "D set-override" "the old user value (key: \"user\")" '^-[[:space:]]*key: "user"' present
+expect "D set-override" "the new value (key: \"override\")" '^\+[[:space:]]*key: "override"' present
+
+finish