From 3ce9137c19650eae2d9a9012b4ac6d24afd30774 Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Fri, 3 Oct 2025 01:16:37 -0600 Subject: [PATCH] Provisioning: Refactor to combine validation and test endpoint logic (#111965) Provisioning: Refactor test endpoint --- apps/provisioning/pkg/repository/tester.go | 84 ++++++ .../pkg/repository/tester_test.go | 204 +++++++++++++++ .../pkg/repository/{test.go => validator.go} | 92 +++---- .../{test_test.go => validator_test.go} | 241 ++++-------------- pkg/operators/provisioning/repo_operator.go | 25 +- .../apis/provisioning/controller/health.go | 6 +- .../provisioning/controller/health_test.go | 22 +- pkg/registry/apis/provisioning/register.go | 43 +--- .../provisioning/register_validate_test.go | 3 +- pkg/registry/apis/provisioning/test.go | 9 +- 10 files changed, 437 insertions(+), 292 deletions(-) create mode 100644 apps/provisioning/pkg/repository/tester.go create mode 100644 apps/provisioning/pkg/repository/tester_test.go rename apps/provisioning/pkg/repository/{test.go => validator.go} (60%) rename apps/provisioning/pkg/repository/{test_test.go => validator_test.go} (64%) diff --git a/apps/provisioning/pkg/repository/tester.go b/apps/provisioning/pkg/repository/tester.go new file mode 100644 index 00000000000..6bd65443d92 --- /dev/null +++ b/apps/provisioning/pkg/repository/tester.go @@ -0,0 +1,84 @@ +package repository + +import ( + "context" + "net/http" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" +) + +// SimpleRepositoryTester will validate the repository configuration, and then proceed to test the connection to the repository +type SimpleRepositoryTester struct { + validator RepositoryValidator +} + +func NewSimpleRepositoryTester(validator RepositoryValidator) SimpleRepositoryTester { + return SimpleRepositoryTester{ + validator: validator, + } +} + +// TestRepository validates the repository and then runs a health check +func (t *SimpleRepositoryTester) TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) { + errors := t.validator.ValidateRepository(repo) + if len(errors) > 0 { + rsp := &provisioning.TestResults{ + Code: http.StatusUnprocessableEntity, // Invalid + Success: false, + Errors: make([]provisioning.ErrorDetails, len(errors)), + } + for i, err := range errors { + rsp.Errors[i] = provisioning.ErrorDetails{ + Type: metav1.CauseType(err.Type), + Field: err.Field, + Detail: err.Detail, + } + } + return rsp, nil + } + + return repo.Test(ctx) +} + +type VerifyAgainstExistingRepositories func(ctx context.Context, cfg *provisioning.Repository) *field.Error // defined this way to prevent an import cycle + +// RepositoryTesterWithExistingChecker will validate the repository configuration, run a health check, and then compare it against existing repositories +type RepositoryTesterWithExistingChecker struct { + tester SimpleRepositoryTester + verify VerifyAgainstExistingRepositories +} + +func NewRepositoryTesterWithExistingChecker(tester SimpleRepositoryTester, verify VerifyAgainstExistingRepositories) RepositoryTesterWithExistingChecker { + return RepositoryTesterWithExistingChecker{ + tester: tester, + verify: verify, + } +} + +// TestRepositoryAndCheckExisting validates the repository, runs a health check, and then compares it against existing repositories +func (c *RepositoryTesterWithExistingChecker) TestRepositoryAndCheckExisting(ctx context.Context, repo Repository) (*provisioning.TestResults, error) { + rsp, err := c.tester.TestRepository(ctx, repo) + if err != nil { + return nil, err + } + + if rsp.Success { + cfg := repo.Config() + if validationErr := c.verify(ctx, cfg); validationErr != nil { + rsp = &provisioning.TestResults{ + Success: false, + Code: http.StatusUnprocessableEntity, + Errors: []provisioning.ErrorDetails{{ + Type: metav1.CauseType(validationErr.Type), + Field: validationErr.Field, + Detail: validationErr.Detail, + }}, + } + } + } + + return rsp, nil +} diff --git a/apps/provisioning/pkg/repository/tester_test.go b/apps/provisioning/pkg/repository/tester_test.go new file mode 100644 index 00000000000..91f29d15ceb --- /dev/null +++ b/apps/provisioning/pkg/repository/tester_test.go @@ -0,0 +1,204 @@ +package repository + +import ( + "context" + "fmt" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" +) + +func TestTestRepository(t *testing.T) { + tests := []struct { + name string + repository *MockRepository + expectedCode int + expectedErrs []provisioning.ErrorDetails + expectedError error + }{ + { + name: "validation fails", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + // Missing required title + }, + }) + m.On("Validate").Return(field.ErrorList{}) + return m + }(), + expectedCode: http.StatusUnprocessableEntity, + expectedErrs: []provisioning.ErrorDetails{{ + Type: metav1.CauseTypeFieldValueRequired, + Field: "spec.title", + Detail: "a repository title must be given", + }}, + }, + { + name: "test passes", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + }, + }) + m.On("Validate").Return(field.ErrorList{}) + m.On("Test", mock.Anything).Return(&provisioning.TestResults{ + Code: http.StatusOK, + Success: true, + }, nil) + return m + }(), + expectedCode: http.StatusOK, + expectedErrs: nil, + }, + { + name: "test fails with error", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + }, + }) + m.On("Validate").Return(field.ErrorList{}) + m.On("Test", mock.Anything).Return(nil, fmt.Errorf("test error")) + return m + }(), + expectedError: fmt.Errorf("test error"), + }, + { + name: "test fails with results", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + }, + }) + m.On("Validate").Return(field.ErrorList{}) + m.On("Test", mock.Anything).Return(&provisioning.TestResults{ + Code: http.StatusBadRequest, + Success: false, + Errors: []provisioning.ErrorDetails{{ + Type: metav1.CauseTypeFieldValueInvalid, + Field: "spec.property", + }}, + }, nil) + return m + }(), + expectedCode: http.StatusBadRequest, + expectedErrs: []provisioning.ErrorDetails{{ + Type: metav1.CauseTypeFieldValueInvalid, + Field: "spec.property", + }}, + }, + } + + tester := NewSimpleRepositoryTester(NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + results, err := tester.TestRepository(context.Background(), tt.repository) + + if tt.expectedError != nil { + require.Error(t, err) + require.Equal(t, tt.expectedError.Error(), err.Error()) + return + } + + require.NoError(t, err) + require.NotNil(t, results) + require.Equal(t, tt.expectedCode, results.Code) + + if tt.expectedErrs != nil { + require.Equal(t, tt.expectedErrs, results.Errors) + require.False(t, results.Success) + } else { + require.True(t, results.Success) + require.Empty(t, results.Errors) + } + }) + } +} + +func TestTester_TestRepository(t *testing.T) { + repository := NewMockRepository(t) + repository.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + }, + }) + repository.On("Validate").Return(field.ErrorList{}) + repository.On("Test", mock.Anything).Return(&provisioning.TestResults{ + Code: http.StatusOK, + Success: true, + }, nil) + + tester := NewSimpleRepositoryTester(NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)) + results, err := tester.TestRepository(context.Background(), repository) + require.NoError(t, err) + require.NotNil(t, results) + require.Equal(t, http.StatusOK, results.Code) + require.True(t, results.Success) +} + +func TestFromFieldError(t *testing.T) { + tests := []struct { + name string + fieldError *field.Error + expectedCode int + expectedField string + expectedType metav1.CauseType + expectedDetail string + }{ + { + name: "required field error", + fieldError: &field.Error{ + Type: field.ErrorTypeRequired, + Field: "spec.title", + Detail: "a repository title must be given", + }, + expectedCode: http.StatusBadRequest, + expectedField: "spec.title", + expectedType: metav1.CauseTypeFieldValueRequired, + expectedDetail: "a repository title must be given", + }, + { + name: "not supported field error", + fieldError: &field.Error{ + Type: field.ErrorTypeNotSupported, + Field: "spec.workflow", + Detail: "branch is only supported on git repositories", + }, + expectedCode: http.StatusBadRequest, + expectedField: "spec.workflow", + expectedType: metav1.CauseTypeFieldValueNotSupported, + expectedDetail: "branch is only supported on git repositories", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := FromFieldError(tt.fieldError) + + require.NotNil(t, result) + require.Equal(t, tt.expectedCode, result.Code) + require.False(t, result.Success) + require.Len(t, result.Errors, 1) + + errorDetail := result.Errors[0] + require.Equal(t, tt.expectedField, errorDetail.Field) + require.Equal(t, tt.expectedType, errorDetail.Type) + require.Equal(t, tt.expectedDetail, errorDetail.Detail) + }) + } +} diff --git a/apps/provisioning/pkg/repository/test.go b/apps/provisioning/pkg/repository/validator.go similarity index 60% rename from apps/provisioning/pkg/repository/test.go rename to apps/provisioning/pkg/repository/validator.go index 251c8a9f06c..73198f0d7ee 100644 --- a/apps/provisioning/pkg/repository/test.go +++ b/apps/provisioning/pkg/repository/validator.go @@ -1,10 +1,10 @@ package repository import ( - "context" "fmt" "net/http" "slices" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" @@ -12,57 +12,27 @@ import ( provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" ) -// RepositoryValidator interface for validating repositories against existing ones -type RepositoryValidator interface { - VerifyAgainstExistingRepositories(ctx context.Context, cfg *provisioning.Repository) *field.Error +type RepositoryValidator struct { + allowedTargets []provisioning.SyncTargetType + allowImageRendering bool + minSyncInterval time.Duration } -func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) { - return TestRepositoryWithValidator(ctx, repo, nil) +func NewValidator(minSyncInterval time.Duration, allowedTargets []provisioning.SyncTargetType, allowImageRendering bool) RepositoryValidator { + // do not allow minsync interval to be less than 10 + if minSyncInterval <= 10*time.Second { + minSyncInterval = 10 * time.Second + } + + return RepositoryValidator{ + allowedTargets: allowedTargets, + allowImageRendering: allowImageRendering, + minSyncInterval: minSyncInterval, + } } -func TestRepositoryWithValidator(ctx context.Context, repo Repository, validator RepositoryValidator) (*provisioning.TestResults, error) { - errors := ValidateRepository(repo) - if len(errors) > 0 { - rsp := &provisioning.TestResults{ - Code: http.StatusUnprocessableEntity, // Invalid - Success: false, - Errors: make([]provisioning.ErrorDetails, len(errors)), - } - for i, err := range errors { - rsp.Errors[i] = provisioning.ErrorDetails{ - Type: metav1.CauseType(err.Type), - Field: err.Field, - Detail: err.Detail, - } - } - return rsp, nil - } - - rsp, err := repo.Test(ctx) - if err != nil { - return nil, err - } - - if rsp.Success && validator != nil { - cfg := repo.Config() - if validationErr := validator.VerifyAgainstExistingRepositories(ctx, cfg); validationErr != nil { - rsp = &provisioning.TestResults{ - Success: false, - Code: http.StatusUnprocessableEntity, - Errors: []provisioning.ErrorDetails{{ - Type: metav1.CauseType(validationErr.Type), - Field: validationErr.Field, - Detail: validationErr.Detail, - }}, - } - } - } - - return rsp, nil -} - -func ValidateRepository(repo Repository) field.ErrorList { +// ValidateRepository solely does configuration checks on the repository object. It does not run a health check or compare against existing repositories. +func (v *RepositoryValidator) ValidateRepository(repo Repository) field.ErrorList { list := repo.Validate() cfg := repo.Config() @@ -70,9 +40,22 @@ func ValidateRepository(repo Repository) field.ErrorList { list = append(list, field.Required(field.NewPath("spec", "title"), "a repository title must be given")) } - if cfg.Spec.Sync.Enabled && cfg.Spec.Sync.Target == "" { - list = append(list, field.Required(field.NewPath("spec", "sync", "target"), - "The target type is required when sync is enabled")) + if cfg.Spec.Sync.Enabled { + if cfg.Spec.Sync.Target == "" { + list = append(list, field.Required(field.NewPath("spec", "sync", "target"), + "The target type is required when sync is enabled")) + } else if !slices.Contains(v.allowedTargets, cfg.Spec.Sync.Target) { + list = append(list, + field.Invalid( + field.NewPath("spec", "target"), + cfg.Spec.Sync.Target, + "sync target is not supported")) + } + + if cfg.Spec.Sync.IntervalSeconds < int64(v.minSyncInterval.Seconds()) { + list = append(list, field.Invalid(field.NewPath("spec", "sync", "intervalSeconds"), + cfg.Spec.Sync.IntervalSeconds, fmt.Sprintf("Interval must be at least %d seconds", int64(v.minSyncInterval.Seconds())))) + } } // Reserved names (for now) @@ -131,6 +114,13 @@ func ValidateRepository(repo Repository) field.ErrorList { } } + if !v.allowImageRendering && cfg.Spec.GitHub != nil && cfg.Spec.GitHub.GenerateDashboardPreviews { + list = append(list, + field.Invalid(field.NewPath("spec", "generateDashboardPreviews"), + cfg.Spec.GitHub.GenerateDashboardPreviews, + "image rendering is not enabled")) + } + return list } diff --git a/apps/provisioning/pkg/repository/test_test.go b/apps/provisioning/pkg/repository/validator_test.go similarity index 64% rename from apps/provisioning/pkg/repository/test_test.go rename to apps/provisioning/pkg/repository/validator_test.go index 4ebdd619d77..cb8b726a9bf 100644 --- a/apps/provisioning/pkg/repository/test_test.go +++ b/apps/provisioning/pkg/repository/validator_test.go @@ -1,12 +1,9 @@ package repository import ( - "context" - "fmt" - "net/http" "testing" + "time" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" @@ -74,6 +71,28 @@ func TestValidateRepository(t *testing.T) { require.Contains(t, errors.ToAggregate().Error(), "spec.sync.target: Required value") }, }, + { + name: "sync interval too low", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + Sync: provisioning.SyncOptions{ + Enabled: true, + Target: provisioning.SyncTargetTypeFolder, + IntervalSeconds: 5, + }, + }, + }) + m.On("Validate").Return(field.ErrorList{}) + return m + }(), + expectedErrs: 1, + validateError: func(t *testing.T, errors field.ErrorList) { + require.Contains(t, errors.ToAggregate().Error(), "spec.sync.intervalSeconds: Invalid value") + }, + }, { name: "reserved name", repository: func() *MockRepository { @@ -132,6 +151,27 @@ func TestValidateRepository(t *testing.T) { require.Contains(t, errors.ToAggregate().Error(), "spec.github: Invalid value") }, }, + { + name: "github enabled when image rendering is not allowed", + repository: func() *MockRepository { + m := NewMockRepository(t) + m.On("Config").Return(&provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + Type: provisioning.GitHubRepositoryType, + GitHub: &provisioning.GitHubRepositoryConfig{ + GenerateDashboardPreviews: true, + }, + }, + }) + m.On("Validate").Return(field.ErrorList{}) + return m + }(), + expectedErrs: 1, + validateError: func(t *testing.T, errors field.ErrorList) { + require.Contains(t, errors.ToAggregate().Error(), "spec.generateDashboardPreviews: Invalid value") + }, + }, { name: "mismatched git config", repository: func() *MockRepository { @@ -163,16 +203,18 @@ func TestValidateRepository(t *testing.T) { Sync: provisioning.SyncOptions{ Enabled: true, IntervalSeconds: 5, + Target: provisioning.SyncTargetTypeInstance, }, }, }) m.On("Validate").Return(field.ErrorList{}) return m }(), - expectedErrs: 3, + expectedErrs: 4, // 1. missing title // 2. sync target missing // 3. reserved name + // 4. sync target not supported }, { name: "branch workflow for non-github repository", @@ -258,9 +300,10 @@ func TestValidateRepository(t *testing.T) { }, } + validator := NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder}, false) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errors := ValidateRepository(tt.repository) + errors := validator.ValidateRepository(tt.repository) require.Len(t, errors, tt.expectedErrs) if tt.validateError != nil { tt.validateError(t, errors) @@ -268,189 +311,3 @@ func TestValidateRepository(t *testing.T) { }) } } - -func TestTestRepository(t *testing.T) { - tests := []struct { - name string - repository *MockRepository - expectedCode int - expectedErrs []provisioning.ErrorDetails - expectedError error - }{ - { - name: "validation fails", - repository: func() *MockRepository { - m := NewMockRepository(t) - m.On("Config").Return(&provisioning.Repository{ - Spec: provisioning.RepositorySpec{ - // Missing required title - }, - }) - m.On("Validate").Return(field.ErrorList{}) - return m - }(), - expectedCode: http.StatusUnprocessableEntity, - expectedErrs: []provisioning.ErrorDetails{{ - Type: metav1.CauseTypeFieldValueRequired, - Field: "spec.title", - Detail: "a repository title must be given", - }}, - }, - { - name: "test passes", - repository: func() *MockRepository { - m := NewMockRepository(t) - m.On("Config").Return(&provisioning.Repository{ - Spec: provisioning.RepositorySpec{ - Title: "Test Repo", - }, - }) - m.On("Validate").Return(field.ErrorList{}) - m.On("Test", mock.Anything).Return(&provisioning.TestResults{ - Code: http.StatusOK, - Success: true, - }, nil) - return m - }(), - expectedCode: http.StatusOK, - expectedErrs: nil, - }, - { - name: "test fails with error", - repository: func() *MockRepository { - m := NewMockRepository(t) - m.On("Config").Return(&provisioning.Repository{ - Spec: provisioning.RepositorySpec{ - Title: "Test Repo", - }, - }) - m.On("Validate").Return(field.ErrorList{}) - m.On("Test", mock.Anything).Return(nil, fmt.Errorf("test error")) - return m - }(), - expectedError: fmt.Errorf("test error"), - }, - { - name: "test fails with results", - repository: func() *MockRepository { - m := NewMockRepository(t) - m.On("Config").Return(&provisioning.Repository{ - Spec: provisioning.RepositorySpec{ - Title: "Test Repo", - }, - }) - m.On("Validate").Return(field.ErrorList{}) - m.On("Test", mock.Anything).Return(&provisioning.TestResults{ - Code: http.StatusBadRequest, - Success: false, - Errors: []provisioning.ErrorDetails{{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: "spec.property", - }}, - }, nil) - return m - }(), - expectedCode: http.StatusBadRequest, - expectedErrs: []provisioning.ErrorDetails{{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: "spec.property", - }}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - results, err := TestRepository(context.Background(), tt.repository) - - if tt.expectedError != nil { - require.Error(t, err) - require.Equal(t, tt.expectedError.Error(), err.Error()) - return - } - - require.NoError(t, err) - require.NotNil(t, results) - require.Equal(t, tt.expectedCode, results.Code) - - if tt.expectedErrs != nil { - require.Equal(t, tt.expectedErrs, results.Errors) - require.False(t, results.Success) - } else { - require.True(t, results.Success) - require.Empty(t, results.Errors) - } - }) - } -} - -func TestTester_TestRepository(t *testing.T) { - repository := NewMockRepository(t) - repository.On("Config").Return(&provisioning.Repository{ - Spec: provisioning.RepositorySpec{ - Title: "Test Repo", - }, - }) - repository.On("Validate").Return(field.ErrorList{}) - repository.On("Test", mock.Anything).Return(&provisioning.TestResults{ - Code: http.StatusOK, - Success: true, - }, nil) - - results, err := TestRepository(context.Background(), repository) - require.NoError(t, err) - require.NotNil(t, results) - require.Equal(t, http.StatusOK, results.Code) - require.True(t, results.Success) -} - -func TestFromFieldError(t *testing.T) { - tests := []struct { - name string - fieldError *field.Error - expectedCode int - expectedField string - expectedType metav1.CauseType - expectedDetail string - }{ - { - name: "required field error", - fieldError: &field.Error{ - Type: field.ErrorTypeRequired, - Field: "spec.title", - Detail: "a repository title must be given", - }, - expectedCode: http.StatusBadRequest, - expectedField: "spec.title", - expectedType: metav1.CauseTypeFieldValueRequired, - expectedDetail: "a repository title must be given", - }, - { - name: "not supported field error", - fieldError: &field.Error{ - Type: field.ErrorTypeNotSupported, - Field: "spec.workflow", - Detail: "branch is only supported on git repositories", - }, - expectedCode: http.StatusBadRequest, - expectedField: "spec.workflow", - expectedType: metav1.CauseTypeFieldValueNotSupported, - expectedDetail: "branch is only supported on git repositories", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := FromFieldError(tt.fieldError) - - require.NotNil(t, result) - require.Equal(t, tt.expectedCode, result.Code) - require.False(t, result.Success) - require.Len(t, result.Errors, 1) - - errorDetail := result.Errors[0] - require.Equal(t, tt.expectedField, errorDetail.Field) - require.Equal(t, tt.expectedType, errorDetail.Type) - require.Equal(t, tt.expectedDetail, errorDetail.Detail) - }) - } -} diff --git a/pkg/operators/provisioning/repo_operator.go b/pkg/operators/provisioning/repo_operator.go index 0ec243bb41e..f96653a1d87 100644 --- a/pkg/operators/provisioning/repo_operator.go +++ b/pkg/operators/provisioning/repo_operator.go @@ -11,9 +11,11 @@ import ( "github.com/grafana/grafana-app-sdk/logging" appcontroller "github.com/grafana/grafana/apps/provisioning/pkg/controller" + "github.com/grafana/grafana/apps/provisioning/pkg/repository" "github.com/prometheus/client_golang/prometheus" "k8s.io/client-go/tools/cache" + "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/registry/apis/provisioning/controller" "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs" @@ -66,8 +68,14 @@ func RunRepoController(deps server.OperatorDependencies) error { if err != nil { return fmt.Errorf("create API client job store: %w", err) } + + allowedTargets := []v0alpha1.SyncTargetType{} + for _, target := range controllerCfg.allowedTargets { + allowedTargets = append(allowedTargets, v0alpha1.SyncTargetType(target)) + } + validator := repository.NewValidator(controllerCfg.minSyncInterval, allowedTargets, controllerCfg.allowImageRendering) statusPatcher := appcontroller.NewRepositoryStatusPatcher(controllerCfg.provisioningClient.ProvisioningV0alpha1()) - healthChecker := controller.NewHealthChecker(statusPatcher, deps.Registerer) + healthChecker := controller.NewHealthChecker(statusPatcher, deps.Registerer, repository.NewSimpleRepositoryTester(validator)) repoInformer := informerFactory.Provisioning().V0alpha1().Repositories() controller, err := controller.NewRepositoryController( @@ -98,7 +106,10 @@ func RunRepoController(deps server.OperatorDependencies) error { type repoControllerConfig struct { provisioningControllerConfig - workerCount int + workerCount int + allowedTargets []string + allowImageRendering bool + minSyncInterval time.Duration } func getRepoControllerConfig(cfg *setting.Cfg, registry prometheus.Registerer) (*repoControllerConfig, error) { @@ -106,8 +117,18 @@ func getRepoControllerConfig(cfg *setting.Cfg, registry prometheus.Registerer) ( if err != nil { return nil, err } + + allowedTargets := []string{} + cfg.SectionWithEnvOverrides("provisioning").Key("allowed_targets").Strings("|") + if len(allowedTargets) == 0 { + allowedTargets = []string{"folder"} + } + return &repoControllerConfig{ provisioningControllerConfig: *controllerCfg, + allowedTargets: allowedTargets, workerCount: cfg.SectionWithEnvOverrides("operator").Key("worker_count").MustInt(1), + allowImageRendering: cfg.SectionWithEnvOverrides("provisioning").Key("allow_image_rendering").MustBool(false), + minSyncInterval: cfg.SectionWithEnvOverrides("provisioning").Key("min_sync_interval").MustDuration(1 * time.Minute), }, nil } diff --git a/pkg/registry/apis/provisioning/controller/health.go b/pkg/registry/apis/provisioning/controller/health.go index cf3aeedc36d..5e5953b4117 100644 --- a/pkg/registry/apis/provisioning/controller/health.go +++ b/pkg/registry/apis/provisioning/controller/health.go @@ -23,14 +23,16 @@ type StatusPatcher interface { type HealthChecker struct { statusPatcher StatusPatcher healthMetrics healthMetrics + tester repository.SimpleRepositoryTester } // NewHealthChecker creates a new health checker -func NewHealthChecker(statusPatcher StatusPatcher, registry prometheus.Registerer) *HealthChecker { +func NewHealthChecker(statusPatcher StatusPatcher, registry prometheus.Registerer, tester repository.SimpleRepositoryTester) *HealthChecker { healthMetrics := registerHealthMetrics(registry) return &HealthChecker{ statusPatcher: statusPatcher, healthMetrics: healthMetrics, + tester: tester, } } @@ -176,7 +178,7 @@ func (hc *HealthChecker) refreshHealth(ctx context.Context, repo repository.Repo hc.healthMetrics.RecordHealthCheck(outcome, time.Since(start).Seconds()) }() - res, err := repository.TestRepository(ctx, repo) + res, err := hc.tester.TestRepository(ctx, repo) if err != nil { outcome = utils.ErrorOutcome logger.Error("failed to test repository", "error", err) diff --git a/pkg/registry/apis/provisioning/controller/health_test.go b/pkg/registry/apis/provisioning/controller/health_test.go index 74f8a127be6..722500d5a94 100644 --- a/pkg/registry/apis/provisioning/controller/health_test.go +++ b/pkg/registry/apis/provisioning/controller/health_test.go @@ -13,13 +13,15 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + repository "github.com/grafana/grafana/apps/provisioning/pkg/repository" "github.com/grafana/grafana/pkg/registry/apis/provisioning/controller/mocks" ) func TestNewHealthChecker(t *testing.T) { mockPatcher := mocks.NewStatusPatcher(t) - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) assert.NotNil(t, hc) assert.Equal(t, mockPatcher, hc.statusPatcher) @@ -136,7 +138,8 @@ func TestShouldCheckHealth(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockPatcher := mocks.NewStatusPatcher(t) - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) result := hc.ShouldCheckHealth(tt.repo) assert.Equal(t, tt.expected, result) @@ -223,7 +226,8 @@ func TestHasRecentFailure(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockPatcher := mocks.NewStatusPatcher(t) - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) result := hc.HasRecentFailure(tt.healthStatus, tt.failureType) assert.Equal(t, tt.expected, result) @@ -265,7 +269,8 @@ func TestRecordFailure(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockPatcher := mocks.NewStatusPatcher(t) - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) repo := &provisioning.Repository{ Status: provisioning.RepositoryStatus{ @@ -310,7 +315,8 @@ func TestRecordFailure(t *testing.T) { func TestRecordFailureFunction(t *testing.T) { mockPatcher := mocks.NewStatusPatcher(t) - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) testErr := errors.New("test error") result := hc.recordFailure(provisioning.HealthFailureHook, testErr) @@ -447,7 +453,8 @@ func TestRefreshHealth(t *testing.T) { testError: tt.testError, } - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) if tt.expectPatch { if tt.patchError != nil { @@ -557,7 +564,8 @@ func TestHasHealthStatusChanged(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockPatcher := mocks.NewStatusPatcher(t) - hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry()) + validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true) + hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator)) result := hc.hasHealthStatusChanged(tt.old, tt.new) assert.Equal(t, tt.expected, result) diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 5590cbeafe5..2aeb926be21 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -6,7 +6,6 @@ import ( "fmt" "net/http" "net/url" - "slices" "strings" "time" @@ -92,7 +91,6 @@ type APIBuilder struct { allowedTargets []provisioning.SyncTargetType allowImageRendering bool - minSyncInterval time.Duration features featuremgmt.FeatureToggles usageStats usagestats.Service @@ -117,6 +115,7 @@ type APIBuilder struct { access authlib.AccessChecker statusPatcher *appcontroller.RepositoryStatusPatcher healthChecker *controller.HealthChecker + validator repository.RepositoryValidator // Extras provides additional functionality to the API. extras []Extra extraWorkers []jobs.Worker @@ -158,11 +157,6 @@ func NewAPIBuilder( parsers := resources.NewParserFactory(clients) resourceLister := resources.NewResourceListerForMigrations(unified, legacyMigrator, storageStatus) - // do not allow minsync interval to be less than 10 - if minSyncInterval <= 10*time.Second { - minSyncInterval = 10 * time.Second - } - b := &APIBuilder{ onlyApiServer: onlyApiServer, tracer: tracer, @@ -179,11 +173,11 @@ func NewAPIBuilder( access: access, jobHistoryConfig: jobHistoryConfig, extraWorkers: extraWorkers, - allowedTargets: allowedTargets, restConfigGetter: restConfigGetter, + allowedTargets: allowedTargets, allowImageRendering: allowImageRendering, - minSyncInterval: minSyncInterval, registry: registry, + validator: repository.NewValidator(minSyncInterval, allowedTargets, allowImageRendering), } for _, builder := range extraBuilders { @@ -484,7 +478,7 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI storage[provisioning.RepositoryResourceInfo.StoragePath("status")] = repositoryStatusStorage // TODO: Add some logic so that the connectors can registered themselves and we don't have logic all over the place - storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b) + storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b, repository.NewRepositoryTesterWithExistingChecker(repository.NewSimpleRepositoryTester(b.validator), b.VerifyAgainstExistingRepositories)) storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = NewFilesConnector(b, b.parsers, b.clients, b.access) storage[provisioning.RepositoryResourceInfo.StoragePath("refs")] = NewRefsConnector(b) storage[provisioning.RepositoryResourceInfo.StoragePath("resources")] = &listConnector{ @@ -585,29 +579,14 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm return err } - list := repository.ValidateRepository(repo) + // ALL configuration validations should be done in ValidateRepository - + // this is how the UI is able to show proper validation errors + // + // the only time to add configuration checks here is if you need to compare + // the incoming change to the current configuration + list := b.validator.ValidateRepository(repo) cfg := repo.Config() - if !slices.Contains(b.allowedTargets, cfg.Spec.Sync.Target) { - list = append(list, - field.Invalid( - field.NewPath("spec", "target"), - cfg.Spec.Sync.Target, - "sync target is not supported")) - } - - if cfg.Spec.Sync.Enabled && cfg.Spec.Sync.IntervalSeconds < int64(b.minSyncInterval.Seconds()) { - list = append(list, field.Invalid(field.NewPath("spec", "sync", "intervalSeconds"), - cfg.Spec.Sync.IntervalSeconds, fmt.Sprintf("Interval must be at least %d seconds", int64(b.minSyncInterval.Seconds())))) - } - - if !b.allowImageRendering && cfg.Spec.GitHub != nil && cfg.Spec.GitHub.GenerateDashboardPreviews { - list = append(list, - field.Invalid(field.NewPath("spec", "generateDashboardPreviews"), - cfg.Spec.GitHub.GenerateDashboardPreviews, - "image rendering is not enabled")) - } - if a.GetOperation() == admission.Update { oldRepo, err := b.asRepository(ctx, a.GetOldObject(), nil) if err != nil { @@ -683,7 +662,7 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH } b.statusPatcher = appcontroller.NewRepositoryStatusPatcher(b.GetClient()) - b.healthChecker = controller.NewHealthChecker(b.statusPatcher, b.registry) + b.healthChecker = controller.NewHealthChecker(b.statusPatcher, b.registry, repository.NewSimpleRepositoryTester(b.validator)) // if running solely CRUD, skip the rest of the setup if b.onlyApiServer { diff --git a/pkg/registry/apis/provisioning/register_validate_test.go b/pkg/registry/apis/provisioning/register_validate_test.go index 0834b534cc0..18b366e4de8 100644 --- a/pkg/registry/apis/provisioning/register_validate_test.go +++ b/pkg/registry/apis/provisioning/register_validate_test.go @@ -23,11 +23,12 @@ func TestAPIBuilderValidate(t *testing.T) { mockRepo := repository.NewMockConfigRepository(t) mockRepo.EXPECT().Validate().Return(nil) factory.EXPECT().Build(mock.Anything, mock.Anything).Return(mockRepo, nil) + validator := repository.NewValidator(30*time.Second, []v0alpha1.SyncTargetType{v0alpha1.SyncTargetTypeFolder}, false) b := &APIBuilder{ repoFactory: factory, allowedTargets: []v0alpha1.SyncTargetType{v0alpha1.SyncTargetTypeFolder}, allowImageRendering: false, - minSyncInterval: 30 * time.Second, + validator: validator, } t.Run("min sync interval is less than 10 seconds", func(t *testing.T) { diff --git a/pkg/registry/apis/provisioning/test.go b/pkg/registry/apis/provisioning/test.go index b664e9e9039..43e28bdb334 100644 --- a/pkg/registry/apis/provisioning/test.go +++ b/pkg/registry/apis/provisioning/test.go @@ -31,7 +31,6 @@ type HealthCheckerProvider interface { type ConnectorDependencies interface { RepoGetter HealthCheckerProvider - repository.RepositoryValidator GetRepoFactory() repository.Factory } @@ -39,15 +38,15 @@ type testConnector struct { getter RepoGetter factory repository.Factory healthProvider HealthCheckerProvider - validator repository.RepositoryValidator + tester repository.RepositoryTesterWithExistingChecker } -func NewTestConnector(deps ConnectorDependencies) *testConnector { +func NewTestConnector(deps ConnectorDependencies, tester repository.RepositoryTesterWithExistingChecker) *testConnector { return &testConnector{ factory: deps.GetRepoFactory(), getter: deps, healthProvider: deps, - validator: deps, + tester: tester, } } @@ -186,7 +185,7 @@ func (s *testConnector) Connect(ctx context.Context, name string, opts runtime.O } } else { // Testing temporary repository - just run test without status update - rsp, err = repository.TestRepositoryWithValidator(ctx, repo, s.validator) + rsp, err = s.tester.TestRepositoryAndCheckExisting(ctx, repo) if err != nil { responder.Error(err) return