diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index e54a8c2fc28..04e4ac7fe5c 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -720,7 +720,13 @@ func (b *APIBuilder) Mutate(ctx context.Context, a admission.Attributes, o admis // TODO: move logic to a more appropriate place. Probably controller/validation.go func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) (err error) { obj := a.GetObject() - if obj == nil || a.GetOperation() == admission.Connect || a.GetOperation() == admission.Delete { + + // Handle Connection deletion - check for connected repositories + if a.GetOperation() == admission.Delete { + return b.validateDelete(ctx, a) + } + + if obj == nil || a.GetOperation() == admission.Connect { return nil // This is normal for sub-resource } @@ -800,6 +806,42 @@ func invalidRepositoryError(name string, list field.ErrorList) error { name, list) } +// validateDelete handles validation for delete operations +func (b *APIBuilder) validateDelete(ctx context.Context, a admission.Attributes) error { + // Only validate Connection deletions + if a.GetResource().Resource != "connections" { + return nil + } + + connectionName := a.GetName() + namespace := a.GetNamespace() + + // Set namespace in context for the repository store query + ctx, _, err := identity.WithProvisioningIdentity(ctx, namespace) + if err != nil { + return apierrors.NewInternalError(fmt.Errorf("failed to set provisioning identity: %w", err)) + } + + repos, err := GetRepositoriesByConnection(ctx, b.store, connectionName) + if err != nil { + return apierrors.NewInternalError(fmt.Errorf("failed to check for connected repositories: %w", err)) + } + + if len(repos) > 0 { + repoNames := make([]string, 0, len(repos)) + for _, repo := range repos { + repoNames = append(repoNames, repo.Name) + } + return apierrors.NewForbidden( + provisioning.ConnectionResourceInfo.GroupResource(), + connectionName, + fmt.Errorf("cannot delete connection while repositories are using it: %s", strings.Join(repoNames, ", ")), + ) + } + + return nil +} + func (b *APIBuilder) VerifyAgainstExistingRepositories(ctx context.Context, cfg *provisioning.Repository) *field.Error { return VerifyAgainstExistingRepositories(ctx, b.store, cfg) } diff --git a/pkg/registry/apis/provisioning/validation.go b/pkg/registry/apis/provisioning/validation.go index ff84872403c..643305ae8f0 100644 --- a/pkg/registry/apis/provisioning/validation.go +++ b/pkg/registry/apis/provisioning/validation.go @@ -7,6 +7,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/apis/meta/internalversion" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/endpoints/request" @@ -50,6 +51,39 @@ func GetRepositoriesInNamespace(ctx context.Context, store RepositoryLister) ([] return allRepositories, nil } +// GetRepositoriesByConnection retrieves all repositories that reference a specific connection +func GetRepositoriesByConnection(ctx context.Context, store RepositoryLister, connectionName string) ([]provisioning.Repository, error) { + var allRepositories []provisioning.Repository + continueToken := "" + + fieldSelector := fields.OneTermEqualSelector("spec.connection.name", connectionName) + + for { + obj, err := store.List(ctx, &internalversion.ListOptions{ + Limit: 100, + Continue: continueToken, + FieldSelector: fieldSelector, + }) + 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) diff --git a/pkg/registry/apis/provisioning/validation_test.go b/pkg/registry/apis/provisioning/validation_test.go new file mode 100644 index 00000000000..e6f0a04df4f --- /dev/null +++ b/pkg/registry/apis/provisioning/validation_test.go @@ -0,0 +1,200 @@ +package provisioning + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/runtime" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" +) + +// mockRepositoryLister is a mock implementation of RepositoryLister for testing +type mockRepositoryLister struct { + repositories []provisioning.Repository + listErr error + // Track the field selector used in List calls + lastFieldSelector fields.Selector +} + +func (m *mockRepositoryLister) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) { + if m.listErr != nil { + return nil, m.listErr + } + + // Store the field selector for verification + m.lastFieldSelector = options.FieldSelector + + // Filter repositories based on field selector if present + filteredRepos := m.repositories + if options.FieldSelector != nil && !options.FieldSelector.Empty() { + filteredRepos = make([]provisioning.Repository, 0) + for _, repo := range m.repositories { + // Simulate field selector matching for spec.connection.name + repoFields := fields.Set{ + "spec.connection.name": getRepoConnectionName(&repo), + } + if options.FieldSelector.Matches(repoFields) { + filteredRepos = append(filteredRepos, repo) + } + } + } + + return &provisioning.RepositoryList{ + Items: filteredRepos, + }, nil +} + +func getRepoConnectionName(repo *provisioning.Repository) string { + if repo.Spec.Connection == nil { + return "" + } + return repo.Spec.Connection.Name +} + +func TestGetRepositoriesByConnection(t *testing.T) { + tests := []struct { + name string + repositories []provisioning.Repository + connectionName string + expectedCount int + expectedNames []string + expectedErr bool + }{ + { + name: "empty repository list returns empty", + repositories: []provisioning.Repository{}, + connectionName: "test-conn", + expectedCount: 0, + expectedNames: []string{}, + }, + { + name: "finds single matching repository", + repositories: []provisioning.Repository{ + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-1"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "conn-a"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-2"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "conn-b"}, + }, + }, + }, + connectionName: "conn-a", + expectedCount: 1, + expectedNames: []string{"repo-1"}, + }, + { + name: "finds multiple matching repositories", + repositories: []provisioning.Repository{ + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-1"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "shared-conn"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-2"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "shared-conn"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-3"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "different-conn"}, + }, + }, + }, + connectionName: "shared-conn", + expectedCount: 2, + expectedNames: []string{"repo-1", "repo-2"}, + }, + { + name: "no matches returns empty list", + repositories: []provisioning.Repository{ + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-1"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "conn-a"}, + }, + }, + }, + connectionName: "non-existent", + expectedCount: 0, + expectedNames: []string{}, + }, + { + name: "empty connection name matches repos without connection", + repositories: []provisioning.Repository{ + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-with-conn"}, + Spec: provisioning.RepositorySpec{ + Connection: &provisioning.ConnectionInfo{Name: "some-conn"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "repo-without-conn"}, + Spec: provisioning.RepositorySpec{ + Connection: nil, + }, + }, + }, + connectionName: "", + expectedCount: 1, + expectedNames: []string{"repo-without-conn"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockRepositoryLister{repositories: tt.repositories} + ctx := context.Background() + + repos, err := GetRepositoriesByConnection(ctx, mock, tt.connectionName) + + if tt.expectedErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Len(t, repos, tt.expectedCount) + + // Verify the field selector was used + require.NotNil(t, mock.lastFieldSelector, "field selector should have been set") + expectedSelector := fields.OneTermEqualSelector("spec.connection.name", tt.connectionName) + assert.Equal(t, expectedSelector.String(), mock.lastFieldSelector.String()) + + // Verify the correct repositories were returned + actualNames := make([]string, len(repos)) + for i, repo := range repos { + actualNames[i] = repo.Name + } + for _, expectedName := range tt.expectedNames { + assert.Contains(t, actualNames, expectedName) + } + }) + } +} + +func TestGetRepositoriesByConnection_ListError(t *testing.T) { + mock := &mockRepositoryLister{ + listErr: assert.AnError, + } + ctx := context.Background() + + repos, err := GetRepositoriesByConnection(ctx, mock, "any-conn") + + require.Error(t, err) + assert.Nil(t, repos) +} diff --git a/pkg/tests/apis/provisioning/connection_test.go b/pkg/tests/apis/provisioning/connection_test.go index 99f32dffa93..ffa7ba8b561 100644 --- a/pkg/tests/apis/provisioning/connection_test.go +++ b/pkg/tests/apis/provisioning/connection_test.go @@ -731,3 +731,204 @@ func TestIntegrationProvisioning_RepositoryFieldSelectorByConnection(t *testing. assert.Contains(t, names, "repo-with-different-connection") }) } + +func TestIntegrationProvisioning_ConnectionDeletionBlocking(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + helper := runGrafana(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + t.Cleanup(cancel) + + createOptions := metav1.CreateOptions{} + + // Create a connection for testing deletion blocking + connName := "test-conn-delete-blocking" + _, err := helper.Connections.Resource.Create(ctx, &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Connection", + "metadata": map[string]any{ + "name": connName, + "namespace": "default", + }, + "spec": map[string]any{ + "type": "github", + "github": map[string]any{ + "appID": "123456", + "installationID": "454545", + }, + }, + "secure": map[string]any{ + "privateKey": map[string]any{ + "create": "someSecret", + }, + }, + }, + }, createOptions) + require.NoError(t, err, "failed to create test connection") + + t.Run("delete connection without connected repositories succeeds", func(t *testing.T) { + // Create a connection that has no repositories + emptyConnName := "test-conn-no-repos" + _, err := helper.Connections.Resource.Create(ctx, &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Connection", + "metadata": map[string]any{ + "name": emptyConnName, + "namespace": "default", + }, + "spec": map[string]any{ + "type": "github", + "github": map[string]any{ + "appID": "123457", + "installationID": "454546", + }, + }, + "secure": map[string]any{ + "privateKey": map[string]any{ + "create": "someSecret", + }, + }, + }, + }, createOptions) + require.NoError(t, err, "failed to create test connection without repos") + + // Delete should succeed since no repositories reference it + err = helper.Connections.Resource.Delete(ctx, emptyConnName, metav1.DeleteOptions{}) + require.NoError(t, err, "deleting connection without connected repositories should succeed") + + // Verify the connection is deleted + _, err = helper.Connections.Resource.Get(ctx, emptyConnName, metav1.GetOptions{}) + require.True(t, k8serrors.IsNotFound(err), "connection should be deleted") + }) + + t.Run("delete connection with connected repository fails", func(t *testing.T) { + // Create a repository that uses the connection + repoName := "repo-using-connection" + _, err := helper.Repositories.Resource.Create(ctx, &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Repository", + "metadata": map[string]any{ + "name": repoName, + "namespace": "default", + }, + "spec": map[string]any{ + "title": "Test Repository", + "type": "local", + "sync": map[string]any{ + "enabled": false, + "target": "folder", + }, + "local": map[string]any{ + "path": helper.ProvisioningPath, + }, + "connection": map[string]any{ + "name": connName, + }, + }, + }, + }, createOptions) + require.NoError(t, err, "failed to create repository using connection") + + // Attempt to delete the connection - should fail + err = helper.Connections.Resource.Delete(ctx, connName, metav1.DeleteOptions{}) + require.Error(t, err, "deleting connection with connected repository should fail") + require.True(t, k8serrors.IsForbidden(err), "error should be Forbidden, got: %v", err) + assert.Contains(t, err.Error(), repoName, "error should mention the connected repository name") + assert.Contains(t, err.Error(), "cannot delete connection while repositories are using it", "error should explain why deletion is blocked") + + // Clean up: delete the repository first + err = helper.Repositories.Resource.Delete(ctx, repoName, metav1.DeleteOptions{}) + require.NoError(t, err, "failed to delete test repository") + + // Wait for the repository to be deleted + require.Eventually(t, func() bool { + _, err := helper.Repositories.Resource.Get(ctx, repoName, metav1.GetOptions{}) + return k8serrors.IsNotFound(err) + }, 10*time.Second, 100*time.Millisecond, "repository should be deleted") + }) + + t.Run("delete connection after disconnecting repository succeeds", func(t *testing.T) { + // Now that the repository is deleted, the connection should be deletable + err = helper.Connections.Resource.Delete(ctx, connName, metav1.DeleteOptions{}) + require.NoError(t, err, "deleting connection after removing connected repositories should succeed") + + // Verify the connection is deleted + _, err = helper.Connections.Resource.Get(ctx, connName, metav1.GetOptions{}) + require.True(t, k8serrors.IsNotFound(err), "connection should be deleted") + }) + + t.Run("delete connection with multiple connected repositories lists all", func(t *testing.T) { + // Create a new connection + multiConnName := "test-conn-multi-repos" + _, err := helper.Connections.Resource.Create(ctx, &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Connection", + "metadata": map[string]any{ + "name": multiConnName, + "namespace": "default", + }, + "spec": map[string]any{ + "type": "github", + "github": map[string]any{ + "appID": "123458", + "installationID": "454547", + }, + }, + "secure": map[string]any{ + "privateKey": map[string]any{ + "create": "someSecret", + }, + }, + }, + }, createOptions) + require.NoError(t, err, "failed to create multi-repo test connection") + + // Create multiple repositories using this connection + repoNames := []string{"multi-repo-1", "multi-repo-2"} + for _, repoName := range repoNames { + _, err := helper.Repositories.Resource.Create(ctx, &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Repository", + "metadata": map[string]any{ + "name": repoName, + "namespace": "default", + }, + "spec": map[string]any{ + "title": "Test Repository " + repoName, + "type": "local", + "sync": map[string]any{ + "enabled": false, + "target": "folder", + }, + "local": map[string]any{ + "path": helper.ProvisioningPath, + }, + "connection": map[string]any{ + "name": multiConnName, + }, + }, + }, + }, createOptions) + require.NoError(t, err, "failed to create repository %s", repoName) + } + + // Attempt to delete - should fail and list all repos + err = helper.Connections.Resource.Delete(ctx, multiConnName, metav1.DeleteOptions{}) + require.Error(t, err, "deleting connection with multiple repos should fail") + require.True(t, k8serrors.IsForbidden(err), "error should be Forbidden") + for _, repoName := range repoNames { + assert.Contains(t, err.Error(), repoName, "error should mention repository %s", repoName) + } + + // Clean up + for _, repoName := range repoNames { + _ = helper.Repositories.Resource.Delete(ctx, repoName, metav1.DeleteOptions{}) + } + _ = helper.Connections.Resource.Delete(ctx, multiConnName, metav1.DeleteOptions{}) + }) +}