From 4d0124af7aab12b5c9f8c27d167594b5784e5fa2 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Mon, 19 May 2025 15:15:20 +0200 Subject: [PATCH] Advisor: Fix retry behavior for missing item (#105608) --- .../pkg/app/checks/authchecks/check.go | 4 ++ .../pkg/app/checks/authchecks/check_test.go | 11 +++++ .../pkg/app/checks/datasourcecheck/check.go | 10 ++++- .../app/checks/datasourcecheck/check_test.go | 20 +++++++++ .../pkg/app/checks/plugincheck/check.go | 2 +- .../pkg/app/checks/plugincheck/check_test.go | 19 ++++++++ apps/advisor/pkg/app/utils.go | 43 ++++++++++--------- apps/advisor/pkg/app/utils_test.go | 32 ++++++++++++++ 8 files changed, 119 insertions(+), 22 deletions(-) diff --git a/apps/advisor/pkg/app/checks/authchecks/check.go b/apps/advisor/pkg/app/checks/authchecks/check.go index d20dcd27621..ff3158a2ad2 100644 --- a/apps/advisor/pkg/app/checks/authchecks/check.go +++ b/apps/advisor/pkg/app/checks/authchecks/check.go @@ -2,6 +2,7 @@ package authchecks import ( "context" + "errors" "fmt" "github.com/grafana/grafana/apps/advisor/pkg/app/checks" @@ -53,6 +54,9 @@ func (c *check) Items(ctx context.Context) ([]any, error) { func (c *check) Item(ctx context.Context, id string) (any, error) { ssoSetting, err := c.ssoSettingsService.GetForProviderWithRedactedSecrets(ctx, id) if err != nil { + if errors.Is(err, ssosettings.ErrNotFound) { + return nil, nil + } return nil, err } return ssoSetting, nil diff --git a/apps/advisor/pkg/app/checks/authchecks/check_test.go b/apps/advisor/pkg/app/checks/authchecks/check_test.go index 8b117c10da2..001c8f2beec 100644 --- a/apps/advisor/pkg/app/checks/authchecks/check_test.go +++ b/apps/advisor/pkg/app/checks/authchecks/check_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" // Correct import path for the mock "github.com/stretchr/testify/require" @@ -91,4 +92,14 @@ func TestCheck_Item(t *testing.T) { require.Nil(t, item) require.ErrorIs(t, err, expectedErr) }) + + t.Run("should return nil when sso settings are not found", func(t *testing.T) { + mockService := ssosettingstests.NewMockService(t) + mockService.On("GetForProviderWithRedactedSecrets", ctx, providerID).Return(nil, ssosettings.ErrNotFound) + + c := New(mockService) + item, err := c.Item(ctx, providerID) + require.NoError(t, err) + require.Nil(t, item) + }) } diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check.go b/apps/advisor/pkg/app/checks/datasourcecheck/check.go index 6e7b158b56c..7bcab079160 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check.go @@ -69,10 +69,18 @@ func (c *check) Item(ctx context.Context, id string) (any, error) { if err != nil { return nil, err } - return c.DatasourceSvc.GetDataSource(ctx, &datasources.GetDataSourceQuery{ + ds, err := c.DatasourceSvc.GetDataSource(ctx, &datasources.GetDataSourceQuery{ UID: id, OrgID: requester.GetOrgID(), }) + if err != nil { + if errors.Is(err, datasources.ErrDataSourceNotFound) { + // The data source does not exist, skip the check + return nil, nil + } + return nil, err + } + return ds, nil } func (c *check) ID() string { diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go index 74f78168853..d9cb532b9d5 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go @@ -231,6 +231,19 @@ func TestCheck_Run(t *testing.T) { }) } +func TestCheck_Item(t *testing.T) { + t.Run("should return nil when datasource is not found", func(t *testing.T) { + mockDatasourceSvc := &MockDatasourceSvc{dss: []*datasources.DataSource{}} + check := &check{ + DatasourceSvc: mockDatasourceSvc, + } + ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) + item, err := check.Item(ctx, "invalid-uid") + assert.NoError(t, err) + assert.Nil(t, item) + }) +} + type MockDatasourceSvc struct { datasources.DataSourceService @@ -241,6 +254,13 @@ func (m *MockDatasourceSvc) GetAllDataSources(context.Context, *datasources.GetA return m.dss, nil } +func (m *MockDatasourceSvc) GetDataSource(context.Context, *datasources.GetDataSourceQuery) (*datasources.DataSource, error) { + if len(m.dss) == 0 { + return nil, datasources.ErrDataSourceNotFound + } + return m.dss[0], nil +} + type MockPluginContextProvider struct { pCtx backend.PluginContext } diff --git a/apps/advisor/pkg/app/checks/plugincheck/check.go b/apps/advisor/pkg/app/checks/plugincheck/check.go index 0bbf36fb4f5..43a5491f235 100644 --- a/apps/advisor/pkg/app/checks/plugincheck/check.go +++ b/apps/advisor/pkg/app/checks/plugincheck/check.go @@ -57,7 +57,7 @@ func (c *check) Items(ctx context.Context) ([]any, error) { func (c *check) Item(ctx context.Context, id string) (any, error) { p, exists := c.PluginStore.Plugin(ctx, id) if !exists { - return nil, fmt.Errorf("plugin %s not found", id) + return nil, nil } return p, nil } diff --git a/apps/advisor/pkg/app/checks/plugincheck/check_test.go b/apps/advisor/pkg/app/checks/plugincheck/check_test.go index 387708f8f7d..09a566f03ca 100644 --- a/apps/advisor/pkg/app/checks/plugincheck/check_test.go +++ b/apps/advisor/pkg/app/checks/plugincheck/check_test.go @@ -154,6 +154,18 @@ func TestRun(t *testing.T) { } } +func TestCheck_Item(t *testing.T) { + t.Run("should return nil when plugin is not found", func(t *testing.T) { + pluginStore := &mockPluginStore{plugins: []pluginstore.Plugin{}} + check := &check{ + PluginStore: pluginStore, + } + item, err := check.Item(context.Background(), "invalid-uid") + assert.NoError(t, err) + assert.Nil(t, item) + }) +} + type mockPluginStore struct { pluginstore.Store plugins []pluginstore.Plugin @@ -163,6 +175,13 @@ func (m *mockPluginStore) Plugins(ctx context.Context, t ...plugins.Type) []plug return m.plugins } +func (m *mockPluginStore) Plugin(ctx context.Context, id string) (pluginstore.Plugin, bool) { + if len(m.plugins) == 0 { + return pluginstore.Plugin{}, false + } + return m.plugins[0], true +} + type mockPluginRepo struct { repo.Service pluginInfo []repo.PluginInfo diff --git a/apps/advisor/pkg/app/utils.go b/apps/advisor/pkg/app/utils.go index 9a57096a21d..1b86c495dc0 100644 --- a/apps/advisor/pkg/app/utils.go +++ b/apps/advisor/pkg/app/utils.go @@ -123,6 +123,7 @@ func processCheckRetry(ctx context.Context, log logging.Logger, client resource. if err != nil { return fmt.Errorf("error initializing check: %w", err) } + failures := []advisorv0alpha1.CheckReportFailure{} item, err := check.Item(ctx, itemToRetry) if err != nil { setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError) @@ -131,27 +132,29 @@ func processCheckRetry(ctx context.Context, log logging.Logger, client resource. } return fmt.Errorf("error initializing check: %w", err) } - // Get the check type - var checkType resource.Object - checkType, err = typesClient.Get(ctx, resource.Identifier{ - Namespace: obj.GetNamespace(), - Name: check.ID(), - }) - if err != nil { - return err - } - // Run the steps - steps, err := filterSteps(checkType, check.Steps()) - if err != nil { - return err - } - failures, err := runStepsInParallel(ctx, log, &c.Spec, steps, []any{item}) - if err != nil { - setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError) - if setErr != nil { - return setErr + if item != nil { + // Get the check type + var checkType resource.Object + checkType, err = typesClient.Get(ctx, resource.Identifier{ + Namespace: obj.GetNamespace(), + Name: check.ID(), + }) + if err != nil { + return err + } + // Run the steps + steps, err := filterSteps(checkType, check.Steps()) + if err != nil { + return err + } + failures, err = runStepsInParallel(ctx, log, &c.Spec, steps, []any{item}) + if err != nil { + setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError) + if setErr != nil { + return setErr + } + return fmt.Errorf("error running steps: %w", err) } - return fmt.Errorf("error running steps: %w", err) } // Pull failures from the report for the items to retry c.CheckStatus.Report.Failures = slices.DeleteFunc(c.CheckStatus.Report.Failures, func(f advisorv0alpha1.CheckReportFailure) bool { diff --git a/apps/advisor/pkg/app/utils_test.go b/apps/advisor/pkg/app/utils_test.go index cf04dbad4f8..d852696d78d 100644 --- a/apps/advisor/pkg/app/utils_test.go +++ b/apps/advisor/pkg/app/utils_test.go @@ -226,6 +226,38 @@ func TestProcessCheckRetry_RetryError(t *testing.T) { assert.Equal(t, checks.StatusAnnotationError, obj.GetAnnotations()[checks.StatusAnnotation]) } +func TestProcessCheckRetry_SkipMissingItem(t *testing.T) { + obj := &advisorv0alpha1.Check{} + obj.SetAnnotations(map[string]string{ + checks.RetryAnnotation: "item", + checks.StatusAnnotation: checks.StatusAnnotationProcessed, + }) + obj.CheckStatus.Report.Failures = []advisorv0alpha1.CheckReportFailure{ + { + ItemID: "item", + StepID: "step", + }, + } + meta, err := utils.MetaAccessor(obj) + if err != nil { + t.Fatal(err) + } + meta.SetCreatedBy("user:1") + client := &mockClient{} + typesClient := &mockTypesClient{} + ctx := context.TODO() + + check := &mockCheck{ + items: []any{nil}, + } + + err = processCheckRetry(ctx, logging.DefaultLogger, client, typesClient, obj, check) + assert.NoError(t, err) + assert.Equal(t, checks.StatusAnnotationProcessed, obj.GetAnnotations()[checks.StatusAnnotation]) + assert.Empty(t, obj.GetAnnotations()[checks.RetryAnnotation]) + assert.Empty(t, obj.CheckStatus.Report.Failures) +} + func TestProcessCheckRetry_Success(t *testing.T) { obj := &advisorv0alpha1.Check{} obj.SetAnnotations(map[string]string{