Allow encryptInTransit to be specfied on the StorageClass

[jonathanrainer]

Is this a bug fix or adding new feature?
This PR adds a new parameter to the StorageClass parameters so encryptInTransit can be toggled on and off on a per StorageClass basis under DynamicProvisioning.

What is this PR about? / Why do we need it?
In response to #586. This means that those wanting to use Dynamic Provisioning to create their PVs have the same level of control as those who use static provisioning.

What testing is done?
Have extended the unit tests to ensure the defaulting is done properly etc. Don't think this needs extra E2E test coverage as the external_provisioner should deal with translating it onto the PersistentVolume and the actual ability to set this flag is already well covered by existing test suites, though I'm more than willing to take advice on this!

fixes #586

[k8s-ci-robot]

Hi @jonathanrainer. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

Details

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.

[jonathanrainer]

Have spun up this new code on my own EKS cluster with the following StorageClass

allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  creationTimestamp: "2022-06-19T05:58:31Z"
  labels:
    provisioning-type: dynamic
  name: efs-dynamic
  resourceVersion: "6662"
  uid: 3aa5319f-61b0-4976-8298-e8666552f98c
parameters:
  basePath: /dynamic
  directoryPerms: "777"
  encryptInTransit: "false" <- Turns off encryptInTransit
  fileSystemId: fs-05ccad61c1d5167f2
  gidRangeEnd: "2000"
  gidRangeStart: "1000"
  provisioningMode: efs-ap
provisioner: efs.csi.aws.com
reclaimPolicy: Delete
volumeBindingMode: Immediate

Which then led to the following PV being created

apiVersion: v1
kind: PersistentVolume
metadata:
  annotations:
    pv.kubernetes.io/provisioned-by: efs.csi.aws.com
  creationTimestamp: "2022-06-19T06:08:17Z"
  finalizers:
  - kubernetes.io/pv-protection
  name: pvc-fc245df4-d643-41d8-aaad-46ba06ae73f2
  resourceVersion: "8425"
  uid: 9790182d-0fa3-4585-9cd0-2f73d06ad16c
spec:
  accessModes:
  - ReadWriteMany
  capacity:
    storage: 5Gi
  claimRef:
    apiVersion: v1
    kind: PersistentVolumeClaim
    name: efs-dynamically-provisioned
    namespace: disable-encrypt-in-transit
    resourceVersion: "8416"
    uid: fc245df4-d643-41d8-aaad-46ba06ae73f2
  csi:
    driver: efs.csi.aws.com
    volumeAttributes:
      encryptInTransit: "false"
      storage.kubernetes.io/csiProvisionerIdentity: 1655618784326-8081-efs.csi.aws.com
    volumeHandle: fs-05ccad61c1d5167f2::fsap-06ca8819127017d60
  persistentVolumeReclaimPolicy: Delete
  storageClassName: efs-dynamic
  volumeMode: Filesystem
status:
  phase: Bound

So am happy to say that this does now all encryptInTransit to be specified on the StorageClass itself

[juanjimenezcasas]

Hi, when do you expect this feature to be merged?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jonathanrainer
Once this PR has been reviewed and has the lgtm label, please assign d-nishi for approval by writing /assign @d-nishi 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.

Details

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

[elimumford]

So, been trying to understand this stuff myself.

This change will never be merged, as use of EFS AP endpoints REQUIRES TLS. The source starting at https://github.com/kubernetes-sigs/aws-efs-csi-driver/blob/master/pkg/driver/node.go#L114 indicates this, and adds the tls mount option, regardless of the encryptInTransit setting. The only time the encryptInTransit would be respected is in Static provisioning when the PV is pointed at the entire EFS filesystem only.

So, while it would seem convenient to elevate this setting, StorageClasses are generally used by PVCs, which in the case of this driver generally implies dynamic provisioning, which this drivers does only via EFS APs that it creates, where TLS is required and the setting is therefore moot.

[k8s-triage-robot]

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

This bot triages issues and 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 issue is closed

You can:

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

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

/lifecycle stale