diff --git a/controller/pkg/bucketclaim/bucketclaim.go b/controller/pkg/bucketclaim/bucketclaim.go index 2f4d565e..8ad7baed 100644 --- a/controller/pkg/bucketclaim/bucketclaim.go +++ b/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", diff --git a/controller/pkg/bucketclaim/bucketclaim_test.go b/controller/pkg/bucketclaim/bucketclaim_test.go index 284185b6..e2e2d3d2 100644 --- a/controller/pkg/bucketclaim/bucketclaim_test.go +++ b/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)) + } +} diff --git a/sidecar/pkg/bucket/bucket_controller.go b/sidecar/pkg/bucket/bucket_controller.go index a934d0c5..bf8b5311 100644 --- a/sidecar/pkg/bucket/bucket_controller.go +++ b/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 } diff --git a/sidecar/pkg/bucket/bucket_controller_test.go b/sidecar/pkg/bucket/bucket_controller_test.go index 9be6cc4a..ae63464e 100644 --- a/sidecar/pkg/bucket/bucket_controller_test.go +++ b/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) + } +} diff --git a/sidecar/pkg/bucketaccess/bucketaccess_controller.go b/sidecar/pkg/bucketaccess/bucketaccess_controller.go index c6d0ed07..dd18202f 100644 --- a/sidecar/pkg/bucketaccess/bucketaccess_controller.go +++ b/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", diff --git a/sidecar/pkg/bucketaccess/bucketaccess_controller_test.go b/sidecar/pkg/bucketaccess/bucketaccess_controller_test.go index 2371c81e..d8da44a2 100644 --- a/sidecar/pkg/bucketaccess/bucketaccess_controller_test.go +++ b/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) + } +}