Plugins: Update renderer plugin source (#80643)

* rework renderer plugin source

* add tests

pkg/plugins/manager/loader/loader.go

@@ -75,6 +75,10 @@ func (l *Loader) instrumentLoad(ctx context.Context, src plugins.PluginSource) f
 
 	return func(logger log.Logger, start time.Time) func([]*plugins.Plugin) {
 		return func(plugins []*plugins.Plugin) {
+			if len(plugins) == 0 {
+				logger.Debug("Plugin source loaded, though no plugins were found")
+				return
+			}
 			names := make([]string, len(plugins))
 			for i, p := range plugins {
 				names[i] = p.ID

pkg/plugins/manager/sources/source_local_disk.go

@@ -2,6 +2,10 @@ package sources
 
 import (
 	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"slices"
 
 	"github.com/grafana/grafana/pkg/plugins"
 )
@@ -36,3 +40,31 @@ func (s *LocalSource) DefaultSignature(_ context.Context) (plugins.Signature, bo
 		return plugins.Signature{}, false
 	}
 }
+
+func DirAsLocalSources(pluginsPath string, class plugins.Class) ([]*LocalSource, error) {
+	if pluginsPath == "" {
+		return []*LocalSource{}, errors.New("plugins path not configured")
+	}
+
+	// It's safe to ignore gosec warning G304 since the variable part of the file path comes from a configuration
+	// variable.
+	// nolint:gosec
+	d, err := os.ReadDir(pluginsPath)
+	if err != nil {
+		return []*LocalSource{}, errors.New("failed to open plugins path")
+	}
+
+	var pluginDirs []string
+	for _, dir := range d {
+		if dir.IsDir() || dir.Type()&os.ModeSymlink == os.ModeSymlink {
+			pluginDirs = append(pluginDirs, filepath.Join(pluginsPath, dir.Name()))
+		}
+	}
+	slices.Sort(pluginDirs)
+
+	var sources []*LocalSource
+	for _, dir := range pluginDirs {
+		sources = append(sources, NewLocalSource(class, []string{dir}))
+	}
+	return sources, nil
+}

pkg/plugins/manager/sources/source_local_disk_test.go

@@ -0,0 +1,71 @@
+package sources
+
+import (
+	"errors"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/plugins"
+)
+
+func TestDirAsLocalSources(t *testing.T) {
+	testdataDir := "../testdata"
+
+	tests := []struct {
+		name        string
+		pluginsPath string
+		expected    []*LocalSource
+		err         error
+	}{
+		{
+			name:        "Empty path returns an error",
+			pluginsPath: "",
+			expected:    []*LocalSource{},
+			err:         errors.New("plugins path not configured"),
+		},
+		{
+			name:        "Directory with subdirectories",
+			pluginsPath: filepath.Join(testdataDir, "pluginRootWithDist"),
+			expected: []*LocalSource{
+				{
+					paths: []string{filepath.Join(testdataDir, "pluginRootWithDist", "datasource")},
+					class: plugins.ClassExternal,
+				},
+				{
+					paths: []string{filepath.Join(testdataDir, "pluginRootWithDist", "dist")},
+					class: plugins.ClassExternal,
+				},
+				{
+					paths: []string{filepath.Join(testdataDir, "pluginRootWithDist", "panel")},
+					class: plugins.ClassExternal,
+				},
+			},
+		},
+		{
+			name:        "Directory with no subdirectories",
+			pluginsPath: filepath.Join(testdataDir, "pluginRootWithDist", "datasource"),
+			expected:    nil,
+		},
+		{
+			name:        "Directory with a symlink to a directory",
+			pluginsPath: filepath.Join(testdataDir, "symbolic-plugin-dirs"),
+			expected: []*LocalSource{
+				{
+					paths: []string{filepath.Join(testdataDir, "symbolic-plugin-dirs", "plugin")},
+					class: plugins.ClassExternal,
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := DirAsLocalSources(tt.pluginsPath, plugins.ClassExternal)
+			if tt.err != nil {
+				require.Errorf(t, err, tt.err.Error())
+			}
+			require.Equal(t, tt.expected, got)
+		})
+	}
+}

pkg/plugins/manager/sources/sources.go

@@ -2,9 +2,7 @@ package sources
 
 import (
 	"context"
-	"os"
 	"path/filepath"
-	"slices"
 
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/log"
@@ -34,33 +32,17 @@ func (s *Service) List(_ context.Context) []plugins.PluginSource {
 }
 
 func (s *Service) externalPluginSources() []plugins.PluginSource {
-	var sources []plugins.PluginSource
+	localSrcs, err := DirAsLocalSources(s.cfg.PluginsPath, plugins.ClassExternal)
-	if s.cfg.PluginsPath == "" {
-		return sources
-	}
-
-	pluginsPath := s.cfg.PluginsPath
-	// It's safe to ignore gosec warning G304 since the variable part of the file path comes from a configuration
-	// variable.
-	// nolint:gosec
-	d, err := os.ReadDir(pluginsPath)
 	if err != nil {
-		s.log.Error("Failed to open plugins path", "path", pluginsPath, "error", err)
+		s.log.Error("Failed to load external plugins", "error", err)
-		return sources
+		return []plugins.PluginSource{}
 	}
 
-	var pluginDirs []string
+	var srcs []plugins.PluginSource
-	for _, dir := range d {
+	for _, src := range localSrcs {
-		if dir.IsDir() || dir.Type()&os.ModeSymlink == os.ModeSymlink {
+		srcs = append(srcs, src)
-			pluginDirs = append(pluginDirs, filepath.Join(pluginsPath, dir.Name()))
-		}
 	}
-	slices.Sort(pluginDirs)
+	return srcs
-
-	for _, dir := range pluginDirs {
-		sources = append(sources, NewLocalSource(plugins.ClassExternal, []string{dir}))
-	}
-	return sources
 }
 
 func (s *Service) pluginSettingSources() []plugins.PluginSource {

pkg/services/pluginsintegration/renderer/renderer.go

@@ -19,22 +19,16 @@ import (
 	"github.com/grafana/grafana/pkg/plugins/manager/registry"
 	"github.com/grafana/grafana/pkg/plugins/manager/signature"
 	"github.com/grafana/grafana/pkg/plugins/manager/sources"
-	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/rendering"
 )
 
-func ProvideService(cfg *config.Cfg, registry registry.Service, licensing plugins.Licensing,
+func ProvideService(cfg *config.Cfg, registry registry.Service, licensing plugins.Licensing) (*Manager, error) {
-	features featuremgmt.FeatureToggles) (*Manager, error) {
+	l, err := createLoader(cfg, registry, licensing)
-	l, err := createLoader(cfg, registry, licensing, features)
 	if err != nil {
 		return nil, err
 	}
 
-	return &Manager{
+	return NewManager(cfg, l), nil
-		cfg:    cfg,
-		loader: l,
-		log:    log.New("plugins.renderer"),
-	}, nil
 }
 
 type Manager struct {
@@ -45,6 +39,14 @@ type Manager struct {
 	renderer *Plugin
 }
 
+func NewManager(cfg *config.Cfg, loader loader.Service) *Manager {
+	return &Manager{
+		cfg:    cfg,
+		loader: loader,
+		log:    log.New("renderer.manager"),
+	}
+}
+
 type Plugin struct {
 	plugin *plugins.Plugin
 
@@ -77,22 +79,29 @@ func (m *Manager) Renderer(ctx context.Context) (rendering.Plugin, bool) {
 		return m.renderer, true
 	}
 
-	ps, err := m.loader.Load(ctx, sources.NewLocalSource(plugins.ClassExternal, []string{m.cfg.PluginsPath}))
+	srcs, err := sources.DirAsLocalSources(m.cfg.PluginsPath, plugins.ClassExternal)
 	if err != nil {
-		m.log.Error("Failed to load renderer plugin", "error", err)
+		m.log.Error("Failed to get renderer plugin sources", "error", err)
 		return nil, false
 	}
 
-	if len(ps) >= 1 {
+	for _, src := range srcs {
-		m.renderer = &Plugin{plugin: ps[0]}
+		ps, err := m.loader.Load(ctx, src)
-		return m.renderer, true
+		if err != nil {
+			m.log.Error("Failed to load renderer plugin", "error", err)
+			return nil, false
+		}
+
+		if len(ps) >= 1 {
+			m.renderer = &Plugin{plugin: ps[0]}
+			return m.renderer, true
+		}
 	}
 
 	return nil, false
 }
 
-func createLoader(cfg *config.Cfg, pr registry.Service, l plugins.Licensing,
+func createLoader(cfg *config.Cfg, pr registry.Service, l plugins.Licensing) (loader.Service, error) {
-	features featuremgmt.FeatureToggles) (loader.Service, error) {
 	d := discovery.New(cfg, discovery.Opts{
 		FindFilterFuncs: []discovery.FindFilterFunc{
 			discovery.NewPermittedPluginTypesFilterStep([]plugins.Type{plugins.TypeRenderer}),

pkg/services/pluginsintegration/renderer/renderer_test.go

@@ -0,0 +1,82 @@
+package renderer
+
+import (
+	"context"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/plugins"
+	"github.com/grafana/grafana/pkg/plugins/config"
+	"github.com/grafana/grafana/pkg/plugins/manager/fakes"
+)
+
+func TestRenderer(t *testing.T) {
+	t.Run("Test Renderer will treat directories under plugins path as individual sources", func(t *testing.T) {
+		testdataDir := filepath.Join("testdata", "plugins")
+
+		numLoaded := 0
+		numUnloaded := 0
+		loader := &fakes.FakeLoader{
+			LoadFunc: func(ctx context.Context, src plugins.PluginSource) ([]*plugins.Plugin, error) {
+				require.True(t, src.PluginClass(ctx) == plugins.ClassExternal)
+				require.Len(t, src.PluginURIs(ctx), 1)
+				require.True(t, strings.HasPrefix(src.PluginURIs(ctx)[0], testdataDir))
+
+				numLoaded++
+				return []*plugins.Plugin{}, nil
+			},
+			UnloadFunc: func(_ context.Context, _ *plugins.Plugin) (*plugins.Plugin, error) {
+				numUnloaded++
+				return nil, nil
+			},
+		}
+		cfg := &config.Cfg{
+			PluginsPath: filepath.Join(testdataDir),
+		}
+
+		m := NewManager(cfg, loader)
+
+		r, exists := m.Renderer(context.Background())
+		require.False(t, exists)
+		require.Equal(t, 4, numLoaded)
+		require.Equal(t, 0, numUnloaded)
+		require.Nil(t, r)
+	})
+
+	t.Run("Test Renderer load all directories until a plugin is returned", func(t *testing.T) {
+		testdataDir := filepath.Join("testdata", "plugins")
+
+		numLoaded := 0
+		numUnloaded := 0
+		p := &plugins.Plugin{
+			JSONData: plugins.JSONData{ID: "test"},
+		}
+		loader := &fakes.FakeLoader{
+			LoadFunc: func(ctx context.Context, src plugins.PluginSource) ([]*plugins.Plugin, error) {
+				numLoaded++
+				if strings.HasPrefix(src.PluginURIs(ctx)[0], filepath.Join(testdataDir, "renderer")) {
+					return []*plugins.Plugin{p}, nil
+				}
+				return []*plugins.Plugin{}, nil
+			},
+			UnloadFunc: func(_ context.Context, _ *plugins.Plugin) (*plugins.Plugin, error) {
+				numUnloaded++
+				return nil, nil
+			},
+		}
+		cfg := &config.Cfg{
+			PluginsPath: filepath.Join(testdataDir),
+		}
+
+		m := NewManager(cfg, loader)
+
+		r, exists := m.Renderer(context.Background())
+		require.True(t, exists)
+		require.Equal(t, 3, numLoaded)
+		require.Equal(t, 0, numUnloaded)
+		require.NotNil(t, r)
+	})
+}

pkg/services/pluginsintegration/renderer/testdata/plugins/app/plugin.json

@@ -0,0 +1,3 @@
+{
+  "type": "app"
+}

pkg/services/pluginsintegration/renderer/testdata/plugins/datasource/plugin.json

@@ -0,0 +1,3 @@
+{
+  "type": "datasource"
+}

pkg/services/pluginsintegration/renderer/testdata/plugins/renderer/plugin.json

@@ -0,0 +1,3 @@
+{
+  "type": "renderer"
+}

pkg/services/pluginsintegration/renderer/testdata/plugins/secrets-manager/plugin.json

@@ -0,0 +1,3 @@
+{
+  "type": "secretsmanager"
+}