diff --git a/cmd/helm.go b/cmd/helm.go index 4281aa51..e71d1f0a 100644 --- a/cmd/helm.go +++ b/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) diff --git a/cmd/main_test.go b/cmd/main_test.go index 3b4fc58e..56e9d454 100644 --- a/cmd/main_test.go +++ b/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) + }) } diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index e7a79151..d948c4f8 100644 --- a/cmd/upgrade_test.go +++ b/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) + } }) } } diff --git a/scripts/issues/464.sh b/scripts/issues/464.sh new file mode 100755 index 00000000..fd27429c --- /dev/null +++ b/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 +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 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 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