diff --git a/go.mod b/go.mod index 84fc76e..05d8583 100644 --- a/go.mod +++ b/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 ) diff --git a/go.sum b/go.sum index ae66e2e..81b165c 100644 --- a/go.sum +++ b/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= diff --git a/pkg/bucket/bucket_controller.go b/pkg/bucket/bucket_controller.go index 63a40ee..5adf241 100644 --- a/pkg/bucket/bucket_controller.go +++ b/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) diff --git a/pkg/bucket/bucket_controller_test.go b/pkg/bucket/bucket_controller_test.go index 68d75ce..460ec6f 100644 --- a/pkg/bucket/bucket_controller_test.go +++ b/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) + } +} diff --git a/pkg/bucketaccess/bucketaccess_controller.go b/pkg/bucketaccess/bucketaccess_controller.go index 8dde3c5..7b7b0b0 100644 --- a/pkg/bucketaccess/bucketaccess_controller.go +++ b/pkg/bucketaccess/bucketaccess_controller.go @@ -324,6 +324,36 @@ 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 { @@ -331,7 +361,7 @@ func (bal *BucketAccessListener) deleteBucketAccessOp(ctx context.Context, bucke } 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 {