Remove the "in-use" Finalizer (#265)

* Remove references to in-use finalizer * Remove references to in-use finalizer in chainsaw * Update sample bucket - unrelated
linode · Jun 5, 2024 · 1832017 · 1832017
1 parent e0d7647
commit 1832017
Show file tree

Hide file tree

Showing 9 changed files with 14 additions and 196 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,
-Original file line number
+Diff line change
@@ Expand Up / @@ -18,5 +18,4 @@ package v1alpha1 @@
     const (
     	BackendLabelPrefix = "provider-ceph.backends."
-    	InUseFinalizer     = "bucket-in-use.provider-ceph.crossplane.io"
     )