From 20f3a87bf541794be471c60d9060fb62c89bc825 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Tue, 21 Nov 2023 11:51:13 +0100 Subject: [PATCH] Bug: Fix loading behavior with FlagExternalCorePlugins (#78388) --- pkg/cmd/grafana-cli/services/services.go | 3 +- pkg/plugins/manager/loader/finder/local.go | 10 ++-- .../manager/loader/finder/local_test.go | 49 ++++++++++++++++--- .../manager/pipeline/discovery/steps.go | 2 +- .../plugin-with-dist/plugin/dist/MANIFEST.txt | 28 +++++++++++ .../plugin-with-dist/plugin/dist/plugin.json | 16 ++++++ .../plugin-with-dist/plugin/plugin.json | 16 ++++++ pkg/plugins/pfs/pfs_test.go | 4 ++ .../pluginsintegration/loader/loader_test.go | 6 ++- .../pluginsintegration/test_helper.go | 4 +- 10 files changed, 123 insertions(+), 15 deletions(-) create mode 100644 pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/MANIFEST.txt create mode 100644 pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/plugin.json create mode 100644 pkg/plugins/manager/testdata/plugin-with-dist/plugin/plugin.json diff --git a/pkg/cmd/grafana-cli/services/services.go b/pkg/cmd/grafana-cli/services/services.go index 00fffc487e2..53bebf8f552 100644 --- a/pkg/cmd/grafana-cli/services/services.go +++ b/pkg/cmd/grafana-cli/services/services.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/manager/loader/finder" "github.com/grafana/grafana/pkg/plugins/manager/sources" + "github.com/grafana/grafana/pkg/services/featuremgmt" ) var ( @@ -77,7 +78,7 @@ func GetLocalPlugin(pluginDir, pluginID string) (plugins.FoundPlugin, error) { } func GetLocalPlugins(pluginDir string) []*plugins.FoundBundle { - f := finder.NewLocalFinder(true) + f := finder.NewLocalFinder(true, featuremgmt.WithFeatures()) res, err := f.Find(context.Background(), sources.NewLocalSource(plugins.ClassExternal, []string{pluginDir})) if err != nil { diff --git a/pkg/plugins/manager/loader/finder/local.go b/pkg/plugins/manager/loader/finder/local.go index 9428a4bac2b..bfb72a5ca6a 100644 --- a/pkg/plugins/manager/loader/finder/local.go +++ b/pkg/plugins/manager/loader/finder/local.go @@ -14,6 +14,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/services/featuremgmt" "github.com/grafana/grafana/pkg/util" ) @@ -26,17 +27,19 @@ var ( type Local struct { log log.Logger production bool + features plugins.FeatureToggles } -func NewLocalFinder(devMode bool) *Local { +func NewLocalFinder(devMode bool, features plugins.FeatureToggles) *Local { return &Local{ production: !devMode, log: log.New("local.finder"), + features: features, } } func ProvideLocalFinder(cfg *config.Cfg) *Local { - return NewLocalFinder(cfg.DevMode) + return NewLocalFinder(cfg.DevMode, cfg.Features) } func (l *Local) Find(ctx context.Context, src plugins.PluginSource) ([]*plugins.FoundBundle, error) { @@ -58,7 +61,8 @@ func (l *Local) Find(ctx context.Context, src plugins.PluginSource) ([]*plugins. } followDistFolder := true - if src.PluginClass(ctx) == plugins.ClassCore { + if src.PluginClass(ctx) == plugins.ClassCore && + !l.features.IsEnabledGlobally(featuremgmt.FlagExternalCorePlugins) { followDistFolder = false } paths, err := l.getAbsPluginJSONPaths(path, followDistFolder) diff --git a/pkg/plugins/manager/loader/finder/local_test.go b/pkg/plugins/manager/loader/finder/local_test.go index 0e3c5a8bcfb..d7da23e0e15 100644 --- a/pkg/plugins/manager/loader/finder/local_test.go +++ b/pkg/plugins/manager/loader/finder/local_test.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/manager/fakes" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/util" ) @@ -25,6 +26,7 @@ func TestFinder_Find(t *testing.T) { testCases := []struct { name string pluginDirs []string + pluginClass plugins.Class expectedBundles []*plugins.FoundBundle err error }{ @@ -245,11 +247,46 @@ func TestFinder_Find(t *testing.T) { }, }, }, + { + name: "Plugin with dist folder (core class)", + pluginDirs: []string{filepath.Join(testData, "plugin-with-dist")}, + pluginClass: plugins.ClassCore, + expectedBundles: []*plugins.FoundBundle{ + { + Primary: plugins.FoundPlugin{ + JSONData: plugins.JSONData{ + ID: "test-datasource", + Type: plugins.TypeDataSource, + Name: "Test", + Info: plugins.Info{ + Author: plugins.InfoLink{ + Name: "Will Browne", + URL: "https://willbrowne.com", + }, + Description: "Test", + Version: "1.0.0", + }, + Dependencies: plugins.Dependencies{ + GrafanaVersion: "*", + Plugins: []plugins.Dependency{}, + }, + State: plugins.ReleaseStateAlpha, + Backend: true, + Executable: "test", + }, + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "plugin-with-dist/plugin/dist")), + }, + }, + }, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - f := NewLocalFinder(false) + f := NewLocalFinder(false, featuremgmt.WithFeatures(featuremgmt.FlagExternalCorePlugins)) pluginBundles, err := f.Find(context.Background(), &fakes.FakePluginSource{ + PluginClassFunc: func(ctx context.Context) plugins.Class { + return tc.pluginClass + }, PluginURIsFunc: func(ctx context.Context) []string { return tc.pluginDirs }, @@ -281,7 +318,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(false) + finder := NewLocalFinder(false, featuremgmt.WithFeatures()) paths, err := finder.getAbsPluginJSONPaths("test", true) require.NoError(t, err) require.Empty(t, paths) @@ -296,7 +333,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(false) + finder := NewLocalFinder(false, featuremgmt.WithFeatures()) paths, err := finder.getAbsPluginJSONPaths("test", true) require.NoError(t, err) require.Empty(t, paths) @@ -311,7 +348,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(false) + finder := NewLocalFinder(false, featuremgmt.WithFeatures()) paths, err := finder.getAbsPluginJSONPaths("test", true) require.Error(t, err) require.Empty(t, paths) @@ -329,7 +366,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder(false) + finder := NewLocalFinder(false, featuremgmt.WithFeatures()) paths, err := finder.getAbsPluginJSONPaths("test", false) require.ErrorIs(t, err, filepath.SkipDir) require.Empty(t, paths) @@ -366,7 +403,7 @@ func TestFinder_getAbsPluginJSONPaths_PluginClass(t *testing.T) { }, } for _, tc := range tcs { - pluginBundles, err := NewLocalFinder(false).getAbsPluginJSONPaths(dir, tc.followDist) + pluginBundles, err := NewLocalFinder(false, featuremgmt.WithFeatures()).getAbsPluginJSONPaths(dir, tc.followDist) require.NoError(t, err) sort.Strings(pluginBundles) diff --git a/pkg/plugins/manager/pipeline/discovery/steps.go b/pkg/plugins/manager/pipeline/discovery/steps.go index 651d2f78d29..ceb36d5c331 100644 --- a/pkg/plugins/manager/pipeline/discovery/steps.go +++ b/pkg/plugins/manager/pipeline/discovery/steps.go @@ -13,7 +13,7 @@ import ( // DefaultFindFunc is the default function used for the Find step of the Discovery stage. It will scan the local // filesystem for plugins. func DefaultFindFunc(cfg *config.Cfg) FindFunc { - return finder.NewLocalFinder(cfg.DevMode).Find + 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 diff --git a/pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/MANIFEST.txt b/pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/MANIFEST.txt new file mode 100644 index 00000000000..6bb05dbe46a --- /dev/null +++ b/pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/MANIFEST.txt @@ -0,0 +1,28 @@ + +-----BEGIN PGP SIGNED MESSAGE----- +Hash: SHA512 + +{ + "manifestVersion": "2.0.0", + "signatureType": "grafana", + "signedByOrg": "grafana", + "signedByOrgName": "Grafana Labs", + "plugin": "test-datasource", + "version": "1.0.0", + "time": 1661171059101, + "keyId": "7e4d0c6a708866e7", + "files": { + "plugin.json": "203ef4a613c5693c437a665cd67f95e2756a0f71b336b2ffb265db7c180d0b19" + } +} +-----BEGIN PGP SIGNATURE----- +Version: OpenPGP.js v4.10.10 +Comment: https://openpgpjs.org + +wrgEARMKAAYFAmMDdXMAIQkQfk0ManCIZucWIQTzOyW2kQdOhGNlcPN+TQxq +cIhm54zLAgdfVimeut6Gw9MrIACBZUSH0ht9p9j+iG6MDjpmEFIpqVJrem6f +8wBv0/kmYU3LV9MWyPuUeRfBdccjQKSjEXlfEAIJAVmut9LcSKIykhWuQA+7 +VMVvJPXzlPkeoYsGYvzAlxh8i2UomCU15UChe62Gzq5V5HgGYkX5layIb5XX +y2Pio0lc +=/TR0 +-----END PGP SIGNATURE----- diff --git a/pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/plugin.json b/pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/plugin.json new file mode 100644 index 00000000000..71a9a05b586 --- /dev/null +++ b/pkg/plugins/manager/testdata/plugin-with-dist/plugin/dist/plugin.json @@ -0,0 +1,16 @@ +{ + "type": "datasource", + "name": "Test", + "id": "test-datasource", + "backend": true, + "executable": "test", + "state": "alpha", + "info": { + "version": "1.0.0", + "description": "Test", + "author": { + "name": "Will Browne", + "url": "https://willbrowne.com" + } + } +} diff --git a/pkg/plugins/manager/testdata/plugin-with-dist/plugin/plugin.json b/pkg/plugins/manager/testdata/plugin-with-dist/plugin/plugin.json new file mode 100644 index 00000000000..b959255ff16 --- /dev/null +++ b/pkg/plugins/manager/testdata/plugin-with-dist/plugin/plugin.json @@ -0,0 +1,16 @@ +{ + "type": "datasource", + "name": "Test", + "id": "test-datasource", + "backend": true, + "executable": "test", + "state": "alpha", + "info": { + "version": "2.0.0", + "description": "Test", + "author": { + "name": "Will Browne", + "url": "https://willbrowne.com" + } + } +} diff --git a/pkg/plugins/pfs/pfs_test.go b/pkg/plugins/pfs/pfs_test.go index 377d5a7a125..ebff347be95 100644 --- a/pkg/plugins/pfs/pfs_test.go +++ b/pkg/plugins/pfs/pfs_test.go @@ -107,6 +107,10 @@ func TestParsePluginTestdata(t *testing.T) { rootid: "test-datasource", subpath: "plugin", }, + "plugin-with-dist": { + rootid: "test-datasource", + subpath: "plugin", + }, "no-rootfile": { err: ErrNoRootFile, }, diff --git a/pkg/services/pluginsintegration/loader/loader_test.go b/pkg/services/pluginsintegration/loader/loader_test.go index bfec4fe0a36..f9862be67be 100644 --- a/pkg/services/pluginsintegration/loader/loader_test.go +++ b/pkg/services/pluginsintegration/loader/loader_test.go @@ -1623,7 +1623,8 @@ func newLoader(t *testing.T, cfg *config.Cfg, reg registry.Service, proc process terminate, err := pipeline.ProvideTerminationStage(cfg, reg, proc) require.NoError(t, err) - return ProvideService(pipeline.ProvideDiscoveryStage(cfg, finder.NewLocalFinder(false), reg), + return ProvideService(pipeline.ProvideDiscoveryStage(cfg, + finder.NewLocalFinder(false, featuremgmt.WithFeatures()), reg), pipeline.ProvideBootstrapStage(cfg, signature.DefaultCalculator(cfg), assets), pipeline.ProvideValidationStage(cfg, signature.NewValidator(signature.NewUnsignedAuthorizer(cfg)), angularInspector, sigErrTracker), pipeline.ProvideInitializationStage(cfg, reg, lic, backendFactory, proc, &fakes.FakeAuthService{}, fakes.NewFakeRoleRegistry()), @@ -1655,7 +1656,8 @@ func newLoaderWithOpts(t *testing.T, cfg *config.Cfg, opts loaderDepOpts) *Loade backendFactoryProvider = fakes.NewFakeBackendProcessProvider() } - return ProvideService(pipeline.ProvideDiscoveryStage(cfg, finder.NewLocalFinder(false), reg), + return ProvideService(pipeline.ProvideDiscoveryStage(cfg, + finder.NewLocalFinder(false, featuremgmt.WithFeatures()), reg), pipeline.ProvideBootstrapStage(cfg, signature.DefaultCalculator(cfg), assets), pipeline.ProvideValidationStage(cfg, signature.NewValidator(signature.NewUnsignedAuthorizer(cfg)), angularInspector, sigErrTracker), pipeline.ProvideInitializationStage(cfg, reg, lic, backendFactoryProvider, proc, authServiceRegistry, fakes.NewFakeRoleRegistry()), diff --git a/pkg/services/pluginsintegration/test_helper.go b/pkg/services/pluginsintegration/test_helper.go index b958b05681c..5c63d56c1c6 100644 --- a/pkg/services/pluginsintegration/test_helper.go +++ b/pkg/services/pluginsintegration/test_helper.go @@ -51,7 +51,7 @@ func CreateIntegrationTestCtx(t *testing.T, cfg *setting.Cfg, coreRegistry *core proc := process.ProvideService() errTracker := pluginerrs.ProvideSignatureErrorTracker() - disc := pipeline.ProvideDiscoveryStage(pCfg, finder.NewLocalFinder(true), reg) + disc := pipeline.ProvideDiscoveryStage(pCfg, finder.NewLocalFinder(true, pCfg.Features), reg) boot := pipeline.ProvideBootstrapStage(pCfg, signature.ProvideService(pCfg, statickey.New()), assetpath.ProvideService(pCfg, cdn)) valid := pipeline.ProvideValidationStage(pCfg, signature.NewValidator(signature.NewUnsignedAuthorizer(pCfg)), angularInspector, errTracker) init := pipeline.ProvideInitializationStage(pCfg, reg, fakes.NewFakeLicensingService(), provider.ProvideService(coreRegistry), proc, &fakes.FakeAuthService{}, fakes.NewFakeRoleRegistry()) @@ -86,7 +86,7 @@ type LoaderOpts struct { func CreateTestLoader(t *testing.T, cfg *pluginsCfg.Cfg, opts LoaderOpts) *loader.Loader { if opts.Discoverer == nil { - opts.Discoverer = pipeline.ProvideDiscoveryStage(cfg, finder.NewLocalFinder(cfg.DevMode), registry.ProvideService()) + opts.Discoverer = pipeline.ProvideDiscoveryStage(cfg, finder.NewLocalFinder(cfg.DevMode, cfg.Features), registry.ProvideService()) } if opts.Bootstrapper == nil {