Provisioning: Add finalizer-based deletion handling for connections

This change adds a finalizer to connections to prevent race conditions
when deleting connections while repositories reference them. The finalizer
ensures that even if a repository is created during a connection deletion,
the connection will not be deleted until all repositories are removed.

Implementation:
- Add BlockDeletionFinalizer constant for connections
- Add finalizer to connections on creation in Mutate function
- Update ConnectionController to handle deletion and check for repositories
- Controller blocks deletion by keeping finalizer when repositories exist
- Controller removes finalizer only when no repositories reference connection
- Add comprehensive unit tests for finalizer handling

This complements the admission webhook validation by providing controller-level
protection against race conditions.

apps/provisioning/pkg/connection/finalizers.go

@@ -0,0 +1,4 @@
+package connection
+
+// BlockDeletionFinalizer prevents deletion of connections while repositories reference them
+const BlockDeletionFinalizer = "block-deletion-while-repositories-exist"

pkg/registry/apis/provisioning/controller/connection.go

@@ -4,9 +4,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	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/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/cache"
@@ -14,9 +19,11 @@ import (
 
 	"github.com/grafana/grafana-app-sdk/logging"
 	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+	connectionvalidation "github.com/grafana/grafana/apps/provisioning/pkg/connection"
 	client "github.com/grafana/grafana/apps/provisioning/pkg/generated/clientset/versioned/typed/provisioning/v0alpha1"
 	informer "github.com/grafana/grafana/apps/provisioning/pkg/generated/informers/externalversions/provisioning/v0alpha1"
 	listers "github.com/grafana/grafana/apps/provisioning/pkg/generated/listers/provisioning/v0alpha1"
+	"k8s.io/apimachinery/pkg/fields"
 )
 
 const connectionLoggerName = "provisioning-connection-controller"
@@ -41,6 +48,11 @@ type ConnectionStatusPatcher interface {
 	Patch(ctx context.Context, conn *provisioning.Connection, patchOperations ...map[string]interface{}) error
 }
 
+// RepositoryLister interface for listing repositories
+type RepositoryLister interface {
+	List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error)
+}
+
 // ConnectionController controls Connection resources.
 type ConnectionController struct {
 	client     client.ProvisioningV0alpha1Interface
@@ -49,6 +61,7 @@ type ConnectionController struct {
 	logger     logging.Logger
 
 	statusPatcher ConnectionStatusPatcher
+	repoLister    RepositoryLister
 
 	queue workqueue.TypedRateLimitingInterface[*connectionQueueItem]
 }
@@ -58,6 +71,7 @@ func NewConnectionController(
 	provisioningClient client.ProvisioningV0alpha1Interface,
 	connInformer informer.ConnectionInformer,
 	statusPatcher ConnectionStatusPatcher,
+	repoLister RepositoryLister,
 ) (*ConnectionController, error) {
 	cc := &ConnectionController{
 		client:     provisioningClient,
@@ -70,6 +84,7 @@ func NewConnectionController(
 			},
 		),
 		statusPatcher: statusPatcher,
+		repoLister:    repoLister,
 		logger:        logging.DefaultLogger.With("logger", connectionLoggerName),
 	}
 
@@ -171,10 +186,9 @@ func (cc *ConnectionController) process(ctx context.Context, item *connectionQue
 		return err
 	}
 
-	// Skip if being deleted
+	// Handle deletion if being deleted
 	if conn.DeletionTimestamp != nil {
-		logger.Info("connection is being deleted, skipping")
-		return nil
+		return cc.handleDelete(ctx, conn)
 	}
 
 	hasSpecChanged := conn.Generation != conn.Status.ObservedGeneration
@@ -229,6 +243,82 @@ func (cc *ConnectionController) process(ctx context.Context, item *connectionQue
 	return nil
 }
 
+func (cc *ConnectionController) handleDelete(ctx context.Context, conn *provisioning.Connection) error {
+	logger := logging.FromContext(ctx)
+	logger.Info("handle connection delete")
+
+	// Check if finalizer is present
+	hasFinalizer := false
+	for _, f := range conn.Finalizers {
+		if f == connectionvalidation.BlockDeletionFinalizer {
+			hasFinalizer = true
+			break
+		}
+	}
+
+	if !hasFinalizer {
+		logger.Info("no finalizer to process")
+		return nil
+	}
+
+	// Check if any repositories reference this connection using field selector
+	fieldSelector := fields.OneTermEqualSelector("spec.connection.name", conn.Name)
+	var allRepos []provisioning.Repository
+	continueToken := ""
+	var err error
+
+	for {
+		var obj runtime.Object
+		obj, err = cc.repoLister.List(ctx, &internalversion.ListOptions{
+			Limit:         100,
+			Continue:      continueToken,
+			FieldSelector: fieldSelector,
+		})
+		if err != nil {
+			logger.Error("failed to check for connected repositories", "error", err)
+			return fmt.Errorf("check for connected repositories: %w", err)
+		}
+
+		repositoryList, ok := obj.(*provisioning.RepositoryList)
+		if !ok {
+			logger.Error("expected repository list", "type", fmt.Sprintf("%T", obj))
+			return fmt.Errorf("expected repository list, got %T", obj)
+		}
+
+		allRepos = append(allRepos, repositoryList.Items...)
+
+		continueToken = repositoryList.GetContinue()
+		if continueToken == "" {
+			break
+		}
+	}
+
+	if len(allRepos) > 0 {
+		repoNames := make([]string, 0, len(allRepos))
+		for _, repo := range allRepos {
+			repoNames = append(repoNames, repo.Name)
+		}
+		logger.Info("cannot delete connection while repositories reference it", "repositories", repoNames)
+		// Don't remove finalizer - this will prevent deletion
+		// The connection will remain in deletion state until repositories are removed
+		return fmt.Errorf("cannot delete connection while repositories are using it: %s", strings.Join(repoNames, ", "))
+	}
+
+	// No repositories reference this connection, remove finalizer to allow deletion
+	logger.Info("no repositories reference connection, removing finalizer")
+	_, err = cc.client.Connections(conn.GetNamespace()).
+		Patch(ctx, conn.Name, types.JSONPatchType, []byte(`[
+			{ "op": "remove", "path": "/metadata/finalizers" }
+		]`), metav1.PatchOptions{
+			FieldManager: "provisioning-connection-controller",
+		})
+	if err != nil {
+		return fmt.Errorf("remove finalizer: %w", err)
+	}
+
+	return nil
+}
+
 // shouldCheckHealth determines if a connection health check should be performed.
 func (cc *ConnectionController) shouldCheckHealth(conn *provisioning.Connection) bool {
 	// If the connection has been updated, always check health

pkg/registry/apis/provisioning/controller/connection_test.go

@@ -1,13 +1,25 @@
 package controller
 
 import (
+	"context"
+	"errors"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/watch"
+	"k8s.io/client-go/rest"
 
 	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+	connectionvalidation "github.com/grafana/grafana/apps/provisioning/pkg/connection"
+	applyconfiguration "github.com/grafana/grafana/apps/provisioning/pkg/generated/applyconfiguration/provisioning/v0alpha1"
+	client "github.com/grafana/grafana/apps/provisioning/pkg/generated/clientset/versioned/typed/provisioning/v0alpha1"
 )
 
 func TestConnectionController_shouldCheckHealth(t *testing.T) {
@@ -285,3 +297,327 @@ func TestConnectionController_processNextWorkItem(t *testing.T) {
 		assert.NotNil(t, cc)
 	})
 }
+
+// mockRepositoryLister is a mock implementation of RepositoryLister for testing
+type mockRepositoryLister struct {
+	mock.Mock
+}
+
+func (m *mockRepositoryLister) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) {
+	args := m.Called(ctx, options)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(runtime.Object), args.Error(1)
+}
+
+// mockConnectionInterface is a mock implementation of client.ConnectionInterface for testing
+type mockConnectionInterface struct {
+	mock.Mock
+}
+
+func (m *mockConnectionInterface) Create(ctx context.Context, connection *provisioning.Connection, opts metav1.CreateOptions) (*provisioning.Connection, error) {
+	args := m.Called(ctx, connection, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+func (m *mockConnectionInterface) Update(ctx context.Context, connection *provisioning.Connection, opts metav1.UpdateOptions) (*provisioning.Connection, error) {
+	args := m.Called(ctx, connection, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+func (m *mockConnectionInterface) UpdateStatus(ctx context.Context, connection *provisioning.Connection, opts metav1.UpdateOptions) (*provisioning.Connection, error) {
+	args := m.Called(ctx, connection, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+func (m *mockConnectionInterface) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error {
+	args := m.Called(ctx, name, opts)
+	return args.Error(0)
+}
+
+func (m *mockConnectionInterface) DeleteCollection(ctx context.Context, opts metav1.DeleteOptions, listOpts metav1.ListOptions) error {
+	args := m.Called(ctx, opts, listOpts)
+	return args.Error(0)
+}
+
+func (m *mockConnectionInterface) Get(ctx context.Context, name string, opts metav1.GetOptions) (*provisioning.Connection, error) {
+	args := m.Called(ctx, name, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+func (m *mockConnectionInterface) List(ctx context.Context, opts metav1.ListOptions) (*provisioning.ConnectionList, error) {
+	args := m.Called(ctx, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.ConnectionList), args.Error(1)
+}
+
+func (m *mockConnectionInterface) Watch(ctx context.Context, opts metav1.ListOptions) (watch.Interface, error) {
+	args := m.Called(ctx, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(watch.Interface), args.Error(1)
+}
+
+func (m *mockConnectionInterface) Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (*provisioning.Connection, error) {
+	args := m.Called(ctx, name, pt, data, opts, subresources)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+func (m *mockConnectionInterface) Apply(ctx context.Context, connection *applyconfiguration.ConnectionApplyConfiguration, opts metav1.ApplyOptions) (*provisioning.Connection, error) {
+	args := m.Called(ctx, connection, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+func (m *mockConnectionInterface) ApplyStatus(ctx context.Context, connection *applyconfiguration.ConnectionApplyConfiguration, opts metav1.ApplyOptions) (*provisioning.Connection, error) {
+	args := m.Called(ctx, connection, opts)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*provisioning.Connection), args.Error(1)
+}
+
+// mockProvisioningV0alpha1InterfaceForConnections is a mock implementation of client.ProvisioningV0alpha1Interface for connection tests
+type mockProvisioningV0alpha1InterfaceForConnections struct {
+	mock.Mock
+	connections *mockConnectionInterface
+}
+
+func (m *mockProvisioningV0alpha1InterfaceForConnections) RESTClient() rest.Interface {
+	panic("not needed for testing")
+}
+
+func (m *mockProvisioningV0alpha1InterfaceForConnections) HistoricJobs(namespace string) client.HistoricJobInterface {
+	panic("not needed for testing")
+}
+
+func (m *mockProvisioningV0alpha1InterfaceForConnections) Jobs(namespace string) client.JobInterface {
+	panic("not needed for testing")
+}
+
+func (m *mockProvisioningV0alpha1InterfaceForConnections) Connections(namespace string) client.ConnectionInterface {
+	return m.connections
+}
+
+func (m *mockProvisioningV0alpha1InterfaceForConnections) Repositories(namespace string) client.RepositoryInterface {
+	panic("not needed for testing")
+}
+
+func TestConnectionController_handleDelete(t *testing.T) {
+	ctx := context.Background()
+
+	tests := []struct {
+		name                   string
+		connection             *provisioning.Connection
+		repoListerSetup        func(*mockRepositoryLister)
+		connectionSetup        func(*mockConnectionInterface)
+		expectedError          string
+		expectFinalizerRemoved bool
+	}{
+		{
+			name: "no finalizer present, should return nil",
+			connection: &provisioning.Connection{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-conn",
+					Namespace:         "default",
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{},
+				},
+			},
+			repoListerSetup:        func(m *mockRepositoryLister) {},
+			connectionSetup:        func(m *mockConnectionInterface) {},
+			expectedError:          "",
+			expectFinalizerRemoved: false,
+		},
+		{
+			name: "finalizer present but repositories exist, should block deletion",
+			connection: &provisioning.Connection{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-conn",
+					Namespace:         "default",
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{connectionvalidation.BlockDeletionFinalizer},
+				},
+			},
+			repoListerSetup: func(m *mockRepositoryLister) {
+				m.On("List", ctx, mock.MatchedBy(func(opts *internalversion.ListOptions) bool {
+					return opts.FieldSelector != nil && opts.FieldSelector.String() == "spec.connection.name=test-conn"
+				})).Return(&provisioning.RepositoryList{
+					Items: []provisioning.Repository{
+						{
+							ObjectMeta: metav1.ObjectMeta{Name: "repo-1"},
+							Spec: provisioning.RepositorySpec{
+								Connection: &provisioning.ConnectionInfo{Name: "test-conn"},
+							},
+						},
+						{
+							ObjectMeta: metav1.ObjectMeta{Name: "repo-2"},
+							Spec: provisioning.RepositorySpec{
+								Connection: &provisioning.ConnectionInfo{Name: "test-conn"},
+							},
+						},
+					},
+				}, nil)
+			},
+			connectionSetup:        func(m *mockConnectionInterface) {},
+			expectedError:          "cannot delete connection while repositories are using it: repo-1, repo-2",
+			expectFinalizerRemoved: false,
+		},
+		{
+			name: "finalizer present and no repositories, should remove finalizer",
+			connection: &provisioning.Connection{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-conn",
+					Namespace:         "default",
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{connectionvalidation.BlockDeletionFinalizer},
+				},
+			},
+			repoListerSetup: func(m *mockRepositoryLister) {
+				m.On("List", ctx, mock.MatchedBy(func(opts *internalversion.ListOptions) bool {
+					return opts.FieldSelector != nil && opts.FieldSelector.String() == "spec.connection.name=test-conn"
+				})).Return(&provisioning.RepositoryList{
+					Items: []provisioning.Repository{},
+				}, nil)
+			},
+			connectionSetup: func(m *mockConnectionInterface) {
+				m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{
+					FieldManager: "provisioning-connection-controller",
+				}, mock.Anything).Return(&provisioning.Connection{}, nil)
+			},
+			expectedError:          "",
+			expectFinalizerRemoved: true,
+		},
+		{
+			name: "error checking repositories, should return error",
+			connection: &provisioning.Connection{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-conn",
+					Namespace:         "default",
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{connectionvalidation.BlockDeletionFinalizer},
+				},
+			},
+			repoListerSetup: func(m *mockRepositoryLister) {
+				m.On("List", ctx, mock.Anything).Return(nil, errors.New("list error"))
+			},
+			connectionSetup:        func(m *mockConnectionInterface) {},
+			expectedError:          "check for connected repositories: list error",
+			expectFinalizerRemoved: false,
+		},
+		{
+			name: "error removing finalizer, should return error",
+			connection: &provisioning.Connection{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-conn",
+					Namespace:         "default",
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{connectionvalidation.BlockDeletionFinalizer},
+				},
+			},
+			repoListerSetup: func(m *mockRepositoryLister) {
+				m.On("List", ctx, mock.Anything).Return(&provisioning.RepositoryList{
+					Items: []provisioning.Repository{},
+				}, nil)
+			},
+			connectionSetup: func(m *mockConnectionInterface) {
+				m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{
+					FieldManager: "provisioning-connection-controller",
+				}, mock.Anything).Return(nil, errors.New("patch error"))
+			},
+			expectedError:          "remove finalizer: patch error",
+			expectFinalizerRemoved: false,
+		},
+		{
+			name: "pagination handled correctly",
+			connection: &provisioning.Connection{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-conn",
+					Namespace:         "default",
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{connectionvalidation.BlockDeletionFinalizer},
+				},
+			},
+			repoListerSetup: func(m *mockRepositoryLister) {
+				// First call returns empty with continue token (testing pagination even when empty)
+				m.On("List", ctx, mock.MatchedBy(func(opts *internalversion.ListOptions) bool {
+					return opts.Continue == ""
+				})).Return(&provisioning.RepositoryList{
+					Items:    []provisioning.Repository{},
+					ListMeta: metav1.ListMeta{Continue: "continue-token"},
+				}, nil)
+				// Second call returns empty with no continue token
+				m.On("List", ctx, mock.MatchedBy(func(opts *internalversion.ListOptions) bool {
+					return opts.Continue == "continue-token"
+				})).Return(&provisioning.RepositoryList{
+					Items: []provisioning.Repository{},
+				}, nil)
+			},
+			connectionSetup: func(m *mockConnectionInterface) {
+				m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{
+					FieldManager: "provisioning-connection-controller",
+				}, mock.Anything).Return(&provisioning.Connection{}, nil)
+			},
+			expectedError:          "",
+			expectFinalizerRemoved: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			repoLister := new(mockRepositoryLister)
+			connInterface := new(mockConnectionInterface)
+			client := &mockProvisioningV0alpha1InterfaceForConnections{connections: connInterface}
+
+			tt.repoListerSetup(repoLister)
+			tt.connectionSetup(connInterface)
+
+			cc := &ConnectionController{
+				client:     client,
+				repoLister: repoLister,
+				logger:     nil, // logger is optional for testing
+			}
+
+			err := cc.handleDelete(ctx, tt.connection)
+
+			if tt.expectedError != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.expectedError)
+			} else {
+				require.NoError(t, err)
+			}
+
+			if tt.expectFinalizerRemoved {
+				connInterface.AssertCalled(t, "Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{
+					FieldManager: "provisioning-connection-controller",
+				}, mock.Anything)
+			} else {
+				connInterface.AssertNotCalled(t, "Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{}, mock.Anything)
+			}
+
+			repoLister.AssertExpectations(t)
+			connInterface.AssertExpectations(t)
+		})
+	}
+}

pkg/registry/apis/provisioning/register.go

@@ -685,6 +685,12 @@ func (b *APIBuilder) Mutate(ctx context.Context, a admission.Attributes, o admis
 	// TODO: complete this as part of https://github.com/grafana/git-ui-sync-project/issues/700
 	c, ok := obj.(*provisioning.Connection)
 	if ok {
+		// Add finalizer on create to prevent deletion while repositories reference it
+		if len(c.Finalizers) == 0 && a.GetOperation() == admission.Create {
+			c.Finalizers = []string{
+				connectionvalidation.BlockDeletionFinalizer,
+			}
+		}
 		return connectionvalidation.MutateConnection(c)
 	}
 
@@ -1014,6 +1020,7 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH
 				b.GetClient(),
 				connInformer,
 				connStatusPatcher,
+				b.store,
 			)
 			if err != nil {
 				return err