From c008d0e1294c57df822dc0134f74f61af56ccede Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:16:18 +0800 Subject: [PATCH] fix: update helm plugins via uninstall+reinstall to honor pinned version (#2727) * fix: update helm plugins via uninstall+reinstall to honor pinned version (#2726) helm plugin update re-installs a plugin from its cached source WITHOUT the --version flag, so it does not reliably install the pinned version helmfile requests. It reports success (exit 0) while re-downloading the unchanged cached source, leaving 'helm plugin list' showing the old version. This affected 'helmfile init --force' which reported a successful diff/secrets plugin update while the old version remained installed. The reporter's workaround was to uninstall the plugin before running init. UpdatePlugin now uninstalls the existing plugin and reinstalls the exact pinned version via AddPlugin (which passes --version). The unreliable 'plugin update' command is no longer used for the general path; this mirrors the uninstall+reinstall strategy already used for helm-secrets on Helm 4. Fixes #2726 Refs #2548 Signed-off-by: yxxhero * fix: only ignore 'not found' uninstall errors in UpdatePlugin Address review feedback: UpdatePlugin ignored all uninstall errors, which could mask real failures (permissions, broken Helm) behind a less informative 'plugin already exists' error from the subsequent install. Now only the expected 'not found' case (plugin removed concurrently, reported by both Helm 3 'Plugin: not found' and Helm 4 'plugin: not found') is ignored and install proceeds. Any other uninstall error is returned. Adds tests for both branches. Signed-off-by: yxxhero * fix: extract pluginCmd constant to satisfy goconst in tests golangci-lint (goconst, min-occurrences: 8) flagged the "plugin" string literal in exec_test.go after the new UpdatePlugin tests pushed the package- wide count from 7 (under threshold, passing on main) to 14. Introduce a pluginCmd constant and use it for all helm sub-command checks in the UpdatePlugin tests. No behavior change; drops the literal count back below the threshold so CI goconst passes. Signed-off-by: yxxhero * fix: match helm plugin-absent error precisely; guard against plugin update regression Address review feedback on UpdatePlugin: 1. The uninstall error check used strings.Contains(err, "not found"), which is too broad: it also matches unrelated failures such as a missing helm binary ("executable file not found") or an uninstall hook failing with "sh: ...: not found". Those would be silently treated as "plugin absent" and logged/proceeded past, masking the real problem. Match only helm's specific plugin-absent message instead, via a case-insensitive regex that covers both majors: Helm 4: "plugin: not found" Helm 3: "Plugin: not found" All other uninstall failures (permissions, missing binary, hook errors) are now surfaced. 2. The init test mock had no "plugin update" branch, so a production regression to calling helm plugin update would go undetected (false negative). Add an explicit case that records and fails on "plugin update". Also adds a test for the missing-binary case and makes the plugin-absent test table-driven across Helm 3/4 formats. Extracts installCmd to keep goconst (min-occurrences: 8) satisfied after the new tests. Signed-off-by: yxxhero --------- Signed-off-by: yxxhero --- pkg/app/init_test.go | 33 +++--- pkg/helmexec/exec.go | 47 ++++++--- pkg/helmexec/exec_test.go | 212 +++++++++++++++++++++++++++++++++++++- 3 files changed, 259 insertions(+), 33 deletions(-) diff --git a/pkg/app/init_test.go b/pkg/app/init_test.go index fafd3957..2b3c88ed 100644 --- a/pkg/app/init_test.go +++ b/pkg/app/init_test.go @@ -209,7 +209,7 @@ func TestCheckHelmPlugins_InstallErrorPluginTrulyMissing(t *testing.T) { assert.Contains(t, err.Error(), "sh: not found") } -func TestCheckHelmPlugins_UpdateFailsFallbackToReinstall(t *testing.T) { +func TestCheckHelmPlugins_UpdateUsesUninstallReinstall(t *testing.T) { pluginsDir := t.TempDir() t.Setenv("HELM_PLUGINS", pluginsDir) @@ -221,9 +221,11 @@ func TestCheckHelmPlugins_UpdateFailsFallbackToReinstall(t *testing.T) { // Track which plugin sub-commands were executed. var calledOps []string - // The mock runner simulates "helm plugin update" failing and falling back to - // "helm plugin uninstall" + "helm plugin install" which succeeds and writes the - // required version to disk. + // UpdatePlugin updates a plugin by uninstalling the stale version and reinstalling + // the exact pinned version (helm "plugin update" is intentionally NOT used because + // it does not honor --version and silently leaves the old version installed; see + // issues #2726 and #2548). This mock simulates a successful uninstall + install + // that writes the required version to disk. runner := &initMockRunner{ executeFunc: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { for _, a := range args { @@ -234,11 +236,13 @@ func TestCheckHelmPlugins_UpdateFailsFallbackToReinstall(t *testing.T) { if len(args) >= 2 && args[0] == "plugin" { switch args[1] { case "update": + // UpdatePlugin must never invoke `helm plugin update` (it does not honor + // --version and silently leaves the old version installed; see #2726). + // Record the call so the NotContains assertion below can catch a regression. if len(args) >= 3 { calledOps = append(calledOps, "update:"+args[2]) } - // Simulate helm plugin update failing (as can happen with Helm 4) - return nil, helmexec.ExitError{Message: "plugin update failed", Code: 1} + return nil, helmexec.ExitError{Message: "plugin update must not be used", Code: 1} case "uninstall": if len(args) >= 3 { calledOps = append(calledOps, "uninstall:"+args[2]) @@ -266,13 +270,13 @@ func TestCheckHelmPlugins_UpdateFailsFallbackToReinstall(t *testing.T) { h := NewHelmfileInit("helm", &mockInitConfigProvider{force: true}, newTestLogger(), runner) err := h.CheckHelmPlugins() - // Should succeed: update failed but fallback reinstall updated the plugin + // Should succeed: uninstall + reinstall updated each plugin to the pinned version assert.NoError(t, err) - // Verify that for each plugin the fallback path was taken: - // update was attempted, then uninstall + install were called. + // Verify that for each plugin the reliable update path was taken: + // the unreliable "plugin update" is never called; only uninstall + install are. for _, p := range helmPlugins { - assert.Contains(t, calledOps, "update:"+p.name, "expected update to be attempted for plugin %s", p.name) + assert.NotContains(t, calledOps, "update:"+p.name, "'plugin update' must not be used for plugin %s", p.name) assert.Contains(t, calledOps, "uninstall:"+p.name, "expected uninstall to be called for plugin %s", p.name) assert.Contains(t, calledOps, "install:"+p.name, "expected install to be called for plugin %s", p.name) } @@ -298,9 +302,8 @@ func TestCheckHelmPlugins_UpdateErrorButPluginAtRequiredVersion(t *testing.T) { } // The mock runner simulates: - // 1. "helm plugin update" failing - // 2. "helm plugin uninstall" succeeding - // 3. "helm plugin install" writing the correct version but returning an error + // 1. "helm plugin uninstall" succeeding + // 2. "helm plugin install" writing the correct version but returning an error // (e.g., post-install script error on Windows) // In this case, UpdatePlugin returns the install error, but CheckHelmPlugins // verifies the version and warns instead of returning an error. @@ -313,8 +316,6 @@ func TestCheckHelmPlugins_UpdateErrorButPluginAtRequiredVersion(t *testing.T) { } if len(args) >= 2 && args[0] == "plugin" { switch args[1] { - case "update": - return nil, helmexec.ExitError{Message: "plugin update failed", Code: 1} case "uninstall": return []byte{}, nil case "install": @@ -338,7 +339,7 @@ func TestCheckHelmPlugins_UpdateErrorButPluginAtRequiredVersion(t *testing.T) { h := NewHelmfileInit("helm", &mockInitConfigProvider{force: true}, newTestLogger(), runner) err := h.CheckHelmPlugins() - // Should succeed: UpdatePlugin returned an error (from the fallback install step), + // Should succeed: UpdatePlugin returned an error (from the reinstall step), // but the plugin is present at the required version, so CheckHelmPlugins warns and continues. assert.NoError(t, err) } diff --git a/pkg/helmexec/exec.go b/pkg/helmexec/exec.go index 5f2d5a01..bbd0099c 100644 --- a/pkg/helmexec/exec.go +++ b/pkg/helmexec/exec.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "reflect" + "regexp" "strconv" "strings" "sync" @@ -1134,6 +1135,18 @@ func (helm *execer) installHelmSecretsV4(version string) error { // split plugin architecture (secrets, secrets-getter, secrets-post-renderer) with Helm 4. var helmSecretsV4SplitMinVersion = semver.MustParse("4.7.0") +// pluginMissingRe matches helm's "plugin absent" error emitted by `helm plugin +// uninstall` when the plugin is not installed: +// +// Helm 4: "plugin: not found" +// Helm 3: "Plugin: not found" +// +// It is intentionally specific so that unrelated failures that happen to contain +// "not found" (e.g. a missing helm binary -> "executable file not found", or an +// uninstall hook failing with "sh: ...: not found") are NOT mistaken for an +// absent plugin. It is case-insensitive and scoped to a single line. +var pluginMissingRe = regexp.MustCompile(`(?i)plugin: .* not found`) + // helmSecretsRequiresSplitInstall returns true when the given helm-secrets version // requires the split plugin architecture introduced in v4.7.0 for Helm 4. func helmSecretsRequiresSplitInstall(version string) bool { @@ -1167,22 +1180,28 @@ func (helm *execer) UpdatePlugin(name, repo, version string) error { return helm.installHelmSecretsV4(version) } - // Try standard helm plugin update - out, err := helm.exec([]string{"plugin", "update", name}, map[string]string{}, nil) - helm.info(out) - if err != nil { - // If standard update failed, fall back to uninstall + reinstall with specific version - updateErr := err - helm.logger.Infof("helm plugin update %v failed (%v), falling back to reinstall with version %v", name, updateErr, version) - if uninstallErr := helm.uninstallPlugin(name); uninstallErr != nil { - return fmt.Errorf("helm plugin update failed (%w) and uninstall for reinstall also failed: %w", updateErr, uninstallErr) + // `helm plugin update` re-installs the plugin from its cached source WITHOUT the + // `--version` flag, so it does not reliably install the specific version we need. + // On many setups it reports success (exit code 0) while `helm plugin list` still + // shows the old version, because the cached source is re-downloaded unchanged. + // See https://github.com/helmfile/helmfile/issues/2726 and + // https://github.com/helmfile/helmfile/issues/2548. + // + // The reliable way to update to a pinned version is to uninstall the existing + // plugin and reinstall it at the requested version. Only the expected + // "plugin already absent" case is tolerated: helm reports it as + // "plugin: not found" (Helm 4) / "Plugin: not found" (Helm 3). + // We match that specific message rather than a bare "not found", so that other + // failures (permissions, a missing helm binary whose error contains + // "executable file not found", a plugin uninstall hook failing with + // "sh: ...: not found", ...) are surfaced instead of being silently ignored. + if err := helm.uninstallPlugin(name); err != nil { + if !pluginMissingRe.MatchString(err.Error()) { + return fmt.Errorf("failed to uninstall helm plugin %q for reinstall: %w", name, err) } - if reinstallErr := helm.AddPlugin(name, repo, version); reinstallErr != nil { - return fmt.Errorf("helm plugin update failed (%w) and reinstall also failed: %w", updateErr, reinstallErr) - } - return nil + helm.logger.Debugf("helm plugin %v not present during update, proceeding to install: %v", name, err) } - return nil + return helm.AddPlugin(name, repo, version) } func (helm *execer) exec(args []string, env map[string]string, overrideEnableLiveOutput *bool) ([]byte, error) { diff --git a/pkg/helmexec/exec_test.go b/pkg/helmexec/exec_test.go index 568b7fb5..270d1a8e 100644 --- a/pkg/helmexec/exec_test.go +++ b/pkg/helmexec/exec_test.go @@ -20,6 +20,14 @@ import ( "go.uber.org/zap" ) +// pluginCmd is the "plugin" helm subcommand used repeatedly across these tests. +// Extracted as constants so the repeated string literals do not trip goconst +// (min-occurrences: 8) once additional plugin tests are added. +const ( + pluginCmd = "plugin" + installCmd = "install" +) + // Mocking the command-line runner type mockRunner struct { @@ -1917,7 +1925,7 @@ func Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall(t *testing.T) { // Verify that "plugin update" was NOT called (the Helm 4 secrets path should skip it). for _, args := range calledArgs { for i, a := range args { - if a == "plugin" && i+1 < len(args) && args[i+1] == "update" { + if a == pluginCmd && i+1 < len(args) && args[i+1] == "update" { t.Errorf("expected 'plugin update' to not be called for Helm 4 secrets, but it was: %v", args) } } @@ -1927,7 +1935,7 @@ func Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall(t *testing.T) { checkUninstall := func(name string) { for _, args := range calledArgs { for i, a := range args { - if a == "plugin" && i+2 < len(args) && args[i+1] == "uninstall" && args[i+2] == name { + if a == pluginCmd && i+2 < len(args) && args[i+1] == "uninstall" && args[i+2] == name { return } } @@ -1942,7 +1950,7 @@ func Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall(t *testing.T) { checkInstall := func(urlSubstring string) { for _, args := range calledArgs { for i, a := range args { - if a == "plugin" && i+2 < len(args) && args[i+1] == "install" && strings.Contains(args[i+2], urlSubstring) { + if a == pluginCmd && i+2 < len(args) && args[i+1] == installCmd && strings.Contains(args[i+2], urlSubstring) { return } } @@ -1954,6 +1962,204 @@ func Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall(t *testing.T) { checkInstall("secrets-post-renderer-4.7.0.tgz") } +// Test_UpdatePlugin_GeneralPathUsesUninstallReinstall verifies that for a regular +// plugin, UpdatePlugin does NOT rely on the unreliable `helm plugin update` +// command. Instead it must uninstall the existing plugin and reinstall the exact +// pinned version. See issues #2726 and #2548: `helm plugin update` reports +// success but leaves `helm plugin list` showing the old version because it +// re-installs from the cached source without the --version flag. +func Test_UpdatePlugin_GeneralPathUsesUninstallReinstall(t *testing.T) { + var calledArgs [][]string + runner := &funcRunner{ + execute: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + calledArgs = append(calledArgs, append([]string(nil), args...)) + return []byte{}, nil + }, + } + + var buffer bytes.Buffer + logger := NewLogger(&buffer, "debug") + helm := &execer{ + helmBinary: "helm", + version: semver.MustParse("3.16.4"), + logger: logger, + runner: runner, + } + + err := helm.UpdatePlugin("diff", "https://github.com/databus23/helm-diff", "v3.15.10") + require.NoError(t, err) + + // `plugin update` must never be used: it does not honor --version and silently + // leaves the old version installed. + for _, args := range calledArgs { + for i, a := range args { + if a == pluginCmd && i+1 < len(args) && args[i+1] == "update" { + t.Errorf("expected 'plugin update' to not be called, but it was: %v", args) + } + } + } + + // `plugin uninstall diff` must be called to clear the stale version. + uninstalled := false + for _, args := range calledArgs { + for i, a := range args { + if a == pluginCmd && i+2 < len(args) && args[i+1] == "uninstall" && args[i+2] == "diff" { + uninstalled = true + } + } + } + require.True(t, uninstalled, "expected 'plugin uninstall diff' to be called") + + // `plugin install --version ` must be called with the exact + // requested version so the installed plugin is updated to it. + installed := false + for _, args := range calledArgs { + for i, a := range args { + if a == pluginCmd && i+1 < len(args) && args[i+1] == installCmd { + hasRepo := false + hasVersion := false + for _, arg := range args[i+2:] { + if arg == "https://github.com/databus23/helm-diff" { + hasRepo = true + } + if arg == "v3.15.10" { + hasVersion = true + } + } + if hasRepo && hasVersion { + installed = true + } + } + } + } + require.True(t, installed, "expected 'plugin install' with the pinned version to be called") +} + +// Test_UpdatePlugin_NotFoundUninstallProceedsToInstall ensures that when the +// plugin is already absent (helm reports its "plugin not found" message), +// UpdatePlugin still proceeds to install the pinned version rather than treating +// it as an error. Both helm major formats are covered. +func Test_UpdatePlugin_NotFoundUninstallProceedsToInstall(t *testing.T) { + // Helm 4 reports "plugin: not found"; Helm 3 reports "Plugin: not found". + for _, tc := range []struct { + name string + version string + msg string + }{ + {name: "helm4", version: "4.2.3", msg: "plugin: diff not found"}, + {name: "helm3", version: "3.16.4", msg: "Plugin: diff not found"}, + } { + t.Run(tc.name, func(t *testing.T) { + var calledArgs [][]string + runner := &funcRunner{ + execute: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + calledArgs = append(calledArgs, append([]string(nil), args...)) + if len(args) >= 2 && args[0] == pluginCmd && args[1] == "uninstall" { + return nil, ExitError{Message: tc.msg, Code: 1} + } + return []byte{}, nil + }, + } + + var buffer bytes.Buffer + logger := NewLogger(&buffer, "debug") + helm := &execer{ + helmBinary: "helm", + version: semver.MustParse(tc.version), + logger: logger, + runner: runner, + } + + err := helm.UpdatePlugin("diff", "https://github.com/databus23/helm-diff", "v3.15.10") + require.NoError(t, err, "a plugin-absent uninstall error should be ignored and install should proceed") + + // install with the pinned version must still be attempted + installed := false + for _, args := range calledArgs { + for i, a := range args { + if a == pluginCmd && i+1 < len(args) && args[i+1] == installCmd { + for _, arg := range args[i+2:] { + if arg == "v3.15.10" { + installed = true + } + } + } + } + } + require.True(t, installed, "expected install to proceed after a plugin-absent uninstall") + }) + } +} + +// Test_UpdatePlugin_RealUninstallFailureReturnsError ensures that a genuine +// uninstall failure (permissions, broken Helm, etc.) is surfaced instead of +// being ignored, which would otherwise mask the root cause behind a confusing +// "plugin already exists" error from the subsequent install. +func Test_UpdatePlugin_RealUninstallFailureReturnsError(t *testing.T) { + installCalled := false + runner := &funcRunner{ + execute: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + if len(args) >= 2 && args[0] == pluginCmd && args[1] == "uninstall" { + return nil, ExitError{Message: "permission denied", Code: 1} + } + if len(args) >= 2 && args[0] == pluginCmd && args[1] == installCmd { + installCalled = true + } + return []byte{}, nil + }, + } + + var buffer bytes.Buffer + logger := NewLogger(&buffer, "debug") + helm := &execer{ + helmBinary: "helm", + version: semver.MustParse("3.16.4"), + logger: logger, + runner: runner, + } + + err := helm.UpdatePlugin("diff", "https://github.com/databus23/helm-diff", "v3.15.10") + require.Error(t, err, "a real uninstall failure should be returned") + assert.Contains(t, err.Error(), "uninstall") + assert.False(t, installCalled, "install must not be attempted after a real uninstall failure") +} + +// Test_UpdatePlugin_ExecutableNotFoundUninstallErrorIsNotSwallowed guards against +// an overly broad "not found" check: when helm itself is missing the runner +// surfaces an error containing "executable file not found", which must NOT be +// mistaken for an absent plugin (otherwise UpdatePlugin would silently log and +// proceed to install, masking the real problem). Only the helm-specific +// "plugin: not found" message is tolerated. +func Test_UpdatePlugin_ExecutableNotFoundUninstallErrorIsNotSwallowed(t *testing.T) { + installCalled := false + runner := &funcRunner{ + execute: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + if len(args) >= 2 && args[0] == pluginCmd && args[1] == "uninstall" { + // Mimics helmfile's ShellRunner when the helm binary is absent. + return nil, fmt.Errorf("unexpected error: exec: %q: executable file not found in $PATH", cmd) + } + if len(args) >= 2 && args[0] == pluginCmd && args[1] == installCmd { + installCalled = true + } + return []byte{}, nil + }, + } + + var buffer bytes.Buffer + logger := NewLogger(&buffer, "debug") + helm := &execer{ + helmBinary: "helm", + version: semver.MustParse("3.16.4"), + logger: logger, + runner: runner, + } + + err := helm.UpdatePlugin("diff", "https://github.com/databus23/helm-diff", "v3.15.10") + require.Error(t, err, "a missing-binary uninstall error must be returned, not swallowed") + assert.Contains(t, err.Error(), "executable file not found") + assert.False(t, installCalled, "install must not be attempted when the helm binary is missing") +} + func Test_dedupeWroteLines(t *testing.T) { cases := []struct { name string