diff --git a/internal/plugin/installer/vcs_installer.go b/internal/plugin/installer/vcs_installer.go index 13aba74dc..3d1ddfafa 100644 --- a/internal/plugin/installer/vcs_installer.go +++ b/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) { diff --git a/internal/plugin/installer/vcs_installer_test.go b/internal/plugin/installer/vcs_installer_test.go index 0fee301a4..2324df25d 100644 --- a/internal/plugin/installer/vcs_installer_test.go +++ b/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)) +}