Merge pull request #89 from kvaps/fix-deletion

Fix reconciliation logic for existing and deleted objects

Author: k8s-ci-robot

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)
+	}
+}