From ba12ac68ccf5065388b014927c432bd9ef769c53 Mon Sep 17 00:00:00 2001 From: Roberto Jimenez Sanchez Date: Fri, 9 Jan 2026 16:34:41 +0100 Subject: [PATCH] Provisioning: Block connection deletion when repositories reference it This change prevents deletion of Connections when any Repository references them via spec.connection.name. This uses the field selector feature added in the previous commit. Implementation: - Add GetRepositoriesByConnection function to query repositories by connection name using the spec.connection.name field selector - Add validateDelete method to handle Connection deletion validation - Return Forbidden error with list of connected repository names Closes: https://github.com/grafana/git-ui-sync-project/issues/730 --- pkg/registry/apis/provisioning/register.go | 44 +++- pkg/registry/apis/provisioning/validation.go | 34 +++ .../apis/provisioning/validation_test.go | 200 +++++++++++++++++ .../apis/provisioning/connection_test.go | 201 ++++++++++++++++++ 4 files changed, 478 insertions(+), 1 deletion(-) create mode 100644 pkg/registry/apis/provisioning/validation_test.go 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{}) + }) +}