Merge 92d6fd8846 into 87268ee077

internal/plugin/installer/vcs_installer.go

@@ -38,16 +38,18 @@ type VCSInstaller struct {
 	base
 }
 
+// existingVCSRepo reads the remote source of an installed plugin and returns
+// a VCSInstaller operating on Helm's cached copy of that repository. The
+// installed plugin directory is never used as the VCS working tree: a
+// plugin's install/update hooks may legitimately modify files there (for
+// example to place the plugin's binary), which would otherwise leave the
+// checkout permanently dirty and block every future update.
 func existingVCSRepo(location string) (Installer, error) {
 	repo, err := vcs.NewRepo("", location)
 	if err != nil {
 		return nil, err
 	}
-	i := &VCSInstaller{
-		Repo: repo,
-		base: newBase(repo.Remote()),
-	}
-	return i, nil
+	return NewVCSInstaller(repo.Remote(), "")
 }
 
 // NewVCSInstaller creates a new VCSInstaller.
@@ -95,19 +97,26 @@ func (i *VCSInstaller) Install() error {
 	return fs.CopyDir(i.Repo.LocalPath(), i.Path())
 }
 
-// Update updates a remote repository
+// Update fetches the latest version of the plugin repository into Helm's
+// plugin cache and refreshes the installed copy from that clean checkout.
+// The plugin's hooks only ever run against the installed copy, so changes
+// they make are not mistaken for user modifications of the repository.
 func (i *VCSInstaller) Update() error {
 	slog.Debug("updating", "source", i.Repo.Remote())
-	if i.Repo.IsDirty() {
+	if i.Repo.CheckLocal() && i.Repo.IsDirty() {
 		return errors.New("plugin repo was modified")
 	}
-	if err := i.Repo.Update(); err != nil {
+	if err := i.sync(i.Repo); err != nil {
 		return err
 	}
 	if !isPlugin(i.Repo.LocalPath()) {
 		return ErrMissingMetadata
 	}
-	return nil
+	if err := os.RemoveAll(i.Path()); err != nil {
+		return err
+	}
+	slog.Debug("copying files", "source", i.Repo.LocalPath(), "destination", i.Path())
+	return fs.CopyDir(i.Repo.LocalPath(), i.Path())
 }
 
 func (i *VCSInstaller) solveVersion(repo vcs.Repo) (string, error) {

internal/plugin/installer/vcs_installer_test.go

@@ -18,6 +18,7 @@ package installer
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -143,3 +144,61 @@ func TestVCSInstallerUpdate(t *testing.T) {
 	// Testing update for error
 	require.EqualErrorf(t, Update(vcsInstaller), "plugin repo was modified", "expected error for plugin modified")
 }
+
+// runGit runs a git command in dir, failing the test on error.
+func runGit(t *testing.T, dir string, args ...string) {
+	t.Helper()
+	args = append([]string{"-C", dir, "-c", "user.name=Helm Test", "-c", "user.email=helm@example.com", "-c", "commit.gpgsign=false"}, args...)
+	out, err := exec.CommandContext(t.Context(), "git", args...).CombinedOutput()
+	require.NoErrorf(t, err, "git %v: %s", args, out)
+}
+
+// TestVCSInstallerUpdateHookModifiedPlugin ensures a plugin can still be
+// updated after its install hook has modified files inside the installed
+// plugin directory, as helm-unittest's install-binary.sh does. Update must
+// run against Helm's cached repository, not the installed copy, whose
+// checkout is legitimately dirtied by hooks. See
+// https://github.com/helm/helm/issues/31664.
+func TestVCSInstallerUpdateHookModifiedPlugin(t *testing.T) {
+	ensure.HelmHome(t)
+
+	require.NoErrorf(t, os.MkdirAll(helmpath.DataPath("plugins"), 0o755), "Could not create %s", helmpath.DataPath("plugins"))
+
+	// Create a local git repository containing the plugin.
+	upstream := filepath.Join(t.TempDir(), "hookmod")
+	require.NoError(t, os.MkdirAll(upstream, 0o755))
+	pluginYAML := "name: \"hookmod\"\nversion: \"0.1.0\"\ntype: cli/v1\napiVersion: v1\nruntime: subprocess\nconfig:\n  shortHelp: \"hook modified plugin\"\n  longHelp: \"hook modified plugin\"\nruntimeConfig:\n  command: \"echo Hello\"\n"
+	require.NoError(t, os.WriteFile(filepath.Join(upstream, "plugin.yaml"), []byte(pluginYAML), 0o644))
+	runGit(t, upstream, "init")
+	runGit(t, upstream, "add", "plugin.yaml")
+	runGit(t, upstream, "commit", "-m", "initial commit")
+
+	source := "file://" + upstream
+	i, err := NewForSource(source, "")
+	require.NoError(t, err)
+	require.IsType(t, &VCSInstaller{}, i, "expected a VCSInstaller")
+	require.NoError(t, Install(i))
+
+	// Simulate an install hook rewriting a tracked file inside the
+	// installed plugin directory.
+	require.NoError(t, os.WriteFile(filepath.Join(i.Path(), "plugin.yaml"), []byte(pluginYAML+"# rewritten by install hook\n"), 0o644))
+
+	// Publish a new version upstream.
+	require.NoError(t, os.WriteFile(filepath.Join(upstream, "plugin.yaml"), []byte(strings.ReplaceAll(pluginYAML, "0.1.0", "0.2.0")), 0o644))
+	runGit(t, upstream, "commit", "-am", "release 0.2.0")
+
+	upd, err := FindSource(i.Path())
+	require.NoError(t, err)
+	require.NoError(t, Update(upd), "update must not treat hook-produced changes as user modifications")
+
+	// The installed copy is refreshed from the clean checkout.
+	data, err := os.ReadFile(filepath.Join(i.Path(), "plugin.yaml"))
+	require.NoError(t, err)
+	require.Contains(t, string(data), `version: "0.2.0"`)
+	require.NotContains(t, string(data), "rewritten by install hook")
+
+	// A second update must keep working.
+	upd, err = FindSource(i.Path())
+	require.NoError(t, err)
+	require.NoError(t, Update(upd))
+}