Secrets in ListVolumes request

[lucianoq]

It would be nice to have secrets available in the ListVolumes request. In our use case, we need to know which volumes the user is allowed to see and which not.

I can attach a PR.

[tomereli]

@lucianoq how do you handle ListVolumes in the meanwhile? We are in the same boat, and the only solution we came up with is passing the secret via a volumeMount to the plugin container.

[lucianoq]

No solutions from this side. We decided to postpone the implementation of the ListVolumes capability. So we are actually blocked by this.

[jdef]

I'm not 100% clear on the use case for this. There are secrets for use w/ calls pertaining to singular volumes/snapshots. This looks like it might be the first place where secrets would be added for a "multi volume" call, and the motivation is not clear to me. If the plugin requires credentials to access a backend API then those credentials should be provided to the plugin executable via env or CLI args, etc.

What am I missing here?

[jdef]

I'm not 100% clear on the use case for this. There are secrets for use w/ calls pertaining to singular volumes/snapshots. This looks like it might be the first place where secrets would be added for a "multi volume" call, and the motivation is not clear to me. If the plugin requires credentials to access a backend API then those credentials should be provided to the plugin executable via env or CLI args, etc.

What am I missing here?

Upon further review, it appears the the ListSnapshotsRequest RPC had secrets added recently. Im still not 100% clear on why this was needed vs. passing the credentials to the plugin executable at startup time. It sounds like @saad-ali spent some time thinking on this, and that it was related to different storage pools having different credentials. It's unclear to me how the CO decides which secrets are mapped to which storage pools, but perhaps that's not worth debating here since it's a CO implementation detail?

That said/asked, I wonder what other context there might be for this issue.

[lucianoq]

There are secrets on single volume requests, in order to check if the requester has the right to perform an action on the single volume (view/attach/mount/delete/...) For the same reason, they have to be on multi-volume requests. On ListVolumes, the plugin can decide to show only a subset of them, related to which secrets the request contains. Some credentials can have a limited view permissions on the whole set.

[saad-ali]

I think you are referring to this comment https://github.com/container-storage-interface/spec/issues/370#issuecomment-517850572

We discussed this at the CSI community meeting. Initially I was opposed to the proposal, because in a traditional storage system, any credential required to auth with the backend should be populated in to the driver out of band from the CSI spec (e.g. for Kubernetes the CSI driver pod has secrets injected at deployment time). However, some one pointed out that some storage systems (like Ceph) may have multiple "storage pools" where each storage pool has different credentials. In that case, operations like ListSnapshot would require credentials (for the storage pool). Which makes sense to me.

That is why secrets was added to ListSnapshotsRequest.

By the same logic I would be fine with adding it to ListVolumes request.

[michael-db]

Formally, I see this is covered by #164, but I think it is good to have it as a separate issue. ListVolumes is the only one we need secrets in right now in order to support that call.

[ctrox]

Just adding another use case here, in csi-s3 you might setup a StorageClass that mounts a bucket from AWS S3 and another one which uses DigitalOcean. As they both have completely different credentials it makes little sense to inject the secrets at deployment time to the driver and there is a need for something more dynamic. So right now all secret names/namespaces are simply defined in the StorageClass (e.g. csi.storage.k8s.io/provisioner-secret-name). Every time a Request does not have secrets attached I basically cannot implement it. I was just trying to implement ControllerGetVolume which has the same problem as ListVolumes.