From 7f748fec1b979a74aa7793e7a4d2f27944ad9748 Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Fri, 31 Jul 2026 22:22:31 +0800 Subject: [PATCH] feat: support Helm 4 --rollback-on-failure alongside deprecated --atomic (#2722) * feat: support Helm 4 --rollback-on-failure alongside deprecated --atomic (#2712) Helm 4 renamed the `--atomic` flag to `--rollback-on-failure` (helm/helm#13629). The old flag still works under Helm 4 but is deprecated (prints a warning) and slated for removal in Helm 5. Add a `rollbackOnFailure` key to both `helmDefaults` (HelmSpec) and `releases[]` (ReleaseSpec) that emits `--rollback-on-failure`. It requires Helm 4+ (errors otherwise) and is mutually exclusive with `atomic`. Additionally, when the resolved Helm binary is v4+, an existing `atomic: true` now emits `--rollback-on-failure` instead of `--atomic`, so users are migrated off the deprecated flag automatically without any config change. On older Helm, `atomic: true` continues to emit `--atomic`. Updated the spew-based values-ID hashes in temp_test.go that change whenever ReleaseSpec gains a field (same approach as the --force-conflicts change in #2480). Closes #2712. Signed-off-by: yxxhero * test: add integration test for rollback-on-failure / atomic migration (#2712) Covers the end-to-end plumbing that unit tests cannot (real helm version detection + cluster deploy) via test/integration/run.sh: 1. atomic: true parses, deploys a ConfigMap, and emits the version-correct flag: --rollback-on-failure on Helm 4 (auto-migration of the deprecated --atomic) and --atomic on Helm 3. 2. rollbackOnFailure: true emits --rollback-on-failure on Helm 4 and is rejected with a clear Helm-4-required error on Helm 3. Flag assertions grep the `exec: helm upgrade --install` lines logged under --debug, matching flags as standalone tokens so the release name "issue-2712-atomic" cannot be confused with the "--atomic" flag. Verified locally against Helm 4.2.3: both atomic:true and rollbackOnFailure:true emit --rollback-on-failure with no --atomic. Signed-off-by: yxxhero --------- Signed-off-by: yxxhero --- docs/configuration.md | 8 +- pkg/state/state.go | 26 ++- pkg/state/state_test.go | 154 +++++++++++++++++- pkg/state/temp_test.go | 12 +- test/integration/run.sh | 1 + .../issue-2712-rollback-on-failure.sh | 88 ++++++++++ .../input/atomic.yaml | 14 ++ .../input/rollback.yaml | 14 ++ 8 files changed, 306 insertions(+), 11 deletions(-) create mode 100644 test/integration/test-cases/issue-2712-rollback-on-failure.sh create mode 100644 test/integration/test-cases/issue-2712-rollback-on-failure/input/atomic.yaml create mode 100644 test/integration/test-cases/issue-2712-rollback-on-failure/input/rollback.yaml diff --git a/docs/configuration.md b/docs/configuration.md index 084cea4b..cabb7a14 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -246,8 +246,12 @@ releases: # Defines the strategy to use when updating. Possible value is: # - "reinstallIfForbidden": Performs an uninstall before the update only if the update is forbidden (e.g., due to permission issues or conflicts). updateStrategy: "" - # restores previous state in case of failed release (default false) + # restores previous state in case of failed release (default false). + # On Helm 4+ this emits --rollback-on-failure (the successor to the deprecated --atomic). atomic: true + # restores previous state on a failed release via the Helm 4 --rollback-on-failure flag + # (default false). Requires Helm 4 or greater. Mutually exclusive with atomic. + rollbackOnFailure: false # when true, cleans up any new resources created during a failed release (default false) cleanupOnFail: false # --kube-context to be passed to helm commands @@ -437,6 +441,8 @@ The following `helmDefaults` fields are also available but not shown in the exam | `enableDNS` | bool | false | Enable DNS lookups when rendering templates | | `skipCRDs` | bool | false | Skip CRDs during installation | | `skipRefresh` | bool | false | Skip running `helm dependency up` | +| `atomic` | bool | false | Restore previous state on a failed install/upgrade. On Helm 4+ emits `--rollback-on-failure` (the successor to the deprecated `--atomic`); on older Helm emits `--atomic` | +| `rollbackOnFailure` | bool | false | Restore previous state on a failed install/upgrade via the Helm 4 `--rollback-on-failure` flag. Requires Helm 4 or greater. Mutually exclusive with `atomic` | | `forceConflicts` | bool | false | Force server-side apply changes against conflicts (Helm 4 only) | | `takeOwnership` | bool | false | Take ownership of existing resources | | `serverSide` | string | | Controls the helm 4 `--server-side` flag. Must be `"true"`, `"false"`, or `"auto"` (Helm 4 only) | diff --git a/pkg/state/state.go b/pkg/state/state.go index 06c8b295..85bbcc1b 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -239,6 +239,9 @@ type HelmSpec struct { Force bool `yaml:"force"` // Atomic, when set to true, restore previous state in case of a failed install/upgrade attempt Atomic bool `yaml:"atomic"` + // RollbackOnFailure, when set to true, restores previous state on a failed install/upgrade via the + // Helm 4 --rollback-on-failure flag (the successor to the deprecated --atomic flag). Requires Helm 4 or greater. + RollbackOnFailure bool `yaml:"rollbackOnFailure"` // CleanupOnFail, when set to true, the --cleanup-on-fail helm flag is passed to the upgrade command CleanupOnFail bool `yaml:"cleanupOnFail,omitempty"` // ForceConflicts, when set to true, force server-side apply changes against conflicts (Helm 4 only) @@ -363,6 +366,9 @@ type ReleaseSpec struct { UpdateStrategy string `yaml:"updateStrategy,omitempty"` // Atomic, when set to true, restore previous state in case of a failed install/upgrade attempt Atomic *bool `yaml:"atomic,omitempty"` + // RollbackOnFailure, when set to true, restores previous state on a failed install/upgrade via the + // Helm 4 --rollback-on-failure flag (the successor to the deprecated --atomic flag). Requires Helm 4 or greater. + RollbackOnFailure *bool `yaml:"rollbackOnFailure,omitempty"` // CleanupOnFail, when set to true, the --cleanup-on-fail helm flag is passed to the upgrade command CleanupOnFail *bool `yaml:"cleanupOnFail,omitempty"` // ForceConflicts, when set to true, force server-side apply changes against conflicts (Helm 4 only) @@ -4002,8 +4008,26 @@ func (st *HelmState) flagsForUpgrade(helm helmexec.Interface, release *ReleaseSp flags = append(flags, "--recreate-pods") } - if release.Atomic != nil && *release.Atomic || release.Atomic == nil && st.HelmDefaults.Atomic { + atomicEnabled := (release.Atomic != nil && *release.Atomic) || (release.Atomic == nil && st.HelmDefaults.Atomic) + rollbackOnFailureEnabled := (release.RollbackOnFailure != nil && *release.RollbackOnFailure) || (release.RollbackOnFailure == nil && st.HelmDefaults.RollbackOnFailure) + + if atomicEnabled && rollbackOnFailureEnabled { + return nil, nil, fmt.Errorf("atomic and rollbackOnFailure are mutually exclusive (check both releases[].atomic/rollbackOnFailure and helmDefaults.atomic/rollbackOnFailure)") + } + + // rollbackOnFailure maps to the Helm 4 --rollback-on-failure flag and is unsupported on older Helm. + if rollbackOnFailureEnabled && !helm.IsHelm4() { + return nil, nil, fmt.Errorf("rollbackOnFailure requires Helm 4 or greater (set via releases[].rollbackOnFailure or helmDefaults.rollbackOnFailure)") + } + + // Helm 4 renamed --atomic to --rollback-on-failure; the former is deprecated (prints a warning) + // and slated for removal in Helm 5. Emit the new flag on Helm 4+ for both `atomic: true` and + // `rollbackOnFailure: true` so users migrate off it automatically; on older Helm, `atomic: true` + // still emits --atomic. See issue #2712. + if atomicEnabled && !helm.IsHelm4() { flags = append(flags, "--atomic") + } else if atomicEnabled || rollbackOnFailureEnabled { + flags = append(flags, "--rollback-on-failure") } if release.CleanupOnFail != nil && *release.CleanupOnFail || release.CleanupOnFail == nil && st.HelmDefaults.CleanupOnFail { diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 2dd29dc0..552f1b8c 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -646,8 +646,10 @@ func TestHelmState_flagsForUpgrade(t *testing.T) { { name: "atomic", defaults: HelmSpec{ - Atomic: false, + Atomic: false, + CreateNamespace: &disable, }, + version: semver.MustParse("3.10.0"), release: &ReleaseSpec{ Chart: "test/chart", Version: "0.1", @@ -661,11 +663,33 @@ func TestHelmState_flagsForUpgrade(t *testing.T) { "--namespace", "test-namespace", }, }, + { + name: "atomic-helm4", + defaults: HelmSpec{ + Atomic: false, + CreateNamespace: &disable, + }, + version: semver.MustParse("4.0.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + Atomic: &enable, + Name: "test-charts", + Namespace: "test-namespace", + }, + want: []string{ + "--version", "0.1", + "--rollback-on-failure", + "--namespace", "test-namespace", + }, + }, { name: "atomic-override-default", defaults: HelmSpec{ - Atomic: true, + Atomic: true, + CreateNamespace: &disable, }, + version: semver.MustParse("3.10.0"), release: &ReleaseSpec{ Chart: "test/chart", Version: "0.1", @@ -681,8 +705,10 @@ func TestHelmState_flagsForUpgrade(t *testing.T) { { name: "atomic-from-default", defaults: HelmSpec{ - Atomic: true, + Atomic: true, + CreateNamespace: &disable, }, + version: semver.MustParse("3.10.0"), release: &ReleaseSpec{ Chart: "test/chart", Version: "0.1", @@ -695,6 +721,128 @@ func TestHelmState_flagsForUpgrade(t *testing.T) { "--namespace", "test-namespace", }, }, + { + name: "atomic-from-default-helm4", + defaults: HelmSpec{ + Atomic: true, + CreateNamespace: &disable, + }, + version: semver.MustParse("4.0.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + Name: "test-charts", + Namespace: "test-namespace", + }, + want: []string{ + "--version", "0.1", + "--rollback-on-failure", + "--namespace", "test-namespace", + }, + }, + { + name: "rollback-on-failure", + defaults: HelmSpec{ + CreateNamespace: &disable, + }, + version: semver.MustParse("4.0.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + RollbackOnFailure: &enable, + Name: "test-charts", + Namespace: "test-namespace", + }, + want: []string{ + "--version", "0.1", + "--rollback-on-failure", + "--namespace", "test-namespace", + }, + }, + { + name: "rollback-on-failure-from-default", + defaults: HelmSpec{ + RollbackOnFailure: true, + CreateNamespace: &disable, + }, + version: semver.MustParse("4.0.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + Name: "test-charts", + Namespace: "test-namespace", + }, + want: []string{ + "--version", "0.1", + "--rollback-on-failure", + "--namespace", "test-namespace", + }, + }, + { + name: "rollback-on-failure-override-default", + defaults: HelmSpec{ + RollbackOnFailure: true, + CreateNamespace: &disable, + }, + version: semver.MustParse("4.0.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + RollbackOnFailure: &disable, + Name: "test-charts", + Namespace: "test-namespace", + }, + want: []string{ + "--version", "0.1", + "--namespace", "test-namespace", + }, + }, + { + name: "rollback-on-failure-helm3-error", + defaults: HelmSpec{ + CreateNamespace: &disable, + }, + version: semver.MustParse("3.10.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + RollbackOnFailure: &enable, + Name: "test-charts", + Namespace: "test-namespace", + }, + wantErr: "rollbackOnFailure requires Helm 4 or greater (set via releases[].rollbackOnFailure or helmDefaults.rollbackOnFailure)", + }, + { + name: "rollback-on-failure-from-default-helm3-error", + defaults: HelmSpec{ + RollbackOnFailure: true, + CreateNamespace: &disable, + }, + version: semver.MustParse("3.10.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + Name: "test-charts", + Namespace: "test-namespace", + }, + wantErr: "rollbackOnFailure requires Helm 4 or greater (set via releases[].rollbackOnFailure or helmDefaults.rollbackOnFailure)", + }, + { + name: "atomic-and-rollback-on-failure-mutually-exclusive-helm4", + defaults: HelmSpec{ + CreateNamespace: &disable, + }, + version: semver.MustParse("4.0.0"), + release: &ReleaseSpec{ + Chart: "test/chart", + Version: "0.1", + Atomic: &enable, + RollbackOnFailure: &enable, + Name: "test-charts", + Namespace: "test-namespace", + }, + wantErr: "atomic and rollbackOnFailure are mutually exclusive (check both releases[].atomic/rollbackOnFailure and helmDefaults.atomic/rollbackOnFailure)", + }, { name: "cleanup-on-fail", defaults: HelmSpec{ diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index 7aeb40d7..fb562383 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) { run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-6765d87f7c", + want: "foo-values-5bcd864488", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-6d7db5f8b", + want: "foo-values-59cd566bbc", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]any{"k": "v"}, - want: "foo-values-5c55f66dd", + want: "foo-values-58bcc85765", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-7665888bf4", + want: "foo-values-7796c46c49", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-bc6f974bd", + want: "bar-values-5b5f6f54cc", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-76f8c7c596", + want: "myns-foo-values-547578788f", }) for id, n := range ids { diff --git a/test/integration/run.sh b/test/integration/run.sh index cfd1e8de..7d19edc9 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -151,6 +151,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/issue-1172-selector-required-env.sh . ${dir}/test-cases/kubedog-tracking.sh . ${dir}/test-cases/lookup.sh +. ${dir}/test-cases/issue-2712-rollback-on-failure.sh # ALL DONE ----------------------------------------------------------------------------------------------------------- diff --git a/test/integration/test-cases/issue-2712-rollback-on-failure.sh b/test/integration/test-cases/issue-2712-rollback-on-failure.sh new file mode 100644 index 00000000..d33e8ee5 --- /dev/null +++ b/test/integration/test-cases/issue-2712-rollback-on-failure.sh @@ -0,0 +1,88 @@ +# Issue #2712: Support Helm 4 --rollback-on-failure alongside deprecated --atomic +# https://github.com/helmfile/helmfile/issues/2712 +# +# End-to-end coverage: +# 1. atomic: true is accepted (no unknown-field parse error), deploys, and emits the +# version-correct flag: --rollback-on-failure on Helm 4 (auto-migration of the +# deprecated --atomic), --atomic on Helm 3. +# 2. rollbackOnFailure: true emits --rollback-on-failure on Helm 4 and is rejected +# with a clear Helm-4-required error on Helm 3. +# +# The flag-emission assertions inspect the `exec: helm upgrade --install ...` lines +# that helmfile logs under --debug (see pkg/helmexec/exec.go exec helper). + +issue_2712_input_dir="${cases_dir}/issue-2712-rollback-on-failure/input" +issue_2712_tmp=$(mktemp -d) + +cleanup_issue_2712() { + rm -rf "${issue_2712_tmp}" +} +trap cleanup_issue_2712 EXIT + +# Succeeds when the captured --debug log contains a `helm upgrade --install` command +# carrying the given flag. The flag is matched as a standalone argument (surrounded by +# whitespace) so a release name such as "issue-2712-atomic" can never be confused with +# the "--atomic" flag. $1 = log file, $2 = flag token (e.g. --rollback-on-failure). +issue_2712_assert_upgrade_flag() { + grep -E "upgrade --install" "$1" | grep -qE "(^|[[:space:]])${2}([[:space:]]|$)" +} + +# --- Test 1: atomic: true is accepted, deploys, and emits the version-correct flag --- +if [ "${HELMFILE_HELM4}" = "1" ]; then + test_start "issue-2712 atomic migrates to --rollback-on-failure (Helm 4)" +else + test_start "issue-2712 atomic emits --atomic (Helm 3)" +fi + +info "Syncing release with atomic: true" +if ! ${helmfile} --debug -f "${issue_2712_input_dir}/atomic.yaml" sync > "${issue_2712_tmp}/atomic.log" 2>&1; then + cat "${issue_2712_tmp}/atomic.log" + fail "helmfile sync with atomic: true should succeed" +fi + +info "Verifying ConfigMap issue-2712-atomic-cm was applied" +${kubectl} get configmap issue-2712-atomic-cm > /dev/null \ + || fail "ConfigMap issue-2712-atomic-cm should exist after sync" + +if [ "${HELMFILE_HELM4}" = "1" ]; then + issue_2712_assert_upgrade_flag "${issue_2712_tmp}/atomic.log" "--rollback-on-failure" \ + || { cat "${issue_2712_tmp}/atomic.log"; fail "Helm 4 should migrate atomic:true to --rollback-on-failure"; } + if issue_2712_assert_upgrade_flag "${issue_2712_tmp}/atomic.log" "--atomic"; then + fail "Helm 4 should not emit the deprecated --atomic flag for atomic:true" + fi + test_pass "issue-2712 atomic migrates to --rollback-on-failure (Helm 4)" +else + issue_2712_assert_upgrade_flag "${issue_2712_tmp}/atomic.log" "--atomic" \ + || { cat "${issue_2712_tmp}/atomic.log"; fail "Helm 3 should emit --atomic for atomic:true"; } + test_pass "issue-2712 atomic emits --atomic (Helm 3)" +fi + +# --- Test 2: rollbackOnFailure behavior is version-gated --- +if [ "${HELMFILE_HELM4}" = "1" ]; then + test_start "issue-2712 rollbackOnFailure emits --rollback-on-failure (Helm 4)" + info "Syncing release with rollbackOnFailure: true" + if ! ${helmfile} --debug -f "${issue_2712_input_dir}/rollback.yaml" sync > "${issue_2712_tmp}/rollback.log" 2>&1; then + cat "${issue_2712_tmp}/rollback.log" + fail "helmfile sync with rollbackOnFailure: true should succeed on Helm 4" + fi + ${kubectl} get configmap issue-2712-rollback-cm > /dev/null \ + || fail "ConfigMap issue-2712-rollback-cm should exist after sync" + issue_2712_assert_upgrade_flag "${issue_2712_tmp}/rollback.log" "--rollback-on-failure" \ + || { cat "${issue_2712_tmp}/rollback.log"; fail "rollbackOnFailure:true should emit --rollback-on-failure on Helm 4"; } + test_pass "issue-2712 rollbackOnFailure emits --rollback-on-failure (Helm 4)" +else + test_start "issue-2712 rollbackOnFailure rejected on Helm 3" + info "Syncing release with rollbackOnFailure: true (expected to fail)" + if ${helmfile} -f "${issue_2712_input_dir}/rollback.yaml" sync > "${issue_2712_tmp}/rollback.log" 2>&1; then + cat "${issue_2712_tmp}/rollback.log" + fail "helmfile sync with rollbackOnFailure: true should fail on Helm 3" + fi + grep -q "rollbackOnFailure requires Helm 4" "${issue_2712_tmp}/rollback.log" \ + || { cat "${issue_2712_tmp}/rollback.log"; fail "expected 'rollbackOnFailure requires Helm 4' error on Helm 3"; } + test_pass "issue-2712 rollbackOnFailure rejected on Helm 3" +fi + +# --- Cleanup --- +info "Cleaning up issue-2712 releases" +${helmfile} -f "${issue_2712_input_dir}/atomic.yaml" destroy > /dev/null 2>&1 || true +${helmfile} -f "${issue_2712_input_dir}/rollback.yaml" destroy > /dev/null 2>&1 || true diff --git a/test/integration/test-cases/issue-2712-rollback-on-failure/input/atomic.yaml b/test/integration/test-cases/issue-2712-rollback-on-failure/input/atomic.yaml new file mode 100644 index 00000000..c7878e97 --- /dev/null +++ b/test/integration/test-cases/issue-2712-rollback-on-failure/input/atomic.yaml @@ -0,0 +1,14 @@ +releases: +- name: issue-2712-atomic + chart: ../../../charts/raw + atomic: true + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: {{ .Release.Name }}-cm + namespace: {{ .Release.Namespace }} + data: + version: "1" diff --git a/test/integration/test-cases/issue-2712-rollback-on-failure/input/rollback.yaml b/test/integration/test-cases/issue-2712-rollback-on-failure/input/rollback.yaml new file mode 100644 index 00000000..3f7bbfaa --- /dev/null +++ b/test/integration/test-cases/issue-2712-rollback-on-failure/input/rollback.yaml @@ -0,0 +1,14 @@ +releases: +- name: issue-2712-rollback + chart: ../../../charts/raw + rollbackOnFailure: true + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: {{ .Release.Name }}-cm + namespace: {{ .Release.Namespace }} + data: + version: "1"