From 335a55047b2aee183baf3fe7e6f79e1435f33071 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Fri, 11 Apr 2025 12:11:05 +0200 Subject: [PATCH] Advisor: Move the missing plugin check to its own step (#103861) --- .../pkg/app/checkregistry/checkregistry.go | 1 + .../pkg/app/checks/datasourcecheck/check.go | 83 +++++-- .../app/checks/datasourcecheck/check_test.go | 210 +++++++++++------- 3 files changed, 199 insertions(+), 95 deletions(-) diff --git a/apps/advisor/pkg/app/checkregistry/checkregistry.go b/apps/advisor/pkg/app/checkregistry/checkregistry.go index 31be8ae936e..5b07b827cdd 100644 --- a/apps/advisor/pkg/app/checkregistry/checkregistry.go +++ b/apps/advisor/pkg/app/checkregistry/checkregistry.go @@ -53,6 +53,7 @@ func (s *Service) Checks() []checks.Check { s.pluginStore, s.pluginContextProvider, s.pluginClient, + s.pluginRepo, ), plugincheck.New( s.pluginStore, diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check.go b/apps/advisor/pkg/app/checks/datasourcecheck/check.go index edb37410299..92a325f3211 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/repo" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" "github.com/grafana/grafana/pkg/util" @@ -20,6 +21,7 @@ const ( CheckID = "datasource" HealthCheckStepID = "health-check" UIDValidationStepID = "uid-validation" + MissingPluginStepID = "missing-plugin" ) type check struct { @@ -27,6 +29,7 @@ type check struct { PluginStore pluginstore.Store PluginContextProvider pluginContextProvider PluginClient plugins.Client + PluginRepo repo.Service log log.Logger } @@ -35,12 +38,14 @@ func New( pluginStore pluginstore.Store, pluginContextProvider pluginContextProvider, pluginClient plugins.Client, + pluginRepo repo.Service, ) checks.Check { return &check{ DatasourceSvc: datasourceSvc, PluginStore: pluginStore, PluginContextProvider: pluginContextProvider, PluginClient: pluginClient, + PluginRepo: pluginRepo, log: log.New("advisor.datasourcecheck"), } } @@ -69,6 +74,11 @@ func (c *check) Steps() []checks.Step { PluginClient: c.PluginClient, log: c.log, }, + &missingPluginStep{ + PluginStore: c.PluginStore, + PluginRepo: c.PluginRepo, + log: c.log, + }, } } @@ -145,22 +155,8 @@ func (s *healthCheckStep) Run(ctx context.Context, obj *advisor.CheckSpec, i any pCtx, err := s.PluginContextProvider.GetWithDataSource(ctx, ds.Type, requester, ds) if err != nil { if errors.Is(err, plugins.ErrPluginNotRegistered) { - // The plugin is not installed - return checks.NewCheckReportFailure( - advisor.CheckReportFailureSeverityHigh, - s.ID(), - ds.Name, - []advisor.CheckErrorLink{ - { - Message: "Delete data source", - Url: fmt.Sprintf("/connections/datasources/edit/%s", ds.UID), - }, - { - Message: "Install plugin", - Url: fmt.Sprintf("/plugins/%s", ds.Type), - }, - }, - ), nil + // The plugin is not installed, handle this in the missing plugin step + return nil, nil } // Unable to check health check s.log.Error("Failed to get plugin context", "datasource_uid", ds.UID, "error", err) @@ -196,6 +192,61 @@ func (s *healthCheckStep) Run(ctx context.Context, obj *advisor.CheckSpec, i any return nil, nil } +type missingPluginStep struct { + PluginStore pluginstore.Store + PluginRepo repo.Service + log log.Logger +} + +func (s *missingPluginStep) Title() string { + return "Missing plugin check" +} + +func (s *missingPluginStep) Description() string { + return "Checks if the plugin associated with the data source is installed." +} + +func (s *missingPluginStep) Resolution() string { + return "Delete the datasource or install the plugin." +} + +func (s *missingPluginStep) ID() string { + return MissingPluginStepID +} + +func (s *missingPluginStep) Run(ctx context.Context, obj *advisor.CheckSpec, i any) (*advisor.CheckReportFailure, error) { + ds, ok := i.(*datasources.DataSource) + if !ok { + return nil, fmt.Errorf("invalid item type %T", i) + } + + _, exists := s.PluginStore.Plugin(ctx, ds.Type) + if !exists { + links := []advisor.CheckErrorLink{ + { + Message: "Delete data source", + Url: fmt.Sprintf("/connections/datasources/edit/%s", ds.UID), + }, + } + _, err := s.PluginRepo.PluginInfo(ctx, ds.Type) + if err == nil { + // Plugin is available in the repo + links = append(links, advisor.CheckErrorLink{ + Message: "Install plugin", + Url: fmt.Sprintf("/plugins/%s", ds.Type), + }) + } + // The plugin is not installed + return checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + s.ID(), + ds.Name, + links, + ), nil + } + return nil, nil +} + type pluginContextProvider interface { GetWithDataSource(ctx context.Context, pluginID string, user identity.Requester, ds *datasources.DataSource) (backend.PluginContext, error) } diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go index 09a7487bd8b..7c538da83f7 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go @@ -2,6 +2,7 @@ package datasourcecheck import ( "context" + "errors" "testing" "github.com/grafana/grafana-plugin-sdk-go/backend" @@ -9,11 +10,37 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/repo" "github.com/grafana/grafana/pkg/services/datasources" + "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" "github.com/grafana/grafana/pkg/services/user" "github.com/stretchr/testify/assert" ) +// runChecks executes all steps for all items and returns the failures +func runChecks(check *check) ([]advisor.CheckReportFailure, error) { + ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) + items, err := check.Items(ctx) + if err != nil { + return nil, err + } + + failures := []advisor.CheckReportFailure{} + for _, step := range check.Steps() { + for _, item := range items { + stepFailures, err := step.Run(ctx, &advisor.CheckSpec{}, item) + if err != nil { + return nil, err + } + if stepFailures != nil { + failures = append(failures, *stepFailures) + } + } + } + + return failures, nil +} + func TestCheck_Run(t *testing.T) { t.Run("should return no failures when all datasources are healthy", func(t *testing.T) { datasources := []*datasources.DataSource{ @@ -24,30 +51,20 @@ func TestCheck_Run(t *testing.T) { mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} mockPluginClient := &MockPluginClient{res: &backend.CheckHealthResult{Status: backend.HealthStatusOk}} + mockPluginRepo := &MockPluginRepo{exists: true} + mockPluginStore := &MockPluginStore{exists: true} check := &check{ DatasourceSvc: mockDatasourceSvc, PluginContextProvider: mockPluginContextProvider, PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, log: log.New("advisor.datasourcecheck"), } - ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - items, err := check.Items(ctx) + failures, err := runChecks(check) assert.NoError(t, err) - failures := []advisor.CheckReportFailure{} - for _, step := range check.Steps() { - for _, item := range items { - stepFailures, err := step.Run(ctx, &advisor.CheckSpec{}, item) - assert.NoError(t, err) - if stepFailures != nil { - failures = append(failures, *stepFailures) - } - } - } - - assert.NoError(t, err) - assert.Equal(t, 2, len(items)) assert.Empty(t, failures) }) @@ -59,30 +76,20 @@ func TestCheck_Run(t *testing.T) { mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} mockPluginClient := &MockPluginClient{res: &backend.CheckHealthResult{Status: backend.HealthStatusOk}} + mockPluginRepo := &MockPluginRepo{exists: true} + mockPluginStore := &MockPluginStore{exists: true} check := &check{ DatasourceSvc: mockDatasourceSvc, PluginContextProvider: mockPluginContextProvider, PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, log: log.New("advisor.datasourcecheck"), } - ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - items, err := check.Items(ctx) + failures, err := runChecks(check) assert.NoError(t, err) - failures := []advisor.CheckReportFailure{} - for _, step := range check.Steps() { - for _, item := range items { - stepFailures, err := step.Run(ctx, &advisor.CheckSpec{}, item) - assert.NoError(t, err) - if stepFailures != nil { - failures = append(failures, *stepFailures) - } - } - } - - assert.NoError(t, err) - assert.Equal(t, 1, len(items)) assert.Len(t, failures, 1) assert.Equal(t, "uid-validation", failures[0].StepID) }) @@ -95,30 +102,20 @@ func TestCheck_Run(t *testing.T) { mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} mockPluginClient := &MockPluginClient{res: &backend.CheckHealthResult{Status: backend.HealthStatusError}} + mockPluginRepo := &MockPluginRepo{exists: true} + mockPluginStore := &MockPluginStore{exists: true} check := &check{ DatasourceSvc: mockDatasourceSvc, PluginContextProvider: mockPluginContextProvider, PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, log: log.New("advisor.datasourcecheck"), } - ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - items, err := check.Items(ctx) + failures, err := runChecks(check) assert.NoError(t, err) - failures := []advisor.CheckReportFailure{} - for _, step := range check.Steps() { - for _, item := range items { - stepFailures, err := step.Run(ctx, &advisor.CheckSpec{}, item) - assert.NoError(t, err) - if stepFailures != nil { - failures = append(failures, *stepFailures) - } - } - } - - assert.NoError(t, err) - assert.Equal(t, 1, len(items)) assert.Len(t, failures, 1) assert.Equal(t, "health-check", failures[0].StepID) }) @@ -130,31 +127,21 @@ func TestCheck_Run(t *testing.T) { mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} mockPluginClient := &MockPluginClient{err: plugins.ErrMethodNotImplemented} + mockPluginRepo := &MockPluginRepo{exists: true} + mockPluginStore := &MockPluginStore{exists: true} check := &check{ DatasourceSvc: mockDatasourceSvc, PluginContextProvider: mockPluginContextProvider, PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, log: log.New("advisor.datasourcecheck"), } - ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - items, err := check.Items(ctx) + failures, err := runChecks(check) assert.NoError(t, err) - failures := []advisor.CheckReportFailure{} - for _, step := range check.Steps() { - for _, item := range items { - stepFailures, err := step.Run(ctx, &advisor.CheckSpec{}, item) - assert.NoError(t, err) - if stepFailures != nil { - failures = append(failures, *stepFailures) - } - } - } - - assert.NoError(t, err) - assert.Equal(t, 1, len(items)) - assert.Len(t, failures, 0) + assert.Empty(t, failures) }) t.Run("should return failure when plugin is not installed", func(t *testing.T) { @@ -164,33 +151,75 @@ func TestCheck_Run(t *testing.T) { mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} mockPluginClient := &MockPluginClient{err: plugins.ErrPluginNotRegistered} + mockPluginRepo := &MockPluginRepo{exists: true} + mockPluginStore := &MockPluginStore{exists: true} check := &check{ DatasourceSvc: mockDatasourceSvc, PluginContextProvider: mockPluginContextProvider, PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, log: log.New("advisor.datasourcecheck"), } - ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - items, err := check.Items(ctx) + failures, err := runChecks(check) assert.NoError(t, err) - failures := []advisor.CheckReportFailure{} - for _, step := range check.Steps() { - for _, item := range items { - stepFailures, err := step.Run(ctx, &advisor.CheckSpec{}, item) - assert.NoError(t, err) - if stepFailures != nil { - failures = append(failures, *stepFailures) - } - } - } - - assert.NoError(t, err) - assert.Equal(t, 1, len(items)) assert.Len(t, failures, 1) assert.Equal(t, "health-check", failures[0].StepID) }) + + t.Run("should return failure when plugin is not installed and the plugin is available in the repo", func(t *testing.T) { + datasources := []*datasources.DataSource{ + {UID: "valid-uid-1", Type: "prometheus", Name: "Prometheus"}, + } + mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} + mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} + mockPluginClient := &MockPluginClient{res: &backend.CheckHealthResult{Status: backend.HealthStatusOk}} + mockPluginRepo := &MockPluginRepo{exists: true} + mockPluginStore := &MockPluginStore{exists: false} + + check := &check{ + DatasourceSvc: mockDatasourceSvc, + PluginContextProvider: mockPluginContextProvider, + PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, + log: log.New("advisor.datasourcecheck"), + } + + failures, err := runChecks(check) + assert.NoError(t, err) + assert.Len(t, failures, 1) + assert.Equal(t, MissingPluginStepID, failures[0].StepID) + assert.Len(t, failures[0].Links, 2) + }) + + t.Run("should return failure when plugin is not installed and the plugin is not available in the repo", func(t *testing.T) { + datasources := []*datasources.DataSource{ + {UID: "valid-uid-1", Type: "prometheus", Name: "Prometheus"}, + } + mockDatasourceSvc := &MockDatasourceSvc{dss: datasources} + mockPluginContextProvider := &MockPluginContextProvider{pCtx: backend.PluginContext{}} + mockPluginClient := &MockPluginClient{res: &backend.CheckHealthResult{Status: backend.HealthStatusOk}} + mockPluginRepo := &MockPluginRepo{exists: false} + mockPluginStore := &MockPluginStore{exists: false} + + check := &check{ + DatasourceSvc: mockDatasourceSvc, + PluginContextProvider: mockPluginContextProvider, + PluginClient: mockPluginClient, + PluginRepo: mockPluginRepo, + PluginStore: mockPluginStore, + log: log.New("advisor.datasourcecheck"), + } + + failures, err := runChecks(check) + assert.NoError(t, err) + assert.Len(t, failures, 1) + assert.Equal(t, MissingPluginStepID, failures[0].StepID) + assert.Len(t, failures[0].Links, 1) + }) } type MockDatasourceSvc struct { @@ -199,7 +228,7 @@ type MockDatasourceSvc struct { dss []*datasources.DataSource } -func (m *MockDatasourceSvc) GetAllDataSources(ctx context.Context, query *datasources.GetAllDataSourcesQuery) ([]*datasources.DataSource, error) { +func (m *MockDatasourceSvc) GetAllDataSources(context.Context, *datasources.GetAllDataSourcesQuery) ([]*datasources.DataSource, error) { return m.dss, nil } @@ -207,7 +236,7 @@ type MockPluginContextProvider struct { pCtx backend.PluginContext } -func (m *MockPluginContextProvider) GetWithDataSource(ctx context.Context, pluginID string, user identity.Requester, ds *datasources.DataSource) (backend.PluginContext, error) { +func (m *MockPluginContextProvider) GetWithDataSource(context.Context, string, identity.Requester, *datasources.DataSource) (backend.PluginContext, error) { return m.pCtx, nil } @@ -218,6 +247,29 @@ type MockPluginClient struct { err error } -func (m *MockPluginClient) CheckHealth(ctx context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { +func (m *MockPluginClient) CheckHealth(context.Context, *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) { return m.res, m.err } + +type MockPluginStore struct { + pluginstore.Store + + exists bool +} + +func (m *MockPluginStore) Plugin(context.Context, string) (pluginstore.Plugin, bool) { + return pluginstore.Plugin{}, m.exists +} + +type MockPluginRepo struct { + repo.Service + + exists bool +} + +func (m *MockPluginRepo) PluginInfo(context.Context, string) (*repo.PluginInfo, error) { + if !m.exists { + return nil, errors.New("plugin not found") + } + return &repo.PluginInfo{}, nil +}