From 18320171184f846f90cad053cb0ebdd3940f561f Mon Sep 17 00:00:00 2001 From: Conor Nolan Date: Wed, 5 Jun 2024 15:13:44 +0100 Subject: [PATCH] Remove the "in-use" Finalizer (#265) * Remove references to in-use finalizer * Remove references to in-use finalizer in chainsaw * Update sample bucket - unrelated --- apis/provider-ceph/v1alpha1/utils.go | 1 - e2e/tests/ceph/chainsaw-test.yaml | 4 - e2e/tests/stable/chainsaw-test.yaml | 7 -- examples/sample/bucket.yaml | 2 - internal/controller/bucket/delete.go | 20 +--- internal/controller/bucket/delete_test.go | 110 ++------------------- internal/controller/bucket/observe.go | 8 -- internal/controller/bucket/observe_test.go | 54 +--------- internal/controller/bucket/update.go | 4 - 9 files changed, 14 insertions(+), 196 deletions(-) diff --git a/apis/provider-ceph/v1alpha1/utils.go b/apis/provider-ceph/v1alpha1/utils.go index 40311a90..bee55090 100644 --- a/apis/provider-ceph/v1alpha1/utils.go +++ b/apis/provider-ceph/v1alpha1/utils.go @@ -18,5 +18,4 @@ package v1alpha1 const ( BackendLabelPrefix = "provider-ceph.backends." - InUseFinalizer = "bucket-in-use.provider-ceph.crossplane.io" ) diff --git a/e2e/tests/ceph/chainsaw-test.yaml b/e2e/tests/ceph/chainsaw-test.yaml index 98f7ca83..473591d8 100755 --- a/e2e/tests/ceph/chainsaw-test.yaml +++ b/e2e/tests/ceph/chainsaw-test.yaml @@ -81,7 +81,6 @@ spec: name: test-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" status: atProvider: backends: @@ -105,7 +104,6 @@ spec: name: test-bucket-all finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" status: atProvider: backends: @@ -185,7 +183,6 @@ spec: name: test-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" status: atProvider: backends: @@ -257,7 +254,6 @@ spec: name: test-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" status: atProvider: backends: diff --git a/e2e/tests/stable/chainsaw-test.yaml b/e2e/tests/stable/chainsaw-test.yaml index 11f949a7..03e6bb95 100755 --- a/e2e/tests/stable/chainsaw-test.yaml +++ b/e2e/tests/stable/chainsaw-test.yaml @@ -155,7 +155,6 @@ spec: name: lc-config-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" labels: provider-ceph.backends.localstack-a: "true" provider-ceph.backends.localstack-b: "true" @@ -220,7 +219,6 @@ spec: name: auto-pause-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" labels: crossplane.io/paused: "true" provider-ceph.backends.localstack-a: "true" @@ -377,7 +375,6 @@ spec: name: localstack-b-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" status: atProvider: backends: @@ -648,7 +645,6 @@ spec: name: auto-pause-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" labels: crossplane.io/paused: "true" provider-ceph.backends.localstack-a: "true" @@ -685,7 +681,6 @@ spec: name: avoid-localstack-c-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" labels: provider-ceph.backends.localstack-a: "true" provider-ceph.backends.localstack-b: "true" @@ -738,7 +733,6 @@ spec: name: auto-pause-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" labels: crossplane.io/paused: "true" provider-ceph.backends.localstack-a: "true" @@ -777,7 +771,6 @@ spec: name: avoid-localstack-c-bucket finalizers: - "finalizer.managedresource.crossplane.io" - - "bucket-in-use.provider-ceph.crossplane.io" labels: crossplane.io/paused: "true" provider-ceph.backends.localstack-a: "true" diff --git a/examples/sample/bucket.yaml b/examples/sample/bucket.yaml index 4b1f02d5..082fa502 100644 --- a/examples/sample/bucket.yaml +++ b/examples/sample/bucket.yaml @@ -3,6 +3,4 @@ kind: Bucket metadata: name: test-bucket spec: - providers: - - ceph-admin-cfg forProvider: {} diff --git a/internal/controller/bucket/delete.go b/internal/controller/bucket/delete.go index 9d90b61f..2feb48c8 100644 --- a/internal/controller/bucket/delete.go +++ b/internal/controller/bucket/delete.go @@ -13,7 +13,6 @@ import ( xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/resource" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/linode/provider-ceph/apis/provider-ceph/v1alpha1" "github.com/linode/provider-ceph/internal/consts" @@ -101,8 +100,7 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error { // 1. The caller attempts to delete the CR and an error occurs during the call to // the bucket's backends. In this case the bucket may be successfully deleted // from some backends, but not from others. As such, we must update the bucket CR - // status accordingly as Delete has ultimately failed and the 'in-use' finalizer - // will not be removed. + // status accordingly as Delete has ultimately failed. // 2. The caller attempts to delete the bucket from it's backends without deleting // the bucket CR. This is done by setting the Disabled flag on the bucket // CR spec. If the deletion is successful or unsuccessful, the bucket CR status must be @@ -155,21 +153,5 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error { c.log.Info("All buckets successfully deleted from backends for Bucket CR", consts.KeyBucketName, bucket.Name) - // No errors occurred - the bucket has successfully been deleted from all backends. - // We do not need to update the Bucket CR Status, we simply remove the "in-use" finalizer. - if err := c.updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { - c.log.Info("Removing 'in-use' finalizer from Bucket CR", consts.KeyBucketName, bucket.Name) - - controllerutil.RemoveFinalizer(bucketLatest, v1alpha1.InUseFinalizer) - - return NeedsObjectUpdate - }); err != nil { - err = errors.Wrap(err, errUpdateBucketCR) - c.log.Info("Failed to remove 'in-use' finalizer from Bucket CR", consts.KeyBucketName, bucket.Name, "error", err.Error()) - traces.SetAndRecordError(span, err) - - return err - } - return nil } diff --git a/internal/controller/bucket/delete_test.go b/internal/controller/bucket/delete_test.go index 5198f0bd..64640d07 100644 --- a/internal/controller/bucket/delete_test.go +++ b/internal/controller/bucket/delete_test.go @@ -126,9 +126,8 @@ func TestDelete(t *testing.T) { } type want struct { - err error - statusDiff func(t *testing.T, mg resource.Managed) - finalizerDiff func(t *testing.T, mg resource.Managed) + err error + statusDiff func(t *testing.T, mg resource.Managed) } cases := map[string]struct { @@ -160,8 +159,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "test-bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -212,15 +210,6 @@ func TestDelete(t *testing.T) { }(bucket.Status.AtProvider.Backends), "s3-backend-2 should not exist in backends") }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Empty(t, - len(bucket.Finalizers), - "unexpeceted finalizers", - ) - }, }, }, "Delete buckets on all backends": { @@ -246,8 +235,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "test-bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -299,15 +287,6 @@ func TestDelete(t *testing.T) { }(bucket.Status.AtProvider.Backends), "s3-backend-2 should not exist in backends") }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Empty(t, - len(bucket.Finalizers), - "unexpeceted finalizers", - ) - }, }, }, "Error deleting buckets on all specified backends": { @@ -332,8 +311,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -372,16 +350,6 @@ func TestDelete(t *testing.T) { bucket.Status.AtProvider.Backends[s3Backend2].BucketCondition.Equal(xpv1.Deleting().WithMessage(errors.Wrap(errRandom, "failed to perform head bucket").Error())), "unexpected bucket condition on s3-backend-2") }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Equal(t, - []string{v1alpha1.InUseFinalizer}, - bucket.Finalizers, - "unexpected finalizers", - ) - }, }, }, "Error deleting buckets on all backends": { @@ -406,8 +374,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -442,16 +409,6 @@ func TestDelete(t *testing.T) { bucket.Status.AtProvider.Backends[s3Backend2].BucketCondition.Equal(xpv1.Deleting().WithMessage(errors.Wrap(errRandom, "failed to perform head bucket").Error())), "unexpected bucket condition on s3-backend-2") }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Equal(t, - []string{v1alpha1.InUseFinalizer}, - bucket.Finalizers, - "unexpected finalizers", - ) - }, }, }, @@ -475,8 +432,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -511,16 +467,6 @@ func TestDelete(t *testing.T) { bucket.Status.AtProvider.Backends[s3Backend2].BucketCondition.Equal(xpv1.Deleting().WithMessage(errors.Wrap(errors.Wrap(errRandom, "failed to assume role"), "Failed to create s3 client via assume role").Error())), "unexpected bucket condition on s3-backend-2") }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Equal(t, - []string{v1alpha1.InUseFinalizer}, - bucket.Finalizers, - "unexpected finalizers", - ) - }, }, }, @@ -556,8 +502,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -605,16 +550,6 @@ func TestDelete(t *testing.T) { }(bucket.Status.AtProvider.Backends), "s3-backend-2 should not exist in backends") }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Equal(t, - []string{v1alpha1.InUseFinalizer}, - bucket.Finalizers, - "unexpeceted finalizers", - ) - }, }, }, @@ -654,8 +589,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -709,16 +643,6 @@ func TestDelete(t *testing.T) { "Disabled flag should be false", ) }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Equal(t, - []string{v1alpha1.InUseFinalizer}, - bucket.Finalizers, - "unexpeceted finalizers", - ) - }, }, }, "Error deleting disabled bucket because one specified bucket is not empty": { @@ -757,8 +681,7 @@ func TestDelete(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket", Labels: map[string]string{ v1alpha1.BackendLabelPrefix + s3Backend1: "true", v1alpha1.BackendLabelPrefix + s3Backend2: "true", @@ -813,16 +736,6 @@ func TestDelete(t *testing.T) { "Disabled flag should be false", ) }, - finalizerDiff: func(t *testing.T, mg resource.Managed) { - t.Helper() - bucket, _ := mg.(*v1alpha1.Bucket) - - assert.Equal(t, - []string{v1alpha1.InUseFinalizer}, - bucket.Finalizers, - "unexpeceted finalizers", - ) - }, }, }, } @@ -854,9 +767,6 @@ func TestDelete(t *testing.T) { if tc.want.statusDiff != nil { tc.want.statusDiff(t, tc.args.mg) } - if tc.want.finalizerDiff != nil { - tc.want.finalizerDiff(t, tc.args.mg) - } }) } } diff --git a/internal/controller/bucket/observe.go b/internal/controller/bucket/observe.go index 62e7785b..0d1b8a41 100644 --- a/internal/controller/bucket/observe.go +++ b/internal/controller/bucket/observe.go @@ -5,7 +5,6 @@ import ( "go.opentelemetry.io/otel" "golang.org/x/sync/errgroup" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/errors" @@ -52,13 +51,6 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex }, nil } - if !controllerutil.ContainsFinalizer(bucket, v1alpha1.InUseFinalizer) { - return managed.ExternalObservation{ - ResourceExists: true, - ResourceUpToDate: false, - }, nil - } - if !bucket.Status.GetCondition(xpv1.TypeReady).Equal(xpv1.Available()) { return managed.ExternalObservation{ ResourceExists: true, diff --git a/internal/controller/bucket/observe_test.go b/internal/controller/bucket/observe_test.go index 86222113..904f4045 100644 --- a/internal/controller/bucket/observe_test.go +++ b/internal/controller/bucket/observe_test.go @@ -159,42 +159,6 @@ func TestObserve(t *testing.T) { }, }, }, - "Bucket doesn't have finalizer": { - fields: fields{ - backendStore: func() *backendstore.BackendStore { - bs := backendstore.NewBackendStore() - bs.AddOrUpdateBackend("s3-backend-1", nil, nil, true, apisv1alpha1.HealthStatusHealthy) - - return bs - }(), - }, - args: args{ - mg: &v1alpha1.Bucket{ - Spec: v1alpha1.BucketSpec{ - ResourceSpec: v1.ResourceSpec{ - ProviderConfigReference: &v1.Reference{ - Name: "s3-backend-1", - }, - }, - }, - Status: v1alpha1.BucketStatus{ - AtProvider: v1alpha1.BucketObservation{ - Backends: v1alpha1.Backends{ - "s3-backend-1": &v1alpha1.BackendInfo{ - BucketCondition: v1.Available(), - }, - }, - }, - }, - }, - }, - want: want{ - o: managed.ExternalObservation{ - ResourceExists: true, - ResourceUpToDate: false, - }, - }, - }, "Bucket status is not available": { fields: fields{ backendStore: func() *backendstore.BackendStore { @@ -206,9 +170,6 @@ func TestObserve(t *testing.T) { }, args: args{ mg: &v1alpha1.Bucket{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{v1alpha1.InUseFinalizer}, - }, Spec: v1alpha1.BucketSpec{ ResourceSpec: v1.ResourceSpec{ ProviderConfigReference: &v1.Reference{ @@ -252,9 +213,6 @@ func TestObserve(t *testing.T) { }, args: args{ mg: &v1alpha1.Bucket{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{v1alpha1.InUseFinalizer}, - }, Spec: v1alpha1.BucketSpec{ Providers: []string{"s3-backend-1"}, ResourceSpec: v1.ResourceSpec{ @@ -306,8 +264,7 @@ func TestObserve(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket-check-external-error", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket-check-external-error", }, Spec: v1alpha1.BucketSpec{ Providers: []string{"s3-backend-1"}, @@ -361,8 +318,7 @@ func TestObserve(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket-check-external-not-exists", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket-check-external-not-exists", }, Spec: v1alpha1.BucketSpec{ Providers: []string{"s3-backend-1"}, @@ -416,8 +372,7 @@ func TestObserve(t *testing.T) { args: args{ mg: &v1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ - Name: "bucket-check-external-ok", - Finalizers: []string{v1alpha1.InUseFinalizer}, + Name: "bucket-check-external-ok", }, Spec: v1alpha1.BucketSpec{ Providers: []string{"s3-backend-1"}, @@ -471,9 +426,6 @@ func TestObserve(t *testing.T) { }, args: args{ mg: &v1alpha1.Bucket{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{v1alpha1.InUseFinalizer}, - }, Spec: v1alpha1.BucketSpec{ Providers: []string{"s3-backend-1"}, ResourceSpec: v1.ResourceSpec{ diff --git a/internal/controller/bucket/update.go b/internal/controller/bucket/update.go index 63b06244..9e7016f5 100644 --- a/internal/controller/bucket/update.go +++ b/internal/controller/bucket/update.go @@ -6,7 +6,6 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "golang.org/x/sync/errgroup" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/errors" @@ -121,9 +120,6 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // intended to be updated on. This is to ensure the bucket will eventually be updated // on these backends whenever they become active again. setAllBackendLabels(bucketLatest, allBackendsToUpdateOn) - // Add the in-use finalizer to ensure that the Bucket CR cannot be deleted while - // it has existing buckets. - controllerutil.AddFinalizer(bucketLatest, v1alpha1.InUseFinalizer) return NeedsObjectUpdate })