Allow PVC name/namespace as template for provisioner-secret

[saad-ali]

Background:

Issue #170

You can specify a provisioner-secret-name and provisioner-secret-namespace as a parameter in the StorageClass. Every volume provisioned using that StorageClass uses that secret. There was a desire to be able to specify a different secret for every volume generated.

To enable this @liggitt introduced templating so that secret references could be different per volume (see pull #69 and see documentation).

The problem is that for provisioner-secret-name and provisioner-secret-namespace the only supported template variable is ${pv.name}. They do not support PVC.

Problem:
This is problematic because pv.name is generated during volume provisioning, it is hard to predict what the generated PV name will be, so it is hard to provision a secret named pv.name before the volume is provisioned.

Effectively it means that provisioner-secret-name and provisioner-secret-namespace don't really support templating and have to be hard coded so provisioning secrets effectively can not be different per volume.

Proposal:
Support the template variable ${pvc.namespace}, ${pvc.name}, and ${pvc.annotations['<ANNOTATION_KEY>']} for both the provisioner-secret-name and provisioner-secret-namespace secret values. This would enable a different secret per provisioned volume.

The counter argument to this proposal per @liggitt:

The reason the secrets in the pvc namespace are not made available for paired create/delete operations is that the PVC and its namespace may not exist at deletion time

My suggestion:
Unless there is a security issue, relax the limitation and adopt the proposal so that storage systems (like Portworx) where secrets are not needed for volume delete can do this. And add documentation to discourage storage systems that require secret on delete from doing this.

CC @lpabon @harsh-px @liggitt @msau42

[msau42]

discourage storage systems that require secret on delete from doing this.

One way to strongly discourage this is to not pass in secrets generated from pvc on deletion.

[pohly]

One way to strongly discourage this is to not pass in secrets generated from pvc on deletion.

But does that then still meet the original use case (allowing users to provision volumes with their own storage backend credentials) if de-provisioning cannot be done by that same user because the CSI drivers doesn't get his credentials?

/cc @kfox1111 - this is relevant for ceph-csi (ceph/ceph-csi#80 (comment))

[lpabon]

discourage storage systems that require secret on delete from doing this.

I do not think this is possible.

I think we need to put this in a new light. The CSI spec shows that secrets are passed on each of the calls, and so the CO must honor that, in my opinion. I can see that pvc deletion, from a K8S point of view, may not require secret since the kubernetes has already checked if the user is part of that namespace and access to that pvc, but the CSI driver does not know this, specially not knowing which CO it is installed in.

[gnufied]

FWIW, this may not be unique to provisioner. I think snapshot controller has same problem. It also picks secret from storageclass. Is it desirable to have per-volume secret for snapshotting? We are also discussing this for resizing.

[pohly]

Hemant Kumar <notifications@github.com> writes:

FWIW, this may not be unique to provisioner. I think snapshot controller has same problem. It also picks secret from storageclass. Is it desirable to have per-volume secret for snapshotting? We are also discussing this for resizing.

I think so. Basically all clusters where different users share the same installation of a driver that needs per-user credentials in its storage backend have this issue, regardless of the operation (create, snapshot, resize, delete).

[saad-ali]

Spoke with @msau42 and @liggitt about this.

Proposal that we agreed on:

1. Always call DeleteVolume and ControllerUnpublishVolume even if we fail to fetch the secret (because some storage systems may not require secret on those operations but we'll never know if we don't try). That said, for CreateVolume, ControllerPublishVolume, NodeStageVolume, NodePublishVolume we should not call CSI operation if we fail to fetch secret.
2. Adopt the proposal in original comment in this issue to enable a different secret per provisioned volume.
3. Add documentation to warn users/storage vendors:
   • Put secrets in users namespace at your own risk -- if secret is deleted or changed by user (
     or if entire namespace is deleted, you may end up with a volume that can't be detached or deleted (if the volume requires secret for those operations).
   • Similarly, if DeleteVolume secret depends on an annotation template, and the annotation was removed or changed, you may end up with a volume that can't be deleted (if the volume requires secret for delete).

[msau42]

Correction, we cannot support PVC.annotation as a DeleteVolume secret. To trigger a DeleteVolume requires deleting the PVC object, so for sure, the PVC.annotations will be gone and not accessible.

[akgunjal]

@msau42 @saad-ali : I have a suggestion. If the PVC name/namespace are passed as template for create/delete volume then can we do the following.

(1) The Provision method in the external-provisioner can add the provisionerSecretRef in PV object after the CreateVolume call just like its done for controllerPublishSecretRef today. This is to use this secret in DeleteVolume call as PVC will be deleted but PV is still active.

(2) The delete method in the external-provisioner call will fetch the provisionerSecretRef from PV and get the provisionerCredentials. This provisionerCredentials will be passed along with volumeId to the DeleteVolume CSI method. The CSI driver will now have the secrets passed to use if its needed to delete the underlying storage.

This also applies for using ${pvc.annotations['<ANNOTATION_KEY>']} as I think it will have better control to specify secrets for per volume provisioning.

[lpabon]

@msau42 @liggitt and I just met to discuss a set of requirements for this issue, and after discussion, we noticed that the proposal above to support pvc name and pvc namespace as templates satisfy the requirement.

This will allow the cluster administrator to support the following scenarios:

1. Users create a single secret for all their PVCs in the same namespace with a certain name given to them by the cluster administrator.
2. Users create a secret for each PVC with the same name as the PVC in the same namespace.
3. Cluster admins setup the storage class to use a single secret in a namespace different from the PVC.
4. Cluster admins setup the storage class to use a secret they set up in the namespace of the PVC

Also we discussed that on deletion, the CSI DeleteVolume will still be called even though it may or may not be able to populate the secrets argument from the Kubernetes Secret.

[msau42]

It's also important to note that if the secret for deletion is in the same namesapce as the PVC, then it might be possible for the secret to be deleted before the PV is deleted. In that case, manual intervention is needed to manually delete the volume. This should be documented as a warning. One possible way to address this is to have the user also put a finalizer on that secret. But then something needs to manage removing that finalizer.

Also another scenario that could be supported:
5. An admin/operator can generate a secret for each user namespace with the name of that namespace, stored in a separate, fixed admin namespace.