Modification of names used for credentials #168
Conversation
|
Thanks for the PR. Related to #140 ? Also, please don't change generated files manually. Instead the root-level spec.md should be changed, and then run |
|
@sbezverk I expect to see 3 files changed, 2 of them are for generated files (csi.proto and the golang bindings (pb.go file)) and 1 file is manually altered (the changes made to spec.md). After making changes to spec.md, run |
|
/assign @saad-ali |
spec.md
Outdated
| // sensitive and MUST be treated as such (not logged, etc.) by the CO. | ||
| // This field is OPTIONAL. | ||
| map<string, string> user_credentials = 7; | ||
| map<string, string> node_credentials = 7; |
There was a problem hiding this comment.
As discussed in #140, NodePublishVolume might be called one per workload by a CO. Calling it node_credentials makes it sound like the credentials are for the entire node.
There was a problem hiding this comment.
@jdef before you had user_credentials which made sound like the same credential is used for everything.
controller_credential and node_credential are per function, the content of it depends on the usage of corresponding function controller or node. Making them even more granular is an option but imho is an overkill.
spec.md
Outdated
| // sensitive and MUST be treated as such (not logged, etc.) by the CO. | ||
| // This field is OPTIONAL. | ||
| map<string, string> user_credentials = 6; | ||
| map<string, string> controller_credentials = 6; |
There was a problem hiding this comment.
It's possible that the credentials used for CreateVolume/DestroyVolume are different than those used for ControllerPublishVolume/ControllerUnpublishVolume. Calling them controller_credentials makes it sounds like they should be the same. I think we either follow the suggestion in #140, or just name it as "credentials".
There was a problem hiding this comment.
I agree, let's go with just credentials or create_credentials, delete_credentials, controller_publish_credentials, etc.
spec.md
Outdated
| // sensitive and MUST be treated as such (not logged, etc.) by the CO. | ||
| // This field is OPTIONAL. | ||
| map<string, string> user_credentials = 6; | ||
| map<string, string> controller_credentials = 6; |
There was a problem hiding this comment.
I agree, let's go with just credentials or create_credentials, delete_credentials, controller_publish_credentials, etc.
|
@saad-ali done changes with slight modification, since controller and node has publish/unpublish calls, I suggest to use the following model: controller_create_credentials, controller_publish_credentials, node_publish_credentials etc. In this case there is no confusion which credentials are used for what purpose. |
|
@saad-ali Could you mark your review as completed then? thanks a lot for reviewing. |
spec.md
Outdated
| } | ||
|
|
||
| message NodeUnpublishVolumeResponse {} | ||
| ``` | ||
|
|
||
| ##### NodeUnpublishVolume Errors | ||
|
|
||
| If the plugin is unable to complete the NodeUnpublishVolume call successfully, it MUST return a non-ok gRPC code in the gRPC status. | ||
| If the plugin is unable to |
There was a problem hiding this comment.
please undo this seemingly unintentional change
There was a problem hiding this comment.
thank you for catching it, fixed.
This PR makes credentials related fields more intuitive and less confusing.
Closes: #140