From 25819432bf87ac0b54f0d3fa54982add2cac609e Mon Sep 17 00:00:00 2001 From: George Jenkins Date: Thu, 12 Mar 2026 14:30:41 -0700 Subject: [PATCH] ignore error plugin loads (cli, getter) Signed-off-by: George Jenkins --- cmd/helm/helm_test.go | 2 +- internal/plugin/loader.go | 37 +++++++++++--- internal/plugin/loader_test.go | 30 +++++------- internal/plugin/metadata.go | 5 +- internal/plugin/metadata_legacy_test.go | 4 ++ internal/plugin/metadata_test.go | 49 ++++++++++--------- internal/plugin/metadata_v1_test.go | 13 +++-- internal/plugin/plugin_test.go | 2 +- internal/plugin/runtime_subprocess_test.go | 2 +- pkg/cmd/plugin_test.go | 1 + pkg/cmd/plugin_uninstall.go | 2 +- pkg/cmd/plugin_update.go | 2 +- .../helm/plugins/noversion/plugin.yaml | 7 +++ 13 files changed, 95 insertions(+), 61 deletions(-) create mode 100644 pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml diff --git a/cmd/helm/helm_test.go b/cmd/helm/helm_test.go index 0458e8037..8a65c2b1a 100644 --- a/cmd/helm/helm_test.go +++ b/cmd/helm/helm_test.go @@ -67,7 +67,7 @@ func TestCliPluginExitCode(t *testing.T) { assert.Empty(t, stdout.String()) - expectedStderr := "Error: plugin \"exitwith\" exited with error\n" + expectedStderr := "level=WARN msg=\"failed to load plugin (ignoring)\" plugin_yaml=../../pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml error=\"failed to load plugin \\\"../../pkg/cmd/testdata/helmhome/helm/plugins/noversion\\\": plugin `version` is required\"\nError: plugin \"exitwith\" exited with error\n" if stderr.String() != expectedStderr { t.Errorf("Expected %q written to stderr: Got %q", expectedStderr, stderr.String()) } diff --git a/internal/plugin/loader.go b/internal/plugin/loader.go index 2f051b99e..d99395641 100644 --- a/internal/plugin/loader.go +++ b/internal/plugin/loader.go @@ -19,6 +19,7 @@ import ( "bytes" "fmt" "io" + "log/slog" "os" "path/filepath" @@ -158,18 +159,27 @@ func LoadDir(dirname string) (Plugin, error) { return pm.CreatePlugin(dirname, m) } -// LoadAll loads all plugins found beneath the base directory. +func LogIgnorePluginLoadErrorFilterFunc(pluginYAML string, err error) error { + slog.Warn("failed to load plugin (ignoring)", slog.String("plugin_yaml", pluginYAML), slog.Any("error", err)) + return nil +} + +// errorFilterFunc is a function that can filter errors during plugin loading +type ErrorFilterFunc func(string, error) error + +// LoadAllDir load all plugins found beneath the base directory, using the provided error filter to determine whether to fail on individual plugin load errors. // // This scans only one directory level. -func LoadAll(basedir string) ([]Plugin, error) { - var plugins []Plugin - // We want basedir/*/plugin.yaml +func LoadAllDir(basedir string, errorFilter ErrorFilterFunc) ([]Plugin, error) { + // We want /*/plugin.yaml scanpath := filepath.Join(basedir, "*", PluginFileName) matches, err := filepath.Glob(scanpath) if err != nil { return nil, fmt.Errorf("failed to search for plugins in %q: %w", scanpath, err) } + plugins := make([]Plugin, 0, len(matches)) + // empty dir should load if len(matches) == 0 { return plugins, nil @@ -179,9 +189,12 @@ func LoadAll(basedir string) ([]Plugin, error) { dir := filepath.Dir(yamlFile) p, err := LoadDir(dir) if err != nil { - return plugins, err + if errNew := errorFilter(yamlFile, err); errNew != nil { + return plugins, errNew + } + } else { + plugins = append(plugins, p) } - plugins = append(plugins, p) } return plugins, detectDuplicates(plugins) } @@ -193,8 +206,12 @@ type findFunc func(pluginsDir string) ([]Plugin, error) type filterFunc func(Plugin) bool // FindPlugins returns a list of plugins that match the descriptor +// Errors loading a plugin are ignored with a warning func FindPlugins(pluginsDirs []string, descriptor Descriptor) ([]Plugin, error) { - return findPlugins(pluginsDirs, LoadAll, makeDescriptorFilter(descriptor)) + loadAllIgnoreErrors := func(pluginsDir string) ([]Plugin, error) { + return LoadAllDir(pluginsDir, LogIgnorePluginLoadErrorFilterFunc) + } + return findPlugins(pluginsDirs, loadAllIgnoreErrors, makeDescriptorFilter(descriptor)) } // findPlugins is the internal implementation that uses the find and filter functions @@ -237,7 +254,11 @@ func makeDescriptorFilter(descriptor Descriptor) filterFunc { // FindPlugin returns a single plugin that matches the descriptor func FindPlugin(dirs []string, descriptor Descriptor) (Plugin, error) { - plugins, err := FindPlugins(dirs, descriptor) + loadAllIgnoreErrors := func(pluginsDir string) ([]Plugin, error) { + return LoadAllDir(pluginsDir, LogIgnorePluginLoadErrorFilterFunc) + } + + plugins, err := findPlugins(dirs, loadAllIgnoreErrors, makeDescriptorFilter(descriptor)) if err != nil { return nil, err } diff --git a/internal/plugin/loader_test.go b/internal/plugin/loader_test.go index 16b8581c3..0fc3c9444 100644 --- a/internal/plugin/loader_test.go +++ b/internal/plugin/loader_test.go @@ -205,16 +205,16 @@ func TestDetectDuplicates(t *testing.T) { } } -func TestLoadAll(t *testing.T) { - // Verify that empty dir loads: - { - plugs, err := LoadAll("testdata") - require.NoError(t, err) - assert.Len(t, plugs, 0) - } +func TestLoadAllDir_Empty(t *testing.T) { + emptyDir := t.TempDir() + plugs, err := LoadAllDir(emptyDir, func(_ string, err error) error { return err }) + require.NoError(t, err) + assert.Len(t, plugs, 0) +} +func TestLoadAllPluginsDir(t *testing.T) { basedir := "testdata/plugdir/good" - plugs, err := LoadAll(basedir) + plugs, err := LoadAllDir(basedir, func(_ string, err error) error { return err }) require.NoError(t, err) require.NotEmpty(t, plugs, "expected plugins to be loaded from %s", basedir) @@ -233,7 +233,7 @@ func TestLoadAll(t *testing.T) { assert.Contains(t, plugsMap, "postrenderer-v1") } -func TestFindPlugins(t *testing.T) { +func TestLoadAllPluginsDir_Zero(t *testing.T) { cases := []struct { name string plugdirs string @@ -241,28 +241,20 @@ func TestFindPlugins(t *testing.T) { }{ { name: "plugdirs is empty", - plugdirs: "", - expected: 0, + plugdirs: t.TempDir(), }, { name: "plugdirs isn't dir", plugdirs: "./plugin_test.go", - expected: 0, }, { name: "plugdirs doesn't have plugin", plugdirs: ".", - expected: 0, - }, - { - name: "normal", - plugdirs: "./testdata/plugdir/good", - expected: 7, }, } for _, c := range cases { t.Run(t.Name(), func(t *testing.T) { - plugin, err := LoadAll(c.plugdirs) + plugin, err := LoadAllDir(c.plugdirs, func(_ string, err error) error { return err }) require.NoError(t, err) assert.Len(t, plugin, c.expected, "expected %d plugins, got %d", c.expected, len(plugin)) }) diff --git a/internal/plugin/metadata.go b/internal/plugin/metadata.go index 337c73da6..29cc7e9f8 100644 --- a/internal/plugin/metadata.go +++ b/internal/plugin/metadata.go @@ -24,9 +24,9 @@ import ( "helm.sh/helm/v4/internal/plugin/schema" ) -// isValidSemver checks if the given string is a valid semantic version. +// isValidSemver checks if the given string is a valid semantic version func isValidSemver(v string) bool { - _, err := semver.NewVersion(v) + _, err := semver.StrictNewVersion(v) return err == nil } @@ -65,6 +65,7 @@ func (m Metadata) Validate() error { errs = append(errs, fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)) } + // Require version to be valid semver if specified if m.Version != "" && !isValidSemver(m.Version) { errs = append(errs, fmt.Errorf("invalid plugin version %q: must be valid semver", m.Version)) } diff --git a/internal/plugin/metadata_legacy_test.go b/internal/plugin/metadata_legacy_test.go index 24cc48a60..0ecb7e619 100644 --- a/internal/plugin/metadata_legacy_test.go +++ b/internal/plugin/metadata_legacy_test.go @@ -26,6 +26,10 @@ func TestMetadataLegacyValidate(t *testing.T) { "valid metadata": { Name: "myplugin", }, + "valid metadata (empty version)": { + Name: "myplugin", + Version: "", + }, "valid with command": { Name: "myplugin", Command: "echo hello", diff --git a/internal/plugin/metadata_test.go b/internal/plugin/metadata_test.go index e983d32ae..a2acd7925 100644 --- a/internal/plugin/metadata_test.go +++ b/internal/plugin/metadata_test.go @@ -18,6 +18,8 @@ package plugin import ( "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestValidatePluginData(t *testing.T) { @@ -72,36 +74,39 @@ func TestValidatePluginData(t *testing.T) { } } -func TestMetadataValidateVersionPathTraversal(t *testing.T) { - tests := map[string]struct { +func TestMetadataValidateVersion(t *testing.T) { + testValid := map[string]struct { + version string + }{ + "valid semver": {version: "1.0.0"}, + "valid semver with prerelease": {version: "1.2.3-alpha.1+build.123"}, + "empty version": {version: ""}, + } + + testInvalid := map[string]struct { version string - valid bool }{ - "valid semver": {version: "1.0.0", valid: true}, - "valid semver with v prefix": {version: "v1.0.0", valid: true}, - "valid semver with prerelease": {version: "1.2.3-alpha.1+build.123", valid: true}, - "empty version": {version: "", valid: true}, - "path traversal": {version: "../../../../tmp/evil", valid: false}, - "path traversal etc": {version: "../../../etc/passwd", valid: false}, - "path traversal in version": {version: "1.0.0/../../etc", valid: false}, - "not a version": {version: "not-a-version", valid: false}, + "valid semver with v prefix": {version: "v1.0.0"}, + "path traversal": {version: "../../../../tmp/evil"}, + "path traversal in version": {version: "1.0.0/../../etc"}, + "not a version": {version: "not-a-version"}, + } + + for name, tc := range testValid { + t.Run(name, func(t *testing.T) { + m := mockSubprocessCLIPlugin(t, "testplugin") + m.metadata.Version = tc.version + err := m.Metadata().Validate() + assert.NoError(t, err) + }) } - for name, tc := range tests { + for name, tc := range testInvalid { t.Run(name, func(t *testing.T) { m := mockSubprocessCLIPlugin(t, "testplugin") m.metadata.Version = tc.version err := m.Metadata().Validate() - if tc.valid && err != nil { - t.Errorf("expected valid, got error: %s", err) - } - if !tc.valid { - if err == nil { - t.Errorf("expected error for version %q, got nil", tc.version) - } else if !strings.Contains(err.Error(), "invalid plugin version") { - t.Errorf("expected 'invalid plugin version' error, got: %s", err) - } - } + assert.ErrorContains(t, err, "invalid plugin version") }) } } diff --git a/internal/plugin/metadata_v1_test.go b/internal/plugin/metadata_v1_test.go index ed9a2c671..17a02dac0 100644 --- a/internal/plugin/metadata_v1_test.go +++ b/internal/plugin/metadata_v1_test.go @@ -33,11 +33,10 @@ func TestMetadataV1ValidateVersion(t *testing.T) { } testsValid := map[string]string{ - "simple version": "1.0.0", - "v prefix": "v1.0.0", - "with prerelease": "1.2.3-alpha.1", - "with build meta": "1.2.3+build.123", - "full prerelease": "1.2.3-alpha.1+build.123", + "simple version": "1.0.0", + "with prerelease": "1.2.3-alpha.1", + "with build meta": "1.2.3+build.123", + "full prerelease": "1.2.3-alpha.1+build.123", } for name, version := range testsValid { @@ -56,6 +55,10 @@ func TestMetadataV1ValidateVersion(t *testing.T) { version: "", errMsg: "plugin `version` is required", }, + "v prefix": { + version: "v1.0.0", + errMsg: "invalid plugin `version` \"v1.0.0\": must be valid semver", + }, "path traversal": { version: "../../../../tmp/evil", errMsg: "invalid plugin `version`", diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index ae0b343f3..2e3f274f2 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -82,7 +82,7 @@ func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginR return &SubprocessPluginRuntime{ metadata: Metadata{ Name: pluginName, - Version: "v0.1.2", + Version: "0.1.2", Type: "cli/v1", APIVersion: "v1", Runtime: "subprocess", diff --git a/internal/plugin/runtime_subprocess_test.go b/internal/plugin/runtime_subprocess_test.go index ed251d28b..193c21775 100644 --- a/internal/plugin/runtime_subprocess_test.go +++ b/internal/plugin/runtime_subprocess_test.go @@ -41,7 +41,7 @@ func mockSubprocessCLIPluginErrorExit(t *testing.T, pluginName string, exitCode md := Metadata{ Name: pluginName, - Version: "v0.1.2", + Version: "0.1.2", Type: "cli/v1", APIVersion: "v1", Runtime: "subprocess", diff --git a/pkg/cmd/plugin_test.go b/pkg/cmd/plugin_test.go index a250ba221..81df8502d 100644 --- a/pkg/cmd/plugin_test.go +++ b/pkg/cmd/plugin_test.go @@ -117,6 +117,7 @@ func TestLoadCLIPlugins(t *testing.T) { {"exitwith", "exitwith code", "This exits with the specified exit code", "", []string{"2"}, 2}, {"fullenv", "show env vars", "show all env vars", fullEnvOutput, []string{}, 0}, {"shortenv", "env stuff", "show the env", "HELM_PLUGIN_NAME=shortenv\n", []string{}, 0}, + // "noversion": plugin is invalid, and should not be loaded } pluginCmds := cmd.Commands() diff --git a/pkg/cmd/plugin_uninstall.go b/pkg/cmd/plugin_uninstall.go index 85eb46219..c75cf6264 100644 --- a/pkg/cmd/plugin_uninstall.go +++ b/pkg/cmd/plugin_uninstall.go @@ -62,7 +62,7 @@ func (o *pluginUninstallOptions) complete(args []string) error { func (o *pluginUninstallOptions) run(out io.Writer) error { slog.Debug("loading installer plugins", "dir", settings.PluginsDirectory) - plugins, err := plugin.LoadAll(settings.PluginsDirectory) + plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc) if err != nil { return err } diff --git a/pkg/cmd/plugin_update.go b/pkg/cmd/plugin_update.go index 6cc2729fc..83ef35107 100644 --- a/pkg/cmd/plugin_update.go +++ b/pkg/cmd/plugin_update.go @@ -62,7 +62,7 @@ func (o *pluginUpdateOptions) complete(args []string) error { func (o *pluginUpdateOptions) run(out io.Writer) error { slog.Debug("loading installed plugins", "path", settings.PluginsDirectory) - plugins, err := plugin.LoadAll(settings.PluginsDirectory) + plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc) if err != nil { return err } diff --git a/pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml b/pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml new file mode 100644 index 000000000..70c356dea --- /dev/null +++ b/pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +name: noversion +type: cli/v1 +runtime: subprocess +runtimeConfig: + platformCommand: + - command: "echo hello"