fix: prevent panic in helmfile init on plugin install errors (#2401)
toCLIError() panics on unhandled error types (e.g. helmexec.ExitError from a failed helm plugin install). On Windows, plugin install hooks often fail due to missing 'sh', causing helmfile init to crash even when the plugin binary was placed correctly. - Add helmexec.ExitError case to toCLIError and replace panic in the default case with a graceful error return - After AddPlugin/UpdatePlugin errors, verify whether the plugin is actually present before failing; log a warning and continue if so Fixes #1983 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
This commit is contained in:
Aditya Menon
GitHub
parent
0129681222
commit
3dab01c16f
|
|
@ -1,6 +1,7 @@
|
|||
package cmd
|
||||
|
||||
import (
|
||||
stderrors "errors"
|
||||
"fmt"
|
||||
"os"
|
||||
|
||||
|
|
@ -23,6 +24,10 @@ var globalUsage = "Declaratively deploy your Kubernetes manifests, Kustomize con
|
|||
|
||||
func toCLIError(g *config.GlobalImpl, err error) error {
|
||||
if err != nil {
|
||||
var exitErr helmexec.ExitError
|
||||
if stderrors.As(err, &exitErr) {
|
||||
return errors.NewExitError(exitErr.Error(), exitErr.ExitStatus())
|
||||
}
|
||||
switch e := err.(type) {
|
||||
case *app.NoMatchingHelmfileError:
|
||||
noMatchingExitCode := 3
|
||||
|
|
@ -35,7 +40,7 @@ func toCLIError(g *config.GlobalImpl, err error) error {
|
|||
case *app.Error:
|
||||
return errors.NewExitError(e.Error(), e.Code())
|
||||
default:
|
||||
panic(fmt.Errorf("BUG: please file an github issue for this unhandled error: %T: %v", e, e))
|
||||
return errors.NewExitError(fmt.Sprintf("unexpected error: %T: %v", e, e), 1)
|
||||
}
|
||||
}
|
||||
return err
|
||||
|
|
|
|||
|
|
@ -0,0 +1,72 @@
|
|||
package cmd
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
||||
"github.com/helmfile/helmfile/pkg/config"
|
||||
"github.com/helmfile/helmfile/pkg/errors"
|
||||
"github.com/helmfile/helmfile/pkg/helmexec"
|
||||
)
|
||||
|
||||
func TestToCLIError(t *testing.T) {
|
||||
g := config.NewGlobalImpl(&config.GlobalOptions{})
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
err error
|
||||
wantNil bool
|
||||
wantExitCode int
|
||||
wantMsgContains string
|
||||
}{
|
||||
{
|
||||
name: "nil error returns nil",
|
||||
err: nil,
|
||||
wantNil: true,
|
||||
},
|
||||
{
|
||||
name: "helmexec.ExitError returns correct exit code",
|
||||
err: helmexec.ExitError{
|
||||
Message: "helm command failed",
|
||||
Code: 7,
|
||||
},
|
||||
wantExitCode: 7,
|
||||
wantMsgContains: "helm command failed",
|
||||
},
|
||||
{
|
||||
name: "wrapped helmexec.ExitError preserves exit code",
|
||||
err: fmt.Errorf("helm version failed: %w", helmexec.ExitError{Message: "exit status 7", Code: 7}),
|
||||
wantExitCode: 7,
|
||||
wantMsgContains: "exit status 7",
|
||||
},
|
||||
{
|
||||
name: "unknown error type returns exit code 1 without panic",
|
||||
err: fmt.Errorf("some unexpected error"),
|
||||
wantExitCode: 1,
|
||||
wantMsgContains: "unexpected error",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Should never panic
|
||||
var result error
|
||||
assert.NotPanics(t, func() {
|
||||
result = toCLIError(g, tt.err)
|
||||
})
|
||||
|
||||
if tt.wantNil {
|
||||
assert.NoError(t, result)
|
||||
return
|
||||
}
|
||||
|
||||
assert.Error(t, result)
|
||||
exitErr, ok := result.(*errors.ExitError)
|
||||
assert.True(t, ok, "expected *errors.ExitError, got %T", result)
|
||||
assert.Equal(t, tt.wantExitCode, exitErr.ExitCode())
|
||||
assert.Contains(t, exitErr.Error(), tt.wantMsgContains)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -196,9 +196,20 @@ func (h *HelmfileInit) CheckHelmPlugins() error {
|
|||
|
||||
err = helm.AddPlugin(p.name, p.repo, p.version)
|
||||
if err != nil {
|
||||
return err
|
||||
// Check if plugin was installed despite the error (common on Windows where
|
||||
// plugin install scripts fail due to missing 'sh' but the binary is placed correctly)
|
||||
installedVersion, verifyErr := helmexec.GetPluginVersion(p.name, pluginsDir)
|
||||
if verifyErr != nil {
|
||||
return err // Plugin truly not installed
|
||||
}
|
||||
h.logger.Warnf("helm plugin %q install reported an error, but plugin is present at version %s: %v", p.name, installedVersion, err)
|
||||
pluginVersion = installedVersion
|
||||
} else {
|
||||
pluginVersion, err = helmexec.GetPluginVersion(p.name, pluginsDir)
|
||||
if err != nil {
|
||||
return fmt.Errorf("plugin %q was installed but version could not be verified: %w", p.name, err)
|
||||
}
|
||||
}
|
||||
pluginVersion, _ = helmexec.GetPluginVersion(p.name, pluginsDir)
|
||||
}
|
||||
requiredVersion, _ := semver.NewVersion(p.version)
|
||||
if pluginVersion.LessThan(requiredVersion) {
|
||||
|
|
@ -208,7 +219,16 @@ func (h *HelmfileInit) CheckHelmPlugins() error {
|
|||
}
|
||||
err = helm.UpdatePlugin(p.name)
|
||||
if err != nil {
|
||||
return err
|
||||
// Check if plugin was updated despite the error
|
||||
updatedVersion, verifyErr := helmexec.GetPluginVersion(p.name, pluginsDir)
|
||||
if verifyErr != nil {
|
||||
return err
|
||||
}
|
||||
if !updatedVersion.LessThan(requiredVersion) {
|
||||
h.logger.Warnf("helm plugin %q update reported an error, but plugin is at version %s: %v", p.name, updatedVersion, err)
|
||||
} else {
|
||||
return err
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,14 +2,21 @@ package app
|
|||
|
||||
import (
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"regexp"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"go.uber.org/zap"
|
||||
"go.uber.org/zap/zapcore"
|
||||
|
||||
"github.com/helmfile/helmfile/pkg/helmexec"
|
||||
)
|
||||
|
||||
func TestDownloadfile(t *testing.T) {
|
||||
|
|
@ -84,3 +91,119 @@ func TestDownloadfile(t *testing.T) {
|
|||
})
|
||||
}
|
||||
}
|
||||
|
||||
// initMockRunner implements helmexec.Runner for testing with configurable behavior.
|
||||
type initMockRunner struct {
|
||||
// executeFunc is called for each Execute call. If nil, returns empty output and no error.
|
||||
executeFunc func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error)
|
||||
}
|
||||
|
||||
func (m *initMockRunner) Execute(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) {
|
||||
if m.executeFunc != nil {
|
||||
return m.executeFunc(cmd, args, env, enableLiveOutput)
|
||||
}
|
||||
return []byte{}, nil
|
||||
}
|
||||
|
||||
func (m *initMockRunner) ExecuteStdIn(cmd string, args []string, env map[string]string, stdin io.Reader) ([]byte, error) {
|
||||
return []byte{}, nil
|
||||
}
|
||||
|
||||
// mockInitConfigProvider implements InitConfigProvider for testing.
|
||||
type mockInitConfigProvider struct {
|
||||
force bool
|
||||
}
|
||||
|
||||
func (m *mockInitConfigProvider) Force() bool {
|
||||
return m.force
|
||||
}
|
||||
|
||||
func newTestLogger() *zap.SugaredLogger {
|
||||
cfg := zapcore.EncoderConfig{MessageKey: "message"}
|
||||
core := zapcore.NewCore(
|
||||
zapcore.NewConsoleEncoder(cfg),
|
||||
zapcore.AddSync(io.Discard),
|
||||
zapcore.DebugLevel,
|
||||
)
|
||||
return zap.New(core).Sugar()
|
||||
}
|
||||
|
||||
// createPluginYAML creates a plugin.yaml in a temp plugins directory.
|
||||
func createPluginYAML(t *testing.T, pluginsDir, pluginDirName, name, version string) {
|
||||
t.Helper()
|
||||
dir := filepath.Join(pluginsDir, pluginDirName)
|
||||
require.NoError(t, os.MkdirAll(dir, 0o755))
|
||||
content := fmt.Sprintf("name: %s\nversion: %s\n", name, version)
|
||||
require.NoError(t, os.WriteFile(filepath.Join(dir, "plugin.yaml"), []byte(content), 0o644))
|
||||
}
|
||||
|
||||
// newHelmPluginMockRunner creates a mock runner that returns a valid helm version
|
||||
// and fails all "helm plugin" subcommands with the given error.
|
||||
func newHelmPluginMockRunner(pluginErr error) *initMockRunner {
|
||||
return &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
|
||||
}
|
||||
}
|
||||
// Fail any "helm plugin ..." subcommand (install, update, etc.)
|
||||
if len(args) > 0 && args[0] == "plugin" {
|
||||
return nil, pluginErr
|
||||
}
|
||||
return []byte{}, nil
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
func TestCheckHelmPlugins_InstallErrorButPluginPresent(t *testing.T) {
|
||||
pluginsDir := t.TempDir()
|
||||
t.Setenv("HELM_PLUGINS", pluginsDir)
|
||||
|
||||
// Do NOT pre-populate plugins — the directory starts empty so
|
||||
// GetPluginVersion returns "not installed" and the install path is triggered.
|
||||
// The mock runner simulates the Windows scenario where "helm plugin install"
|
||||
// places the binary but the post-install script fails: it creates the
|
||||
// plugin.yaml on disk and then returns 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) > 0 && args[0] == "plugin" && len(args) >= 3 && args[1] == "install" {
|
||||
// Find which plugin is being installed by matching the repo URL.
|
||||
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: "sh: not found", Code: 1}
|
||||
}
|
||||
return []byte{}, nil
|
||||
},
|
||||
}
|
||||
|
||||
h := NewHelmfileInit("helm", &mockInitConfigProvider{force: true}, newTestLogger(), runner)
|
||||
err := h.CheckHelmPlugins()
|
||||
// Should succeed because plugins are present despite install errors
|
||||
assert.NoError(t, err)
|
||||
}
|
||||
|
||||
func TestCheckHelmPlugins_InstallErrorPluginTrulyMissing(t *testing.T) {
|
||||
pluginsDir := t.TempDir()
|
||||
t.Setenv("HELM_PLUGINS", pluginsDir)
|
||||
|
||||
// Don't create any plugin files — the plugins directory is empty.
|
||||
|
||||
runner := newHelmPluginMockRunner(helmexec.ExitError{Message: "sh: not found", Code: 1})
|
||||
|
||||
h := NewHelmfileInit("helm", &mockInitConfigProvider{force: true}, newTestLogger(), runner)
|
||||
err := h.CheckHelmPlugins()
|
||||
// Should fail because plugin is truly not installed
|
||||
assert.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "sh: not found")
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue