From 4bd77ff7bb4aa2e3dd67b9603588a2a11d629dac Mon Sep 17 00:00:00 2001 From: Will Browne Date: Thu, 11 Sep 2025 14:44:29 +0100 Subject: [PATCH] Plugins: StaticFS should implement FSRemover (#110706) (#110944) make staticfs implement fs removal interface --- .../commands/remove_command_test.go | 102 ++++++++++++ .../commands/upgrade_command_test.go | 150 ++++++++++++++++++ pkg/plugins/localfiles.go | 9 ++ pkg/plugins/localfiles_test.go | 17 +- pkg/plugins/manager/installer_test.go | 99 ++++++++++++ .../manager/sources/source_local_disk_test.go | 30 ++++ 6 files changed, 406 insertions(+), 1 deletion(-) create mode 100644 pkg/cmd/grafana-cli/commands/remove_command_test.go create mode 100644 pkg/cmd/grafana-cli/commands/upgrade_command_test.go diff --git a/pkg/cmd/grafana-cli/commands/remove_command_test.go b/pkg/cmd/grafana-cli/commands/remove_command_test.go new file mode 100644 index 00000000000..1fee87dc62c --- /dev/null +++ b/pkg/cmd/grafana-cli/commands/remove_command_test.go @@ -0,0 +1,102 @@ +package commands + +import ( + "flag" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v2" + + "github.com/grafana/grafana/pkg/cmd/grafana-cli/utils" +) + +func TestRemoveCommand_StaticFS_FailsWithImmutableError(t *testing.T) { + t.Run("removeCommand fails with immutable error for plugins using StaticFS", func(t *testing.T) { + tmpDir := t.TempDir() + pluginID := "test-plugin" + pluginDir := filepath.Join(tmpDir, pluginID) + + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "test-plugin", + "name": "Test Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + cmdLine := createCliContextWithArgs(t, []string{pluginID}, "pluginsDir", tmpDir) + require.NotNil(t, cmdLine) + + // Verify plugin directory exists before attempting removal + _, err = os.Stat(pluginDir) + require.NoError(t, err, "Plugin directory should exist before removal attempt") + + err = removeCommand(cmdLine) + require.NoError(t, err) + + // Verify plugin directory has been removed + _, err = os.Stat(pluginDir) + require.ErrorIs(t, err, os.ErrNotExist) + }) +} + +func TestRemoveCommand_PluginNotFound(t *testing.T) { + t.Run("removeCommand should handle missing plugin gracefully", func(t *testing.T) { + tmpDir := t.TempDir() + cmdLine := createCliContextWithArgs(t, []string{"non-existent-plugin"}, "pluginsDir", tmpDir) + require.NotNil(t, cmdLine) + + err := removeCommand(cmdLine) + require.NoError(t, err) + }) +} + +func TestRemoveCommand_MissingPluginParameter(t *testing.T) { + t.Run("removeCommand should error when no plugin ID is provided", func(t *testing.T) { + cmdLine := createCliContextWithArgs(t, []string{}) + require.NotNil(t, cmdLine) + + err := removeCommand(cmdLine) + require.Error(t, err) + require.Contains(t, err.Error(), "missing plugin parameter") + }) +} + +// createCliContextWithArgs creates a CLI context with the specified arguments and optional flag key-value pairs. +// Usage: createCliContextWithArgs(t, []string{"plugin-id"}, "pluginsDir", "/path/to/plugins", "flag2", "value2") +func createCliContextWithArgs(t *testing.T, args []string, flagPairs ...string) *utils.ContextCommandLine { + if len(flagPairs)%2 != 0 { + t.Fatalf("flagPairs must be provided in key-value pairs, got %d arguments", len(flagPairs)) + } + + app := &cli.App{ + Name: "grafana", + } + + flagSet := flag.NewFlagSet("test", 0) + + // Add flags from the key-value pairs + for i := 0; i < len(flagPairs); i += 2 { + key := flagPairs[i] + value := flagPairs[i+1] + flagSet.String(key, "", "") + err := flagSet.Set(key, value) + require.NoError(t, err, "Failed to set flag %s=%s", key, value) + } + + err := flagSet.Parse(args) + require.NoError(t, err) + + ctx := cli.NewContext(app, flagSet, nil) + return &utils.ContextCommandLine{ + Context: ctx, + } +} diff --git a/pkg/cmd/grafana-cli/commands/upgrade_command_test.go b/pkg/cmd/grafana-cli/commands/upgrade_command_test.go new file mode 100644 index 00000000000..097016a0feb --- /dev/null +++ b/pkg/cmd/grafana-cli/commands/upgrade_command_test.go @@ -0,0 +1,150 @@ +package commands + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v2" + + "github.com/grafana/grafana/pkg/cmd/grafana-cli/models" +) + +func TestUpgradeCommand(t *testing.T) { + t.Run("Plugin is removed even if upgrade fails", func(t *testing.T) { + tmpDir := t.TempDir() + pluginID := "test-upgrade-plugin" + pluginDir := filepath.Join(tmpDir, pluginID) + + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "test-upgrade-plugin", + "name": "Test Upgrade Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + // Create a mock HTTP server that returns plugin info with a newer version + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Handle plugin info request + if r.URL.Path == "/repo/"+pluginID { + plugin := models.Plugin{ + ID: pluginID, + Versions: []models.Version{ + { + Version: "2.0.0", // Newer than the local version (1.0.0) + }, + }, + } + + w.Header().Set("Content-Type", "application/json") + err = json.NewEncoder(w).Encode(plugin) + require.NoError(t, err) + return + } + + // For any other request (like installation), return 500 to cause the upgrade to fail + // after the removal attempt, which is what we want to test + w.WriteHeader(http.StatusInternalServerError) + _, err = w.Write([]byte("Server error")) + require.NoError(t, err) + })) + defer mockServer.Close() + + // Use our test implementation that properly implements GcomToken() + cmdLine := newTestCommandLine([]string{pluginID}, tmpDir, mockServer.URL) + + // Verify plugin directory exists before attempting upgrade + _, err = os.Stat(pluginDir) + require.NoError(t, err) + + err = upgradeCommand(cmdLine) + require.Error(t, err) + require.Contains(t, err.Error(), "API returned invalid status: 500 Internal Server Error") + + // Verify plugin directory was removed during the removal step + _, err = os.Stat(pluginDir) + require.True(t, os.IsNotExist(err)) + }) +} + +func TestUpgradeCommand_PluginNotFound(t *testing.T) { + t.Run("upgradeCommand should handle missing plugin gracefully", func(t *testing.T) { + tmpDir := t.TempDir() + cmdLine := createCliContextWithArgs(t, []string{"non-existent-plugin"}, "pluginsDir", tmpDir) + require.NotNil(t, cmdLine) + + err := upgradeCommand(cmdLine) + require.Error(t, err) + // Should fail trying to find the local plugin + require.Contains(t, err.Error(), "could not find plugin non-existent-plugin") + }) +} + +func TestUpgradeCommand_MissingPluginParameter(t *testing.T) { + t.Run("upgradeCommand should error when no plugin ID is provided", func(t *testing.T) { + cmdLine := createCliContextWithArgs(t, []string{}) + require.NotNil(t, cmdLine) + + err := upgradeCommand(cmdLine) + require.Error(t, err) + require.Contains(t, err.Error(), "please specify plugin to update") + }) +} + +// Simple args implementation +type simpleArgs []string + +func (a simpleArgs) First() string { + if len(a) > 0 { + return a[0] + } + return "" +} +func (a simpleArgs) Get(int) string { return "" } +func (a simpleArgs) Tail() []string { return nil } +func (a simpleArgs) Len() int { return len(a) } +func (a simpleArgs) Present() bool { return len(a) > 0 } +func (a simpleArgs) Slice() []string { return []string(a) } + +// Base struct with default implementations for unused CommandLine methods +type baseCommandLine struct{} + +func (b baseCommandLine) ShowHelp() error { return nil } +func (b baseCommandLine) ShowVersion() {} +func (b baseCommandLine) Application() *cli.App { return nil } +func (b baseCommandLine) Int(_ string) int { return 0 } +func (b baseCommandLine) String(_ string) string { return "" } +func (b baseCommandLine) StringSlice(_ string) []string { return nil } +func (b baseCommandLine) FlagNames() []string { return nil } +func (b baseCommandLine) Generic(_ string) any { return nil } +func (b baseCommandLine) Bool(_ string) bool { return false } +func (b baseCommandLine) PluginURL() string { return "" } +func (b baseCommandLine) GcomToken() string { return "" } + +// Test implementation - only implements what we actually need +type testCommandLine struct { + baseCommandLine // Embedded struct provides default implementations + args simpleArgs + pluginDir string + repoURL string +} + +func newTestCommandLine(args []string, pluginDir, repoURL string) *testCommandLine { + return &testCommandLine{args: simpleArgs(args), pluginDir: pluginDir, repoURL: repoURL} +} + +// Only implement the methods actually used by upgradeCommand +func (t *testCommandLine) Args() cli.Args { return t.args } +func (t *testCommandLine) PluginDirectory() string { return t.pluginDir } +func (t *testCommandLine) PluginRepoURL() string { return t.repoURL } diff --git a/pkg/plugins/localfiles.go b/pkg/plugins/localfiles.go index 0385f69dc94..c0ff62b13c3 100644 --- a/pkg/plugins/localfiles.go +++ b/pkg/plugins/localfiles.go @@ -236,6 +236,15 @@ func (f StaticFS) Files() ([]string, error) { return files, nil } +func (f StaticFS) Remove() error { + if remover, ok := f.FS.(FSRemover); ok { + if err := remover.Remove(); err != nil { + return err + } + } + return nil +} + // LocalFile implements a fs.File for accessing the local filesystem. type LocalFile struct { f *os.File diff --git a/pkg/plugins/localfiles_test.go b/pkg/plugins/localfiles_test.go index 9654dac5ce1..c13f459fbf0 100644 --- a/pkg/plugins/localfiles_test.go +++ b/pkg/plugins/localfiles_test.go @@ -270,12 +270,27 @@ func TestStaticFS(t *testing.T) { require.Equal(t, []string{allowedFn, deniedFn}, files) }) - t.Run("staticfs filters underelying fs's files", func(t *testing.T) { + t.Run("staticfs filters underlying fs's files", func(t *testing.T) { files, err := staticFS.Files() require.NoError(t, err) require.Equal(t, []string{allowedFn}, files) }) }) + + t.Run("FSRemover interface implementation verification", func(t *testing.T) { + tmpDir := t.TempDir() + + lfs := NewLocalFS(tmpDir) + var localFSInterface FS = lfs + _, isRemover := localFSInterface.(FSRemover) + require.True(t, isRemover) + + sfs, err := NewStaticFS(localFS) + require.NoError(t, err) + var staticFSInterface FS = sfs + _, isRemover = staticFSInterface.(FSRemover) + require.True(t, isRemover) + }) } // TestFSTwoDotsInFileName ensures that LocalFS and StaticFS allow two dots in file names. diff --git a/pkg/plugins/manager/installer_test.go b/pkg/plugins/manager/installer_test.go index 1ca2f67735e..6a7962ca5a9 100644 --- a/pkg/plugins/manager/installer_test.go +++ b/pkg/plugins/manager/installer_test.go @@ -5,6 +5,8 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" "runtime" "testing" @@ -422,3 +424,100 @@ func createPlugin(t *testing.T, pluginID string, class plugins.Class, managed, b func testCompatOpts() plugins.AddOpts { return plugins.NewAddOpts("10.0.0", runtime.GOOS, runtime.GOARCH, "") } + +func TestPluginInstaller_Removal(t *testing.T) { + tmpDir := t.TempDir() + + t.Run("LocalFS plugin removal succeeds via installer.Remove", func(t *testing.T) { + pluginDir := filepath.Join(tmpDir, "localfs-plugin") + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "localfs-plugin", + "name": "LocalFS Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + localFS := plugins.NewLocalFS(pluginDir) + pluginV1 := createPlugin(t, "localfs-plugin", plugins.ClassExternal, true, true, func(plugin *plugins.Plugin) { + plugin.Info.Version = "1.0.0" + plugin.FS = localFS + }) + + registry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + "localfs-plugin": pluginV1, + }, + } + + loader := &fakes.FakeLoader{ + UnloadFunc: func(_ context.Context, p *plugins.Plugin) (*plugins.Plugin, error) { + return p, nil + }, + } + + _, err = os.Stat(pluginDir) + require.NoError(t, err) + + inst := New(&config.PluginManagementCfg{}, registry, loader, &fakes.FakePluginRepo{}, &fakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &fakes.FakeAuthService{}) + err = inst.Remove(context.Background(), "localfs-plugin", "1.0.0") + require.NoError(t, err) + + _, err = os.Stat(pluginDir) + require.True(t, os.IsNotExist(err)) + }) + + t.Run("StaticFS plugin removal is skipped via installer.Remove", func(t *testing.T) { + pluginDir := filepath.Join(tmpDir, "staticfs-plugin") + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "staticfs-plugin", + "name": "StaticFS Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + localFS := plugins.NewLocalFS(pluginDir) + staticFS, err := plugins.NewStaticFS(localFS) + require.NoError(t, err) + + pluginV1 := createPlugin(t, "staticfs-plugin", plugins.ClassExternal, true, true, func(plugin *plugins.Plugin) { + plugin.Info.Version = "1.0.0" + plugin.FS = staticFS + }) + + registry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + "staticfs-plugin": pluginV1, + }, + } + + loader := &fakes.FakeLoader{ + UnloadFunc: func(_ context.Context, p *plugins.Plugin) (*plugins.Plugin, error) { + return p, nil + }, + } + + _, err = os.Stat(pluginDir) + require.NoError(t, err) + + inst := New(&config.PluginManagementCfg{}, registry, loader, &fakes.FakePluginRepo{}, &fakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &fakes.FakeAuthService{}) + err = inst.Remove(context.Background(), "staticfs-plugin", "1.0.0") + require.NoError(t, err) + + _, err = os.Stat(pluginDir) + require.ErrorIs(t, err, os.ErrNotExist) + }) +} diff --git a/pkg/plugins/manager/sources/source_local_disk_test.go b/pkg/plugins/manager/sources/source_local_disk_test.go index 5ec0855ad13..0a5b050a5d7 100644 --- a/pkg/plugins/manager/sources/source_local_disk_test.go +++ b/pkg/plugins/manager/sources/source_local_disk_test.go @@ -2,6 +2,7 @@ package sources import ( "errors" + "os" "path/filepath" "testing" @@ -110,3 +111,32 @@ func TestDirAsLocalSources(t *testing.T) { }) } } + +func TestLocalSource(t *testing.T) { + t.Run("NewLocalSource should always return plugins with StaticFS", func(t *testing.T) { + tmpDir := t.TempDir() + pluginID := "test-plugin" + pluginDir := filepath.Join(tmpDir, pluginID) + + err := os.MkdirAll(pluginDir, 0750) + require.NoError(t, err) + + pluginJSON := `{ + "id": "test-plugin", + "name": "Test Plugin", + "type": "datasource", + "info": { + "version": "1.0.0" + } + }` + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginJSON), 0644) + require.NoError(t, err) + + bundles, err := NewLocalSource(plugins.ClassExternal, []string{pluginDir}).Discover(t.Context()) + require.NoError(t, err) + require.Len(t, bundles, 1, "Should discover exactly one plugin") + require.Equal(t, pluginID, bundles[0].Primary.JSONData.ID) + _, canRemove := bundles[0].Primary.FS.(plugins.FSRemover) + require.True(t, canRemove) + }) +}