diff --git a/apps/provisioning/pkg/repository/test.go b/apps/provisioning/pkg/repository/test.go index 4c81013c477..251c8a9f06c 100644 --- a/apps/provisioning/pkg/repository/test.go +++ b/apps/provisioning/pkg/repository/test.go @@ -75,11 +75,6 @@ func ValidateRepository(repo Repository) field.ErrorList { "The target type is required when sync is enabled")) } - if cfg.Spec.Sync.Enabled && cfg.Spec.Sync.IntervalSeconds < 10 { - list = append(list, field.Invalid(field.NewPath("spec", "sync", "intervalSeconds"), - cfg.Spec.Sync.IntervalSeconds, fmt.Sprintf("Interval must be at least %d seconds", 10))) - } - // Reserved names (for now) reserved := []string{"classic", "sql", "SQL", "plugins", "legacy", "new", "job", "github", "s3", "gcs", "file", "new", "create", "update", "delete"} if slices.Contains(reserved, cfg.Name) { diff --git a/apps/provisioning/pkg/repository/test_test.go b/apps/provisioning/pkg/repository/test_test.go index 752f28bb979..4ebdd619d77 100644 --- a/apps/provisioning/pkg/repository/test_test.go +++ b/apps/provisioning/pkg/repository/test_test.go @@ -74,28 +74,6 @@ 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: "test", - 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 { @@ -191,11 +169,10 @@ func TestValidateRepository(t *testing.T) { m.On("Validate").Return(field.ErrorList{}) return m }(), - expectedErrs: 4, // Updated from 3 to 4 to match actual errors: + expectedErrs: 3, // 1. missing title // 2. sync target missing - // 3. sync interval too low - // 4. reserved name + // 3. reserved name }, { name: "branch workflow for non-github repository", @@ -447,18 +424,6 @@ func TestFromFieldError(t *testing.T) { expectedType: metav1.CauseTypeFieldValueRequired, expectedDetail: "a repository title must be given", }, - { - name: "invalid field error", - fieldError: &field.Error{ - Type: field.ErrorTypeInvalid, - Field: "spec.sync.intervalSeconds", - Detail: "Interval must be at least 10 seconds", - }, - expectedCode: http.StatusBadRequest, - expectedField: "spec.sync.intervalSeconds", - expectedType: metav1.CauseTypeFieldValueInvalid, - expectedDetail: "Interval must be at least 10 seconds", - }, { name: "not supported field error", fieldError: &field.Error{ diff --git a/conf/defaults.ini b/conf/defaults.ini index bc599285555..1c47f30e6f3 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -2229,3 +2229,8 @@ allowed_targets = instance|folder # Whether image rendering is allowed for dashboard previews. # Requires image rendering service to be configured. allow_image_rendering = true + +# The minimum sync interval that can be set for a repository. This is how often the controller +# will check if there has been any changes to the repository not propagated by a webhook. +# The minimum value is 10 seconds. +min_sync_interval = 10s diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 13994be8aee..95aea89904c 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -92,6 +92,7 @@ type APIBuilder struct { allowedTargets []provisioning.SyncTargetType allowImageRendering bool + minSyncInterval time.Duration features featuremgmt.FeatureToggles usageStats usagestats.Service @@ -144,6 +145,7 @@ func NewAPIBuilder( allowedTargets []provisioning.SyncTargetType, restConfigGetter func(context.Context) (*clientrest.Config, error), allowImageRendering bool, + minSyncInterval time.Duration, registry prometheus.Registerer, newStandaloneClientFactoryFunc func(loopbackConfigProvider apiserver.RestConfigProvider) resources.ClientFactory, // optional, only used for standalone apiserver ) *APIBuilder { @@ -156,6 +158,11 @@ 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, @@ -175,6 +182,7 @@ func NewAPIBuilder( allowedTargets: allowedTargets, restConfigGetter: restConfigGetter, allowImageRendering: allowImageRendering, + minSyncInterval: minSyncInterval, registry: registry, } @@ -261,6 +269,7 @@ func RegisterAPIService( allowedTargets, nil, // will use loopback instead cfg.ProvisioningAllowImageRendering, + cfg.ProvisioningMinSyncInterval, reg, nil, ) @@ -587,6 +596,11 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm "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"), diff --git a/pkg/registry/apis/provisioning/register_validate_test.go b/pkg/registry/apis/provisioning/register_validate_test.go new file mode 100644 index 00000000000..0834b534cc0 --- /dev/null +++ b/pkg/registry/apis/provisioning/register_validate_test.go @@ -0,0 +1,114 @@ +package provisioning + +import ( + "context" + "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/authentication/user" + + "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/apps/provisioning/pkg/repository" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestAPIBuilderValidate(t *testing.T) { + factory := repository.NewMockFactory(t) + mockRepo := repository.NewMockConfigRepository(t) + mockRepo.EXPECT().Validate().Return(nil) + factory.EXPECT().Build(mock.Anything, mock.Anything).Return(mockRepo, nil) + b := &APIBuilder{ + repoFactory: factory, + allowedTargets: []v0alpha1.SyncTargetType{v0alpha1.SyncTargetTypeFolder}, + allowImageRendering: false, + minSyncInterval: 30 * time.Second, + } + + t.Run("min sync interval is less than 10 seconds", func(t *testing.T) { + cfg := &v0alpha1.Repository{ + Spec: v0alpha1.RepositorySpec{ + Title: "repo", + Type: v0alpha1.GitHubRepositoryType, + Sync: v0alpha1.SyncOptions{Enabled: true, Target: v0alpha1.SyncTargetTypeFolder, IntervalSeconds: 5}, + }, + } + mockRepo.EXPECT().Config().Return(cfg) + + obj := newRepoObj("repo1", "default", cfg.Spec, v0alpha1.RepositoryStatus{}) + err := b.Validate(context.Background(), newAttributes(obj, nil, admission.Create), nil) + require.Error(t, err) + require.True(t, apierrors.IsInvalid(err)) + }) + + t.Run("image rendering is not enabled", func(t *testing.T) { + cfg2 := &v0alpha1.Repository{ + Spec: v0alpha1.RepositorySpec{ + Title: "repo", + Type: v0alpha1.GitHubRepositoryType, + Sync: v0alpha1.SyncOptions{Enabled: false, Target: v0alpha1.SyncTargetTypeFolder}, + GitHub: &v0alpha1.GitHubRepositoryConfig{URL: "https://github.com/acme/repo", Branch: "main", GenerateDashboardPreviews: true}, + }, + } + mockRepo.EXPECT().Config().Return(cfg2) + + obj := newRepoObj("repo2", "default", cfg2.Spec, v0alpha1.RepositoryStatus{}) + err := b.Validate(context.Background(), newAttributes(obj, nil, admission.Create), nil) + require.Error(t, err) + require.True(t, apierrors.IsInvalid(err)) + }) + + t.Run("sync target is not supported", func(t *testing.T) { + cfg3 := &v0alpha1.Repository{ + Spec: v0alpha1.RepositorySpec{ + Title: "repo", + Type: v0alpha1.GitHubRepositoryType, + Sync: v0alpha1.SyncOptions{Enabled: true, Target: v0alpha1.SyncTargetTypeInstance}, + }, + } + mockRepo.EXPECT().Config().Return(cfg3) + + obj := newRepoObj("repo3", "default", cfg3.Spec, v0alpha1.RepositoryStatus{}) + err := b.Validate(context.Background(), newAttributes(obj, nil, admission.Create), nil) + require.Error(t, err) + require.True(t, apierrors.IsInvalid(err)) + }) +} + +func newRepoObj(name string, ns string, spec v0alpha1.RepositorySpec, status v0alpha1.RepositoryStatus) *v0alpha1.Repository { + return &v0alpha1.Repository{ + TypeMeta: metav1.TypeMeta{APIVersion: v0alpha1.APIVERSION, Kind: "Repository"}, + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: spec, + Status: status, + } +} + +func newAttributes(obj, old runtime.Object, op admission.Operation) admission.Attributes { + return admission.NewAttributesRecord( + obj, + old, + v0alpha1.RepositoryResourceInfo.GroupVersionKind(), + "default", + func() string { + if obj != nil { + return obj.(*v0alpha1.Repository).Name + } + if old != nil { + return old.(*v0alpha1.Repository).Name + } + return "" + }(), + v0alpha1.RepositoryResourceInfo.GroupVersionResource(), + "", + op, + nil, + false, + &user.DefaultInfo{}, + ) +} diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index e31ceae24aa..11071c2a9fc 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -138,6 +138,7 @@ type Cfg struct { ProvisioningDisableControllers bool ProvisioningAllowedTargets []string ProvisioningAllowImageRendering bool + ProvisioningMinSyncInterval time.Duration ProvisioningRepositoryTypes []string ProvisioningLokiURL string ProvisioningLokiUser string @@ -2125,6 +2126,7 @@ func (cfg *Cfg) readProvisioningSettings(iniFile *ini.File) error { cfg.ProvisioningAllowedTargets = []string{"instance", "folder"} } cfg.ProvisioningAllowImageRendering = iniFile.Section("provisioning").Key("allow_image_rendering").MustBool(true) + cfg.ProvisioningMinSyncInterval = iniFile.Section("provisioning").Key("min_sync_interval").MustDuration(10 * time.Second) // Read job history configuration cfg.ProvisioningLokiURL = valueAsString(iniFile.Section("provisioning"), "loki_url", "")