ignore error plugin loads (cli, getter)

Signed-off-by: George Jenkins <gvjenkins@gmail.com>

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())
 		}

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 <basedir>/*/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
 	}

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))
 		})

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))
 	}

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",

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")
 		})
 	}
 }

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`",

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",

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",

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()

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
 	}

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
 	}

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"