Plugins: Simplify plugin file removal (#66115)

* make explicit class check when attempting to remove plugin * simplify plugin file tracking * fix test * apply feedback * fix linter
2026-08-26 21:37:31 -05:00 · 2023-04-20 11:52:59 +02:00
parent 6e8b17efd8
commit 739c7f1c68
15 changed files with 193 additions and 281 deletions
@@ -18,7 +18,6 @@ import (
 	"github.com/grafana/grafana/pkg/plugins/manager/process"
 	"github.com/grafana/grafana/pkg/plugins/manager/registry"
 	"github.com/grafana/grafana/pkg/plugins/manager/signature"
-	"github.com/grafana/grafana/pkg/plugins/storage"
 	"github.com/grafana/grafana/pkg/util"
 )

@@ -32,24 +31,23 @@ type Loader struct {
 	pluginInitializer   initializer.Initializer
 	signatureValidator  signature.Validator
 	signatureCalculator plugins.SignatureCalculator
-	pluginStorage       storage.Manager
 	assetPath           *assetpath.Service
 	log                 log.Logger
 	cfg                 *config.Cfg
-	errs                map[string]*plugins.SignatureError
+
+	errs map[string]*plugins.SignatureError
 }

 func ProvideService(cfg *config.Cfg, license plugins.Licensing, authorizer plugins.PluginLoaderAuthorizer,
 	pluginRegistry registry.Service, backendProvider plugins.BackendFactoryProvider, pluginFinder finder.Finder,
 	roleRegistry plugins.RoleRegistry, assetPath *assetpath.Service, signatureCalculator plugins.SignatureCalculator) *Loader {
 	return New(cfg, license, authorizer, pluginRegistry, backendProvider, process.NewManager(pluginRegistry),
-		storage.FileSystem(log.NewPrettyLogger("loader.fs"), cfg.PluginsPath), roleRegistry, assetPath,
-		pluginFinder, signatureCalculator)
+		roleRegistry, assetPath, pluginFinder, signatureCalculator)
 }

 func New(cfg *config.Cfg, license plugins.Licensing, authorizer plugins.PluginLoaderAuthorizer,
 	pluginRegistry registry.Service, backendProvider plugins.BackendFactoryProvider,
-	processManager process.Service, pluginStorage storage.Manager, roleRegistry plugins.RoleRegistry,
+	processManager process.Service, roleRegistry plugins.RoleRegistry,
 	assetPath *assetpath.Service, pluginFinder finder.Finder, signatureCalculator plugins.SignatureCalculator) *Loader {
 	return &Loader{
 		pluginFinder:        pluginFinder,
@@ -58,7 +56,6 @@ func New(cfg *config.Cfg, license plugins.Licensing, authorizer plugins.PluginLo
 		signatureValidator:  signature.NewValidator(authorizer),
 		signatureCalculator: signatureCalculator,
 		processManager:      processManager,
-		pluginStorage:       pluginStorage,
 		errs:                make(map[string]*plugins.SignatureError),
 		log:                 log.New("plugin.loader"),
 		roleRegistry:        roleRegistry,
@@ -201,7 +198,7 @@ func (l *Loader) Unload(ctx context.Context, pluginID string) error {
 		return plugins.ErrPluginNotInstalled
 	}

-	if !plugin.IsExternalPlugin() {
+	if plugin.IsCorePlugin() || plugin.IsBundledPlugin() {
 		return plugins.ErrUninstallCorePlugin
 	}

@@ -220,12 +217,6 @@ func (l *Loader) load(ctx context.Context, p *plugins.Plugin) error {
 		l.log.Info("Plugin registered", "pluginID", p.ID)
 	}

-	if p.IsExternalPlugin() {
-		if err := l.pluginStorage.Register(ctx, p.ID, p.FS.Base()); err != nil {
-			return err
-		}
-	}
-
 	return l.processManager.Start(ctx, p.ID)
 }

@@ -241,9 +232,12 @@ func (l *Loader) unload(ctx context.Context, p *plugins.Plugin) error {
 	}
 	l.log.Debug("Plugin unregistered", "pluginId", p.ID)

-	if err := l.pluginStorage.Remove(ctx, p.ID); err != nil {
-		return err
+	if remover, ok := p.FS.(plugins.FSRemover); ok {
+		if err := remover.Remove(); err != nil {
+			return err
+		}
 	}
+
 	return nil
 }

@@ -457,12 +457,10 @@ func TestLoader_Load(t *testing.T) {
 	}
 	for _, tt := range tests {
 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(tt.cfg, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(tt.cfg, procPrvdr, &fakes.FakeLicensingService{})
 		})
@@ -480,7 +478,7 @@ func TestLoader_Load(t *testing.T) {
 				require.Equal(t, tt.pluginErrors[pluginErr.PluginID], pluginErr)
 			}

-			verifyState(t, tt.want, reg, procPrvdr, storage, procMgr)
+			verifyState(t, tt.want, reg, procPrvdr, procMgr)
 		})
 	}
 }
@@ -698,12 +696,10 @@ func TestLoader_Load_MultiplePlugins(t *testing.T) {

 		for _, tt := range tests {
 			reg := fakes.NewFakePluginRegistry()
-			storage := fakes.NewFakePluginStorage()
 			procPrvdr := fakes.NewFakeBackendProcessProvider()
 			procMgr := fakes.NewFakeProcessManager()
 			l := newLoader(tt.cfg, func(l *Loader) {
 				l.pluginRegistry = reg
-				l.pluginStorage = storage
 				l.processManager = procMgr
 				l.pluginInitializer = initializer.New(tt.cfg, procPrvdr, fakes.NewFakeLicensingService())
 			})
@@ -734,7 +730,7 @@ func TestLoader_Load_MultiplePlugins(t *testing.T) {
 				for _, pluginErr := range pluginErrs {
 					require.Equal(t, tt.pluginErrors[pluginErr.PluginID], pluginErr)
 				}
-				verifyState(t, tt.want, reg, procPrvdr, storage, procMgr)
+				verifyState(t, tt.want, reg, procPrvdr, procMgr)
 			})
 		}
 	})
@@ -824,12 +820,10 @@ func TestLoader_Load_RBACReady(t *testing.T) {
 		})
 		setting.AppUrl = "http://localhost:3000"
 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(tt.cfg, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(tt.cfg, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -850,7 +844,7 @@ func TestLoader_Load_RBACReady(t *testing.T) {
 		pluginErrs := l.PluginErrors()
 		require.Len(t, pluginErrs, 0)

-		verifyState(t, tt.want, reg, procPrvdr, storage, procMgr)
+		verifyState(t, tt.want, reg, procPrvdr, procMgr)
 	}
 }

@@ -908,12 +902,10 @@ func TestLoader_Load_Signature_RootURL(t *testing.T) {
 		}

 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(&config.Cfg{}, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -930,7 +922,7 @@ func TestLoader_Load_Signature_RootURL(t *testing.T) {
 		if !cmp.Equal(got, expected, compareOpts...) {
 			t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 		}
-		verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+		verifyState(t, expected, reg, procPrvdr, procMgr)
 	})
 }

@@ -994,12 +986,10 @@ func TestLoader_Load_DuplicatePlugins(t *testing.T) {
 		}

 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(&config.Cfg{}, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -1017,7 +1007,7 @@ func TestLoader_Load_DuplicatePlugins(t *testing.T) {
 			t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 		}

-		verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+		verifyState(t, expected, reg, procPrvdr, procMgr)
 	})
 }

@@ -1086,7 +1076,6 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) {
 		}

 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		// Cause an initialization error
 		procPrvdr.BackendFactoryFunc = func(ctx context.Context, p *plugins.Plugin) backendplugin.PluginFactoryFunc {
@@ -1100,7 +1089,6 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) {
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(&config.Cfg{}, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -1118,7 +1106,7 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) {
 			t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 		}

-		verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+		verifyState(t, expected, reg, procPrvdr, procMgr)
 	})
 }

@@ -1182,7 +1170,6 @@ func TestLoader_Load_UseAPIForManifestPublicKey(t *testing.T) {
 		}

 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		apiCalled := false
@@ -1210,7 +1197,6 @@ func TestLoader_Load_UseAPIForManifestPublicKey(t *testing.T) {
 		cfg.GrafanaComURL = s.URL
 		l := newLoader(cfg, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(cfg, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -1229,7 +1215,7 @@ func TestLoader_Load_UseAPIForManifestPublicKey(t *testing.T) {
 			t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 		}

-		verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+		verifyState(t, expected, reg, procPrvdr, procMgr)
 	})
 }

@@ -1311,12 +1297,10 @@ func TestLoader_Load_NestedPlugins(t *testing.T) {

 	t.Run("Load nested External plugins", func(t *testing.T) {
 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(&config.Cfg{}, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -1341,7 +1325,7 @@ func TestLoader_Load_NestedPlugins(t *testing.T) {
 			t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 		}

-		verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+		verifyState(t, expected, reg, procPrvdr, procMgr)

 		t.Run("Load will exclude plugins that already exist", func(t *testing.T) {
 			got, err := l.Load(context.Background(), &fakes.FakePluginSource{
@@ -1363,7 +1347,7 @@ func TestLoader_Load_NestedPlugins(t *testing.T) {
 				t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 			}

-			verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+			verifyState(t, expected, reg, procPrvdr, procMgr)
 		})
 	})

@@ -1493,12 +1477,10 @@ func TestLoader_Load_NestedPlugins(t *testing.T) {
 		expected := []*plugins.Plugin{parent, child}

 		reg := fakes.NewFakePluginRegistry()
-		storage := fakes.NewFakePluginStorage()
 		procPrvdr := fakes.NewFakeBackendProcessProvider()
 		procMgr := fakes.NewFakeProcessManager()
 		l := newLoader(&config.Cfg{}, func(l *Loader) {
 			l.pluginRegistry = reg
-			l.pluginStorage = storage
 			l.processManager = procMgr
 			l.pluginInitializer = initializer.New(&config.Cfg{}, procPrvdr, fakes.NewFakeLicensingService())
 		})
@@ -1521,7 +1503,7 @@ func TestLoader_Load_NestedPlugins(t *testing.T) {
 			t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, expected, compareOpts...))
 		}

-		verifyState(t, expected, reg, procPrvdr, storage, procMgr)
+		verifyState(t, expected, reg, procPrvdr, procMgr)
 	})
 }

@@ -1550,8 +1532,8 @@ func Test_setPathsBasedOnApp(t *testing.T) {

 func newLoader(cfg *config.Cfg, cbs ...func(loader *Loader)) *Loader {
 	l := New(cfg, &fakes.FakeLicensingService{}, signature.NewUnsignedAuthorizer(cfg), fakes.NewFakePluginRegistry(),
-		fakes.NewFakeBackendProcessProvider(), fakes.NewFakeProcessManager(), fakes.NewFakePluginStorage(),
-		fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(), signature.ProvideService(cfg))
+		fakes.NewFakeBackendProcessProvider(), fakes.NewFakeProcessManager(), fakes.NewFakeRoleRegistry(),
+		assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(), signature.ProvideService(cfg))

 	for _, cb := range cbs {
 		cb(l)
@@ -1561,7 +1543,7 @@ func newLoader(cfg *config.Cfg, cbs ...func(loader *Loader)) *Loader {
 }

 func verifyState(t *testing.T, ps []*plugins.Plugin, reg *fakes.FakePluginRegistry,
-	procPrvdr *fakes.FakeBackendProcessProvider, storage *fakes.FakePluginStorage, procMngr *fakes.FakeProcessManager) {
+	procPrvdr *fakes.FakeBackendProcessProvider, procMngr *fakes.FakeProcessManager) {
 	t.Helper()

 	for _, p := range ps {
@@ -1577,13 +1559,6 @@ func verifyState(t *testing.T, ps []*plugins.Plugin, reg *fakes.FakePluginRegist
 			require.Zero(t, procPrvdr.Invoked[p.ID])
 		}

-		_, exists := storage.Store[p.ID]
-		if p.IsExternalPlugin() {
-			require.True(t, exists)
-		} else {
-			require.False(t, exists)
-		}
-
 		require.Equal(t, 1, procMngr.Started[p.ID])
 		require.Zero(t, procMngr.Stopped[p.ID])
 	}