diff --git a/apps/provisioning/pkg/connection/finalizers.go b/apps/provisioning/pkg/connection/finalizers.go new file mode 100644 index 00000000000..37283410358 --- /dev/null +++ b/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" diff --git a/pkg/registry/apis/provisioning/controller/connection.go b/pkg/registry/apis/provisioning/controller/connection.go index be90908bd49..87df9e67e41 100644 --- a/pkg/registry/apis/provisioning/controller/connection.go +++ b/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 diff --git a/pkg/registry/apis/provisioning/controller/connection_test.go b/pkg/registry/apis/provisioning/controller/connection_test.go index b033ddb39a9..dff9c9abb2f 100644 --- a/pkg/registry/apis/provisioning/controller/connection_test.go +++ b/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) + }) + } +} diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 04e4ac7fe5c..9988abd7594 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/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