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