Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secrets in ListVolumes request #461

Open
lucianoq opened this issue Nov 4, 2020 · 8 comments · May be fixed by #462
Open

Secrets in ListVolumes request #461

lucianoq opened this issue Nov 4, 2020 · 8 comments · May be fixed by #462

Comments

@lucianoq
Copy link

lucianoq commented Nov 4, 2020

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.

@lucianoq lucianoq linked a pull request Nov 4, 2020 that will close this issue
@tomereli
Copy link

@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
Copy link
Author

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

@jdef
Copy link
Member

jdef commented Feb 13, 2021

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
Copy link
Member

jdef commented Feb 13, 2021

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
Copy link
Author

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
Copy link
Member

saad-ali commented Mar 3, 2021

I think you are referring to this comment #370 (comment)

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
Copy link

michael-db commented Mar 9, 2021

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
Copy link

ctrox commented Apr 8, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants