Plugins: Make it possible to support multiple plugin versions (#82116)

* first pass

* use version in more places

* add comment

* update installer

* fix wire

* fix tests

* tidy

* simplify changes

* fix in mem

* remove unused step

* fix step dupe logic for child plugins + add tests

pkg/plugins/ifaces.go

@@ -14,7 +14,7 @@ type Installer interface {
 	// Add adds a new plugin.
 	Add(ctx context.Context, pluginID, version string, opts CompatOpts) error
 	// Remove removes an existing plugin.
-	Remove(ctx context.Context, pluginID string) error
+	Remove(ctx context.Context, pluginID, version string) error
 }
 
 type PluginSource interface {
@@ -25,7 +25,7 @@ type PluginSource interface {
 
 type FileStore interface {
 	// File retrieves a plugin file.
-	File(ctx context.Context, pluginID, filename string) (*File, error)
+	File(ctx context.Context, pluginID, pluginVersion, filename string) (*File, error)
 }
 
 type File struct {

pkg/plugins/manager/client/client.go

@@ -44,7 +44,7 @@ func (s *Service) QueryData(ctx context.Context, req *backend.QueryDataRequest)
 		return nil, errNilRequest
 	}
 
-	p, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	p, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return nil, plugins.ErrPluginNotRegistered
 	}
@@ -87,7 +87,7 @@ func (s *Service) CallResource(ctx context.Context, req *backend.CallResourceReq
 		return errNilSender
 	}
 
-	p, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	p, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return plugins.ErrPluginNotRegistered
 	}
@@ -130,7 +130,7 @@ func (s *Service) CollectMetrics(ctx context.Context, req *backend.CollectMetric
 		return nil, errNilRequest
 	}
 
-	p, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	p, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return nil, plugins.ErrPluginNotRegistered
 	}
@@ -152,7 +152,7 @@ func (s *Service) CheckHealth(ctx context.Context, req *backend.CheckHealthReque
 		return nil, errNilRequest
 	}
 
-	p, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	p, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return nil, plugins.ErrPluginNotRegistered
 	}
@@ -182,7 +182,7 @@ func (s *Service) SubscribeStream(ctx context.Context, req *backend.SubscribeStr
 		return nil, errNilRequest
 	}
 
-	plugin, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	plugin, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return nil, plugins.ErrPluginNotRegistered
 	}
@@ -195,7 +195,7 @@ func (s *Service) PublishStream(ctx context.Context, req *backend.PublishStreamR
 		return nil, errNilRequest
 	}
 
-	plugin, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	plugin, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return nil, plugins.ErrPluginNotRegistered
 	}
@@ -212,7 +212,7 @@ func (s *Service) RunStream(ctx context.Context, req *backend.RunStreamRequest,
 		return errNilSender
 	}
 
-	plugin, exists := s.plugin(ctx, req.PluginContext.PluginID)
+	plugin, exists := s.plugin(ctx, req.PluginContext.PluginID, req.PluginContext.PluginVersion)
 	if !exists {
 		return plugins.ErrPluginNotRegistered
 	}
@@ -221,8 +221,8 @@ func (s *Service) RunStream(ctx context.Context, req *backend.RunStreamRequest,
 }
 
 // plugin finds a plugin with `pluginID` from the registry that is not decommissioned
-func (s *Service) plugin(ctx context.Context, pluginID string) (*plugins.Plugin, bool) {
-	p, exists := s.pluginRegistry.Plugin(ctx, pluginID)
+func (s *Service) plugin(ctx context.Context, pluginID, pluginVersion string) (*plugins.Plugin, bool) {
+	p, exists := s.pluginRegistry.Plugin(ctx, pluginID, pluginVersion)
 	if !exists {
 		return nil, false
 	}

pkg/plugins/manager/fakes/fakes.go

@@ -176,7 +176,7 @@ func NewFakePluginRegistry() *FakePluginRegistry {
 	}
 }
 
-func (f *FakePluginRegistry) Plugin(_ context.Context, id string) (*plugins.Plugin, bool) {
+func (f *FakePluginRegistry) Plugin(_ context.Context, id, _ string) (*plugins.Plugin, bool) {
 	p, exists := f.Store[id]
 	return p, exists
 }
@@ -195,7 +195,7 @@ func (f *FakePluginRegistry) Add(_ context.Context, p *plugins.Plugin) error {
 	return nil
 }
 
-func (f *FakePluginRegistry) Remove(_ context.Context, id string) error {
+func (f *FakePluginRegistry) Remove(_ context.Context, id, _ string) error {
 	delete(f.Store, id)
 	return nil
 }
@@ -423,12 +423,12 @@ func (s *FakePluginSource) DefaultSignature(ctx context.Context) (plugins.Signat
 }
 
 type FakePluginFileStore struct {
-	FileFunc func(ctx context.Context, pluginID, filename string) (*plugins.File, error)
+	FileFunc func(ctx context.Context, pluginID, pluginVersion, filename string) (*plugins.File, error)
 }
 
-func (f *FakePluginFileStore) File(ctx context.Context, pluginID, filename string) (*plugins.File, error) {
+func (f *FakePluginFileStore) File(ctx context.Context, pluginID, pluginVersion, filename string) (*plugins.File, error) {
 	if f.FileFunc != nil {
-		return f.FileFunc(ctx, pluginID, filename)
+		return f.FileFunc(ctx, pluginID, pluginVersion, filename)
 	}
 	return nil, nil
 }

pkg/plugins/manager/filestore/fs.go

@@ -21,8 +21,8 @@ func ProvideService(pluginRegistry registry.Service) *Service {
 	}
 }
 
-func (s *Service) File(ctx context.Context, pluginID, filename string) (*plugins.File, error) {
-	if p, exists := s.pluginRegistry.Plugin(ctx, pluginID); exists {
+func (s *Service) File(ctx context.Context, pluginID, pluginVersion, filename string) (*plugins.File, error) {
+	if p, exists := s.pluginRegistry.Plugin(ctx, pluginID, pluginVersion); exists {
 		f, err := p.File(filename)
 		if err != nil {
 			return nil, err

pkg/plugins/manager/installer.go

@@ -55,7 +55,7 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt
 	}
 
 	var pluginArchive *repo.PluginArchive
-	if plugin, exists := m.plugin(ctx, pluginID); exists {
+	if plugin, exists := m.plugin(ctx, pluginID, version); exists {
 		if plugin.IsCorePlugin() || plugin.IsBundledPlugin() {
 			return plugins.ErrInstallCorePlugin
 		}
@@ -84,7 +84,7 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt
 		}
 
 		// remove existing installation of plugin
-		err = m.Remove(ctx, plugin.ID)
+		err = m.Remove(ctx, plugin.ID, plugin.Info.Version)
 		if err != nil {
 			return err
 		}
@@ -139,8 +139,8 @@ func (m *PluginInstaller) Add(ctx context.Context, pluginID, version string, opt
 	return nil
 }
 
-func (m *PluginInstaller) Remove(ctx context.Context, pluginID string) error {
-	plugin, exists := m.plugin(ctx, pluginID)
+func (m *PluginInstaller) Remove(ctx context.Context, pluginID, version string) error {
+	plugin, exists := m.plugin(ctx, pluginID, version)
 	if !exists {
 		return plugins.ErrPluginNotInstalled
 	}
@@ -168,8 +168,8 @@ func (m *PluginInstaller) Remove(ctx context.Context, pluginID string) error {
 }
 
 // plugin finds a plugin with `pluginID` from the store
-func (m *PluginInstaller) plugin(ctx context.Context, pluginID string) (*plugins.Plugin, bool) {
-	p, exists := m.pluginRegistry.Plugin(ctx, pluginID)
+func (m *PluginInstaller) plugin(ctx context.Context, pluginID, pluginVersion string) (*plugins.Plugin, bool) {
+	p, exists := m.pluginRegistry.Plugin(ctx, pluginID, pluginVersion)
 	if !exists {
 		return nil, false
 	}

pkg/plugins/manager/installer_test.go

@@ -23,6 +23,8 @@ func TestPluginManager_Add_Remove(t *testing.T) {
 		const (
 			pluginID, v1 = "test-panel", "1.0.0"
 			zipNameV1    = "test-panel-1.0.0.zip"
+			v2           = "2.0.0"
+			zipNameV2    = "test-panel-2.0.0.zip"
 		)
 
 		// mock a plugin to be returned automatically by the plugin loader
@@ -83,10 +85,6 @@ func TestPluginManager_Add_Remove(t *testing.T) {
 		})
 
 		t.Run("Update plugin to different version", func(t *testing.T) {
-			const (
-				v2        = "2.0.0"
-				zipNameV2 = "test-panel-2.0.0.zip"
-			)
 			// mock a plugin to be returned automatically by the plugin loader
 			pluginV2 := createPlugin(t, pluginID, plugins.ClassExternal, true, true, func(plugin *plugins.Plugin) {
 				plugin.Info.Version = v2
@@ -138,7 +136,7 @@ func TestPluginManager_Add_Remove(t *testing.T) {
 				},
 			}
 
-			err = inst.Remove(context.Background(), pluginID)
+			err = inst.Remove(context.Background(), pluginID, v2)
 			require.NoError(t, err)
 
 			require.Equal(t, []string{pluginID}, unloadedPlugins)
@@ -146,7 +144,7 @@ func TestPluginManager_Add_Remove(t *testing.T) {
 			t.Run("Won't remove if not exists", func(t *testing.T) {
 				inst.pluginRegistry = fakes.NewFakePluginRegistry()
 
-				err = inst.Remove(context.Background(), pluginID)
+				err = inst.Remove(context.Background(), pluginID, v2)
 				require.Equal(t, plugins.ErrPluginNotInstalled, err)
 			})
 		})
@@ -179,7 +177,7 @@ func TestPluginManager_Add_Remove(t *testing.T) {
 			require.Equal(t, plugins.ErrInstallCorePlugin, err)
 
 			t.Run(fmt.Sprintf("Can't uninstall %s plugin", tc.class), func(t *testing.T) {
-				err = pm.Remove(context.Background(), p.ID)
+				err = pm.Remove(context.Background(), p.ID, p.Info.Version)
 				require.Equal(t, plugins.ErrUninstallCorePlugin, err)
 			})
 		}

pkg/plugins/manager/pipeline/discovery/steps.go

@@ -6,9 +6,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/config"
-	"github.com/grafana/grafana/pkg/plugins/log"
 	"github.com/grafana/grafana/pkg/plugins/manager/loader/finder"
-	"github.com/grafana/grafana/pkg/plugins/manager/registry"
 )
 
 // DefaultFindFunc is the default function used for the Find step of the Discovery stage. It will scan the local
@@ -17,44 +15,6 @@ func DefaultFindFunc(cfg *config.Cfg) FindFunc {
 	return finder.NewLocalFinder(cfg.DevMode, cfg.Features).Find
 }
 
-// DuplicatePluginValidation is a filter step that will filter out any plugins that are already registered with the
-// registry. This includes both the primary plugin and any child plugins, which are matched using the plugin ID field.
-type DuplicatePluginValidation struct {
-	registry registry.Service
-	log      log.Logger
-}
-
-// NewDuplicatePluginFilterStep returns a new DuplicatePluginValidation.
-func NewDuplicatePluginFilterStep(registry registry.Service) *DuplicatePluginValidation {
-	return &DuplicatePluginValidation{
-		registry: registry,
-		log:      log.New("plugins.dedupe"),
-	}
-}
-
-// Filter will filter out any plugins that are already registered with the registry.
-func (d *DuplicatePluginValidation) Filter(ctx context.Context, bundles []*plugins.FoundBundle) ([]*plugins.FoundBundle, error) {
-	res := make([]*plugins.FoundBundle, 0, len(bundles))
-	for _, b := range bundles {
-		_, exists := d.registry.Plugin(ctx, b.Primary.JSONData.ID)
-		if exists {
-			d.log.Warn("Skipping loading of plugin as it's a duplicate", "pluginId", b.Primary.JSONData.ID)
-			continue
-		}
-
-		for _, child := range b.Children {
-			_, exists = d.registry.Plugin(ctx, child.JSONData.ID)
-			if exists {
-				d.log.Warn("Skipping loading of child plugin as it's a duplicate", "pluginId", child.JSONData.ID)
-				continue
-			}
-		}
-		res = append(res, b)
-	}
-
-	return res, nil
-}
-
 // PermittedPluginTypesFilter is a filter step that will filter out any plugins that are not of a permitted type.
 type PermittedPluginTypesFilter struct {
 	permittedTypes []plugins.Type

pkg/plugins/manager/pipeline/termination/steps.go

@@ -54,7 +54,7 @@ func newDeregister(pluginRegistry registry.Service) *Deregister {
 
 // Deregister removes a plugin from the plugin registry.
 func (d *Deregister) Deregister(ctx context.Context, p *plugins.Plugin) error {
-	if err := d.pluginRegistry.Remove(ctx, p.ID); err != nil {
+	if err := d.pluginRegistry.Remove(ctx, p.ID, p.Info.Version); err != nil {
 		return err
 	}
 	d.log.Debug("Plugin unregistered", "pluginId", p.ID)

pkg/plugins/manager/registry/ifaces.go

@@ -8,12 +8,12 @@ import (
 
 // Service is responsible for the internal storing and retrieval of plugins.
 type Service interface {
-	// Plugin finds a plugin by its ID.
-	Plugin(ctx context.Context, id string) (*plugins.Plugin, bool)
+	// Plugin finds a plugin by its ID and version.
+	Plugin(ctx context.Context, id, version string) (*plugins.Plugin, bool)
 	// Plugins returns all plugins.
 	Plugins(ctx context.Context) []*plugins.Plugin
 	// Add adds the provided plugin to the registry.
 	Add(ctx context.Context, plugin *plugins.Plugin) error
 	// Remove deletes the requested plugin from the registry.
-	Remove(ctx context.Context, id string) error
+	Remove(ctx context.Context, id, version string) error
 }

pkg/plugins/manager/registry/in_memory.go

@@ -8,6 +8,7 @@ import (
 	"github.com/grafana/grafana/pkg/plugins"
 )
 
+// InMemory is a registry that only allows a single version of a plugin to be registered at a time.
 type InMemory struct {
 	store map[string]*plugins.Plugin
 	alias map[string]*plugins.Plugin
@@ -25,7 +26,7 @@ func NewInMemory() *InMemory {
 	}
 }
 
-func (i *InMemory) Plugin(_ context.Context, pluginID string) (*plugins.Plugin, bool) {
+func (i *InMemory) Plugin(_ context.Context, pluginID, _ string) (*plugins.Plugin, bool) {
 	return i.plugin(pluginID)
 }
 
@@ -56,7 +57,7 @@ func (i *InMemory) Add(_ context.Context, p *plugins.Plugin) error {
 	return nil
 }
 
-func (i *InMemory) Remove(_ context.Context, pluginID string) error {
+func (i *InMemory) Remove(_ context.Context, pluginID, _ string) error {
 	p, ok := i.plugin(pluginID)
 	if !ok {
 		return fmt.Errorf("plugin %s is not registered", pluginID)

pkg/plugins/manager/registry/in_memory_test.go

@@ -11,29 +11,41 @@ import (
 	"github.com/grafana/grafana/pkg/plugins"
 )
 
-const pluginID = "test-ds"
+const (
+	pluginID = "test-ds"
+	v1       = "1.0.0"
+	v2       = "2.0.0"
+)
 
 func TestInMemory(t *testing.T) {
 	t.Run("Test mix of registry operations", func(t *testing.T) {
 		i := NewInMemory()
 		ctx := context.Background()
 
-		p, exists := i.Plugin(ctx, pluginID)
+		p, exists := i.Plugin(ctx, pluginID, v1)
 		require.False(t, exists)
 		require.Nil(t, p)
 
-		err := i.Remove(ctx, pluginID)
+		err := i.Remove(ctx, pluginID, v1)
 		require.EqualError(t, err, fmt.Errorf("plugin %s is not registered", pluginID).Error())
 
+		pv1 := &plugins.Plugin{JSONData: plugins.JSONData{ID: pluginID, Info: plugins.Info{Version: v1}}}
+		err = i.Add(ctx, pv1)
+		require.NoError(t, err)
+
+		pv2 := &plugins.Plugin{JSONData: plugins.JSONData{ID: pluginID, Info: plugins.Info{Version: v2}}}
+		err = i.Add(ctx, pv2)
+		require.Errorf(t, err, fmt.Sprintf("plugin %s is already registered", pluginID))
+
+		existingP, exists := i.Plugin(ctx, pluginID, v1)
+		require.True(t, exists)
+		require.Equal(t, pv1, existingP)
+
 		p = &plugins.Plugin{JSONData: plugins.JSONData{ID: pluginID}}
 		err = i.Add(ctx, p)
-		require.NoError(t, err)
+		require.Errorf(t, err, fmt.Sprintf("plugin %s is already registered", pluginID))
 
-		existingP, exists := i.Plugin(ctx, pluginID)
-		require.True(t, exists)
-		require.Equal(t, p, existingP)
-
-		err = i.Remove(ctx, pluginID)
+		err = i.Remove(ctx, pluginID, v1)
 		require.NoError(t, err)
 
 		existingPlugins := i.Plugins(ctx)
@@ -87,6 +99,28 @@ func TestInMemory_Add(t *testing.T) {
 			},
 			err: fmt.Errorf("plugin %s is already registered", pluginID),
 		},
+		{
+			name: "Cannot add a plugin to the registry even if it has a different version",
+			mocks: mocks{
+				store: map[string]*plugins.Plugin{
+					pluginID: {
+						JSONData: plugins.JSONData{
+							ID:   pluginID,
+							Info: plugins.Info{Version: v1},
+						},
+					},
+				},
+			},
+			args: args{
+				p: &plugins.Plugin{
+					JSONData: plugins.JSONData{
+						ID:   pluginID,
+						Info: plugins.Info{Version: v2},
+					},
+				},
+			},
+			err: fmt.Errorf("plugin %s is already registered", pluginID),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -151,7 +185,7 @@ func TestInMemory_Plugin(t *testing.T) {
 			i := &InMemory{
 				store: tt.mocks.store,
 			}
-			p, exists := i.Plugin(context.Background(), tt.args.pluginID)
+			p, exists := i.Plugin(context.Background(), tt.args.pluginID, "")
 			if exists != tt.exists {
 				t.Errorf("Plugin() got1 = %v, expected %v", exists, tt.exists)
 			}
@@ -265,7 +299,7 @@ func TestInMemory_Remove(t *testing.T) {
 			i := &InMemory{
 				store: tt.mocks.store,
 			}
-			err := i.Remove(context.Background(), tt.args.pluginID)
+			err := i.Remove(context.Background(), tt.args.pluginID, "")
 			require.Equal(t, tt.err, err)
 		})
 	}
@@ -280,7 +314,7 @@ func TestAliasSupport(t *testing.T) {
 		pluginIdOld := "plugin-old"
 		pluginIdOld2 := "plugin-old2"
 
-		p, exists := i.Plugin(ctx, pluginIdNew)
+		p, exists := i.Plugin(ctx, pluginIdNew, "")
 		require.False(t, exists)
 		require.Nil(t, p)
 
@@ -294,17 +328,17 @@ func TestAliasSupport(t *testing.T) {
 		require.NoError(t, err)
 
 		// Can lookup by the new ID
-		found, exists := i.Plugin(ctx, pluginIdNew)
+		found, exists := i.Plugin(ctx, pluginIdNew, "")
 		require.True(t, exists)
 		require.Equal(t, pluginNew, found)
 
 		// Can lookup by the old ID
-		found, exists = i.Plugin(ctx, pluginIdOld)
+		found, exists = i.Plugin(ctx, pluginIdOld, "")
 		require.True(t, exists)
 		require.Equal(t, pluginNew, found)
 
 		// Can lookup by the other old ID
-		found, exists = i.Plugin(ctx, pluginIdOld2)
+		found, exists = i.Plugin(ctx, pluginIdOld2, "")
 		require.True(t, exists)
 		require.Equal(t, pluginNew, found)
 
@@ -313,7 +347,7 @@ func TestAliasSupport(t *testing.T) {
 			ID: pluginIdOld,
 		}}
 		require.NoError(t, i.Add(ctx, pluginOld))
-		found, exists = i.Plugin(ctx, pluginIdOld)
+		found, exists = i.Plugin(ctx, pluginIdOld, "")
 		require.True(t, exists)
 		require.Equal(t, pluginOld, found)
 	})