From 26bc87b60e4983053824fdb4efce8645a17436ce Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Wed, 7 Feb 2024 15:17:13 +0100 Subject: [PATCH] Chore: Replace core plugins as external warning (#81877) --- .../backgroundsvcs/background_services.go | 3 ++ .../pluginsintegration/pipeline/steps.go | 21 -------- .../pluginsintegration/pipeline/steps_test.go | 21 -------- .../pluginexternal/check.go | 43 +++++++++++++++ .../pluginexternal/check_test.go | 53 +++++++++++++++++++ .../pluginsintegration/pluginsintegration.go | 2 + 6 files changed, 101 insertions(+), 42 deletions(-) create mode 100644 pkg/services/pluginsintegration/pluginexternal/check.go create mode 100644 pkg/services/pluginsintegration/pluginexternal/check_test.go diff --git a/pkg/registry/backgroundsvcs/background_services.go b/pkg/registry/backgroundsvcs/background_services.go index 3f323ab09ae..d31454198b9 100644 --- a/pkg/registry/backgroundsvcs/background_services.go +++ b/pkg/registry/backgroundsvcs/background_services.go @@ -26,6 +26,7 @@ import ( plugindashboardsservice "github.com/grafana/grafana/pkg/services/plugindashboards/service" "github.com/grafana/grafana/pkg/services/pluginsintegration/angulardetectorsprovider" "github.com/grafana/grafana/pkg/services/pluginsintegration/keyretriever/dynamic" + "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginexternal" pluginStore "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" "github.com/grafana/grafana/pkg/services/provisioning" publicdashboardsmetric "github.com/grafana/grafana/pkg/services/publicdashboards/metric" @@ -60,6 +61,7 @@ func ProvideBackgroundServiceRegistry( grafanaAPIServer grafanaapiserver.Service, anon *anonimpl.AnonDeviceService, ssoSettings *ssosettingsimpl.Service, + pluginExternal *pluginexternal.Service, // Need to make sure these are initialized, is there a better place to put them? _ dashboardsnapshots.Service, _ *alerting.AlertNotificationService, _ serviceaccounts.Service, _ *guardian.Provider, @@ -101,6 +103,7 @@ func ProvideBackgroundServiceRegistry( grafanaAPIServer, anon, ssoSettings, + pluginExternal, ) } diff --git a/pkg/services/pluginsintegration/pipeline/steps.go b/pkg/services/pluginsintegration/pipeline/steps.go index 3b414b502f9..c1a013223c1 100644 --- a/pkg/services/pluginsintegration/pipeline/steps.go +++ b/pkg/services/pluginsintegration/pipeline/steps.go @@ -193,26 +193,5 @@ func (c *AsExternal) Filter(cl plugins.Class, bundles []*plugins.FoundBundle) ([ } return res, nil } - - if cl == plugins.ClassExternal { - // Warn if the plugin is not found in the external plugins directory. - asExternal := map[string]bool{} - for pluginID, pluginCfg := range c.cfg.PluginSettings { - if pluginCfg["as_external"] == "true" { - asExternal[pluginID] = true - } - } - for _, bundle := range bundles { - if asExternal[bundle.Primary.JSONData.ID] { - delete(asExternal, bundle.Primary.JSONData.ID) - } - } - if len(asExternal) > 0 { - for p := range asExternal { - c.log.Error("Core plugin expected to be loaded as external, but it is missing", "pluginID", p) - } - } - } - return bundles, nil } diff --git a/pkg/services/pluginsintegration/pipeline/steps_test.go b/pkg/services/pluginsintegration/pipeline/steps_test.go index 5ceb015fef4..edad51c8916 100644 --- a/pkg/services/pluginsintegration/pipeline/steps_test.go +++ b/pkg/services/pluginsintegration/pipeline/steps_test.go @@ -7,7 +7,6 @@ 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/setting" ) @@ -82,24 +81,4 @@ func TestAsExternal(t *testing.T) { require.Len(t, filtered, 1) require.Equal(t, filtered[0].Primary.JSONData.ID, "plugin2") }) - - t.Run("should log an error if an external plugin is not available", func(t *testing.T) { - cfg := &config.Cfg{ - Features: featuremgmt.WithFeatures(featuremgmt.FlagExternalCorePlugins), - PluginSettings: setting.PluginSettings{ - "plugin3": map[string]string{ - "as_external": "true", - }, - }, - } - - fakeLogger := log.NewTestLogger() - s := NewAsExternalStep(cfg) - s.log = fakeLogger - - filtered, err := s.Filter(plugins.ClassExternal, bundles) - require.NoError(t, err) - require.Len(t, filtered, 2) - require.Equal(t, fakeLogger.ErrorLogs.Calls, 1) - }) } diff --git a/pkg/services/pluginsintegration/pluginexternal/check.go b/pkg/services/pluginsintegration/pluginexternal/check.go new file mode 100644 index 00000000000..f96f60653b2 --- /dev/null +++ b/pkg/services/pluginsintegration/pluginexternal/check.go @@ -0,0 +1,43 @@ +package pluginexternal + +import ( + "context" + + "github.com/grafana/grafana/pkg/plugins/log" + "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" + "github.com/grafana/grafana/pkg/setting" +) + +type Service struct { + cfg *setting.Cfg + logger log.Logger + pluginStore pluginstore.Store +} + +func ProvideService( + cfg *setting.Cfg, pluginStore pluginstore.Store, +) (*Service, error) { + logger := log.New("datasources") + s := &Service{ + cfg: cfg, + logger: logger, + pluginStore: pluginStore, + } + return s, nil +} + +func (s *Service) Run(ctx context.Context) error { + s.validateExternal() + return ctx.Err() +} + +func (s *Service) validateExternal() { + for pluginID, pluginCfg := range s.cfg.PluginSettings { + if pluginCfg["as_external"] == "true" { + _, exists := s.pluginStore.Plugin(context.Background(), pluginID) + if !exists { + s.logger.Error("Core plugin expected to be loaded as external, but it is missing", "pluginID", pluginID) + } + } + } +} diff --git a/pkg/services/pluginsintegration/pluginexternal/check_test.go b/pkg/services/pluginsintegration/pluginexternal/check_test.go new file mode 100644 index 00000000000..68cfb68dcfb --- /dev/null +++ b/pkg/services/pluginsintegration/pluginexternal/check_test.go @@ -0,0 +1,53 @@ +package pluginexternal + +import ( + "testing" + + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/log" + "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" + "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/require" +) + +func TestService_validateExternal(t *testing.T) { + cfg := setting.NewCfg() + cfg.PluginSettings = setting.PluginSettings{ + "grafana-testdata-datasource": map[string]string{ + "as_external": "true", + }, + } + + t.Run("should not log error if core plugin is loaded as external", func(t *testing.T) { + l := log.NewTestLogger() + s := &Service{ + cfg: cfg, + logger: l, + pluginStore: &pluginstore.FakePluginStore{ + PluginList: []pluginstore.Plugin{ + { + JSONData: plugins.JSONData{ + ID: "grafana-testdata-datasource", + }, + }, + }, + }, + } + s.validateExternal() + require.Equal(t, l.ErrorLogs.Calls, 0) + }) + + t.Run("should log error if a core plugin is missing", func(t *testing.T) { + l := log.NewTestLogger() + s := &Service{ + cfg: cfg, + logger: l, + pluginStore: &pluginstore.FakePluginStore{ + PluginList: []pluginstore.Plugin{}, + }, + } + s.validateExternal() + require.Equal(t, l.ErrorLogs.Calls, 1) + require.Contains(t, l.ErrorLogs.Message, "Core plugin expected to be loaded as external") + }) +} diff --git a/pkg/services/pluginsintegration/pluginsintegration.go b/pkg/services/pluginsintegration/pluginsintegration.go index 0826cfade83..32f74d7d40a 100644 --- a/pkg/services/pluginsintegration/pluginsintegration.go +++ b/pkg/services/pluginsintegration/pluginsintegration.go @@ -43,6 +43,7 @@ import ( "github.com/grafana/grafana/pkg/services/pluginsintegration/loader" "github.com/grafana/grafana/pkg/services/pluginsintegration/pipeline" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginerrs" + "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginexternal" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings" pluginSettings "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings/service" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" @@ -112,6 +113,7 @@ var WireSet = wire.NewSet( wire.Bind(new(auth.ExternalServiceRegistry), new(*serviceregistration.Service)), renderer.ProvideService, wire.Bind(new(rendering.PluginManager), new(*renderer.Manager)), + pluginexternal.ProvideService, ) // WireExtensionSet provides a wire.ProviderSet of plugin providers that can be