remove opaque parameters from delete request and return bucket id o… #25

wlan0 · 2021-03-04T06:54:59Z

…n succesful bucket creation.

…n succesful bucket creation

jeffvance · 2021-03-10T19:53:05Z

cosi.proto

-    // Intentionally left blank
+    // BucketName returned here is expected to be the globally unique 
+	// identifier for the bucket in the object storage provider 
+	string BucketName = 1;


do we want BucketName here or BucketID?
I thought the response was going to return the full bucket id.

jeffvance · 2021-03-10T19:55:01Z

cosi.proto

 }

 message ProvisionerDeleteBucketRequest {
    // This field is REQUIRED
    // Protocol specific information required by the call is passed in as key,value pairs.
    Protocol protocol = 1;
-


don't we need the bucket id? How does the provisoner know which bucket to delete?

jeffvance · 2021-03-10T19:56:39Z

cosi.proto

-    // Protocol specific information required by the call is passed in as key,value pairs.
-    // The caller should treat the values in parameters as opaque. 
-    // The receiver is responsible for parsing and validating the values.
-    map<string,string> parameters = 2;


In ProvisionerDeleteBucketResponse (L200) should access_policy = 1 (not 4)?

In ProvisionerGrantBucketAccessRequest (L204), I thought that protocol and parameters were being removed. Also where is the id of the bucket for which access is requested? I think we need bucketID or bucketName.

jeffvance · 2021-03-10T20:03:12Z

cosi.proto

@@ -239,13 +235,9 @@ message ProvisionerRevokeBucketAccessRequest {
    // Protocol specific information required by the call is passed in as key,value pairs.
    Protocol protocol = 1;

-    // This field is OPTIONAL


Same comment as above. I thought protocol was being removed. I also think we need the bucket id so that the driver knows which bucket to revoke access to.

Yes, that's correct. I've updated the PR

jeffvance · 2021-03-25T16:09:53Z

lgtm

wlan0 · 2021-03-26T20:08:18Z

cosi.proto

    // be required later to revoke access. 
-    string principal = 1;
+    string accountID = 1;


accountID seems more intuitive and also is symmetric with bucketID

Ack. I don't have the context on this, maybe worth having someone else who was part of that discussion take a look

- Use BucketID returned from ProvisionerCreateBucket as the handle for subsequent bucket related gRPC calls - Remove Name and Version from Protocol

- Requires changes to API from this PR kubernetes-retired/container-object-storage-interface-api#35 - Requires changes to SPEC from this PR kubernetes-retired/container-object-storage-interface-spec#25

- Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-api/pull/35 - Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-spec/pull/25

saad-ali

LGTM mod accountName (I wasn't involved in the discussion to replace principal with accountName, so I'll assume those were discussed with the wider community).

saad-ali · 2021-03-29T07:42:44Z

spec.md

+        S3Parameters s3 = 1;
+        AzureBlobParameters azureBlob = 2;
+        GCSParameters gcs = 3;


nit: why suffix these with Parameters?

Protocol name S3 otherwise collides with the Parameter type S3

Do you mean it collides with enum ProtocolName? if so, is that enum still needed (it is removed in this PR)?

wlan0 · 2021-03-29T21:47:08Z

LGTM mod accountName (I wasn't involved in the discussion to replace principal with accountName, so I'll assume those were discussed with the wider community).

Yeah, this was discussed with the community and accepted

saad-ali

/lgtm
/approve

k8s-ci-robot · 2021-03-29T23:28:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, wlan0

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [saad-ali]

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

- Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-api/pull/35 - Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-spec/pull/25

- Requires changes to API from this PR github.com/kubernetes-retired/pull/35 - Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-spec/pull/25

remove opaque parameters from delete request and return bucket name o…

6f60b7e

…n succesful bucket creation

wlan0 requested a review from saad-ali March 4, 2021 06:54

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 4, 2021

k8s-ci-robot requested review from msau42 and xing-yang March 4, 2021 06:55

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 4, 2021

jeffvance reviewed Mar 10, 2021

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2021

wlan0 force-pushed the master branch from b3a0f9e to f29eca1 Compare March 17, 2021 18:56

wlan0 force-pushed the master branch 2 times, most recently from 5e48ef4 to c0c84e3 Compare March 25, 2021 15:57

wlan0 changed the title ~~remove opaque parameters from delete request and return bucket name o…~~ remove opaque parameters from delete request and return bucket id o… Mar 25, 2021

wlan0 force-pushed the master branch from c0c84e3 to 14d875b Compare March 26, 2021 20:07

wlan0 commented Mar 26, 2021

View reviewed changes

wlan0 force-pushed the master branch 2 times, most recently from 48f7904 to 6f5e0e4 Compare March 26, 2021 20:38

API updates

308a696

- Use BucketID returned from ProvisionerCreateBucket as the handle for subsequent bucket related gRPC calls - Remove Name and Version from Protocol

wlan0 force-pushed the master branch from 6f5e0e4 to 308a696 Compare March 26, 2021 20:40

wlan0 mentioned this pull request Mar 26, 2021

Major refactor and bugfixes kubernetes-retired/container-object-storage-interface-provisioner-sidecar#49

Merged

saad-ali reviewed Mar 29, 2021

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 29, 2021

wlan0 force-pushed the master branch 2 times, most recently from 509238b to 8d93410 Compare March 29, 2021 22:46

Protocol Parameters without suffix and naming convention

d3132c2

wlan0 force-pushed the master branch from 8d93410 to d3132c2 Compare March 29, 2021 22:51

saad-ali approved these changes Mar 29, 2021

View reviewed changes

k8s-ci-robot assigned saad-ali Mar 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2021

k8s-ci-robot merged commit 3bbacbb into kubernetes-retired:master Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove opaque parameters from delete request and return bucket id o… #25

remove opaque parameters from delete request and return bucket id o… #25

wlan0 commented Mar 4, 2021

jeffvance Mar 10, 2021 •

edited

Loading

jeffvance Mar 10, 2021

jeffvance Mar 10, 2021

jeffvance Mar 10, 2021 •

edited

Loading

jeffvance Mar 10, 2021

wlan0 Mar 17, 2021

jeffvance commented Mar 25, 2021

wlan0 Mar 26, 2021

saad-ali Mar 29, 2021

saad-ali left a comment

saad-ali Mar 29, 2021

wlan0 Mar 29, 2021

saad-ali Mar 29, 2021

wlan0 commented Mar 29, 2021

saad-ali left a comment

k8s-ci-robot commented Mar 29, 2021

remove opaque parameters from delete request and return bucket id o… #25

remove opaque parameters from delete request and return bucket id o… #25

Conversation

wlan0 commented Mar 4, 2021

jeffvance Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance commented Mar 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlan0 commented Mar 29, 2021

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 29, 2021

jeffvance Mar 10, 2021 •

edited

Loading

jeffvance Mar 10, 2021 •

edited

Loading