From 92d6fd884670b8f56224976bf3c3ed96368c0a44 Mon Sep 17 00:00:00 2001 From: Bisman-Singh Date: Tue, 1 Sep 2026 05:28:44 +0530 Subject: [PATCH] fix(plugins): update VCS plugins from the cached repo, not the install dir 'helm plugin update' ran the dirty check and pull against the installed plugin directory itself: updatePlugin resolves p.Dir() and FindSource builds the VCS repo on that path, which Install() made a git checkout by copying .git from the cached clone. A plugin whose install hook rewrites files inside its own directory (as helm-unittest's install-binary.sh does) leaves that checkout permanently dirty, so every subsequent update fails with "plugin repo was modified". Point existingVCSRepo at Helm's cached clone of the plugin repository instead. Hooks only ever run against the installed copy, so the cache stays pristine: Update() now syncs the cache (recloning it if it was evicted) and refreshes the installed copy from that clean checkout, mirroring Install() and OCIInstaller.Update(). The dirty guard is kept for the cache itself, where it means what it says again. Closes #31664 Signed-off-by: Bisman-Singh --- internal/plugin/installer/vcs_installer.go | 27 ++++++--- .../plugin/installer/vcs_installer_test.go | 59 +++++++++++++++++++ 2 files changed, 77 insertions(+), 9 deletions(-) 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)) +}