-
Notifications
You must be signed in to change notification settings - Fork 28
Adding logic to process BR and BAR deletes. #29
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ func (b *bucketAccessRequestListener) InitializeBucketClient(bc bucketclientset. | |
b.bucketClient = bc | ||
} | ||
|
||
// Add is in response to user adding a BucketAccessRequest. The call here will respond by creating a BucketAccess Object. | ||
func (b *bucketAccessRequestListener) Add(ctx context.Context, obj *v1alpha1.BucketAccessRequest) error { | ||
klog.V(1).Infof("Add called for BucketAccessRequest %s", obj.Name) | ||
bucketAccessRequest := obj | ||
|
@@ -55,13 +56,22 @@ func (b *bucketAccessRequestListener) Add(ctx context.Context, obj *v1alpha1.Buc | |
return nil | ||
} | ||
|
||
// Update is called in response to a change to BucketAccessRequest. At this point | ||
// BucketAccess cannot be changed once created as the Provisioner might have already acted upon the create BucketAccess and created the backend Bucket Credentials | ||
// Changes to Protocol, Provisioner, BucketInstanceName, BucketRequest cannot be allowed. Best is to delete and recreate a new BucketAccessRequest | ||
// Changes to ServiceAccount, PolicyActionsConfigMapData and Parameters should be considered in lieu with sidecar implementation | ||
func (b *bucketAccessRequestListener) Update(ctx context.Context, old, new *v1alpha1.BucketAccessRequest) error { | ||
klog.V(1).Infof("Update called for BucketAccessRequest %v", old.Name) | ||
if new.ObjectMeta.DeletionTimestamp != nil { | ||
// BucketAccessRequest is being deleted, check and remove finalizer once BA is deleted | ||
return b.removeBucketAccess(ctx, new) | ||
} | ||
return nil | ||
} | ||
|
||
func (b *bucketAccessRequestListener) Delete(ctx context.Context, obj *v1alpha1.BucketAccessRequest) error { | ||
klog.V(1).Infof("Delete called for BucketAccessRequest %v", obj.Name) | ||
// Delete is in response to user deleting a BucketAccessRequest. The call here will respond by deleting a BucketAccess Object. | ||
func (b *bucketAccessRequestListener) Delete(ctx context.Context, bucketAccessRequest *v1alpha1.BucketAccessRequest) error { | ||
klog.V(1).Infof("Delete called for BucketAccessRequest %v/%v", bucketAccessRequest.Namespace, bucketAccessRequest.Name) | ||
return nil | ||
} | ||
|
||
|
@@ -132,7 +142,7 @@ func (b *bucketAccessRequestListener) provisionBucketAccess(ctx context.Context, | |
} | ||
// bucketaccess.Spec.MintedSecretName - set by the driver | ||
bucketaccess.Spec.PolicyActionsConfigMapData, err = util.ReadConfigData(b.kubeClient, bucketAccessClass.PolicyActionsConfigMap) | ||
if err != nil { | ||
if err != nil && err != util.ErrNilConfigMap { | ||
return err | ||
} | ||
// bucketaccess.Spec.Principal - set by the driver | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this and the other set by driver comment -- it's really set by the sidecar, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct. |
||
|
@@ -144,6 +154,10 @@ func (b *bucketAccessRequestListener) provisionBucketAccess(ctx context.Context, | |
return err | ||
} | ||
|
||
if !util.CheckFinalizer(bucketAccessRequest, util.BARDeleteFinalizer) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In L159 above what about the case of the BA already existing? This would be true in brownfield access. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. brownfield case is not coded yet but my understanding is that brownfield case Bucket exist but BA/BAR works the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finalizers to BA are addressed in the Provisioner as discussed. |
||
bucketAccessRequest.ObjectMeta.Finalizers = append(bucketAccessRequest.ObjectMeta.Finalizers, util.BARDeleteFinalizer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this append work if metadata.Finalizers is nil (empty)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be fine as it is empty array. |
||
} | ||
|
||
err = retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
bucketAccessRequest.Status.BucketAccessName = bucketaccess.Name | ||
bucketAccessRequest.Status.AccessGranted = true | ||
|
@@ -159,3 +173,48 @@ func (b *bucketAccessRequestListener) provisionBucketAccess(ctx context.Context, | |
klog.Infof("Finished creating BucketAccess %v", bucketaccess.Name) | ||
return nil | ||
} | ||
|
||
func (b *bucketAccessRequestListener) removeBucketAccess(ctx context.Context, bucketAccessRequest *v1alpha1.BucketAccessRequest) error { | ||
bucketaccess := b.FindBucketAccess(ctx, bucketAccessRequest) | ||
if bucketaccess == nil { | ||
// bucketaccess for this BucketAccessRequest is not found | ||
return util.ErrBucketAccessDoesNotExist | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this an error? The BA is gone. Or do we want to retry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a know error that we need not retry. |
||
} | ||
|
||
// time to delete the BucketAccess Object | ||
err := b.bucketClient.ObjectstorageV1alpha1().BucketAccesses().Delete(context.Background(), bucketaccess.Name, metav1.DeleteOptions{}) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// we can safely remove the finalizer | ||
return b.removeBARDeleteFinalizer(ctx, bucketAccessRequest) | ||
} | ||
|
||
func (b *bucketAccessRequestListener) FindBucketAccess(ctx context.Context, bucketAccessRequest *v1alpha1.BucketAccessRequest) *v1alpha1.BucketAccess { | ||
bucketAccessList, err := b.bucketClient.ObjectstorageV1alpha1().BucketAccesses().List(ctx, metav1.ListOptions{}) | ||
if err != nil || len(bucketAccessList.Items) <= 0 { | ||
return nil | ||
} | ||
for _, bucketaccess := range bucketAccessList.Items { | ||
if bucketaccess.Spec.BucketAccessRequest.Name == bucketAccessRequest.Name && | ||
bucketaccess.Spec.BucketAccessRequest.Namespace == bucketAccessRequest.Namespace && | ||
bucketaccess.Spec.BucketAccessRequest.UID == bucketAccessRequest.UID { | ||
return &bucketaccess | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func (b *bucketAccessRequestListener) removeBARDeleteFinalizer(ctx context.Context, bucketAccessRequest *v1alpha1.BucketAccessRequest) error { | ||
newFinalizers := []string{} | ||
for _, finalizer := range bucketAccessRequest.ObjectMeta.Finalizers { | ||
if finalizer != util.BARDeleteFinalizer { | ||
newFinalizers = append(newFinalizers, finalizer) | ||
} | ||
} | ||
bucketAccessRequest.ObjectMeta.Finalizers = newFinalizers | ||
|
||
_, err := b.bucketClient.ObjectstorageV1alpha1().BucketAccessRequests(bucketAccessRequest.Namespace).Update(ctx, bucketAccessRequest, metav1.UpdateOptions{}) | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the CM now optional in the BAC? The KEP says it is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you meant, should I not check if CM is empty? Nothing guarantees that the CM is populated, we never catch that any other place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that if the CM ref pointer is nil then we don't return an error here, which, to me, means that BAC.CM is optional. If a BAC admission controller is ensuring the BAC.CM ref field is filled in then we should should not have to check for a nil CM ref.