Plugins: Fix loading of dist folders (#80015)

* end to end

* tidy

* fix whitespace

* remove unused code

* fix linter

* fix gosec + add sort

* fix test

* apply cr feedback

pkg/plugins/manager/loader/finder/local_test.go

@@ -354,27 +354,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) {
 		require.Empty(t, paths)
 	})
 
-	t.Run("should forward if the dist folder should be evaluated", func(t *testing.T) {
-		origWalk := walk
-		walk = func(path string, followSymlinks, detectSymlinkInfiniteLoop, followDistFolder bool, walkFn util.WalkFunc) error {
-			if followDistFolder {
-				return walkFn(path, nil, errors.New("unexpected followDistFolder"))
-			}
-			return walkFn(path, nil, filepath.SkipDir)
-		}
-		t.Cleanup(func() {
-			walk = origWalk
-		})
-
-		finder := NewLocalFinder(false, featuremgmt.WithFeatures())
-		paths, err := finder.getAbsPluginJSONPaths("test", false)
-		require.ErrorIs(t, err, filepath.SkipDir)
-		require.Empty(t, paths)
-	})
-}
-
-func TestFinder_getAbsPluginJSONPaths_PluginClass(t *testing.T) {
-	t.Run("When a dist folder exists as a direct child of the plugins path, it will always be resolved", func(t *testing.T) {
+	t.Run("The followDistFolder state controls whether certain folders are followed", func(t *testing.T) {
 		dir, err := filepath.Abs("../../testdata/pluginRootWithDist")
 		require.NoError(t, err)
 
@@ -384,20 +364,17 @@ func TestFinder_getAbsPluginJSONPaths_PluginClass(t *testing.T) {
 			expected   []string
 		}{
 			{
-				name:       "When followDistFolder is enabled, a nested dist folder will also be resolved",
+				name:       "When followDistFolder is enabled, only the nested dist folder will be followed",
 				followDist: true,
 				expected: []string{
-					filepath.Join(dir, "datasource/plugin.json"),
 					filepath.Join(dir, "dist/plugin.json"),
-					filepath.Join(dir, "panel/dist/plugin.json"),
 				},
 			},
 			{
-				name:       "When followDistFolder is disabled, a nested dist folder will not be resolved",
+				name:       "When followDistFolder is disabled, no dist folders will be followed",
 				followDist: false,
 				expected: []string{
 					filepath.Join(dir, "datasource/plugin.json"),
-					filepath.Join(dir, "dist/plugin.json"),
 					filepath.Join(dir, "panel/src/plugin.json"),
 				},
 			},

pkg/plugins/manager/sources/sources.go

@@ -2,34 +2,77 @@ package sources
 
 import (
 	"context"
+	"os"
 	"path/filepath"
+	"slices"
 
 	"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/setting"
 )
 
 type Service struct {
-	gCfg *setting.Cfg
-	cfg  *config.Cfg
-	log  log.Logger
+	cfg *setting.Cfg
+	log log.Logger
 }
 
-func ProvideService(gCfg *setting.Cfg, cfg *config.Cfg) *Service {
+func ProvideService(cfg *setting.Cfg) *Service {
 	return &Service{
-		gCfg: gCfg,
-		cfg:  cfg,
-		log:  log.New("plugin.sources"),
+		cfg: cfg,
+		log: log.New("plugin.sources"),
 	}
 }
 
 func (s *Service) List(_ context.Context) []plugins.PluginSource {
-	return []plugins.PluginSource{
-		NewLocalSource(plugins.ClassCore, corePluginPaths(s.gCfg.StaticRootPath)),
-		NewLocalSource(plugins.ClassBundled, []string{s.gCfg.BundledPluginsPath}),
-		NewLocalSource(plugins.ClassExternal, append([]string{s.cfg.PluginsPath}, pluginFSPaths(s.cfg.PluginSettings)...)),
+	r := []plugins.PluginSource{
+		NewLocalSource(plugins.ClassCore, corePluginPaths(s.cfg.StaticRootPath)),
+		NewLocalSource(plugins.ClassBundled, []string{s.cfg.BundledPluginsPath}),
 	}
+	r = append(r, s.externalPluginSources()...)
+	r = append(r, s.pluginSettingSources()...)
+	return r
+}
+
+func (s *Service) externalPluginSources() []plugins.PluginSource {
+	var sources []plugins.PluginSource
+	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)
+		return sources
+	}
+
+	var pluginDirs []string
+	for _, dir := range d {
+		if dir.IsDir() {
+			pluginDirs = append(pluginDirs, filepath.Join(pluginsPath, dir.Name()))
+		}
+	}
+	slices.Sort(pluginDirs)
+
+	for _, dir := range pluginDirs {
+		sources = append(sources, NewLocalSource(plugins.ClassExternal, []string{dir}))
+	}
+	return sources
+}
+
+func (s *Service) pluginSettingSources() []plugins.PluginSource {
+	var sources []plugins.PluginSource
+	for _, ps := range s.cfg.PluginSettings {
+		path, exists := ps["path"]
+		if !exists || path == "" {
+			continue
+		}
+		sources = append(sources, NewLocalSource(plugins.ClassExternal, []string{path}))
+	}
+	return sources
 }
 
 // corePluginPaths provides a list of the Core plugin file system paths
@@ -38,16 +81,3 @@ func corePluginPaths(staticRootPath string) []string {
 	panelsPath := filepath.Join(staticRootPath, "app/plugins/panel")
 	return []string{datasourcePaths, panelsPath}
 }
-
-// pluginSettingPaths provides plugin file system paths defined in cfg.PluginSettings
-func pluginFSPaths(ps map[string]map[string]string) []string {
-	var pluginSettingDirs []string
-	for _, s := range ps {
-		path, exists := s["path"]
-		if !exists || path == "" {
-			continue
-		}
-		pluginSettingDirs = append(pluginSettingDirs, path)
-	}
-	return pluginSettingDirs
-}

pkg/plugins/manager/sources/sources_test.go

@@ -8,20 +8,21 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/plugins"
-	"github.com/grafana/grafana/pkg/plugins/config"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
 func TestSources_List(t *testing.T) {
 	t.Run("Plugin sources are populated by default and listed in specific order", func(t *testing.T) {
+		testdata, err := filepath.Abs("../testdata")
+		require.NoError(t, err)
+
 		cfg := &setting.Cfg{
-			BundledPluginsPath: "path1",
-		}
-		pCfg := &config.Cfg{
-			PluginsPath: "path2",
+			StaticRootPath:     testdata,
+			PluginsPath:        filepath.Join(testdata, "pluginRootWithDist"),
+			BundledPluginsPath: filepath.Join(testdata, "unsigned-panel"),
 			PluginSettings: setting.PluginSettings{
 				"foo": map[string]string{
-					"path": "path3",
+					"path": filepath.Join(testdata, "test-app"),
 				},
 				"bar": map[string]string{
 					"url": "https://grafana.plugin",
@@ -29,15 +30,18 @@ func TestSources_List(t *testing.T) {
 			},
 		}
 
-		s := ProvideService(cfg, pCfg)
+		s := ProvideService(cfg)
 		srcs := s.List(context.Background())
 
 		ctx := context.Background()
 
-		require.Len(t, srcs, 3)
+		require.Len(t, srcs, 6)
 
 		require.Equal(t, srcs[0].PluginClass(ctx), plugins.ClassCore)
-		require.Equal(t, srcs[0].PluginURIs(ctx), []string{filepath.Join("app", "plugins", "datasource"), filepath.Join("app", "plugins", "panel")})
+		require.Equal(t, srcs[0].PluginURIs(ctx), []string{
+			filepath.Join(testdata, "app", "plugins", "datasource"),
+			filepath.Join(testdata, "app", "plugins", "panel"),
+		})
 		sig, exists := srcs[0].DefaultSignature(ctx)
 		require.True(t, exists)
 		require.Equal(t, plugins.SignatureStatusInternal, sig.Status)
@@ -45,15 +49,33 @@ func TestSources_List(t *testing.T) {
 		require.Equal(t, "", sig.SigningOrg)
 
 		require.Equal(t, srcs[1].PluginClass(ctx), plugins.ClassBundled)
-		require.Equal(t, srcs[1].PluginURIs(ctx), []string{"path1"})
+		require.Equal(t, srcs[1].PluginURIs(ctx), []string{filepath.Join(testdata, "unsigned-panel")})
 		sig, exists = srcs[1].DefaultSignature(ctx)
 		require.False(t, exists)
 		require.Equal(t, plugins.Signature{}, sig)
 
 		require.Equal(t, srcs[2].PluginClass(ctx), plugins.ClassExternal)
-		require.Equal(t, srcs[2].PluginURIs(ctx), []string{"path2", "path3"})
+		require.Equal(t, srcs[2].PluginURIs(ctx), []string{
+			filepath.Join(testdata, "pluginRootWithDist", "datasource"),
+		})
 		sig, exists = srcs[2].DefaultSignature(ctx)
 		require.False(t, exists)
 		require.Equal(t, plugins.Signature{}, sig)
+
+		require.Equal(t, srcs[3].PluginClass(ctx), plugins.ClassExternal)
+		require.Equal(t, srcs[3].PluginURIs(ctx), []string{
+			filepath.Join(testdata, "pluginRootWithDist", "dist"),
+		})
+		sig, exists = srcs[3].DefaultSignature(ctx)
+		require.False(t, exists)
+		require.Equal(t, plugins.Signature{}, sig)
+
+		require.Equal(t, srcs[4].PluginClass(ctx), plugins.ClassExternal)
+		require.Equal(t, srcs[4].PluginURIs(ctx), []string{
+			filepath.Join(testdata, "pluginRootWithDist", "panel"),
+		})
+		sig, exists = srcs[4].DefaultSignature(ctx)
+		require.False(t, exists)
+		require.Equal(t, plugins.Signature{}, sig)
 	})
 }

pkg/services/pluginsintegration/plugins_integration_test.go

@@ -51,30 +51,20 @@ func TestIntegrationPluginManager(t *testing.T) {
 	bundledPluginsPath, err := filepath.Abs("../../../plugins-bundled/internal")
 	require.NoError(t, err)
 
-	// We use the raw config here as it forms the basis for the setting.Provider implementation
-	// The plugin manager also relies directly on the setting.Cfg struct to provide Grafana specific
-	// properties such as the loading paths
-	raw, err := ini.Load([]byte(`
-		app_mode = production
-
-		[plugin.test-app]
-		path=../../plugins/manager/testdata/test-app
-
-		[plugin.test-panel]
-		not=included
-		`),
-	)
-	require.NoError(t, err)
-
 	features := featuremgmt.WithFeatures()
 	cfg := &setting.Cfg{
-		Raw:                raw,
+		Raw:                ini.Empty(),
 		StaticRootPath:     staticRootPath,
 		BundledPluginsPath: bundledPluginsPath,
 		Azure:              &azsettings.AzureSettings{},
-
-		// nolint:staticcheck
-		IsFeatureToggleEnabled: features.IsEnabledGlobally,
+		PluginSettings: map[string]map[string]string{
+			"test-app": {
+				"path": "../../plugins/manager/testdata/test-app",
+			},
+			"test-panel": {
+				"not": "included",
+			},
+		},
 	}
 
 	tracer := tracing.InitializeTracerForTest()

pkg/services/pluginsintegration/test_helper.go

@@ -66,7 +66,7 @@ func CreateIntegrationTestCtx(t *testing.T, cfg *setting.Cfg, coreRegistry *core
 		Terminator:   term,
 	})
 
-	ps, err := pluginstore.ProvideService(reg, sources.ProvideService(cfg, pCfg), l)
+	ps, err := pluginstore.ProvideService(reg, sources.ProvideService(cfg), l)
 	require.NoError(t, err)
 
 	return &IntegrationTestCtx{

pkg/util/filepath.go

@@ -27,7 +27,7 @@ func newWalker(rootDir string) *walker {
 // it calls the walkFn passed.
 //
 // It is similar to filepath.Walk, except that it supports symbolic links and
-// can detect infinite loops while following sym links.
+// can detect infinite loops while following symlinks.
 // It solves the issue where your WalkFunc needs a path relative to the symbolic link
 // (resolving links within walkfunc loses the path to the symbolic link for each traversal).
 func Walk(path string, followSymlinks bool, detectSymlinkInfiniteLoop bool, followDistFolder bool, walkFn WalkFunc) error {
@@ -112,20 +112,22 @@ func (w *walker) walk(path string, info os.FileInfo, resolvedPath string, symlin
 			subFiles = append(subFiles, subFile{path: path2, resolvedPath: resolvedPath2, fileInfo: fileInfo})
 		}
 
-		// If we have found a dist directory in a subdirectory (IE not at root path), and followDistFolder is true,
-		// then we want to follow only the dist directory and ignore all other subdirectories.
-		atRootDir := w.rootDir == path
-		if followDistFolder && w.containsDistFolder(subFiles) && !atRootDir {
-			return w.walk(filepath.Join(path, "dist"), info, filepath.Join(resolvedPath, "dist"), symlinkPathsFollowed,
-				followDistFolder, walkFn)
+		if followDistFolder && w.containsDistFolder(subFiles) {
+			err := w.walk(
+				filepath.Join(path, "dist"),
+				info,
+				filepath.Join(resolvedPath, "dist"),
+				symlinkPathsFollowed,
+				followDistFolder,
+				walkFn)
+			if err != nil {
+				return err
+			}
 		} else {
-			// Follow all subdirectories, with special handling for dist directories.
 			for _, p := range subFiles {
-				// We only want to skip a dist directory if it is not in the root directory, and followDistFolder is false.
-				if p.isDistDir() && !atRootDir && !followDistFolder {
+				if p.isDistDir() && !followDistFolder {
 					continue
 				}
-
 				err = w.walk(p.path, p.fileInfo, p.resolvedPath, symlinkPathsFollowed, followDistFolder, walkFn)
 				if err != nil {
 					return err