Add RevokeAccessContext to DriverRevokeBucketAccess API

[narayviv]

Problem:
The current API 'DriverRevokeBucketAccess' for revoking a bucket's access, when invoked from the sidecar just passes the values for 'BucketId' & 'AccountId' alone.
Whereas the steps for revoking a bucket's access, might as well require the input passed with the 'Parameters' field, in the BucketAccessClass( linked to the Bucket Access CR).
https://github.com/kubernetes-sigs/container-object-storage-interface-api/blob/8f3e219335924b998c49c03056d61674361bd132/crds/objectstorage.k8s.io_bucketaccessclasses.yaml#L43

Implementation/Fix:
The latest available version of 'container-object-storage-interface-spec' supports passing a map<string,string> along with 'BucketId' & 'AccountId' in the API 'DriverRevokeBucketAccess'.
https://github.com/kubernetes-sigs/container-object-storage-interface-spec/blob/af070610a0e16b5713c5e248eb84fa7526626953/cosi.proto#L239
We can leverage this in sidecar code, by fetching the parameters provided in the BucketAccessClass & pass it to the 'DriverRevokeBucketAccess' API along with parameters 'BucketId' & 'AccountId'

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: narayviv
Once this PR has been reviewed and has the lgtm label, please assign saad-ali for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: narayviv / name: Vivek Vishnu (db1d186)

[k8s-ci-robot]

Welcome @narayviv!

It looks like this is your first PR to kubernetes-sigs/container-object-storage-interface-provisioner-sidecar 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/container-object-storage-interface-provisioner-sidecar has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @narayviv. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[BlaineEXE]

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?

[narayviv]

Hi @BlaineEXE ,

Thanks for your suggestion.

Certainly agree with your point here.
As per your suggestion, there's 2 ways (which I can think of), through which Parameters can be made available to the 'DriverRevokeBucketAccess' API.

• Approach-1: Take Parameter from the Bucket CR spec

  Pros:

  1. Given that the Bucket is already retrieved within the method, makes it easy to fetch parameters field from its spec.
     req := &cosi.DriverRevokeBucketAccessRequest{ BucketId: bucket.Status.BucketID, AccountId: bucketAccess.Status.AccountID, RevokeAccessContext: bucket.Spec.Parameters, }
     https://github.com/kubernetes-sigs/container-object-storage-interface-provisioner-sidecar/blob/80979e8992a6a2b2166f3ff1e7d39b4ab03f045c/pkg/bucketaccess/bucketaccess_controller.go#L348

  Cons:

  1. This would imply that the Parameters passed in BucketClass & BucketAccessClass should always be same (which it should not be).
  2. Given, at the time of BucketAccess Grant, Parameter is retrieved & passed from the BucketAccessClass. The
     same should be followed for Revoking BucketAccess as well.
     https://github.com/kubernetes-sigs/container-object-storage-interface-provisioner-sidecar/blob/80979e8992a6a2b2166f3ff1e7d39b4ab03f045c/pkg/bucketaccess/bucketaccess_controller.go#L170

• Approach-2: Save Parameter in BucketAccess CR spec at the time of creation

  Pros:

  1. Same approach seems to be followed for Bucket CR
     https://github.com/kubernetes-sigs/container-object-storage-interface-api/blob/8f3e219335924b998c49c03056d61674361bd132/crds/objectstorage.k8s.io_buckets.yaml#L97
  2. At the time of BucketAccess CR creation, parameter would be retrieved from the BucketAccessClass CR & stored in its spec.
  3. Parameter can be directly retrieved from the BucketAccess CR spec
     req := &cosi.DriverRevokeBucketAccessRequest{ BucketId: bucket.Status.BucketID, AccountId: bucketAccess.Status.AccountID, RevokeAccessContext: bucketAccess.Spec.Parameters, }
  4. Parameters in BucketAccessClass can be different & independent of BucketClass CR ( which seems to be the right way).

  Cons:

  1. Changes has to be made in BucketAccess CRD, to support parameter field in its spec.
     https://github.com/kubernetes-sigs/container-object-storage-interface-api/blob/8f3e219335924b998c49c03056d61674361bd132/crds/objectstorage.k8s.io_bucketaccesses.yaml#L34

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.

[BlaineEXE]

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?

[narayviv]

Hi @BlaineEXE , @xing-yang,
Any update on this..?

[shanduur]

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.

[BlaineEXE]

@xing-yang do you know if there is a K8s/SIG policy about adding copyright attributions like this?

[narayviv]

Will wait for @xing-yang response as well & address this.

[BlaineEXE]

In addition to EasyCLA (required), we are working to consolidate controller and sidecar code into one repo for easier management and testing. That would be this repo and branch: https://github.com/kubernetes-sigs/container-object-storage-interface-api/tree/monorepo

[BlaineEXE]

Requesting changes while discussions are still needed.

[BlaineEXE]

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.

[narayviv]

Sure, will incorporate this in my next commit.

[narayviv]

In addition to EasyCLA (required), we are working to consolidate controller and sidecar code into one repo for easier management and testing. That would be this repo and branch: https://github.com/kubernetes-sigs/container-object-storage-interface-api/tree/monorepo

For the EasyCLA -> Iam waiting for a response from my OSRB team.
For the monorepo -> Could I merge my changes in this repo( if all the changes are reviewed & approved) this time alone & if your team could cherry-pick my changes to the monorepo. Please do guide me here & I will make the changes accordingly.