No VolumeAttributes in either DeleteVolumeRequest or NodeUnpublishVolumeRequest #167
Comments
|
https://github.com/container-storage-interface/spec/blob/716f53e230aa89be23a041cdb274c95611a19402/csi.proto#L253
In general, attributes shouldn't contain secrets. Aside from extending the
structs and costly API queries, are there alternative approaches you're
willing to consider?
…On Thu, Jan 11, 2018 at 12:27 PM, Serguei Bezverkhi < ***@***.***> wrote:
While working on a “clean up” part of CSI RBD plugin I have encountered
the following problem, neither DeleteVolumeRequest nor
NodeUnpublishVolumeRequest carry volume attributes map? It creates a
problem for rbd as without this information I cannot get secret for "rbd
unmap" operation for a rbd volume and also I cannot get the value of RBD
pool.
I mean it can be solved be very costly search through a bunch of api
objects, but these costly searches can be eliminated by just adding
VolumeAttribues map into both of above mentioned structs.
Appreciate a lot if you would accept a PR extending these structs.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#167>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLKazkWJl-kDHo_9PJW0uHaNv2PRtks5tJkSFgaJpZM4RbLcc>
.
|
|
@jdef Thanks so much for prompt reply. Secret does not actually contain a secret key but just a reference to the k8s object where the key is stored. It is the same as you run kubectl get secrets which list all available secrets in the namespace, so there is no security issue and it is already done in CreateVolumeRequest and NodePublishVolumeRequest. At this point I do not see a third alternative to two ways I mentioned in my initial post. |
|
So we used to have "metadata" (instead of "attributes") that was passed
around everywhere. This was contentious and as a compromise we basically
renamed the field, tightened the language around it, and limited which RPCs
it was passed to.
That said, if this is a considerable pain point maybe it's worth
reconsidering. Care to add this to the community sync agenda?
…On Thu, Jan 11, 2018 at 12:38 PM, Serguei Bezverkhi < ***@***.***> wrote:
@jdef <https://github.com/jdef> Thanks so much for prompt reply. Secret
does not actually contain a secret key but just a reference to the k8s
object where the key is stored. It is the same as you run kubectl get
secrets which list all available secrets in the namespace, so there is no
security issue and it is already done in CreateVolumeRequest and
NodePublishVolumeRequest. At this point I do not see a third alternative to
two ways I mentioned in my initial post.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#167 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLOcCZTVBFTUhsGmGcA29zTbJTBaLks5tJkb7gaJpZM4RbLcc>
.
|
|
@jdef I would add it, I just do not know where the agenda is, could you please point me to the right location? |
|
… On Thu, Jan 11, 2018 at 12:56 PM, Serguei Bezverkhi < ***@***.***> wrote:
@jdef <https://github.com/jdef> I would add it, I just do not know where
the agenda is, could you please point me to the right location?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#167 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLJIZeeC1DoB9nMHaYAKBOmZfWpVHks5tJktagaJpZM4RbLcc>
.
|
|
It's not great form to call k8s APIs from a CSI plugin, as the result will be a k8s plugin, and will fail CSI certification tests. I recall that we went back and forth several times about how this type of secret should make its way into the plugin. Is it an option to use the user_credentials field instead? Or is this secret stored in the credentials store by the plugin at volume creation time, and then retrieved later? If that flow is required then really, we ought to have some callback mechanism, or we need a way to explicitly return secrets from the plugin that are furnished to later calls. I recall that we had folks arguing for both flows, and ultimately punted the issue to after 0.1. Probably we should do something to fix this for 0.2... |
|
@julian-hj I agree but there are a couple of things. Secret is stored in k8s secret, to get the key stored there, there is no other way but to get the key using k8s call. Unless of course if the CSI driver would automatically get the key from the object and pass it to the plugin in user_credentials. This approach opens up a security issue when key would be stored in a variable. The secret is only a part of a problem, other pieces of information also needed for efficient volume management. Example for RBD Image: pool, user id and monitors are needed. |
|
Secrets should be shared with the CSI driver via the |
|
I have two concerns, 1 - if a secret gets carried by credential field, it becomes more exposed. 2 - secret is just a part of issue, I also need monitors, pools to be able to unmap rbd image. I can get this info from k8s but it will be costly in terms of performance and then plugin becomes even more k8s specific which as far as I understand is against CSI ideology. RBD will not be the only backend which might require more that just secret info, so the secure and efficient solution would be extremely helpful. |
That's a valid concern, but if you do not trust the CO to secure the gRPC endpoint, then you have the option of only passing in the secret reference, and fetching the secret yourself inside the plugin. This will make the plugin non-portable across COs, but that is a tradeoff you'll have to make.
The philosophy we've adopted is that the CO should be able to ask a volume plugin to clean up a mount even if the CO loses all state except the mount point (i.e. handle orphaned mounts). The volume plugin should maintain enough state internally (up to the plugin how) to be able to unmount correctly. |
|
It will be addressed as a part of #168 |
|
Please propose these changes in a pr separate from #168. Better to keep a
pr small vs mudding the water with many changes.
On Jan 19, 2018 6:51 PM, "Serguei Bezverkhi" <[email protected]> wrote:
It will be addressed as a part of #168
<#168>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#167 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLD5Yfc7dxndon6lCheYaUNwlImoBks5tMSp1gaJpZM4RbLcc>
.
|
|
hi guys, I am not sure the original issue has been fixed or not. I saw |
While working on a “clean up” part of CSI RBD plugin I have encountered the following problem, neither DeleteVolumeRequest nor NodeUnpublishVolumeRequest carry volume attributes map? It creates a problem for rbd as without this information I cannot get secret for "rbd unmap" operation for a rbd volume and also I cannot get the value of RBD pool.
I mean it can be solved be very costly search through a bunch of api objects, but these costly searches can be eliminated by just adding VolumeAttribues map into both of above mentioned structs.
Appreciate a lot if you would accept a PR extending these structs.
The text was updated successfully, but these errors were encountered: