diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 5de39950c5c..79559bb2238 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -612,15 +612,31 @@ func (b *APIBuilder) verifyAgaintsExistingRepositories(cfg *provisioning.Reposit } 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 && v.Spec.Sync.Target == provisioning.SyncTargetTypeInstance { + if v.Name != cfg.Name { return field.Forbidden(field.NewPath("spec", "sync", "target"), - "Another repository is already targeting root: "+v.Name) + "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 { + return field.Forbidden(field.NewPath("spec", "sync", "target"), + "Cannot create folder repository when instance repository exists: "+v.Name) } } } - if len(all) >= 10 { + // 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") } diff --git a/pkg/tests/apis/provisioning/helper_test.go b/pkg/tests/apis/provisioning/helper_test.go index 85ac36e24e9..894f7bd8174 100644 --- a/pkg/tests/apis/provisioning/helper_test.go +++ b/pkg/tests/apis/provisioning/helper_test.go @@ -767,3 +767,49 @@ func countFilesInDir(rootPath string) (int, error) { }) return count, err } + +// CleanupAllRepos deletes all repositories and waits for them to be fully removed +func (h *provisioningTestHelper) CleanupAllRepos(t *testing.T) { + t.Helper() + ctx := context.Background() + + // First, get all repositories that exist + list, err := h.Repositories.Resource.List(ctx, metav1.ListOptions{}) + if err != nil || len(list.Items) == 0 { + return // Nothing to clean up + } + + // Wait for any active jobs to complete before deleting repositories + require.EventuallyWithT(t, func(collect *assert.CollectT) { + activeJobs, err := h.Jobs.Resource.List(ctx, metav1.ListOptions{}) + if !assert.NoError(collect, err, "failed to list active jobs") { + return + } + assert.Equal(collect, 0, len(activeJobs.Items), "all active jobs should complete before cleanup") + }, time.Second*20, time.Millisecond*100, "active jobs should complete before cleanup") + + // Now delete all repositories with retries + require.EventuallyWithT(t, func(collect *assert.CollectT) { + list, err := h.Repositories.Resource.List(ctx, metav1.ListOptions{}) + if !assert.NoError(collect, err) { + return + } + + for _, repo := range list.Items { + err := h.Repositories.Resource.Delete(ctx, repo.GetName(), metav1.DeleteOptions{}) + // Don't fail if already deleted (404 is OK) + if err != nil { + assert.True(collect, apierrors.IsNotFound(err), "Should be able to delete repository %s (or it should already be deleted)", repo.GetName()) + } + } + }, time.Second*10, time.Millisecond*100, "should be able to delete all repositories") + + // Then wait for repositories to be fully deleted to ensure clean state + require.EventuallyWithT(t, func(collect *assert.CollectT) { + list, err := h.Repositories.Resource.List(ctx, metav1.ListOptions{}) + if !assert.NoError(collect, err) { + return + } + assert.Equal(collect, 0, len(list.Items), "repositories should be cleaned up") + }, time.Second*15, time.Millisecond*100, "repositories should be cleaned up between subtests") +} diff --git a/pkg/tests/apis/provisioning/movejob_test.go b/pkg/tests/apis/provisioning/movejob_test.go index 9ef14e75d78..770f0f9bce9 100644 --- a/pkg/tests/apis/provisioning/movejob_test.go +++ b/pkg/tests/apis/provisioning/movejob_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -186,14 +187,24 @@ func TestIntegrationProvisioning_MoveJob(t *testing.T) { }) t.Run("move by resource reference", func(t *testing.T) { + // Delete the existing repository to avoid conflicts with validation rules + err := helper.Repositories.Resource.Delete(ctx, repo, metav1.DeleteOptions{}) + require.NoError(t, err) + + // Wait for repository to be fully deleted + require.EventuallyWithT(t, func(collect *assert.CollectT) { + _, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}) + assert.True(collect, apierrors.IsNotFound(err), "repository should be deleted") + }, time.Second*5, time.Millisecond*50, "repository should be deleted before creating new one") + // Create a unique repository for resource reference testing to avoid contamination const refRepo = "move-ref-test-repo" localRefTmp := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ "Name": refRepo, "SyncEnabled": true, - "SyncTarget": "folder", + "SyncTarget": "instance", }) - _, err := helper.Repositories.Resource.Create(ctx, localRefTmp, metav1.CreateOptions{}) + _, err = helper.Repositories.Resource.Create(ctx, localRefTmp, metav1.CreateOptions{}) require.NoError(t, err) // Create modified test files with unique UIDs for ResourceRef testing diff --git a/pkg/tests/apis/provisioning/repository_test.go b/pkg/tests/apis/provisioning/repository_test.go index 82355bb7604..e3213517869 100644 --- a/pkg/tests/apis/provisioning/repository_test.go +++ b/pkg/tests/apis/provisioning/repository_test.go @@ -3,6 +3,7 @@ package provisioning import ( "context" "encoding/json" + "fmt" "net/http" "strings" "testing" @@ -292,6 +293,12 @@ func TestIntegrationProvisioning_CreatingGitHubRepository(t *testing.T) { assert.Equal(collect, 0, len(found.Items), "expected dashboards to be deleted") }, time.Second*20, time.Millisecond*10, "Expected dashboards to be deleted") + // Wait for repository to be fully deleted before subtests run + require.EventuallyWithT(t, func(collect *assert.CollectT) { + _, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}) + assert.True(collect, apierrors.IsNotFound(err), "repository should be deleted") + }, time.Second*10, time.Millisecond*50, "repository should be deleted before subtests") + t.Run("github url cleanup", func(t *testing.T) { tests := []struct { name string @@ -318,8 +325,9 @@ func TestIntegrationProvisioning_CreatingGitHubRepository(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { input := helper.RenderObject(t, "testdata/github-readonly.json.tmpl", map[string]any{ - "Name": test.name, - "URL": test.input, + "Name": test.name, + "URL": test.input, + "SyncTarget": "instance", }) _, err := helper.Repositories.Resource.Create(ctx, input, metav1.CreateOptions{}) @@ -334,11 +342,220 @@ func TestIntegrationProvisioning_CreatingGitHubRepository(t *testing.T) { err = helper.Repositories.Resource.Delete(ctx, test.name, metav1.DeleteOptions{}) require.NoError(t, err, "failed to delete") + + // Wait for repository to be fully deleted before next test + require.EventuallyWithT(t, func(collect *assert.CollectT) { + _, err := helper.Repositories.Resource.Get(ctx, test.name, metav1.GetOptions{}) + assert.True(collect, apierrors.IsNotFound(err), "repository should be deleted") + }, time.Second*5, time.Millisecond*50, "repository should be deleted") }) } }) } +func TestIntegrationProvisioning_InstanceSyncValidation(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + helper := runGrafana(t) + ctx := context.Background() + + t.Run("single instance sync is allowed", func(t *testing.T) { + // Ensure clean state + helper.CleanupAllRepos(t) + + repoName := "instance-repo-single" + testRepo := TestRepo{ + Name: repoName, + Target: "instance", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: 0, + } + + // Create instance sync repository - should succeed + helper.CreateRepo(t, testRepo) + + // Clean up at end of test + helper.CleanupAllRepos(t) + }) + + t.Run("instance sync rejected when any other repository exists", func(t *testing.T) { + // Ensure clean state + helper.CleanupAllRepos(t) + + existingFolderName := "existing-folder-repo" + instanceRepoName := "instance-repo-blocked" + + // Create a folder sync repository first + folderTestRepo := TestRepo{ + Name: existingFolderName, + Target: "folder", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: 1, // One folder expected after sync + } + helper.CreateRepo(t, folderTestRepo) + + // Try to create an instance sync repository - should fail because any other repository exists + instanceRepo := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": instanceRepoName, + "SyncEnabled": true, + "SyncTarget": "instance", + }) + + _, err := helper.Repositories.Resource.Create(ctx, instanceRepo, metav1.CreateOptions{FieldValidation: "Strict"}) + require.Error(t, err, "instance sync repository should be rejected when any other repository exists") + + // Verify the error message mentions that instance can only be created when no other repositories exist + statusError := helper.RequireApiErrorStatus(err, metav1.StatusReasonInvalid, http.StatusUnprocessableEntity) + require.Contains(t, statusError.Message, "Instance repository can only be created when no other repositories exist. Found: "+existingFolderName) + + // Clean up at end of test + helper.CleanupAllRepos(t) + }) + + t.Run("multiple folder syncs are allowed", func(t *testing.T) { + // Ensure clean state + helper.CleanupAllRepos(t) + + firstFolderName := "folder-repo-multi-1" + secondFolderName := "folder-repo-multi-2" + + // Create first folder sync repository + folderTestRepo1 := TestRepo{ + Name: firstFolderName, + Target: "folder", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: 1, // One folder expected after sync + } + helper.CreateRepo(t, folderTestRepo1) + + // Create second folder sync repository - should succeed + folderTestRepo2 := TestRepo{ + Name: secondFolderName, + Target: "folder", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: 2, // Two folders expected after sync (1 + 1) + } + helper.CreateRepo(t, folderTestRepo2) + + // Clean up at end of test + helper.CleanupAllRepos(t) + }) + + t.Run("folder sync is rejected when instance sync exists", func(t *testing.T) { + // Ensure clean state + helper.CleanupAllRepos(t) + + instanceRepoName := "instance-blocking-folder" + folderRepoName := "folder-blocked-by-instance" + + // Create instance sync repository first + instanceTestRepo := TestRepo{ + Name: instanceRepoName, + Target: "instance", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: 0, + } + helper.CreateRepo(t, instanceTestRepo) + + // Try to create folder sync repository - should fail + folderRepo := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": folderRepoName, + "SyncEnabled": true, + "SyncTarget": "folder", + }) + + _, err := helper.Repositories.Resource.Create(ctx, folderRepo, metav1.CreateOptions{FieldValidation: "Strict"}) + require.Error(t, err, "folder sync repository should be rejected when instance sync exists") + + // Verify the error message mentions the existing instance repository + statusError := helper.RequireApiErrorStatus(err, metav1.StatusReasonInvalid, http.StatusUnprocessableEntity) + require.Contains(t, statusError.Message, "Cannot create folder repository when instance repository exists: "+instanceRepoName) + + // Clean up at end of test + helper.CleanupAllRepos(t) + }) + + t.Run("instance sync can only be created when no repositories exist", func(t *testing.T) { + // Ensure clean state + helper.CleanupAllRepos(t) + + // This test verifies that instance sync can ONLY be created when there are no other repositories + instanceRepoName := "instance-only-when-empty" + + // First, create instance sync repository when no other repositories exist - should succeed + instanceTestRepo := TestRepo{ + Name: instanceRepoName, + Target: "instance", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: 0, + } + helper.CreateRepo(t, instanceTestRepo) + + // Now try to create any other repository - should fail + otherRepoName := "other-repo-blocked" + otherRepo := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": otherRepoName, + "SyncEnabled": true, + "SyncTarget": "folder", + }) + + _, err := helper.Repositories.Resource.Create(ctx, otherRepo, metav1.CreateOptions{FieldValidation: "Strict"}) + require.Error(t, err, "folder sync repository should be rejected when instance sync exists") + + statusError := helper.RequireApiErrorStatus(err, metav1.StatusReasonInvalid, http.StatusUnprocessableEntity) + require.Contains(t, statusError.Message, "Cannot create folder repository when instance repository exists: "+instanceRepoName) + + // Clean up at end of test + helper.CleanupAllRepos(t) + }) + + t.Run("repository limit validation", func(t *testing.T) { + // Ensure clean state + helper.CleanupAllRepos(t) + + // This test verifies the 10 repository limit validation by actually creating 10 repositories + + // Create 10 repositories - should all succeed + for i := 1; i <= 10; i++ { + repoName := fmt.Sprintf("limit-test-repo-%d", i) + limitTestRepo := TestRepo{ + Name: repoName, + Target: "folder", + Copies: map[string]string{}, // No files needed for this test + ExpectedDashboards: 0, + ExpectedFolders: i, // Each repository creates a folder, so total = i + } + helper.CreateRepo(t, limitTestRepo) + } + + // Try to create the 11th repository - should fail due to limit + eleventhRepoName := "limit-test-repo-11" + eleventhRepo := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": eleventhRepoName, + "SyncEnabled": true, + "SyncTarget": "folder", + }) + + _, err := helper.Repositories.Resource.Create(ctx, eleventhRepo, metav1.CreateOptions{FieldValidation: "Strict"}) + require.Error(t, err, "11th repository should be rejected due to limit") + + // Verify the error message mentions the repository limit + statusError := helper.RequireApiErrorStatus(err, metav1.StatusReasonInvalid, http.StatusUnprocessableEntity) + require.Contains(t, statusError.Message, "Maximum number of 10 repositories reached") + + // Clean up at end of test + helper.CleanupAllRepos(t) + }) +} + func TestIntegrationProvisioning_RunLocalRepository(t *testing.T) { if testing.Short() { t.Skip("skipping integration test")