Merge pull request #74 from mukhoakash/finalizer_fix

Fix: Add DeleteContext to DriverDeleteBucket and fix delete BucketAccess flow

Author: k8s-ci-robot

go.mod

@@ -13,7 +13,7 @@ require (
 	k8s.io/client-go v0.24.2
 	k8s.io/klog/v2 v2.70.1
 	sigs.k8s.io/container-object-storage-interface-api v0.0.0-20220806044417-5d7517114883
-	sigs.k8s.io/container-object-storage-interface-spec v0.0.0-20220811182913-3c421cfc2830
+	sigs.k8s.io/container-object-storage-interface-spec v0.1.1-0.20221006174327-ec782953b8ac
 	sigs.k8s.io/controller-runtime v0.12.3
 )
 

go.sum

@@ -727,8 +727,8 @@ rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0=
 rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
 sigs.k8s.io/container-object-storage-interface-api v0.0.0-20220806044417-5d7517114883 h1:CtqK7l2m9Atw8L5daJdsXvVgxxvQkRBbJbUz7NiadD8=
 sigs.k8s.io/container-object-storage-interface-api v0.0.0-20220806044417-5d7517114883/go.mod h1:YiB+i/UGkzqgODDhRG3u7jkbWkQcoUeLEJ7hwOT/2Qk=
-sigs.k8s.io/container-object-storage-interface-spec v0.0.0-20220811182913-3c421cfc2830 h1:o8/7mIJCflt33epl4TZNS/+M5MktS8fQvcNuN8p235k=
-sigs.k8s.io/container-object-storage-interface-spec v0.0.0-20220811182913-3c421cfc2830/go.mod h1:SzF/yVSh88TgYdBOAXqhT96XjU8pCQtoeQKxzIOOmWQ=
+sigs.k8s.io/container-object-storage-interface-spec v0.1.1-0.20221006174327-ec782953b8ac h1:M1ZBBDJVWw3gDmE+kZZmwQ6+29GbWhG9RMqx9oV0tEs=
+sigs.k8s.io/container-object-storage-interface-spec v0.1.1-0.20221006174327-ec782953b8ac/go.mod h1:SzF/yVSh88TgYdBOAXqhT96XjU8pCQtoeQKxzIOOmWQ=
 sigs.k8s.io/controller-runtime v0.12.3 h1:FCM8xeY/FI8hoAfh/V4XbbYMY20gElh9yh+A98usMio=
 sigs.k8s.io/controller-runtime v0.12.3/go.mod h1:qKsk4WE6zW2Hfj0G4v10EnNB2jMG1C+NTb8h+DwCoU0=
 sigs.k8s.io/json v0.0.0-20211208200746-9f7c6b3444d2 h1:kDi4JBNAsJWfz1aEXhO8Jg87JJaPNLh5tIzYHgStQ9Y=

pkg/bucket/bucket_controller.go

@@ -17,6 +17,7 @@ package bucket
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -66,9 +67,13 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket)
 	var err error
 
 	klog.V(3).InfoS("Add Bucket",
-		"name", bucket.ObjectMeta.Name,
-		"bucketclass", bucket.Spec.BucketClassName,
-	)
+		"name", bucket.ObjectMeta.Name)
+
+	if bucket.Spec.BucketClassName == "" {
+		err = errors.New(fmt.Sprintf("BucketClassName not defined for bucket %s", bucket.ObjectMeta.Name))
+		klog.V(3).ErrorS(err, "BucketClassName not defined")
+		return err
+	}
 
 	if !strings.EqualFold(bucket.Spec.DriverName, b.driverName) {
 		klog.V(5).InfoS("Skipping bucket for driver",
@@ -93,6 +98,24 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket)
 	if bucket.Spec.ExistingBucketID != "" {
 		bucketReady = true
 		bucketID = bucket.Spec.ExistingBucketID
+		if bucket.Spec.Parameters == nil {
+			bucketClass, err := b.bucketClasses().Get(ctx, bucket.Spec.BucketClassName, metav1.GetOptions{})
+			if err != nil {
+				klog.V(3).ErrorS(err, "Error fetching bucketClass",
+					"bucketClass", bucket.Spec.BucketClassName,
+					"bucket", bucket.ObjectMeta.Name)
+				return err
+			}
+
+			if bucketClass.Parameters != nil {
+				var param map[string]string
+				for k, v := range bucketClass.Parameters {
+					param[k] = v
+				}
+
+				bucket.Spec.Parameters = param
+			}
+		}
 	} else {
 		req := &cosi.DriverCreateBucketRequest{
 			Parameters: bucket.Spec.Parameters,
@@ -110,7 +133,7 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket)
 		}
 
 		if rsp == nil {
-			err = errors.New("DriverCreateBucket returned a nil response")
+			err = errors.New(fmt.Sprintf("DriverCreateBucket returned a nil response for bucket: %s", bucket.ObjectMeta.Name))
 			klog.V(3).ErrorS(err, "Internal Error from driver",
 				"bucket", bucket.ObjectMeta.Name)
 			return err
@@ -122,7 +145,7 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket)
 		} else {
 			klog.V(3).ErrorS(err, "DriverCreateBucket returned an empty bucketID",
 				"bucket", bucket.ObjectMeta.Name)
-			err = errors.New("DriverCreateBucket returned an empty bucketID")
+			err = errors.New(fmt.Sprintf("DriverCreateBucket returned an empty bucketID for bucket: %s", bucket.ObjectMeta.Name))
 			return err
 		}
 
@@ -217,6 +240,16 @@ func (b *BucketListener) Update(ctx context.Context, old, new *v1alpha1.Bucket)
 			if err != nil {
 				return err
 			}
+
+			controllerutil.RemoveFinalizer(bucket, consts.BucketFinalizer)
+			klog.V(5).Infof("Successfully removed finalizer: %s of bucket: %s", consts.BucketFinalizer, bucket.ObjectMeta.Name)
+		}
+
+		_, err = b.buckets().Update(ctx, bucket, metav1.UpdateOptions{})
+		if err != nil {
+			klog.V(3).ErrorS(err, "Error updating bucket after removing finalizers",
+				"bucket", bucket.ObjectMeta.Name)
+			return err
 		}
 	}
 
@@ -227,14 +260,38 @@ func (b *BucketListener) Update(ctx context.Context, old, new *v1alpha1.Bucket)
 }
 
 // Delete attemps to delete a bucket. This function must be idempotent
+// Delete function is called when the bucket was not able to add finalizers while creation.
+// Hence we will take care of removing the BucketClaim finalizer before deleting the Bucket object.
 // Return values
 //    nil - Bucket successfully deleted
 //    non-nil err - Internal error                                [requeue'd with exponential backoff]
 func (b *BucketListener) Delete(ctx context.Context, inputBucket *v1alpha1.Bucket) error {
 	klog.V(3).InfoS("Delete Bucket",
 		"name", inputBucket.ObjectMeta.Name,
-		"bucketclass", inputBucket.Spec.BucketClassName,
-	)
+		"bucketclass", inputBucket.Spec.BucketClassName)
+
+	if inputBucket.Spec.BucketClaim != nil {
+		ref := inputBucket.Spec.BucketClaim
+		klog.V(3).Infof("Removing finalizer of bucketClaim: %s before deleting bucket: %s", ref.Name, inputBucket.ObjectMeta.Name)
+
+		bucketClaim, err := b.bucketClaims(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{})
+		if err != nil {
+			klog.V(3).ErrorS(err, "Error getting bucketClaim for removing finalizer",
+				"bucket", inputBucket.ObjectMeta.Name,
+				"bucketClaim", ref.Name)
+			return err
+		}
+
+		if controllerutil.RemoveFinalizer(bucketClaim, consts.BCFinalizer) {
+			_, err := b.bucketClaims(bucketClaim.ObjectMeta.Namespace).Update(ctx, bucketClaim, metav1.UpdateOptions{})
+			if err != nil {
+				klog.V(3).ErrorS(err, "Error removing bucketClaim finalizer",
+					"bucket", inputBucket.ObjectMeta.Name,
+					"bucketClaim", bucketClaim.ObjectMeta.Name)
+				return err
+			}
+		}
+	}
 
 	return nil
 
@@ -270,7 +327,8 @@ func (b *BucketListener) deleteBucketOp(ctx context.Context, bucket *v1alpha1.Bu
 	// only when the retain policy is set to Delete
 	if bucket.Spec.DeletionPolicy == v1alpha1.DeletionPolicyDelete {
 		req := &cosi.DriverDeleteBucketRequest{
-			BucketId: bucket.Status.BucketID,
+			BucketId:      bucket.Status.BucketID,
+			DeleteContext: bucket.Spec.Parameters,
 		}
 
 		if _, err := b.provisionerClient.DriverDeleteBucket(ctx, req); err != nil {
@@ -317,6 +375,13 @@ func (b *BucketListener) buckets() bucketapi.BucketInterface {
 	panic("uninitialized listener")
 }
 
+func (b *BucketListener) bucketClasses() bucketapi.BucketClassInterface {
+	if b.bucketClient != nil {
+		return b.bucketClient.ObjectstorageV1alpha1().BucketClasses()
+	}
+	panic("uninitialized listener")
+}
+
 func (b *BucketListener) bucketClaims(namespace string) bucketapi.BucketClaimInterface {
 	if b.bucketClient != nil {
 		return b.bucketClient.ObjectstorageV1alpha1().BucketClaims(namespace)

pkg/bucket/bucket_controller_test.go

@@ -17,6 +17,7 @@ package bucket
 
 import (
 	"context"
+	"errors"
 	"reflect"
 	"testing"
 
@@ -25,6 +26,7 @@ import (
 	cosi "sigs.k8s.io/container-object-storage-interface-spec"
 	fakespec "sigs.k8s.io/container-object-storage-interface-spec/fake"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilversion "k8s.io/apimachinery/pkg/util/version"
 	"k8s.io/apimachinery/pkg/version"
 	fakediscovery "k8s.io/client-go/discovery/fake"
@@ -86,7 +88,8 @@ func TestAddWrongProvisioner(t *testing.T) {
 
 	b := v1alpha1.Bucket{
 		Spec: v1alpha1.BucketSpec{
-			DriverName: "driver2",
+			DriverName:      "driver2",
+			BucketClassName: "test-bucket",
 		},
 	}
 	ctx := context.TODO()
@@ -95,3 +98,34 @@ func TestAddWrongProvisioner(t *testing.T) {
 		t.Errorf("Error returned: %+v", err)
 	}
 }
+
+func TestMissingBucketClassName(t *testing.T) {
+	driver := "driver1"
+	mpc := struct{ fakespec.FakeProvisionerClient }{}
+	mpc.FakeDriverCreateBucket = func(ctx context.Context,
+		in *cosi.DriverCreateBucketRequest,
+		opts ...grpc.CallOption) (*cosi.DriverCreateBucketResponse, error) {
+		t.Errorf("grpc client called")
+		return nil, nil
+	}
+
+	bl := BucketListener{
+		driverName:        driver,
+		provisionerClient: &mpc,
+	}
+
+	b := v1alpha1.Bucket{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "testbucket",
+		},
+		Spec: v1alpha1.BucketSpec{
+			DriverName: "driver1",
+		},
+	}
+	ctx := context.TODO()
+	err := bl.Add(ctx, &b)
+	expectedErr := errors.New("BucketClassName not defined for bucket testbucket")
+	if err == nil || err.Error() != expectedErr.Error() {
+		t.Errorf("Expecter error: %+v \n Returned error: %+v", expectedErr, err)
+	}
+}

pkg/bucketaccess/bucketaccess_controller.go

@@ -324,14 +324,44 @@ func (bal *BucketAccessListener) Delete(ctx context.Context, bucketAccess *v1alp
 }
 
 func (bal *BucketAccessListener) deleteBucketAccessOp(ctx context.Context, bucketAccess *v1alpha1.BucketAccess) error {
+	// Fetching bucketClaim and corresponding bucket to get the bucketID
+	// for performing DriverRevokeBucketAccess request.
+	bucketClaimName := bucketAccess.Spec.BucketClaimName
+	bucketClaim, err := bal.bucketClaims(bucketAccess.ObjectMeta.Namespace).Get(ctx, bucketClaimName, metav1.GetOptions{})
+	if err != nil {
+		klog.V(3).ErrorS(err, "Failed to fetch bucketClaim", "bucketClaim", bucketClaimName)
+		return errors.Wrap(err, "Failed to fetch bucketClaim")
+	}
+
+	bucket, err := bal.buckets().Get(ctx, bucketClaim.Status.BucketName, metav1.GetOptions{})
+	if err != nil {
+		klog.V(3).ErrorS(err, "Failed to fetch bucket", "bucket", bucketClaim.Status.BucketName)
+		return errors.Wrap(err, "Failed to fetch bucket")
+	}
+
+	req := &cosi.DriverRevokeBucketAccessRequest{
+		BucketId:  bucket.Status.BucketID,
+		AccountId: bucketAccess.Status.AccountID,
+	}
+
+	// First we revoke the bucketAccess from the driver
+	if _, err := bal.provisionerClient.DriverRevokeBucketAccess(ctx, req); err != nil {
+		klog.V(3).ErrorS(err,
+			"Failed to revoke bucket access",
+			"bucketAccess", bucketAccess.ObjectMeta.Name,
+			"bucketClaim", bucketClaimName,
+		)
+		return errors.Wrap(err, "failed to revoke access")
+	}
+
 	credSecretName := bucketAccess.Spec.CredentialsSecretName
 	secret, err := bal.secrets(bucketAccess.ObjectMeta.Namespace).Get(ctx, credSecretName, metav1.GetOptions{})
 	if err != nil {
 		return err
 	}
 
 	if controllerutil.RemoveFinalizer(secret, consts.SecretFinalizer) {
-		_, err = bal.secrets(bucketAccess.ObjectMeta.Namespace).Update(ctx, secret, metav1.UpdateOptions{})
+		_, err = bal.secrets(secret.ObjectMeta.Namespace).Update(ctx, secret, metav1.UpdateOptions{})
 		if err != nil {
 			klog.V(3).ErrorS(err, "Error removing finalizer from secret",
 				"secret", secret.ObjectMeta.Name,
@@ -342,6 +372,15 @@ func (bal *BucketAccessListener) deleteBucketAccessOp(ctx context.Context, bucke
 		klog.V(5).Infof("Successfully removed finalizer from secret: %s, bucketAccess: %s", secret.ObjectMeta.Name, bucketAccess.ObjectMeta.Name)
 	}
 
+	err = bal.secrets(secret.ObjectMeta.Namespace).Delete(ctx, credSecretName, metav1.DeleteOptions{})
+	if err != nil {
+		klog.V(3).ErrorS(err, "Error deleting secret",
+			"secret", secret.ObjectMeta.Name,
+			"bucketAccess", bucketAccess.ObjectMeta.Name,
+			"ns", bucketAccess.ObjectMeta.Namespace)
+		return nil
+	}
+
 	if controllerutil.RemoveFinalizer(bucketAccess, consts.BAFinalizer) {
 		_, err = bal.bucketAccesses(bucketAccess.ObjectMeta.Namespace).Update(ctx, bucketAccess, metav1.UpdateOptions{})
 		if err != nil {