Fix reconciliation logic for existing and deleted objects

This PR addresses two cases:

- Controller Restarts:
  When the controller restarts, it does not handle deletions for removed objects.
  Specifically, when an object with a deletion timestamp is passed into the Add() method, the deletion process was not handled correctly.

- Recreation of Objects:
  The recreation of objects is not functioning as expected. For example, when an object is created, deleted, and then created again.
  In this scenario, the object may enter the Update method, where the creation process was not being handled properly.

Signed-off-by: Andrei Kvapil <[email protected]>

Author: kvaps

controller/pkg/bucketclaim/bucketclaim.go

@@ -32,6 +32,10 @@ func NewBucketClaimListener() *BucketClaimListener {
 
 // Add creates a bucket in response to a bucketClaim
 func (b *BucketClaimListener) Add(ctx context.Context, bucketClaim *v1alpha1.BucketClaim) error {
+	if !bucketClaim.GetDeletionTimestamp().IsZero() {
+		return b.handleDeletion(ctx, bucketClaim)
+	}
+
 	klog.V(3).InfoS("Add BucketClaim",
 		"name", bucketClaim.ObjectMeta.Name,
 		"ns", bucketClaim.ObjectMeta.Namespace,
@@ -76,18 +80,11 @@ func (b *BucketClaimListener) Update(ctx context.Context, old, new *v1alpha1.Buc
 	bucketClaim := new.DeepCopy()
 
 	if !new.GetDeletionTimestamp().IsZero() {
-		if controllerutil.ContainsFinalizer(bucketClaim, util.BucketClaimFinalizer) {
-			bucketName := bucketClaim.Status.BucketName
-			err := b.buckets().Delete(ctx, bucketName, metav1.DeleteOptions{})
-			if err != nil {
-				klog.V(3).ErrorS(err, "Error deleting bucket",
-					"bucket", bucketName,
-					"bucketClaim", bucketClaim.ObjectMeta.Name)
-				return b.recordError(bucketClaim, v1.EventTypeWarning, v1alpha1.FailedDeleteBucket, err)
-			}
-
-			klog.V(5).Infof("Successfully deleted bucket: %s from bucketClaim: %s", bucketName, bucketClaim.ObjectMeta.Name)
-		}
+		return b.handleDeletion(ctx, bucketClaim)
+	}
+
+	if err := b.Add(ctx, bucketClaim); err != nil {
+		return err
 	}
 
 	klog.V(3).InfoS("Update BucketClaim success",
@@ -96,6 +93,27 @@ func (b *BucketClaimListener) Update(ctx context.Context, old, new *v1alpha1.Buc
 	return nil
 }
 
+// handleDeletion processes the deletion of a bucketClaim.
+func (b *BucketClaimListener) handleDeletion(ctx context.Context, bucketClaim *v1alpha1.BucketClaim) error {
+	if !controllerutil.ContainsFinalizer(bucketClaim, util.BucketClaimFinalizer) {
+		return nil
+	}
+
+	bucketName := bucketClaim.Status.BucketName
+	if bucketName != "" {
+		if err := b.buckets().Delete(ctx, bucketName, metav1.DeleteOptions{}); err != nil && !kubeerrors.IsNotFound(err) {
+			klog.V(3).ErrorS(err, "Error deleting bucket",
+				"bucket", bucketName,
+				"bucketClaim", bucketClaim.ObjectMeta.Name)
+			return b.recordError(bucketClaim, v1.EventTypeWarning, v1alpha1.FailedDeleteBucket, err)
+		}
+		klog.V(5).Infof("Successfully requested deletion of bucket: %s for bucketClaim: %s",
+			bucketName, bucketClaim.ObjectMeta.Name)
+	}
+
+	return nil
+}
+
 // Delete processes a bucket for which bucket request is deleted
 func (b *BucketClaimListener) Delete(ctx context.Context, bucketClaim *v1alpha1.BucketClaim) error {
 	klog.V(3).InfoS("Delete BucketClaim",

controller/pkg/bucketclaim/bucketclaim_test.go

@@ -323,3 +323,32 @@ func TestRecordEvents(t *testing.T) {
 func newEvent(eventType, reason, message string) string {
 	return fmt.Sprintf("%s %s %s", eventType, reason, message)
 }
+
+// Claim already marked for deletion must not create a bucket
+func TestAddDeletedBucketClaim(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	client := fakebucketclientset.NewSimpleClientset()
+	kubeClient := fakekubeclientset.NewSimpleClientset()
+	eventRecorder := record.NewFakeRecorder(3)
+
+	listener := NewBucketClaimListener()
+	listener.InitializeKubeClient(kubeClient)
+	listener.InitializeBucketClient(client)
+	listener.InitializeEventRecorder(eventRecorder)
+
+	_, _ = util.CreateBucketClass(ctx, client, &goldClass)
+
+	claimToDelete := bucketClaim1.DeepCopy()
+	now := metav1.Now()
+	claimToDelete.ObjectMeta.DeletionTimestamp = &now
+
+	if err := listener.Add(ctx, claimToDelete); err != nil {
+		t.Fatalf("Add returned error for deleted claim: %v", err)
+	}
+
+	if bl := util.GetBuckets(ctx, client, 0); len(bl.Items) != 0 {
+		t.Fatalf("expected 0 buckets, got %d", len(bl.Items))
+	}
+}

sidecar/pkg/bucket/bucket_controller.go

@@ -68,6 +68,10 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket)
 
 	var err error
 
+	if !bucket.GetDeletionTimestamp().IsZero() {
+		return b.handleDeletion(ctx, bucket)
+	}
+
 	klog.V(3).InfoS("Add Bucket",
 		"name", bucket.ObjectMeta.Name)
 
@@ -212,55 +216,60 @@ func (b *BucketListener) Update(ctx context.Context, old, new *v1alpha1.Bucket)
 	var err error
 
 	if !bucket.GetDeletionTimestamp().IsZero() {
-		if controllerutil.ContainsFinalizer(bucket, consts.BABucketFinalizer) {
-			bucketClaimNs := bucket.Spec.BucketClaim.Namespace
-			bucketClaimName := bucket.Spec.BucketClaim.Name
-			bucketAccessList, err := b.bucketAccesses(bucketClaimNs).List(ctx, metav1.ListOptions{})
-			if err != nil {
-				klog.V(3).ErrorS(err, "Error fetching BucketAccessList",
-					"bucket", bucket.ObjectMeta.Name)
-				return err
-			}
-
-			for _, bucketAccess := range bucketAccessList.Items {
-				if strings.EqualFold(bucketAccess.Spec.BucketClaimName, bucketClaimName) {
-					err = b.bucketAccesses(bucketClaimNs).Delete(ctx, bucketAccess.Name, metav1.DeleteOptions{})
-					if err != nil {
-						klog.V(3).ErrorS(err, "Error deleting BucketAccess",
-							"name", bucketAccess.Name,
-							"bucket", bucket.ObjectMeta.Name)
-						return err
-					}
-				}
-			}
+		return b.handleDeletion(ctx, bucket)
+	}
 
-			klog.V(5).Infof("Successfully deleted dependent bucketAccess of bucket:%s", bucket.ObjectMeta.Name)
+	if err = b.Add(ctx, bucket); err != nil {
+		return err
+	}
 
-			controllerutil.RemoveFinalizer(bucket, consts.BABucketFinalizer)
-			klog.V(5).Infof("Successfully removed finalizer: %s of bucket: %s", consts.BABucketFinalizer, bucket.ObjectMeta.Name)
-		}
+	klog.V(3).InfoS("Update Bucket success",
+		"name", bucket.ObjectMeta.Name,
+		"ns", bucket.ObjectMeta.Namespace)
+	return nil
+}
 
-		if controllerutil.ContainsFinalizer(bucket, consts.BucketFinalizer) {
-			err = b.deleteBucketOp(ctx, bucket)
-			if err != nil {
-				return b.recordError(bucket, v1.EventTypeWarning, v1alpha1.FailedDeleteBucket, err)
-			}
+func (b *BucketListener) handleDeletion(ctx context.Context, bucket *v1alpha1.Bucket) error {
+	var err error
 
-			controllerutil.RemoveFinalizer(bucket, consts.BucketFinalizer)
-			klog.V(5).Infof("Successfully removed finalizer: %s of bucket: %s", consts.BucketFinalizer, bucket.ObjectMeta.Name)
-		}
+	if controllerutil.ContainsFinalizer(bucket, consts.BABucketFinalizer) {
+		bucketClaimNs := bucket.Spec.BucketClaim.Namespace
+		bucketClaimName := bucket.Spec.BucketClaim.Name
 
-		_, err = b.buckets().Update(ctx, bucket, metav1.UpdateOptions{})
+		bucketAccessList, err := b.bucketAccesses(bucketClaimNs).List(ctx, metav1.ListOptions{})
 		if err != nil {
-			klog.V(3).ErrorS(err, "Error updating bucket after removing finalizers",
+			klog.V(3).ErrorS(err, "Error fetching BucketAccessList",
 				"bucket", bucket.ObjectMeta.Name)
 			return err
 		}
+
+		for _, ba := range bucketAccessList.Items {
+			if strings.EqualFold(ba.Spec.BucketClaimName, bucketClaimName) {
+				if err = b.bucketAccesses(bucketClaimNs).Delete(ctx, ba.Name, metav1.DeleteOptions{}); err != nil {
+					klog.V(3).ErrorS(err, "Error deleting BucketAccess",
+						"name", ba.Name,
+						"bucket", bucket.ObjectMeta.Name)
+					return err
+				}
+			}
+		}
+
+		controllerutil.RemoveFinalizer(bucket, consts.BABucketFinalizer)
+	}
+
+	if controllerutil.ContainsFinalizer(bucket, consts.BucketFinalizer) {
+		if err = b.deleteBucketOp(ctx, bucket); err != nil {
+			return b.recordError(bucket, v1.EventTypeWarning, v1alpha1.FailedDeleteBucket, err)
+		}
+		controllerutil.RemoveFinalizer(bucket, consts.BucketFinalizer)
+	}
+
+	if _, err = b.buckets().Update(ctx, bucket, metav1.UpdateOptions{}); err != nil {
+		klog.V(3).ErrorS(err, "Error updating bucket after removing finalizers",
+			"bucket", bucket.ObjectMeta.Name)
+		return err
 	}
 
-	klog.V(3).InfoS("Update Bucket success",
-		"name", bucket.ObjectMeta.Name,
-		"ns", bucket.ObjectMeta.Namespace)
 	return nil
 }
 

sidecar/pkg/bucket/bucket_controller_test.go

@@ -310,3 +310,43 @@ func TestRecordEvents(t *testing.T) {
 func newEvent(eventType, reason, message string) string {
 	return fmt.Sprintf("%s %s %s", eventType, reason, message)
 }
+
+// TestAddDeletedBucket tests that the Add method does not call the driver
+func TestAddDeletedBucket(t *testing.T) {
+	driver := "driver1"
+
+	mpc := struct{ fakespec.FakeProvisionerClient }{}
+	mpc.FakeDriverDeleteBucket = func(
+		_ context.Context,
+		_ *cosi.DriverDeleteBucketRequest,
+		_ ...grpc.CallOption,
+	) (*cosi.DriverDeleteBucketResponse, error) {
+		t.Fatalf("driver should NOT be called from Add when object has DeletionTimestamp")
+		return nil, nil
+	}
+
+	now := metav1.Now()
+	b := v1alpha1.Bucket{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "testbucket",
+			DeletionTimestamp: &now,
+			ResourceVersion:   "1",
+		},
+		Spec: v1alpha1.BucketSpec{
+			DriverName:      driver,
+			BucketClassName: "ignored",
+		},
+	}
+
+	client := fakebucketclientset.NewSimpleClientset(&b)
+
+	bl := BucketListener{
+		driverName:        driver,
+		provisionerClient: &mpc,
+	}
+	bl.InitializeBucketClient(client)
+
+	if err := bl.Add(context.TODO(), &b); err != nil {
+		t.Fatalf("Add returned error for deleted bucket: %v", err)
+	}
+}

sidecar/pkg/bucketaccess/bucketaccess_controller.go

@@ -68,6 +68,12 @@ func NewBucketAccessListener(driverName string, client cosi.ProvisionerClient) *
 func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1alpha1.BucketAccess) error {
 	bucketAccess := inputBucketAccess.DeepCopy()
 
+	if !bucketAccess.GetDeletionTimestamp().IsZero() {
+		klog.V(3).InfoS("BucketAccess has deletion timestamp, handling deletion",
+			"name", bucketAccess.ObjectMeta.Name)
+		return bal.deleteBucketAccessOp(ctx, bucketAccess)
+	}
+
 	if bucketAccess.Status.AccessGranted && bucketAccess.Status.AccountID != "" {
 		klog.V(3).InfoS("BucketAccess already exists", bucketAccess.ObjectMeta.Name)
 		return nil
@@ -310,10 +316,13 @@ func (bal *BucketAccessListener) Update(ctx context.Context, old, new *v1alpha1.
 
 	bucketAccess := new.DeepCopy()
 	if !bucketAccess.GetDeletionTimestamp().IsZero() {
-		err := bal.deleteBucketAccessOp(ctx, bucketAccess)
-		if err != nil {
+		if err := bal.deleteBucketAccessOp(ctx, bucketAccess); err != nil {
 			return bal.recordError(bucketAccess, v1.EventTypeWarning, v1alpha1.FailedRevokeAccess, err)
 		}
+	} else {
+		if err := bal.Add(ctx, bucketAccess); err != nil {
+			return bal.recordError(bucketAccess, v1.EventTypeWarning, v1alpha1.FailedGrantAccess, err)
+		}
 	}
 
 	klog.V(3).InfoS("Update BucketAccess success",

sidecar/pkg/bucketaccess/bucketaccess_controller_test.go

@@ -502,3 +502,94 @@ func TestRecordEvents(t *testing.T) {
 func newEvent(eventType, reason, message string) string {
 	return fmt.Sprintf("%s %s %s", eventType, reason, message)
 }
+
+// TestAddDeletedBucketAccess tests that a deleted BucketAccess does not
+// trigger a call to the driver to grant access, and that no secrets are created.
+func TestAddDeletedBucketAccess(t *testing.T) {
+	driver := "driver"
+	baName := "bucketaccess-deleted"
+	ns := "testns"
+
+	mpc := struct{ fakespec.FakeProvisionerClient }{}
+	mpc.FakeDriverGrantBucketAccess = func(
+		_ context.Context,
+		_ *cosi.DriverGrantBucketAccessRequest,
+		_ ...grpc.CallOption,
+	) (*cosi.DriverGrantBucketAccessResponse, error) {
+		t.Fatalf("driver Grant should NOT be called on deleted BA")
+		return nil, nil
+	}
+	mpc.FakeDriverRevokeBucketAccess = func(
+		_ context.Context,
+		_ *cosi.DriverRevokeBucketAccessRequest,
+		_ ...grpc.CallOption,
+	) (*cosi.DriverRevokeBucketAccessResponse, error) {
+		return &cosi.DriverRevokeBucketAccessResponse{}, nil
+	}
+
+	// minimal stub objects just to satisfy look-ups inside delete-path
+	bac := &v1alpha1.BucketAccessClass{
+		ObjectMeta: metav1.ObjectMeta{Name: "bac"},
+		DriverName: driver,
+	}
+	claim := &v1alpha1.BucketClaim{
+		ObjectMeta: metav1.ObjectMeta{Name: "claim", Namespace: ns},
+		Status: v1alpha1.BucketClaimStatus{
+			BucketReady: true,
+			BucketName:  "bucket",
+		},
+	}
+	bucket := &v1alpha1.Bucket{
+		ObjectMeta: metav1.ObjectMeta{Name: "bucket"},
+		Status: v1alpha1.BucketStatus{
+			BucketReady: true,
+			BucketID:    "id",
+		},
+	}
+
+	now := metav1.Now()
+	ba := &v1alpha1.BucketAccess{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              baName,
+			Namespace:         ns,
+			DeletionTimestamp: &now,
+			Finalizers:        []string{consts.BAFinalizer},
+		},
+		Spec: v1alpha1.BucketAccessSpec{
+			BucketClaimName:       claim.Name,
+			BucketAccessClassName: bac.Name,
+			CredentialsSecretName: "creds",
+		},
+		Status: v1alpha1.BucketAccessStatus{
+			AccountID:     "acc",
+			AccessGranted: true,
+		},
+	}
+
+	secret := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "creds",
+			Namespace:  ns,
+			Finalizers: []string{consts.SecretFinalizer},
+		},
+		StringData: map[string]string{"dummy": "val"},
+	}
+
+	client := fakebucketclientset.NewSimpleClientset(bac, claim, bucket, ba)
+	kubeClient := fakekubeclientset.NewSimpleClientset(secret)
+
+	bal := BucketAccessListener{
+		driverName:        driver,
+		provisionerClient: &mpc,
+		bucketClient:      client,
+		kubeClient:        kubeClient,
+	}
+
+	if err := bal.Add(context.TODO(), ba); err != nil {
+		t.Fatalf("Add returned error for deleted BucketAccess: %v", err)
+	}
+
+	if _, err := bal.secrets(ns).Get(context.TODO(), "creds", metav1.GetOptions{}); !kubeerrors.IsNotFound(err) {
+		t.Fatalf("secret was not cleaned up, err=%v", err)
+	}
+}