Plugins: Add validation stage to plugin loader pipeline (#73053)

* first pass

* change validation signature

* err tracking

* fix

* undo golden

* 1 more

* fix

* adjust doc

* add test helper

* fix linter

pkg/plugins/manager/loader/angular/angularinspector/angularinspector.go

@@ -73,6 +73,6 @@ func NewDefaultStaticDetectorsProvider() angulardetector.DetectorsProvider {
 
 // NewStaticInspector returns the default Inspector, which is a PatternsListInspector that only uses the
 // static (hardcoded) angular detection patterns.
-func NewStaticInspector() (Inspector, error) {
-	return &PatternsListInspector{DetectorsProvider: NewDefaultStaticDetectorsProvider()}, nil
+func NewStaticInspector() Inspector {
+	return &PatternsListInspector{DetectorsProvider: NewDefaultStaticDetectorsProvider()}
 }

pkg/plugins/manager/loader/loader.go

@@ -2,154 +2,63 @@ package loader
 
 import (
 	"context"
-	"errors"
-	"time"
 
 	"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/angular/angularinspector"
-	"github.com/grafana/grafana/pkg/plugins/manager/loader/assetpath"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/bootstrap"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/discovery"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/initialization"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/termination"
-	"github.com/grafana/grafana/pkg/plugins/manager/process"
-	"github.com/grafana/grafana/pkg/plugins/manager/registry"
-	"github.com/grafana/grafana/pkg/plugins/manager/signature"
-	"github.com/grafana/grafana/pkg/plugins/oauth"
+	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/validation"
 )
 
-var _ plugins.ErrorResolver = (*Loader)(nil)
-
 type Loader struct {
 	discovery   discovery.Discoverer
 	bootstrap   bootstrap.Bootstrapper
 	initializer initialization.Initializer
 	termination termination.Terminator
-
-	processManager          process.Service
-	pluginRegistry          registry.Service
-	roleRegistry            plugins.RoleRegistry
-	signatureValidator      signature.Validator
-	externalServiceRegistry oauth.ExternalServiceRegistry
-	assetPath               *assetpath.Service
-	log                     log.Logger
-	cfg                     *config.Cfg
-
-	angularInspector angularinspector.Inspector
-
-	errs map[string]*plugins.SignatureError
+	validation  validation.Validator
+	log         log.Logger
 }
 
-func ProvideService(cfg *config.Cfg, authorizer plugins.PluginLoaderAuthorizer,
-	pluginRegistry registry.Service, roleRegistry plugins.RoleRegistry, assetPath *assetpath.Service,
-	angularInspector angularinspector.Inspector, externalServiceRegistry oauth.ExternalServiceRegistry,
-	discovery discovery.Discoverer, bootstrap bootstrap.Bootstrapper, initializer initialization.Initializer,
-	termination termination.Terminator) *Loader {
-	return New(cfg, authorizer, pluginRegistry, process.NewManager(pluginRegistry), roleRegistry, assetPath,
-		angularInspector, externalServiceRegistry, discovery, bootstrap, initializer, termination)
+func ProvideService(discovery discovery.Discoverer, bootstrap bootstrap.Bootstrapper, validation validation.Validator,
+	initializer initialization.Initializer, termination termination.Terminator) *Loader {
+	return New(discovery, bootstrap, validation, initializer, termination)
 }
 
-func New(cfg *config.Cfg, authorizer plugins.PluginLoaderAuthorizer, pluginRegistry registry.Service,
-	processManager process.Service, roleRegistry plugins.RoleRegistry, assetPath *assetpath.Service,
-	angularInspector angularinspector.Inspector, externalServiceRegistry oauth.ExternalServiceRegistry,
-	discovery discovery.Discoverer, bootstrap bootstrap.Bootstrapper, initializer initialization.Initializer,
-	termination termination.Terminator) *Loader {
+func New(
+	discovery discovery.Discoverer, bootstrap bootstrap.Bootstrapper, validation validation.Validator,
+	initializer initialization.Initializer, termination termination.Terminator) *Loader {
 	return &Loader{
-		pluginRegistry:          pluginRegistry,
-		signatureValidator:      signature.NewValidator(authorizer),
-		processManager:          processManager,
-		errs:                    make(map[string]*plugins.SignatureError),
-		log:                     log.New("plugin.loader"),
-		roleRegistry:            roleRegistry,
-		cfg:                     cfg,
-		assetPath:               assetPath,
-		angularInspector:        angularInspector,
-		externalServiceRegistry: externalServiceRegistry,
-		discovery:               discovery,
-		bootstrap:               bootstrap,
-		initializer:             initializer,
-		termination:             termination,
+		discovery:   discovery,
+		bootstrap:   bootstrap,
+		validation:  validation,
+		initializer: initializer,
+		termination: termination,
+		log:         log.New("plugin.loader"),
 	}
 }
 
 func (l *Loader) Load(ctx context.Context, src plugins.PluginSource) ([]*plugins.Plugin, error) {
-	// <DISCOVERY STAGE>
 	discoveredPlugins, err := l.discovery.Discover(ctx, src)
 	if err != nil {
 		return nil, err
 	}
-	// </DISCOVERY STAGE>
 
-	// <BOOTSTRAP STAGE>
 	bootstrappedPlugins, err := l.bootstrap.Bootstrap(ctx, src, discoveredPlugins)
 	if err != nil {
 		return nil, err
 	}
-	// </BOOTSTRAP STAGE>
 
-	// <VERIFICATION STAGE>
-	verifiedPlugins := make([]*plugins.Plugin, 0, len(bootstrappedPlugins))
-	for _, plugin := range bootstrappedPlugins {
-		signingError := l.signatureValidator.Validate(plugin)
-		if signingError != nil {
-			l.log.Warn("Skipping loading plugin due to problem with signature",
-				"pluginID", plugin.ID, "status", signingError.SignatureStatus)
-			plugin.SignatureError = signingError
-			l.errs[plugin.ID] = signingError
-			// skip plugin so it will not be loaded any further
-			continue
-		}
-
-		// clear plugin error if a pre-existing error has since been resolved
-		delete(l.errs, plugin.ID)
-
-		// verify module.js exists for SystemJS to load.
-		// CDN plugins can be loaded with plugin.json only, so do not warn for those.
-		if !plugin.IsRenderer() && !plugin.IsCorePlugin() {
-			f, err := plugin.FS.Open("module.js")
-			if err != nil {
-				if errors.Is(err, plugins.ErrFileNotExist) {
-					l.log.Warn("Plugin missing module.js", "pluginID", plugin.ID,
-						"warning", "Missing module.js, If you loaded this plugin from git, make sure to compile it.")
-				}
-			} else if f != nil {
-				if err := f.Close(); err != nil {
-					l.log.Warn("Could not close module.js", "pluginID", plugin.ID, "err", err)
-				}
-			}
-		}
-
-		// detect angular for external plugins
-		if plugin.IsExternalPlugin() {
-			var err error
-
-			cctx, canc := context.WithTimeout(ctx, time.Second*10)
-			plugin.AngularDetected, err = l.angularInspector.Inspect(cctx, plugin)
-			canc()
-
-			if err != nil {
-				l.log.Warn("Could not inspect plugin for angular", "pluginID", plugin.ID, "err", err)
-			}
-
-			// Do not initialize plugins if they're using Angular and Angular support is disabled
-			if plugin.AngularDetected && !l.cfg.AngularSupportEnabled {
-				l.log.Error("Refusing to initialize plugin because it's using Angular, which has been disabled", "pluginID", plugin.ID)
-				continue
-			}
-		}
-
-		verifiedPlugins = append(verifiedPlugins, plugin)
+	verifiedPlugins, err := l.validation.Validate(ctx, bootstrappedPlugins)
+	if err != nil {
+		return nil, err
 	}
-	// </VERIFICATION STAGE>
 
-	// <INITIALIZATION STAGE>
 	initializedPlugins, err := l.initializer.Initialize(ctx, verifiedPlugins)
 	if err != nil {
 		return nil, err
 	}
-	// </INITIALIZATION STAGE>
 
 	return initializedPlugins, nil
 }
@@ -157,15 +66,3 @@ func (l *Loader) Load(ctx context.Context, src plugins.PluginSource) ([]*plugins
 func (l *Loader) Unload(ctx context.Context, pluginID string) error {
 	return l.termination.Terminate(ctx, pluginID)
 }
-
-func (l *Loader) PluginErrors() []*plugins.Error {
-	errs := make([]*plugins.Error, 0, len(l.errs))
-	for _, err := range l.errs {
-		errs = append(errs, &plugins.Error{
-			PluginID:  err.PluginID,
-			ErrorCode: err.AsErrorCode(),
-		})
-	}
-
-	return errs
-}

pkg/plugins/manager/loader/loader_test.go

@@ -14,15 +14,12 @@ import (
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/config"
 	"github.com/grafana/grafana/pkg/plugins/manager/fakes"
-	"github.com/grafana/grafana/pkg/plugins/manager/loader/angular/angularinspector"
-	"github.com/grafana/grafana/pkg/plugins/manager/loader/assetpath"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/bootstrap"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/discovery"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/initialization"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/termination"
-	"github.com/grafana/grafana/pkg/plugins/manager/signature"
+	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/validation"
 	"github.com/grafana/grafana/pkg/plugins/manager/sources"
-	"github.com/grafana/grafana/pkg/plugins/pluginscdn"
 	"github.com/grafana/grafana/pkg/services/org"
 )
 
@@ -61,12 +58,11 @@ func TestLoader_Load(t *testing.T) {
 		return
 	}
 	tests := []struct {
-		name         string
-		class        plugins.Class
-		cfg          *config.Cfg
-		pluginPaths  []string
-		want         []*plugins.Plugin
-		pluginErrors map[string]*plugins.Error
+		name        string
+		class       plugins.Class
+		cfg         *config.Cfg
+		pluginPaths []string
+		want        []*plugins.Plugin
 	}{
 		{
 			name:        "Load a Core plugin",
@@ -272,18 +268,13 @@ func TestLoader_Load(t *testing.T) {
 					Signature: "unsigned",
 				},
 			},
-		}, {
+		},
+		{
 			name:        "Load an unsigned plugin (production)",
 			class:       plugins.ClassExternal,
 			cfg:         &config.Cfg{},
 			pluginPaths: []string{"../testdata/unsigned-datasource"},
 			want:        []*plugins.Plugin{},
-			pluginErrors: map[string]*plugins.Error{
-				"test-datasource": {
-					PluginID:  "test-datasource",
-					ErrorCode: "signatureMissing",
-				},
-			},
 		},
 		{
 			name:  "Load an unsigned plugin using PluginsAllowUnsigned config (production)",
@@ -330,12 +321,6 @@ func TestLoader_Load(t *testing.T) {
 			cfg:         &config.Cfg{},
 			pluginPaths: []string{"../testdata/lacking-files"},
 			want:        []*plugins.Plugin{},
-			pluginErrors: map[string]*plugins.Error{
-				"test-datasource": {
-					PluginID:  "test-datasource",
-					ErrorCode: "signatureInvalid",
-				},
-			},
 		},
 		{
 			name:  "Load a plugin with v1 manifest using PluginsAllowUnsigned config (production) should return signatureInvali",
@@ -345,12 +330,6 @@ func TestLoader_Load(t *testing.T) {
 			},
 			pluginPaths: []string{"../testdata/lacking-files"},
 			want:        []*plugins.Plugin{},
-			pluginErrors: map[string]*plugins.Error{
-				"test-datasource": {
-					PluginID:  "test-datasource",
-					ErrorCode: "signatureInvalid",
-				},
-			},
 		},
 		{
 			name:  "Load a plugin with manifest which has a file not found in plugin folder",
@@ -360,12 +339,6 @@ func TestLoader_Load(t *testing.T) {
 			},
 			pluginPaths: []string{"../testdata/invalid-v2-missing-file"},
 			want:        []*plugins.Plugin{},
-			pluginErrors: map[string]*plugins.Error{
-				"test-datasource": {
-					PluginID:  "test-datasource",
-					ErrorCode: "signatureModified",
-				},
-			},
 		},
 		{
 			name:  "Load a plugin with file which is missing from the manifest",
@@ -375,12 +348,6 @@ func TestLoader_Load(t *testing.T) {
 			},
 			pluginPaths: []string{"../testdata/invalid-v2-extra-file"},
 			want:        []*plugins.Plugin{},
-			pluginErrors: map[string]*plugins.Error{
-				"test-datasource": {
-					PluginID:  "test-datasource",
-					ErrorCode: "signatureModified",
-				},
-			},
 		},
 		{
 			name:  "Load an app with includes",
@@ -434,31 +401,19 @@ func TestLoader_Load(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
-		angularInspector, err := angularinspector.NewStaticInspector()
-		require.NoError(t, err)
-
-		terminationStage, err := termination.New(tt.cfg, termination.Opts{})
-		require.NoError(t, err)
-
-		l := New(tt.cfg, signature.NewUnsignedAuthorizer(tt.cfg), fakes.NewFakePluginRegistry(),
-			fakes.NewFakeProcessManager(), fakes.NewFakeRoleRegistry(),
-			assetpath.ProvideService(pluginscdn.ProvideService(tt.cfg)), angularInspector, &fakes.FakeOauthService{},
-			discovery.New(tt.cfg, discovery.Opts{}), bootstrap.New(tt.cfg, bootstrap.Opts{}),
-			initialization.New(tt.cfg, initialization.Opts{}),
-			terminationStage)
-
 		t.Run(tt.name, func(t *testing.T) {
+			terminationStage, err := termination.New(tt.cfg, termination.Opts{})
+			require.NoError(t, err)
+
+			l := New(discovery.New(tt.cfg, discovery.Opts{}), bootstrap.New(tt.cfg, bootstrap.Opts{}),
+				validation.New(tt.cfg, validation.Opts{}), initialization.New(tt.cfg, initialization.Opts{}),
+				terminationStage)
+
 			got, err := l.Load(context.Background(), sources.NewLocalSource(tt.class, tt.pluginPaths))
 			require.NoError(t, err)
 			if !cmp.Equal(got, tt.want, compareOpts...) {
 				t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(got, tt.want, compareOpts...))
 			}
-
-			pluginErrs := l.PluginErrors()
-			require.Equal(t, len(tt.pluginErrors), len(pluginErrs))
-			for _, pluginErr := range pluginErrs {
-				require.Equal(t, tt.pluginErrors[pluginErr.PluginID], pluginErr)
-			}
 		})
 	}
 
@@ -474,41 +429,41 @@ func TestLoader_Load(t *testing.T) {
 				return plugins.Signature{}, false
 			},
 		}
-		pJSON := plugins.JSONData{ID: "test-datasource", Type: plugins.TypeDataSource, Info: plugins.Info{Version: "1.0.0"}}
-		p := &plugins.Plugin{
-			JSONData:      pJSON,
+		pluginJSON := plugins.JSONData{ID: "test-datasource", Type: plugins.TypeDataSource, Info: plugins.Info{Version: "1.0.0"}}
+		plugin := &plugins.Plugin{
+			JSONData:      pluginJSON,
 			Signature:     plugins.SignatureStatusValid,
 			SignatureType: plugins.SignatureTypeCommunity,
 			FS:            plugins.NewFakeFS(),
 		}
 
-		cfg := &config.Cfg{}
-		angularInspector, err := angularinspector.NewStaticInspector()
-		require.NoError(t, err)
-
 		var steps []string
-		l := New(cfg, signature.NewUnsignedAuthorizer(cfg), fakes.NewFakePluginRegistry(), fakes.NewFakeProcessManager(),
-			fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(cfg)),
-			angularInspector, &fakes.FakeOauthService{},
+		l := New(
 			&fakes.FakeDiscoverer{
 				DiscoverFunc: func(ctx context.Context, s plugins.PluginSource) ([]*plugins.FoundBundle, error) {
 					require.Equal(t, src, s)
 					steps = append(steps, "discover")
-					return []*plugins.FoundBundle{{Primary: plugins.FoundPlugin{JSONData: pJSON}}}, nil
+					return []*plugins.FoundBundle{{Primary: plugins.FoundPlugin{JSONData: pluginJSON}}}, nil
 				},
 			}, &fakes.FakeBootstrapper{
 				BootstrapFunc: func(ctx context.Context, s plugins.PluginSource, b []*plugins.FoundBundle) ([]*plugins.Plugin, error) {
 					require.True(t, len(b) == 1)
-					require.Equal(t, b[0].Primary.JSONData, pJSON)
+					require.Equal(t, b[0].Primary.JSONData, pluginJSON)
 					require.Equal(t, src, s)
 
 					steps = append(steps, "bootstrap")
-					return []*plugins.Plugin{p}, nil
+					return []*plugins.Plugin{plugin}, nil
 				},
-			}, &fakes.FakeInitializer{
+			}, &fakes.FakeValidator{ValidateFunc: func(ctx context.Context, ps []*plugins.Plugin) ([]*plugins.Plugin, error) {
+				require.Equal(t, []*plugins.Plugin{plugin}, ps)
+
+				steps = append(steps, "validate")
+				return ps, nil
+			}},
+			&fakes.FakeInitializer{
 				IntializeFunc: func(ctx context.Context, ps []*plugins.Plugin) ([]*plugins.Plugin, error) {
 					require.True(t, len(ps) == 1)
-					require.Equal(t, ps[0].JSONData, pJSON)
+					require.Equal(t, ps[0].JSONData, pluginJSON)
 					steps = append(steps, "initialize")
 					return ps, nil
 				},
@@ -516,19 +471,14 @@ func TestLoader_Load(t *testing.T) {
 
 		got, err := l.Load(context.Background(), src)
 		require.NoError(t, err)
-		require.Equal(t, []*plugins.Plugin{p}, got)
-		require.Equal(t, []string{"discover", "bootstrap", "initialize"}, steps)
-		require.Zero(t, len(l.PluginErrors()))
+		require.Equal(t, []*plugins.Plugin{plugin}, got)
+		require.Equal(t, []string{"discover", "bootstrap", "validate", "initialize"}, steps)
 	})
 }
 
 func TestLoader_Unload(t *testing.T) {
 	t.Run("Termination stage error is returned from Unload", func(t *testing.T) {
 		pluginID := "grafana-test-panel"
-		cfg := &config.Cfg{}
-		angularInspector, err := angularinspector.NewStaticInspector()
-		require.NoError(t, err)
-
 		tcs := []struct {
 			expectedErr error
 		}{
@@ -541,9 +491,10 @@ func TestLoader_Unload(t *testing.T) {
 		}
 
 		for _, tc := range tcs {
-			l := New(cfg, signature.NewUnsignedAuthorizer(cfg), fakes.NewFakePluginRegistry(), fakes.NewFakeProcessManager(),
-				fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(cfg)), angularInspector,
-				&fakes.FakeOauthService{}, &fakes.FakeDiscoverer{}, &fakes.FakeBootstrapper{}, &fakes.FakeInitializer{},
+			l := New(&fakes.FakeDiscoverer{},
+				&fakes.FakeBootstrapper{},
+				&fakes.FakeValidator{},
+				&fakes.FakeInitializer{},
 				&fakes.FakeTerminator{
 					TerminateFunc: func(ctx context.Context, pID string) error {
 						require.Equal(t, pluginID, pID)
@@ -551,7 +502,7 @@ func TestLoader_Unload(t *testing.T) {
 					},
 				})
 
-			err = l.Unload(context.Background(), pluginID)
+			err := l.Unload(context.Background(), pluginID)
 			require.ErrorIs(t, err, tc.expectedErr)
 		}
 	})