Add RevokeAccessContext to DriverRevokeBucketAccess API #147
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ | |
| * limitations under the License. | ||
| */ | ||
|
|
||
| // Copyright 2024 Hewlett Packard Enterprise Development LP. | ||
|
|
||
|
Comment on lines
+16
to
+17
There was a problem hiding this comment. @xing-yang do you know if there is a K8s/SIG policy about adding copyright attributions like this? There was a problem hiding this comment. Will wait for @xing-yang response as well & address this. |
||
| package bucketaccess | ||
|
|
||
| import ( | ||
|
|
@@ -350,10 +352,18 @@ func (bal *BucketAccessListener) deleteBucketAccessOp(ctx context.Context, bucke | |
| klog.V(3).ErrorS(err, "Failed to fetch bucket", "bucket", bucketClaim.Status.BucketName) | ||
| return fmt.Errorf("failed to fetch bucket: %w", err) | ||
| } | ||
| bucketAccessClassName := bucketAccess.Spec.BucketAccessClassName | ||
| bucketAccessClass, err := bal.bucketAccessClasses().Get(ctx, bucketAccessClassName, metav1.GetOptions{}) | ||
| if kubeerrors.IsNotFound(err) { | ||
| return bal.recordError(bucketAccess, v1.EventTypeWarning, events.FailedGrantAccess, err) | ||
| } else if err != nil { | ||
| klog.V(3).ErrorS(err, "Failed to fetch bucketAccessClass", "bucketAccessClass", bucketAccessClassName) | ||
| return fmt.Errorf("failed to fetch BucketAccessClass: %w", err) | ||
| } | ||
|
Comment on lines
+355
to
+362
There was a problem hiding this comment. I have seen issues with this approach in CSI drivers. This leads to drivers implementing code that looks to the StorageClass (approx equivalent to BucketAccessClass), which is mutable and can change over time (or be deleted). This then results in a situation where the driver is unable to proceed with the latest operation. Instead, the best pattern I have seen is to have the StorageClass data (including opaque params) copied to the PV so that the PV can be managed regardless of whether the StorageClass has been mutated. I would suggest that the resolution follow the suggested pattern rather than adding the class name, which in my experience encourages inconsistent expectations. @xing-yang or @shanduur do you have other thoughts? There was a problem hiding this comment. Hi @BlaineEXE , Thanks for your suggestion. Certainly agree with your point here.
I am more in-favor of 'Approach-2', but kindly do guide me if there's another way to retrieve 'Parameters' before 'DriverRevokeBucketAccess' API is invoked. Will wait for your, suggestion & incorporate changes accordingly. There was a problem hiding this comment. I think approach 2 is also the more correct one. That means a change to the API in addition to the sidecar. The next question I have is a matter of SIG/KEP process. With a CRD change, we may need to change the KEP. @xing-yang is it okay to make this change (what I consider a minor change) to the repos, or should we update the KEP first? There was a problem hiding this comment. Hi @BlaineEXE , @xing-yang, There was a problem hiding this comment. If I can add my 5-cents, I am also in favor of the approach 2. Unfortunately, I think those changes will be required to be added to the KEP for v1alpha2, as this is pretty major breaking change in the controller behavior. |
||
| req := &cosi.DriverRevokeBucketAccessRequest{ | ||
| BucketId: bucket.Status.BucketID, | ||
| AccountId: bucketAccess.Status.AccountID, | ||
| RevokeAccessContext: bucketAccessClass.Parameters, | ||
| } | ||
|
|
||
| // First we revoke the bucketAccess from the driver | ||
|
|
||
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.
I have reservations about using a monkey patch library. I realize that monkey patching is a common trend in python and ruby development (and other similar scripting languages), but my understanding is that the Golang devs specifically didn't allow for similar functionality.
When I have investigated monkey patching for unit tests in Go, the advice I have seen is to instead use interfaces (even defining custom ones for unit tests if needed) and mocking using Go's builtin language tools.
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.
Sure, will incorporate this in my next commit.