From c57134cda7c64bd3b087dffbe9f6e3260e365335 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Apr 2026 11:04:23 +0800 Subject: [PATCH] Fix helmfile init failing to update outdated helm plugins with Helm v4 (#2554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Initial plan * Fix helmfile init not updating outdated helm plugins with Helm v4 - UpdatePlugin now handles secrets plugin with Helm 4 by using the split plugin architecture (uninstall old + install via installHelmSecretsV4) - UpdatePlugin falls back to uninstall + reinstall when helm plugin update fails (e.g., with Helm 4 or tarball-installed plugins) - Fix string-based semver comparison for helm-secrets version check in both AddPlugin and UpdatePlugin using proper semver comparison - Add helmSecretsRequiresSplitInstall helper for reuse and correctness - Add tests for update failure fallback scenarios Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/533f1b1c-dda6-4934-af27-051e4eaa9927 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * Address reviewer feedback: preserve update error context and add version assertions in tests - exec.go: include original update error in fallback log message; wrap both errors (update + reinstall) when reinstall also fails so callers get full context - init_test.go: add semver import and GetPluginVersion assertions after CheckHelmPlugins to verify plugins are at required versions on disk Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/c784db7d-7d4c-40a0-97f0-a31eb8901cd6 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * Address second round of reviewer feedback - exec.go: rename UpdatePlugin parameter path→repo for clarity - exec.go: fix uninstallPlugin to only emit INFO log when err == nil - exec_test.go: add Test_helmSecretsRequiresSplitInstall table-driven tests covering v4.6.9, v4.7.0, v4.8.0, v4.10.0, pre-release, invalid and empty - exec_test.go: add Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall verifying that Helm 4 + secrets uses uninstall+reinstall (not plugin update) Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/cbd3f8c9-ec7d-4500-b168-cb1c2f7c87bc Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * Add len(args) >= 3 guards in test mock for plugin update/uninstall cases Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/ea0f5afc-d52d-473b-b759-853a8f841a26 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * Return early with combined error when uninstall fails in UpdatePlugin fallback Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/bb9a675c-309d-4b06-83d4-a6fe078dce64 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --- pkg/app/init.go | 2 +- pkg/app/init_test.go | 135 ++++++++++++++++++++++++++++++++++++++ pkg/helmexec/exec.go | 56 ++++++++++++++-- pkg/helmexec/exec_test.go | 97 +++++++++++++++++++++++++++ 4 files changed, 285 insertions(+), 5 deletions(-) diff --git a/pkg/app/init.go b/pkg/app/init.go index 7fcd0e81..a3837095 100644 --- a/pkg/app/init.go +++ b/pkg/app/init.go @@ -217,7 +217,7 @@ func (h *HelmfileInit) CheckHelmPlugins() error { if err != nil { return err } - err = helm.UpdatePlugin(p.name) + err = helm.UpdatePlugin(p.name, p.repo, p.version) if err != nil { // Check if plugin was updated despite the error updatedVersion, verifyErr := helmexec.GetPluginVersion(p.name, pluginsDir) diff --git a/pkg/app/init_test.go b/pkg/app/init_test.go index 76849506..fafd3957 100644 --- a/pkg/app/init_test.go +++ b/pkg/app/init_test.go @@ -11,6 +11,7 @@ import ( "strings" "testing" + "github.com/Masterminds/semver/v3" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" @@ -207,3 +208,137 @@ func TestCheckHelmPlugins_InstallErrorPluginTrulyMissing(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "sh: not found") } + +func TestCheckHelmPlugins_UpdateFailsFallbackToReinstall(t *testing.T) { + pluginsDir := t.TempDir() + t.Setenv("HELM_PLUGINS", pluginsDir) + + // Pre-populate plugins with outdated versions so the update path is triggered. + for _, p := range helmPlugins { + createPluginYAML(t, pluginsDir, p.name, p.name, "0.0.1") + } + + // 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. + runner := &initMockRunner{ + executeFunc: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + for _, a := range args { + if a == "--short" { + return []byte("v3.18.6"), nil + } + } + if len(args) >= 2 && args[0] == "plugin" { + switch args[1] { + case "update": + 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} + case "uninstall": + if len(args) >= 3 { + calledOps = append(calledOps, "uninstall:"+args[2]) + } + // Simulate successful uninstall + return []byte{}, nil + case "install": + // Find which plugin is being installed by matching the repo URL. + if len(args) >= 3 { + repo := args[2] + for _, p := range helmPlugins { + if p.repo == repo { + calledOps = append(calledOps, "install:"+p.name) + createPluginYAML(t, pluginsDir, p.name, p.name, strings.TrimPrefix(p.version, "v")) + break + } + } + } + return []byte{}, nil + } + } + return []byte{}, nil + }, + } + + h := NewHelmfileInit("helm", &mockInitConfigProvider{force: true}, newTestLogger(), runner) + err := h.CheckHelmPlugins() + // Should succeed: update failed but fallback reinstall updated the plugin + assert.NoError(t, err) + + // Verify that for each plugin the fallback path was taken: + // update was attempted, then uninstall + install were called. + for _, p := range helmPlugins { + assert.Contains(t, calledOps, "update:"+p.name, "expected update to be attempted 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) + } + + // Verify that all plugins are now at (or above) the required version on disk. + for _, p := range helmPlugins { + requiredVersion, err := semver.NewVersion(p.version) + require.NoError(t, err) + installedVersion, err := helmexec.GetPluginVersion(p.name, pluginsDir) + require.NoError(t, err, "plugin %s should be present after reinstall", p.name) + assert.False(t, installedVersion.LessThan(requiredVersion), + "plugin %s: installed version %s should be >= required version %s", p.name, installedVersion, requiredVersion) + } +} + +func TestCheckHelmPlugins_UpdateErrorButPluginAtRequiredVersion(t *testing.T) { + pluginsDir := t.TempDir() + t.Setenv("HELM_PLUGINS", pluginsDir) + + // Pre-populate plugins with outdated versions so the update path is triggered. + for _, p := range helmPlugins { + createPluginYAML(t, pluginsDir, p.name, p.name, "0.0.1") + } + + // 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 + // (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. + runner := &initMockRunner{ + executeFunc: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + for _, a := range args { + if a == "--short" { + return []byte("v3.18.6"), nil + } + } + 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": + // Write the correct version to disk, then return an error + // (simulates post-install script failure on Windows) + if len(args) >= 3 { + repo := args[2] + for _, p := range helmPlugins { + if p.repo == repo { + createPluginYAML(t, pluginsDir, p.name, p.name, strings.TrimPrefix(p.version, "v")) + break + } + } + } + return nil, helmexec.ExitError{Message: "post-install script failed", Code: 1} + } + } + return []byte{}, nil + }, + } + + h := NewHelmfileInit("helm", &mockInitConfigProvider{force: true}, newTestLogger(), runner) + err := h.CheckHelmPlugins() + // Should succeed: UpdatePlugin returned an error (from the fallback install 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 5af9add0..cdfa90a8 100644 --- a/pkg/helmexec/exec.go +++ b/pkg/helmexec/exec.go @@ -851,7 +851,7 @@ func (helm *execer) AddPlugin(name, path, version string) error { helm.logger.Infof("Install helm plugin %v", name) // Special handling for helm-secrets 4.7.0+ with Helm 4 which uses split plugin architecture - if name == "secrets" && version >= "v4.7.0" && helm.IsHelm4() { + if name == "secrets" && helmSecretsRequiresSplitInstall(version) && helm.IsHelm4() { return helm.installHelmSecretsV4(version) } @@ -906,11 +906,59 @@ func (helm *execer) installHelmSecretsV4(version string) error { return nil } -func (helm *execer) UpdatePlugin(name string) error { - helm.logger.Infof("Update helm plugin %v", name) +// helmSecretsV4SplitMinVersion is the minimum helm-secrets version that uses the +// split plugin architecture (secrets, secrets-getter, secrets-post-renderer) with Helm 4. +var helmSecretsV4SplitMinVersion = semver.MustParse("4.7.0") + +// 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 { + v, err := semver.NewVersion(version) + if err != nil { + return false + } + return !v.LessThan(helmSecretsV4SplitMinVersion) +} + +func (helm *execer) uninstallPlugin(name string) error { + helm.logger.Infof("Uninstalling helm plugin %v", name) + out, err := helm.exec([]string{"plugin", "uninstall", name}, map[string]string{}, nil) + if err == nil { + helm.info(out) + } + return err +} + +func (helm *execer) UpdatePlugin(name, repo, version string) error { + helm.logger.Infof("Updating helm plugin %v", name) + + // Special handling for helm-secrets 4.7.0+ with Helm 4 which uses split plugin architecture + if name == "secrets" && helmSecretsRequiresSplitInstall(version) && helm.IsHelm4() { + // Uninstall existing secrets plugins; ignore errors as some may not exist + for _, secretsPlugin := range []string{"secrets", "secrets-getter", "secrets-post-renderer"} { + if err := helm.uninstallPlugin(secretsPlugin); err != nil { + helm.logger.Debugf("Failed to uninstall helm plugin %v (may not exist): %v", secretsPlugin, err) + } + } + return helm.installHelmSecretsV4(version) + } + + // Try standard helm plugin update out, err := helm.exec([]string{"plugin", "update", name}, map[string]string{}, nil) helm.info(out) - return err + 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) + } + 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 + } + return nil } 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 c468c8f5..9644c21a 100644 --- a/pkg/helmexec/exec_test.go +++ b/pkg/helmexec/exec_test.go @@ -1547,3 +1547,100 @@ func TestParseHelmVersion(t *testing.T) { }) } } + +func Test_helmSecretsRequiresSplitInstall(t *testing.T) { + tests := []struct { + name string + version string + want bool + }{ + {name: "below threshold", version: "v4.6.9", want: false}, + {name: "exactly at threshold", version: "v4.7.0", want: true}, + {name: "above threshold single digit minor", version: "v4.8.0", want: true}, + {name: "above threshold double digit minor (v4.10.0+)", version: "v4.10.0", want: true}, + {name: "pre-release below threshold", version: "v4.7.0-beta.1", want: false}, + {name: "invalid version string", version: "not-a-version", want: false}, + {name: "empty string", version: "", want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := helmSecretsRequiresSplitInstall(tt.version) + assert.Equal(t, tt.want, got) + }) + } +} + +type funcRunner struct { + execute func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) +} + +func (r *funcRunner) ExecuteStdIn(cmd string, args []string, env map[string]string, stdin io.Reader) ([]byte, error) { + return r.execute(cmd, args, env, false) +} + +func (r *funcRunner) Execute(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + return r.execute(cmd, args, env, enableLiveOutput) +} + +func Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall(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("4.0.0"), + logger: logger, + kubeconfig: "config", + kubeContext: "dev", + runner: runner, + } + + err := helm.UpdatePlugin("secrets", "https://github.com/jkroepke/helm-secrets", "v4.7.0") + require.NoError(t, err) + + // 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" { + t.Errorf("expected 'plugin update' to not be called for Helm 4 secrets, but it was: %v", args) + } + } + } + + // Verify that uninstall was called for all three split plugins. + 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 { + return + } + } + } + t.Errorf("expected 'plugin uninstall %s' to be called", name) + } + checkUninstall("secrets") + checkUninstall("secrets-getter") + checkUninstall("secrets-post-renderer") + + // Verify that install was called for all three split plugin tarballs. + 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) { + return + } + } + } + t.Errorf("expected 'plugin install' for %q to be called", urlSubstring) + } + checkInstall("secrets-4.7.0.tgz") + checkInstall("secrets-getter-4.7.0.tgz") + checkInstall("secrets-post-renderer-4.7.0.tgz") +}