From 31ae013e8de536de5031e96146f3831401fe2e8b Mon Sep 17 00:00:00 2001 From: Costa Alexoglou Date: Thu, 25 Sep 2025 17:10:13 +0200 Subject: [PATCH] chore: add validations to test endpoint (#111622) * chore: add validations to test endpoint * Validate path --------- Co-authored-by: Clarity-89 --- apps/provisioning/pkg/repository/test.go | 31 ++++- pkg/registry/apis/provisioning/register.go | 117 ++--------------- pkg/registry/apis/provisioning/routes.go | 2 +- pkg/registry/apis/provisioning/test.go | 23 ++-- pkg/registry/apis/provisioning/validation.go | 124 ++++++++++++++++++ pkg/tests/apis/provisioning/health_test.go | 4 +- .../apis/provisioning/repository_test.go | 1 - .../provisioning/utils/getFormErrors.ts | 1 + 8 files changed, 185 insertions(+), 118 deletions(-) create mode 100644 pkg/registry/apis/provisioning/validation.go diff --git a/apps/provisioning/pkg/repository/test.go b/apps/provisioning/pkg/repository/test.go index cba9736e2d6..b6307237a0a 100644 --- a/apps/provisioning/pkg/repository/test.go +++ b/apps/provisioning/pkg/repository/test.go @@ -12,7 +12,16 @@ 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 +} + func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) { + return TestRepositoryWithValidator(ctx, repo, nil) +} + +func TestRepositoryWithValidator(ctx context.Context, repo Repository, validator RepositoryValidator) (*provisioning.TestResults, error) { errors := ValidateRepository(repo) if len(errors) > 0 { rsp := &provisioning.TestResults{ @@ -30,7 +39,27 @@ func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestRes return rsp, nil } - return repo.Test(ctx) + 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 { diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 472f0a088ec..a63f42bb987 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -6,14 +6,12 @@ import ( "fmt" "net/http" "net/url" - "path/filepath" "slices" "strings" "time" "github.com/prometheus/client_golang/prometheus" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -21,7 +19,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authorization/authorizer" - "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" clientrest "k8s.io/client-go/rest" @@ -478,7 +475,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, b.repoFactory, b) + storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b) 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{ @@ -622,7 +619,7 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm } // Exit early if we have already found errors - targetError := b.verifyAgainstExistingRepositories(cfg) + targetError := b.VerifyAgainstExistingRepositories(ctx, cfg) if targetError != nil { return invalidRepositoryError(a.GetName(), field.ErrorList{targetError}) } @@ -636,105 +633,8 @@ func invalidRepositoryError(name string, list field.ErrorList) error { name, list) } -func (b *APIBuilder) getRepositoriesInNamespace(ctx context.Context) ([]provisioning.Repository, error) { - var allRepositories []provisioning.Repository - continueToken := "" - - for { - obj, err := b.store.List(ctx, &internalversion.ListOptions{ - Limit: 100, - Continue: continueToken, - }) - if err != nil { - return nil, err - } - - repositoryList, ok := obj.(*provisioning.RepositoryList) - if !ok { - return nil, fmt.Errorf("expected repository list") - } - - allRepositories = append(allRepositories, repositoryList.Items...) - - continueToken = repositoryList.GetContinue() - if continueToken == "" { - break - } - } - - return allRepositories, nil -} - -// TODO: move this to a more appropriate place. Probably controller/validation.go -func (b *APIBuilder) verifyAgainstExistingRepositories(cfg *provisioning.Repository) *field.Error { - ctx, _, err := identity.WithProvisioningIdentity(context.Background(), cfg.Namespace) - if err != nil { - return &field.Error{Type: field.ErrorTypeInternal, Detail: err.Error()} - } - all, err := b.getRepositoriesInNamespace(request.WithNamespace(ctx, cfg.Namespace)) - if err != nil { - return field.Forbidden(field.NewPath("spec"), - "Unable to verify root target: "+err.Error()) - } - - if cfg.Spec.Sync.Target == provisioning.SyncTargetTypeInstance { - // Instance sync can only be created if NO other repositories exist - for _, v := range all { - if v.Name != cfg.Name { - return field.Forbidden(field.NewPath("spec", "sync", "target"), - "Instance repository can only be created when no other repositories exist. Found: "+v.Name) - } - } - } else { - // Folder sync cannot be created if an instance repository exists - for _, v := range all { - if v.Spec.Sync.Target == provisioning.SyncTargetTypeInstance && v.Name != cfg.Name { - return field.Forbidden(field.NewPath("spec", "sync", "target"), - "Cannot create folder repository when instance repository exists: "+v.Name) - } - } - } - - // If repo is git, ensure no other repository is defined with a child path - if cfg.Spec.Type.IsGit() { - for _, v := range all { - // skip itself - if cfg.Name == v.Name { - continue - } - if v.URL() == cfg.URL() { - if v.Path() == cfg.Path() { - return field.Forbidden(field.NewPath("spec", string(cfg.Spec.Type), "path"), - fmt.Sprintf("%s: %s", ErrRepositoryDuplicatePath.Error(), v.Name)) - } - - relPath, err := filepath.Rel(v.Path(), cfg.Path()) - if err != nil { - return field.Forbidden(field.NewPath("spec", string(cfg.Spec.Type), "path"), "failed to evaluate path: "+err.Error()) - } - // https://pkg.go.dev/path/filepath#Rel - // Rel will return "../" if the relative paths are not related - if !strings.HasPrefix(relPath, "../") { - return field.Forbidden(field.NewPath("spec", string(cfg.Spec.Type), "path"), - fmt.Sprintf("%s: %s", ErrRepositoryParentFolderConflict.Error(), v.Name)) - } - } - } - } - - // Count repositories excluding the current one being created/updated - count := 0 - for _, v := range all { - if v.Name != cfg.Name { - count++ - } - } - if count >= 10 { - return field.Forbidden(field.NewPath("spec"), - "Maximum number of 10 repositories reached") - } - - return nil +func (b *APIBuilder) VerifyAgainstExistingRepositories(ctx context.Context, cfg *provisioning.Repository) *field.Error { + return VerifyAgainstExistingRepositories(ctx, b.store, cfg) } func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartHookFunc, error) { @@ -786,7 +686,10 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH } // Create the repository resources factory - usageMetricCollector := usage.MetricCollector(b.tracer, b.getRepositoriesInNamespace, b.unified) + repositoryListerWrapper := func(ctx context.Context) ([]provisioning.Repository, error) { + return GetRepositoriesInNamespace(ctx, b.store) + } + usageMetricCollector := usage.MetricCollector(b.tracer, repositoryListerWrapper, b.unified) b.usageStats.RegisterMetricsFunc(usageMetricCollector) metrics := jobs.RegisterJobMetrics(b.registry) @@ -1373,6 +1276,10 @@ func (b *APIBuilder) GetRepository(ctx context.Context, name string) (repository return b.asRepository(ctx, obj, nil) } +func (b *APIBuilder) GetRepoFactory() repository.Factory { + return b.repoFactory +} + func (b *APIBuilder) GetHealthyRepository(ctx context.Context, name string) (repository.Repository, error) { repo, err := b.GetRepository(ctx, name) if err != nil { diff --git a/pkg/registry/apis/provisioning/routes.go b/pkg/registry/apis/provisioning/routes.go index b653e96d417..4ffefcdfdbb 100644 --- a/pkg/registry/apis/provisioning/routes.go +++ b/pkg/registry/apis/provisioning/routes.go @@ -150,7 +150,7 @@ func (b *APIBuilder) handleSettings(w http.ResponseWriter, r *http.Request) { } // TODO: check if lister could list too many repositories or resources - all, err := b.getRepositoriesInNamespace(request.WithNamespace(r.Context(), u.GetNamespace())) + all, err := GetRepositoriesInNamespace(request.WithNamespace(r.Context(), u.GetNamespace()), b.store) if err != nil { errhttp.Write(r.Context(), err, w) return diff --git a/pkg/registry/apis/provisioning/test.go b/pkg/registry/apis/provisioning/test.go index c1b87ab70ab..b664e9e9039 100644 --- a/pkg/registry/apis/provisioning/test.go +++ b/pkg/registry/apis/provisioning/test.go @@ -28,21 +28,26 @@ type HealthCheckerProvider interface { GetHealthChecker() *controller.HealthChecker } +type ConnectorDependencies interface { + RepoGetter + HealthCheckerProvider + repository.RepositoryValidator + GetRepoFactory() repository.Factory +} + type testConnector struct { getter RepoGetter factory repository.Factory healthProvider HealthCheckerProvider + validator repository.RepositoryValidator } -func NewTestConnector( - getter RepoGetter, - factory repository.Factory, - healthProvider HealthCheckerProvider, -) *testConnector { +func NewTestConnector(deps ConnectorDependencies) *testConnector { return &testConnector{ - factory: factory, - getter: getter, - healthProvider: healthProvider, + factory: deps.GetRepoFactory(), + getter: deps, + healthProvider: deps, + validator: deps, } } @@ -181,7 +186,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.TestRepository(ctx, repo) + rsp, err = repository.TestRepositoryWithValidator(ctx, repo, s.validator) if err != nil { responder.Error(err) return diff --git a/pkg/registry/apis/provisioning/validation.go b/pkg/registry/apis/provisioning/validation.go new file mode 100644 index 00000000000..ff84872403c --- /dev/null +++ b/pkg/registry/apis/provisioning/validation.go @@ -0,0 +1,124 @@ +package provisioning + +import ( + "context" + "fmt" + "path/filepath" + "strings" + + "k8s.io/apimachinery/pkg/apis/meta/internalversion" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/endpoints/request" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" +) + +// RepositoryLister interface for listing repositories +type RepositoryLister interface { + List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) +} + +// GetRepositoriesInNamespace retrieves all repositories in a given namespace +func GetRepositoriesInNamespace(ctx context.Context, store RepositoryLister) ([]provisioning.Repository, error) { + var allRepositories []provisioning.Repository + continueToken := "" + + for { + obj, err := store.List(ctx, &internalversion.ListOptions{ + Limit: 100, + Continue: continueToken, + }) + if err != nil { + return nil, err + } + + repositoryList, ok := obj.(*provisioning.RepositoryList) + if !ok { + return nil, fmt.Errorf("expected repository list") + } + + allRepositories = append(allRepositories, repositoryList.Items...) + + continueToken = repositoryList.GetContinue() + if continueToken == "" { + break + } + } + + return allRepositories, nil +} + +// VerifyAgainstExistingRepositories validates a repository configuration against existing repositories +func VerifyAgainstExistingRepositories(ctx context.Context, store RepositoryLister, cfg *provisioning.Repository) *field.Error { + ctx, _, err := identity.WithProvisioningIdentity(ctx, cfg.Namespace) + if err != nil { + return &field.Error{Type: field.ErrorTypeInternal, Detail: err.Error()} + } + all, err := GetRepositoriesInNamespace(request.WithNamespace(ctx, cfg.Namespace), store) + if err != nil { + return field.Forbidden(field.NewPath("spec"), + "Unable to verify root target: "+err.Error()) + } + + if cfg.Spec.Sync.Target == provisioning.SyncTargetTypeInstance { + // Instance sync can only be created if NO other repositories exist + for _, v := range all { + if v.Name != cfg.Name { + return field.Forbidden(field.NewPath("spec", "sync", "target"), + "Instance repository can only be created when no other repositories exist. Found: "+v.Name) + } + } + } else { + // Folder sync cannot be created if an instance repository exists + for _, v := range all { + if v.Spec.Sync.Target == provisioning.SyncTargetTypeInstance && v.Name != cfg.Name { + return field.Forbidden(field.NewPath("spec", "sync", "target"), + "Cannot create folder repository when instance repository exists: "+v.Name) + } + } + } + + // If repo is git, ensure no other repository is defined with a child path + if cfg.Spec.Type.IsGit() { + for _, v := range all { + // skip itself + if cfg.Name == v.Name { + continue + } + if v.URL() == cfg.URL() { + if v.Path() == cfg.Path() { + return field.Invalid(field.NewPath("spec", string(cfg.Spec.Type), "path"), + cfg.Path(), + fmt.Sprintf("%s: %s", ErrRepositoryDuplicatePath.Error(), v.Name)) + } + + relPath, err := filepath.Rel(v.Path(), cfg.Path()) + if err != nil { + return field.Invalid(field.NewPath("spec", string(cfg.Spec.Type), "path"), cfg.Path(), "failed to evaluate path: "+err.Error()) + } + // https://pkg.go.dev/path/filepath#Rel + // Rel will return "../" if the relative paths are not related + if !strings.HasPrefix(relPath, "../") { + return field.Invalid(field.NewPath("spec", string(cfg.Spec.Type), "path"), cfg.Path(), + fmt.Sprintf("%s: %s", ErrRepositoryParentFolderConflict.Error(), v.Name)) + } + } + } + } + + // Count repositories excluding the current one being created/updated + count := 0 + for _, v := range all { + if v.Name != cfg.Name { + count++ + } + } + if count >= 10 { + return field.Forbidden(field.NewPath("spec"), + "Maximum number of 10 repositories reached") + } + + return nil +} diff --git a/pkg/tests/apis/provisioning/health_test.go b/pkg/tests/apis/provisioning/health_test.go index e5ff3f482e0..6221a489702 100644 --- a/pkg/tests/apis/provisioning/health_test.go +++ b/pkg/tests/apis/provisioning/health_test.go @@ -22,7 +22,9 @@ func TestIntegrationHealth(t *testing.T) { ctx := context.Background() repo := "test-repo-health" helper.CreateRepo(t, TestRepo{ - Name: repo, + Name: repo, + Target: "folder", + ExpectedFolders: 1, }) // Verify the health status before calling the endpoint diff --git a/pkg/tests/apis/provisioning/repository_test.go b/pkg/tests/apis/provisioning/repository_test.go index 4c49ba76814..273d78c4e99 100644 --- a/pkg/tests/apis/provisioning/repository_test.go +++ b/pkg/tests/apis/provisioning/repository_test.go @@ -267,7 +267,6 @@ func TestIntegrationProvisioning_RepositoryValidation(t *testing.T) { if test.expectError != nil { require.Error(t, err, "Expected error for repository with path: %s", test.path) - require.ErrorContains(t, err, test.expectError.Error(), "Error should contain expected message for path: %s", test.path) var statusError *apierrors.StatusError if errors.As(err, &statusError) { diff --git a/public/app/features/provisioning/utils/getFormErrors.ts b/public/app/features/provisioning/utils/getFormErrors.ts index dd3fe985239..7adcca8063c 100644 --- a/public/app/features/provisioning/utils/getFormErrors.ts +++ b/public/app/features/provisioning/utils/getFormErrors.ts @@ -17,6 +17,7 @@ export const getFormErrors = (errors: ErrorDetails[]): FormErrorTuple => { 'local.path', 'github.branch', 'github.url', + 'github.path', 'secure.token', 'gitlab.branch', 'gitlab.url',