From 4f28f89aac807934fb46799f499f2b1a910d36f4 Mon Sep 17 00:00:00 2001 From: ChadiDridi Date: Sat, 15 Aug 2026 19:43:32 +0100 Subject: [PATCH] fix(plugin): install plugins into the first HELM_PLUGINS directory HELM_PLUGINS may contain a list of directories separated by the OS path list separator. Loading and listing plugins already splits that list, but plugin installation joined the whole value with the plugin name, creating a literal directory such as "/tmp/abc:/tmp/xyz/helm-secrets". The install reported success while the plugin was never found afterwards. Add EnvSettings.PluginInstallDirectory(), which returns the first entry of HELM_PLUGINS (the one with the highest precedence when loading), and use it for the install path in both the base and OCI installers. `helm plugin update` and `helm plugin uninstall` had the same problem: they passed the raw setting to LoadAllDir, so no plugin was found when several directories were configured. They now search every configured directory, and uninstall removes the versioned tarball and provenance files next to the plugin instead of assuming the first directory. Closes #11310 Signed-off-by: ChadiDridi --- internal/plugin/installer/base.go | 2 +- internal/plugin/installer/base_test.go | 9 +++++ internal/plugin/installer/oci_installer.go | 2 +- pkg/cli/environment.go | 14 ++++++++ pkg/cli/environment_test.go | 34 ++++++++++++++++++ pkg/cmd/plugin_uninstall.go | 8 +++-- pkg/cmd/plugin_uninstall_test.go | 40 ++++++++++++++++++++++ pkg/cmd/plugin_update.go | 2 +- 8 files changed, 105 insertions(+), 6 deletions(-) diff --git a/internal/plugin/installer/base.go b/internal/plugin/installer/base.go index 54d25287a..b6ea2c99b 100644 --- a/internal/plugin/installer/base.go +++ b/internal/plugin/installer/base.go @@ -32,7 +32,7 @@ func newBase(source string) base { settings := cli.New() return base{ Source: source, - PluginsDirectory: settings.PluginsDirectory, + PluginsDirectory: settings.PluginInstallDirectory(), } } diff --git a/internal/plugin/installer/base_test.go b/internal/plugin/installer/base_test.go index 8ef7ff588..ff605ad1e 100644 --- a/internal/plugin/installer/base_test.go +++ b/internal/plugin/installer/base_test.go @@ -14,6 +14,8 @@ limitations under the License. package installer import ( + "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -33,6 +35,13 @@ func TestPath(t *testing.T) { source: "https://github.com/jkroepke/helm-secrets", helmPluginsDir: "/helm/data/plugins", expectPath: "/helm/data/plugins/helm-secrets", + }, { + // HELM_PLUGINS may hold a list of directories. New plugins are + // installed into the first one, not into a literal path built + // from the whole list. + source: "https://github.com/jkroepke/helm-secrets", + helmPluginsDir: strings.Join([]string{"/helm/data/plugins", "/shared/plugins"}, string(os.PathListSeparator)), + expectPath: "/helm/data/plugins/helm-secrets", }, } diff --git a/internal/plugin/installer/oci_installer.go b/internal/plugin/installer/oci_installer.go index 383ddb914..c70e4adaa 100644 --- a/internal/plugin/installer/oci_installer.go +++ b/internal/plugin/installer/oci_installer.go @@ -195,7 +195,7 @@ func (i OCIInstaller) Path() string { if i.Source == "" { return "" } - return filepath.Join(i.settings.PluginsDirectory, i.PluginName) + return filepath.Join(i.settings.PluginInstallDirectory(), i.PluginName) } // extractTarGz extracts a gzipped tar archive to a directory diff --git a/pkg/cli/environment.go b/pkg/cli/environment.go index 45d773eb4..391279201 100644 --- a/pkg/cli/environment.go +++ b/pkg/cli/environment.go @@ -26,6 +26,7 @@ package cli import ( "net/http" "os" + "path/filepath" "strconv" "strings" @@ -290,6 +291,19 @@ func (s *EnvSettings) SetNamespace(namespace string) { s.namespace = namespace } +// PluginInstallDirectory returns the directory new plugins are installed into. +// +// PluginsDirectory may hold a list of directories separated by the OS specific +// path list separator (as accepted by filepath.SplitList). All of them are +// searched when loading plugins, but a new plugin has to be written to exactly +// one of them, so the first (highest precedence) entry is used. +func (s *EnvSettings) PluginInstallDirectory() string { + if dirs := filepath.SplitList(s.PluginsDirectory); len(dirs) > 0 { + return dirs[0] + } + return s.PluginsDirectory +} + // RESTClientGetter gets the kubeconfig from EnvSettings func (s *EnvSettings) RESTClientGetter() genericclioptions.RESTClientGetter { return s.config diff --git a/pkg/cli/environment_test.go b/pkg/cli/environment_test.go index fc71038f8..e4301ee17 100644 --- a/pkg/cli/environment_test.go +++ b/pkg/cli/environment_test.go @@ -147,6 +147,40 @@ func TestEnvSettings(t *testing.T) { } } +func TestPluginInstallDirectory(t *testing.T) { + first := filepath.Join("home", "user", "plugins") + second := filepath.Join("shared", "plugins") + + tests := []struct { + name string + env string + expected string + }{ + { + name: "single directory", + env: first, + expected: first, + }, + { + name: "list of directories uses the first one", + env: strings.Join([]string{first, second}, string(os.PathListSeparator)), + expected: first, + }, + { + name: "empty value falls back to the raw setting", + env: "", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + settings := &EnvSettings{PluginsDirectory: tt.env} + assert.Equal(t, tt.expected, settings.PluginInstallDirectory()) + }) + } +} + func TestEnvOrBool(t *testing.T) { const envName = "TEST_ENV_OR_BOOL" tests := []struct { diff --git a/pkg/cmd/plugin_uninstall.go b/pkg/cmd/plugin_uninstall.go index 81d9b31ee..faf2a8900 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.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc) + plugins, err := plugin.FindPlugins(filepath.SplitList(settings.PluginsDirectory), plugin.Descriptor{}) if err != nil { return err } @@ -89,11 +89,13 @@ func uninstallPlugin(p plugin.Plugin) error { return err } - // Clean up versioned tarball and provenance files from HELM_PLUGINS directory + // Clean up versioned tarball and provenance files from the plugins directory + // the plugin was installed into. HELM_PLUGINS may list several directories, + // so the files are looked up next to the plugin itself. // These files are saved with pattern: PLUGIN_NAME-VERSION.tgz and PLUGIN_NAME-VERSION.tgz.prov pluginName := p.Metadata().Name pluginVersion := p.Metadata().Version - pluginsDir := settings.PluginsDirectory + pluginsDir := filepath.Dir(p.Dir()) // Remove versioned files: plugin-name-version.tgz and plugin-name-version.tgz.prov if pluginVersion != "" { diff --git a/pkg/cmd/plugin_uninstall_test.go b/pkg/cmd/plugin_uninstall_test.go index bd08f6f13..baffa7bfa 100644 --- a/pkg/cmd/plugin_uninstall_test.go +++ b/pkg/cmd/plugin_uninstall_test.go @@ -17,8 +17,10 @@ package cmd import ( "fmt" + "io" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -126,3 +128,41 @@ command: $HELM_PLUGIN_DIR/test-plugin _, err = os.Stat(otherVersionTarball) assert.False(t, os.IsNotExist(err), "other version tarball should NOT be removed") } + +func TestPluginUninstallWithMultiplePluginDirs(t *testing.T) { + ensure.HelmHome(t) + + firstDir := t.TempDir() + secondDir := t.TempDir() + t.Setenv("HELM_PLUGINS", strings.Join([]string{firstDir, secondDir}, string(os.PathListSeparator))) + + origSettings := settings + settings = cli.New() + t.Cleanup(func() { settings = origSettings }) + + // The plugin lives in the second directory of HELM_PLUGINS, along with the + // versioned files kept next to it. + pluginDir := filepath.Join(secondDir, "test-plugin") + require.NoError(t, os.MkdirAll(pluginDir, 0o755)) + pluginYAML := `name: test-plugin +version: 1.2.3 +description: Test plugin +command: $HELM_PLUGIN_DIR/test-plugin +` + require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "plugin.yaml"), []byte(pluginYAML), 0o644)) + + tarballFile := filepath.Join(secondDir, "test-plugin-1.2.3.tgz") + provFile := filepath.Join(secondDir, "test-plugin-1.2.3.tgz.prov") + require.NoError(t, os.WriteFile(tarballFile, []byte("fake tarball"), 0o644)) + require.NoError(t, os.WriteFile(provFile, []byte("fake provenance"), 0o644)) + + o := &pluginUninstallOptions{names: []string{"test-plugin"}} + require.NoError(t, o.run(io.Discard)) + + _, err := os.Stat(pluginDir) + assert.True(t, os.IsNotExist(err), "plugin directory should be removed") + _, err = os.Stat(tarballFile) + assert.True(t, os.IsNotExist(err), "versioned tarball file should be removed") + _, err = os.Stat(provFile) + assert.True(t, os.IsNotExist(err), "versioned provenance file should be removed") +} diff --git a/pkg/cmd/plugin_update.go b/pkg/cmd/plugin_update.go index 1cba7ffa9..29032f802 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.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc) + plugins, err := plugin.FindPlugins(filepath.SplitList(settings.PluginsDirectory), plugin.Descriptor{}) if err != nil { return err }