Fix helmfile init failing to update outdated helm plugins with Helm v4 (#2554)
* 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>
This commit is contained in:
Copilot
GitHub
parent
261c605305
commit
c57134cda7
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue