nfs: add support for `clients` in the StorageClass #3895

spuiuk · 2023-06-09T13:12:33Z

The clients parameter in the storage class is used to limit access to the export to the set of hostnames, networks or ip addresses specified.

Describe what this PR does

This PR addresses the requirements in Issue #3852 allowing users to specify hostname, networks and ip addresses allowed to access the NFS volume.

Related issues

Fixes: #3852

Future concerns

Users may want to add different access_types for different clients. However this is not supported by go-ceph at the moment. We may have to review this change once we can set this up with go-ceph.

spuiuk · 2023-06-09T13:33:19Z

/test ci/centos/mini-e2e-helm/k8s-1.27

spuiuk · 2023-06-12T10:10:41Z

From my own testing.

Storage-Class:

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: rook-nfs
provisioner: rook-ceph.nfs.csi.ceph.com
parameters:
  nfsCluster: my-nfs
  server: rook-ceph-nfs-my-nfs-a
  clusterID: rook-ceph 
  fsName: myfs
  pool: myfs-replicated
  csi.storage.k8s.io/provisioner-secret-name: rook-csi-cephfs-provisioner
  csi.storage.k8s.io/provisioner-secret-namespace: rook-ceph 
  csi.storage.k8s.io/controller-expand-secret-name: rook-csi-cephfs-provisioner
  csi.storage.k8s.io/controller-expand-secret-namespace: rook-ceph 
  csi.storage.k8s.io/node-stage-secret-name: rook-csi-cephfs-node
  csi.storage.k8s.io/node-stage-secret-namespace: rook-ceph 
  secTypes: sys,krb5
  clients: 192.168.23.1/24,192.168.1.1/24
reclaimPolicy: Delete
allowVolumeExpansion: true
mountOptions:

Test PVC created from

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: nfs-test
spec:
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 1Gi
  storageClassName: rook-nfs

The resulting exports block created by the CSI driver.

EXPORT {
    FSAL {
        name = "CEPH";
        user_id = "nfs.my-nfs.1";
        filesystem = "myfs";
        secret_access_key = "AQAu2IZkVBzGLRAABhHO4zuxlhYxXNChWrdUDA==";
    }
    CLIENT {
        clients = 192.168.23.1/24,  192.168.1.1/24;
        access_type = "rw";
        squash = "none";
    }
    export_id = 1;
    path = "/volumes/csi/csi-vol-a79fb1a5-93d8-40cb-a18d-be4dc5f7c0ea/f6dcc67d-5276-4bf2-8c25-71d29bf4a044";
    pseudo = "/0001-0009-rook-ceph-0000000000000001-a79fb1a5-93d8-40cb-a18d-be4dc5f7c0ea";
    access_type = "none";
    squash = "none";
    attr_expiration_time = 0;
    security_label = true;
    protocols = 4;
    transports = "TCP";
    SecType = "sys", "krb5";
}

Rakshith-R · 2023-06-12T10:36:00Z

/test ci/centos/mini-e2e-helm/k8s-1.27

Rakshith-R · 2023-06-12T10:36:14Z

/test ci/centos/mini-e2e/k8s-1.27

Rakshith-R

A small nit, the pr looks good to me.
I'll let niels do review since he's more familiar with nfs exports.
(helm e2e doesn't test nfs.)

Rakshith-R · 2023-06-12T10:35:23Z

examples/nfs/storageclass.yaml

+  # (optional) List of IP addresses, hostnames or IPv4 network addresses that
+  # that these export permissions apply to. The <client-list> is a comma


Let's word it similar to the commit message and make it clear that only these clients will be able to access
the export ?

nixpanic · 2023-06-12T15:08:02Z

e2e/nfs.go

+				if err != nil {
+					framework.Failf("failed to create NFS storageclass: %v", err)
+				}
+				err = validatePVCAndAppBinding(pvcPath, appPath, f)


Do you expect this to fail, or succeed? I don't know what IP-address is used while testing, so using an IP-address that definitely won't be used might be best to check if mounting fails?

The test currently fails:

�[1mSTEP:�[0m create a storageclass with a list of clients allowed to mount it and a PVC then bind it to an app �[38;5;243m@ 06/12/23 10:55:40.376�[0m Jun 12 10:55:40.380: INFO: ExecWithOptions {Command:[/bin/sh -c ceph fsid] Namespace:rook-ceph PodName:rook-ceph-tools-cbdbf594c-cwdfj ContainerName:rook-ceph-tools Stdin:<nil> CaptureStdout:true CaptureStderr:true PreserveWhitespace:true Quiet:false} Jun 12 10:55:40.381: INFO: >>> kubeConfig: /root/.kube/config Jun 12 10:55:40.381: INFO: ExecWithOptions: Clientset creation Jun 12 10:55:40.381: INFO: ExecWithOptions: execute(POST https://192.168.49.2:8443/api/v1/namespaces/rook-ceph/pods/rook-ceph-tools-cbdbf594c-cwdfj/exec?command=%2Fbin%2Fsh&command=-c&command=ceph+fsid&container=rook-ceph-tools&container=rook-ceph-tools&stderr=true&stdout=true) Jun 12 10:55:41.014: INFO: Waiting up to &PersistentVolumeClaim{ObjectMeta:{cephcsi-nfs-pvc nfs-9603 0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},Spec:PersistentVolumeClaimSpec{AccessModes:[ReadWriteMany],Resources:ResourceRequirements{Limits:ResourceList{},Requests:ResourceList{storage: {{1073741824 0} {<nil>} 1Gi BinarySI},},Claims:[]ResourceClaim{},},VolumeName:,Selector:nil,StorageClassName:*csi-nfs-sc,VolumeMode:nil,DataSource:nil,DataSourceRef:nil,},Status:PersistentVolumeClaimStatus{Phase:,AccessModes:[],Capacity:ResourceList{},Conditions:[]PersistentVolumeClaimCondition{},AllocatedResources:ResourceList{},ResizeStatus:nil,},} to be in Bound state Jun 12 10:55:41.014: INFO: waiting for PVC cephcsi-nfs-pvc (0 seconds elapsed) Jun 12 10:55:43.016: INFO: waiting for PVC cephcsi-nfs-pvc (2 seconds elapsed) Jun 12 10:55:43.021: INFO: Waiting for PV pvc-c2f7bf8f-7300-4ade-959d-2198a50677d8 to bind to PVC cephcsi-nfs-pvc Jun 12 10:55:43.021: INFO: Waiting up to timeout=10m0s for PersistentVolumeClaims [cephcsi-nfs-pvc] to have phase Bound Jun 12 10:55:43.023: INFO: PersistentVolumeClaim cephcsi-nfs-pvc found and phase=Bound (1.967893ms) Jun 12 10:55:43.023: INFO: Waiting up to 10m0s for PersistentVolume pvc-c2f7bf8f-7300-4ade-959d-2198a50677d8 to have phase Bound Jun 12 10:55:43.025: INFO: PersistentVolume pvc-c2f7bf8f-7300-4ade-959d-2198a50677d8 found and phase=Bound (2.020311ms) Jun 12 10:55:43.034: INFO: Waiting up to cephcsi-nfs-demo-pod to be in Running state �[38;5;9m[FAILED]�[0m in [It] - /go/src/github.com/ceph/ceph-csi/e2e/nfs.go:452 �[38;5;243m@ 06/12/23 11:05:43.035�[0m Jun 12 11:05:43.060: INFO: Logs of cephcsi-e2e-35924b08/csi-nfsplugin-provisioner-7cb7dbcffb-jrwt5:csi-provisioner on node minikube

https://jenkins-ceph-csi.apps.ocp.cloud.ci.centos.org/blue/rest/organizations/jenkins/pipelines/mini-e2e_k8s-1.27/runs/164/nodes/90/log/

I didn't realise that it actually mounts and tests it. I thought it simply built the exports file and attached it to confirm that the configuration file is build as expected and no errors were detected. I'll investigate this further.

spuiuk · 2023-06-16T13:40:50Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-06-16T17:53:41Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-06-16T22:30:40Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-06-17T10:45:40Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-06-29T11:01:37Z

/test ci/centos/mini-e2e/k8s-1.27

Madhu-1 · 2023-06-29T11:17:40Z

/test ci/centos/mini-e2e/k8s-1.27

Madhu-1 · 2023-06-29T11:19:40Z

e2e/nfs.go

+				found := false
+				for _, export := range exportList {
+					for _, client := range export.Clients {
+						for _, address := range client.Addresses {
+							if address == clientExample {
+								found = true
+								break
+							}
+						}
+						if found == true {
+							if client.AccessType != "rw" {
+								framework.Failf("Unexpected value for client AccessType: %s", client.AccessType)
+							}
+							break
+						}
+					}
+					if found == true {
+						if export.AccessType != "none" {
+							framework.Failf("Unexpected value for default AccessType: %s", export.AccessType)
+						}
+						break
+					}
+				}


Move this to a helper function which does all the check and return error or nil

Madhu-1 · 2023-06-29T11:20:35Z

e2e/nfs.go

+				pvc, err := loadPVC(pvcPath)
+				if err == nil {
+					framework.Failf("Could not create PVC: %v", err)
+				}


Dont we need to create PVC here and ensure its bound?

Yes. You are right. I am still experimenting with this code and unfortunately the only easy way to test it is to send it to the PR. Thanks for the review but it isn't ready yet. I'll post it when the code is closer to being ready.

Madhu-1 · 2023-06-29T11:21:41Z

examples/nfs/storageclass.yaml

+  # access to the export to the set of hostnames, networks or ip addresses
+  # specified.  The <client-list> is a comma delimited string,
+  # for example: "192.168.0.10,192.168.1.0/8"
+  # clients: <client-list>


Is there a chance we need these IP's to be ever changed on the existing PVC?

There could be requirements to change the ip addresses in the figure. There isn't an easy solution for that and will need users to use a new storageclass with the new set of ips. Unfortunately that is a limitation of this particular design.

Is it possible to move this configuration to the configmap here https://github.com/ceph/ceph-csi/blob/devel/deploy/csi-config-map-sample.yaml#L68?

Not sure how practical that is. Changing the client list after the fact is not something that can (easily) be done through Ceph Mgr once the NFS-export has been created. As the StorageClass parameters are immutable, it shows the right behavior (and difficulties for changing options).

The current design allows creation of different StorageClasses with different client-lists. It becomes ugly and more error prone if that is placed in a ConfigMap.

spuiuk · 2023-06-29T14:28:02Z

/test ci/centos/mini-e2e/k8s-1.27

e2e/nfs.go

spuiuk · 2023-06-29T17:26:28Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-06-29T20:08:52Z

/test ci/centos/mini-e2e/k8s-1.27

e2e/utils.go

nixpanic · 2023-06-30T08:08:47Z

I don't immediately see why the ci fails, it complains about e2e/nfs.go:472 which is the deletion of the StorageClass, but there are no errors logged that help identifying the cause. My guess is that ceph fsid failed for some reason, maybe it needs a retry as we have in other parts of the code?

logs

nixpanic · 2023-06-30T08:09:00Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-06-30T09:10:36Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-07-03T22:38:01Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-07-04T09:08:59Z

/test ci/centos/mini-e2e/k8s-1.27

spuiuk · 2023-07-04T10:55:00Z

/test ci/centos/mini-e2e/k8s-1.27

nixpanic

looks good, thanks for all the iterations to add a test!

nixpanic · 2023-07-05T09:07:46Z

@Mergifyio queue

mergify · 2023-07-05T09:07:57Z

queue

🛑 The pull request has been removed from the queue `default`

Pull request #3895 has been dequeued due to failing checks or checks timeout.

You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

nixpanic · 2023-07-05T18:54:48Z

@Mergifyio refresh

mergify · 2023-07-05T18:54:51Z

refresh

✅ Pull request refreshed

nixpanic · 2023-07-05T18:56:16Z

@Mergifyio rebase

The clients parameter in the storage class is used to limit access to the export to the set of hostnames, networks or ip addresses specified. Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>

mergify · 2023-07-05T18:56:27Z

rebase

✅ Branch has been successfully rebased

ceph-csi-bot · 2023-07-05T18:57:22Z

/test ci/centos/k8s-e2e-external-storage/1.25

ceph-csi-bot · 2023-07-05T18:57:23Z

/test ci/centos/k8s-e2e-external-storage/1.26

ceph-csi-bot · 2023-07-05T18:57:25Z

/test ci/centos/k8s-e2e-external-storage/1.27

ceph-csi-bot · 2023-07-05T18:57:26Z

/test ci/centos/mini-e2e-helm/k8s-1.25

ceph-csi-bot · 2023-07-05T18:57:27Z

/test ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-07-05T18:57:28Z

/test ci/centos/mini-e2e-helm/k8s-1.27

ceph-csi-bot · 2023-07-05T18:57:29Z

/test ci/centos/mini-e2e/k8s-1.25

ceph-csi-bot · 2023-07-05T18:57:30Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-07-05T18:57:31Z

/test ci/centos/mini-e2e/k8s-1.27

ceph-csi-bot · 2023-07-05T18:57:33Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-07-05T18:57:34Z

/test ci/centos/upgrade-tests-rbd

yati1998 · 2023-07-06T04:45:50Z

/retest ci/centos/mini-e2e-helm/k8s-1.25

mergify bot added the component/nfs Issues related to NFS label Jun 9, 2023

Rakshith-R reviewed Jun 12, 2023

View reviewed changes

Rakshith-R requested a review from nixpanic June 12, 2023 10:37

nixpanic reviewed Jun 12, 2023

View reviewed changes

spuiuk force-pushed the clientaddr branch from c024ef9 to 7029c9a Compare June 16, 2023 13:17

spuiuk force-pushed the clientaddr branch from 7029c9a to d03d3fe Compare June 16, 2023 17:52

spuiuk force-pushed the clientaddr branch from d03d3fe to b690dc4 Compare June 16, 2023 22:27

spuiuk force-pushed the clientaddr branch from b690dc4 to 4351885 Compare June 29, 2023 10:54

Madhu-1 reviewed Jun 29, 2023

View reviewed changes

spuiuk force-pushed the clientaddr branch from 4351885 to d204379 Compare June 29, 2023 14:23

nixpanic reviewed Jun 29, 2023

View reviewed changes

e2e/nfs.go Outdated Show resolved Hide resolved

spuiuk force-pushed the clientaddr branch from d204379 to 3428b47 Compare June 29, 2023 17:21

nixpanic reviewed Jun 30, 2023

View reviewed changes

e2e/utils.go Outdated Show resolved Hide resolved

spuiuk force-pushed the clientaddr branch from 3428b47 to 6df0f7e Compare June 30, 2023 08:39

nixpanic approved these changes Jul 4, 2023

View reviewed changes

nixpanic requested review from Madhu-1 and Rakshith-R July 5, 2023 07:38

Madhu-1 approved these changes Jul 5, 2023

View reviewed changes

nfs: add support for clients in the StorageClass

a479353

The clients parameter in the storage class is used to limit access to the export to the set of hostnames, networks or ip addresses specified. Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>

nixpanic force-pushed the clientaddr branch from 9d9389a to a479353 Compare July 5, 2023 18:56

nixpanic added the ok-to-test Label to trigger E2E tests label Jul 5, 2023

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 5, 2023

mergify bot merged commit 254699c into ceph:devel Jul 6, 2023
25 checks passed

		# (optional) List of IP addresses, hostnames or IPv4 network addresses that
		# that these export permissions apply to. The <client-list> is a comma

nfs: add support for clients in the StorageClass #3895

nfs: add support for clients in the StorageClass #3895

Conversation

spuiuk commented Jun 9, 2023

Describe what this PR does

Related issues

Future concerns

spuiuk commented Jun 9, 2023

spuiuk commented Jun 12, 2023

Rakshith-R commented Jun 12, 2023

Rakshith-R commented Jun 12, 2023

Rakshith-R left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic Jun 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spuiuk commented Jun 16, 2023

spuiuk commented Jun 16, 2023

spuiuk commented Jun 16, 2023

spuiuk commented Jun 17, 2023

spuiuk commented Jun 29, 2023

Madhu-1 commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spuiuk commented Jun 29, 2023

spuiuk commented Jun 29, 2023

spuiuk commented Jun 29, 2023

nixpanic commented Jun 30, 2023

nixpanic commented Jun 30, 2023

spuiuk commented Jun 30, 2023

spuiuk commented Jul 3, 2023

spuiuk commented Jul 4, 2023

spuiuk commented Jul 4, 2023

nixpanic left a comment

Choose a reason for hiding this comment

nixpanic commented Jul 5, 2023

mergify bot commented Jul 5, 2023 • edited

🛑 The pull request has been removed from the queue default

nixpanic commented Jul 5, 2023

mergify bot commented Jul 5, 2023

✅ Pull request refreshed

nixpanic commented Jul 5, 2023

mergify bot commented Jul 5, 2023

✅ Branch has been successfully rebased

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

ceph-csi-bot commented Jul 5, 2023

yati1998 commented Jul 6, 2023

nfs: add support for `clients` in the StorageClass #3895

nfs: add support for `clients` in the StorageClass #3895

nixpanic Jun 12, 2023 •

edited

mergify bot commented Jul 5, 2023 •

edited

🛑 The pull request has been removed from the queue `default`