Replace templates in SC parameter with PVC annotations

[vinli-cn]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Replace templates in SC parameter with PVC annotations, this fixes issue 428 in csi-driver-smb repo: kubernetes-csi/csi-driver-smb#428

This change has been tested manually with SMB CSI Driver which uses SC parameter subDir in SMB mount path, code example as below

Storage Class

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: smb
provisioner: smb.csi.k8s.io
parameters:
  subDir: ${pvc.annotations['subDirInPVC']}

PVC

kind: PersistentVolumeClaim
apiVersion: v1
metadata:
  name: pvc-smb
  annotations:
    subDirInPVC: "dir1"

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Replace SC parameter template with PVC annotations

[k8s-ci-robot]

Hi @vinli-cn. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vinli-cn
Once this PR has been reviewed and has the lgtm label, please assign xing-yang for approval by writing /assign @xing-yang in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andyzhangx]

/ok-to-test

[andyzhangx]

@vinli-cn could you add more details in PR description? thanks.

[andyzhangx]

pls add ut for this PR, and also I found resolveTemplate does not have any ut, I think you could submit a separate PR for resolveTemplate ut

[andyzhangx]

@xing-yang could you take a look? this is actually a requirement from smb csi driver, user wants to use subDir: ${pvc.annotations['key']} and we already supports ${pvc.annotations[key] for following parameters, this PR would support ${pvc.annotations[key] for all paramaters in storage class.

csi.storage.k8s.io/provisioner-secret-name
csi.storage.k8s.io/provisioner-secret-namespace
csi.storage.k8s.io/controller-publish-secret-name
csi.storage.k8s.io/controller-publish-secret-namespace
csi.storage.k8s.io/node-stage-secret-name
csi.storage.k8s.io/node-stage-secret-namespace
csi.storage.k8s.io/node-publish-secret-name
csi.storage.k8s.io/node-publish-secret-namespace

e.g.
https://kubernetes-csi.github.io/docs/secrets-and-credentials-storage-class.html

  csi.storage.k8s.io/node-publish-secret-name: ${pvc.annotations['team.example.com/key']}
  csi.storage.k8s.io/node-publish-secret-namespace: ${pvc.namespace}

[andyzhangx]

/assign @xing-yang

[andyzhangx]

/assign @msau42 @jsafrane

[xing-yang]

In the past, we have rejected enhancement requests like this for portability reasons. I think we need to have a discuss on this.
/hold

[xing-yang]

See this issue here: #86

[andyzhangx]

See this issue here: #86

@xing-yang this is not Adding PVC's annotation to CreateVolumeRequest.Parameters, and since csi.storage.k8s.io/node-publish-secret-name already supports pvc.annotations translation, this PR extends the translation support for other storage class fields, as long as it's defined as ${pvc.annotations['team.example.com/key']}, it would be translated for other fields.

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: fast-storage
provisioner: csi-driver.team.example.com
parameters:
  type: pd-ssd
  csi.storage.k8s.io/node-publish-secret-name: ${pvc.annotations['team.example.com/key']}
  csi.storage.k8s.io/node-publish-secret-namespace: ${pvc.namespace}

[vinli-cn]

See this issue here: #86

As Andy mentioned above, external-provisioner can replace templates in secret parameters in Storage Class with values from PVC annotations already, and this PR extends similar "translation" for other parameters in Storage Class, eg: ${pvc.annotations['subDirInPVC']} in my example. We've seen lots of user cases from community.

This PR doesn't have any correlation with #86. In fact, I've sent below PR for similar issue in #86 but it's being discussed now.
#800

[jsafrane]

I understand that passing annotations (or any other per-PVC configuration) to CSI driver can have mane useful use cases and is frequently asked for, my personal favorite is a way to tag volumes in a cloud by user (PVC author) provided tags.

However, it's a double edged sword.

1. As mentioned above, it makes PVCs not portable. This is a huge change, so far you could take PVCs to any Kubernetes with any CSI driver and they would work. All platform specific info was in StorageClass. How a random Helm chart author of a stateful application knows what annotations a particular CSI driver supports? So far they needed to care only about StorageClass name.

2. It opens doors for security issues. For example, Cannot mount an existing share via subDir parameter csi-driver-smb#428 - with that feature, anyone who can create a PVC can actually read data of other PVs that are dynamically provisioned from the same StorageClass, just by guessing the subdirectory name. It may work well in clusters where users trust each other, but it absolute no-go for multi-tenant clusters. It's not obvious from StorageClass parameter description that it lowers security. My experience is that CSI driver authors do not care or do not understand security too much and we end up with very insecure clusters.

Not that I'd like to have some way how to pass custom parameters from PVC to PV and to CSI driver, however, it must be done carefully. I don't have a magic solution that would solve both problems mentioned above, but it may exist.

[andyzhangx]

I understand that passing annotations (or any other per-PVC configuration) to CSI driver can have mane useful use cases and is frequently asked for, my personal favorite is a way to tag volumes in a cloud by user (PVC author) provided tags.

However, it's a double edged sword.

1. As mentioned above, it makes PVCs not portable. This is a huge change, so far you could take PVCs to any Kubernetes with any CSI driver and they would work. All platform specific info was in StorageClass. How a random Helm chart author of a stateful application knows what annotations a particular CSI driver supports? So far they needed to care only about StorageClass name.
2. It opens doors for security issues. For example, Cannot mount an existing share via subDir parameter csi-driver-smb#428 - with that feature, anyone who can create a PVC can actually read data of other PVs that are dynamically provisioned from the same StorageClass, just by guessing the subdirectory name. It may work well in clusters where users trust each other, but it absolute no-go for multi-tenant clusters. It's not obvious from StorageClass parameter description that it lowers security. My experience is that CSI driver authors do not care or do not understand security too much and we end up with very insecure clusters.

Not that I'd like to have some way how to pass custom parameters from PVC to PV and to CSI driver, however, it must be done carefully. I don't have a magic solution that would solve both problems mentioned above, but it may exist.

@jsafrane thanks for the comments. Regrading to point 1 & 2, I am thinking is it possible to add a feature flag for this feature(by default disable), so if users want this feature, they could turn it on in csi-provisioner themselves, by that way, this feature won't affect all csi-provisioner users, and it could also satisfy specific csi driver users.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.