From 794a925a85fa9c8f64542829b0be3305efe18553 Mon Sep 17 00:00:00 2001 From: Cheng Xing Date: Wed, 23 Jun 2021 18:21:36 -0700 Subject: [PATCH] Pass FsGroup to NodeStageVolume --- pkg/volume/csi/csi_attacher.go | 3 +- pkg/volume/csi/csi_attacher_test.go | 90 +++++++++++++++++++------- pkg/volume/csi/csi_block.go | 3 +- pkg/volume/csi/csi_client.go | 23 +++++-- pkg/volume/csi/csi_client_test.go | 98 ++++++++++++++++++++++++----- pkg/volume/csi/fake/fake_client.go | 1 + 6 files changed, 175 insertions(+), 43 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index cda9c344ad6..355434a8b93 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -374,7 +374,8 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo accessMode, nodeStageSecrets, csiSource.VolumeAttributes, - mountOptions) + mountOptions, + deviceMounterArgs.FsGroup) if err != nil { return err diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index df3fe002b49..d25b01752c8 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -1068,22 +1068,27 @@ func TestAttacherGetDeviceMountPath(t *testing.T) { } func TestAttacherMountDevice(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DelegateFSGroupToCSIDriver, true)() + pvName := "test-pv" + var testFSGroup int64 = 3000 nonFinalError := volumetypes.NewUncertainProgressError("") transientError := volumetypes.NewTransientOperationFailure("") testCases := []struct { - testName string - volName string - devicePath string - deviceMountPath string - stageUnstageSet bool - shouldFail bool - createAttachment bool - populateDeviceMountPath bool - exitError error - spec *volume.Spec - watchTimeout time.Duration + testName string + volName string + devicePath string + deviceMountPath string + stageUnstageSet bool + fsGroup *int64 + expectedVolumeMountGroup string + shouldFail bool + createAttachment bool + populateDeviceMountPath bool + exitError error + spec *volume.Spec + watchTimeout time.Duration }{ { testName: "normal PV", @@ -1184,6 +1189,17 @@ func TestAttacherMountDevice(t *testing.T) { shouldFail: true, spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), true), }, + { + testName: "fsgroup set", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + fsGroup: &testFSGroup, + expectedVolumeMountGroup: "3000", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + }, } for _, tc := range testCases { @@ -1209,7 +1225,7 @@ func TestAttacherMountDevice(t *testing.T) { t.Fatalf("failed to create new attacher: %v", err0) } csiAttacher := getCsiAttacherFromVolumeAttacher(attacher, tc.watchTimeout) - csiAttacher.csiClient = setupClient(t, tc.stageUnstageSet) + csiAttacher.csiClient = setupClientWithVolumeMountGroup(t, tc.stageUnstageSet, true /* volumeMountGroupSet */) if tc.deviceMountPath != "" { tc.deviceMountPath = filepath.Join(tmpDir, tc.deviceMountPath) @@ -1247,7 +1263,11 @@ func TestAttacherMountDevice(t *testing.T) { } // Run - err := csiAttacher.MountDevice(tc.spec, tc.devicePath, tc.deviceMountPath, volume.DeviceMounterArgs{}) + err := csiAttacher.MountDevice( + tc.spec, + tc.devicePath, + tc.deviceMountPath, + volume.DeviceMounterArgs{FsGroup: tc.fsGroup}) // Verify if err != nil { @@ -1302,6 +1322,9 @@ func TestAttacherMountDevice(t *testing.T) { if !reflect.DeepEqual(vol.MountFlags, tc.spec.PersistentVolume.Spec.MountOptions) { t.Errorf("expected mount options: %v, got: %v", tc.spec.PersistentVolume.Spec.MountOptions, vol.MountFlags) } + if vol.VolumeMountGroup != tc.expectedVolumeMountGroup { + t.Errorf("expected volume mount group %q, got: %q", tc.expectedVolumeMountGroup, vol.VolumeMountGroup) + } } // Verify the deviceMountPath was created by the plugin @@ -1321,16 +1344,20 @@ func TestAttacherMountDevice(t *testing.T) { func TestAttacherMountDeviceWithInline(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIInlineVolume, true)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DelegateFSGroupToCSIDriver, true)() pvName := "test-pv" + var testFSGroup int64 = 3000 testCases := []struct { - testName string - volName string - devicePath string - deviceMountPath string - stageUnstageSet bool - shouldFail bool - spec *volume.Spec - watchTimeout time.Duration + testName string + volName string + devicePath string + deviceMountPath string + fsGroup *int64 + expectedVolumeMountGroup string + stageUnstageSet bool + shouldFail bool + spec *volume.Spec + watchTimeout time.Duration }{ { testName: "normal PV", @@ -1390,6 +1417,16 @@ func TestAttacherMountDeviceWithInline(t *testing.T) { deviceMountPath: "path2", shouldFail: true, }, + { + testName: "fsgroup set", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + fsGroup: &testFSGroup, + expectedVolumeMountGroup: "3000", + stageUnstageSet: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + }, } for _, tc := range testCases { @@ -1410,7 +1447,7 @@ func TestAttacherMountDeviceWithInline(t *testing.T) { t.Fatalf("failed to create new attacher: %v", err0) } csiAttacher := getCsiAttacherFromVolumeAttacher(attacher, tc.watchTimeout) - csiAttacher.csiClient = setupClient(t, tc.stageUnstageSet) + csiAttacher.csiClient = setupClientWithVolumeMountGroup(t, tc.stageUnstageSet, true /* volumeMountGroup */) if tc.deviceMountPath != "" { tc.deviceMountPath = filepath.Join(tmpDir, tc.deviceMountPath) @@ -1435,7 +1472,11 @@ func TestAttacherMountDeviceWithInline(t *testing.T) { }() // Run - err = csiAttacher.MountDevice(tc.spec, tc.devicePath, tc.deviceMountPath, volume.DeviceMounterArgs{}) + err = csiAttacher.MountDevice( + tc.spec, + tc.devicePath, + tc.deviceMountPath, + volume.DeviceMounterArgs{FsGroup: tc.fsGroup}) // Verify if err != nil { @@ -1467,6 +1508,9 @@ func TestAttacherMountDeviceWithInline(t *testing.T) { if vol.Path != tc.deviceMountPath { t.Errorf("expected mount path: %s. got: %s", tc.deviceMountPath, vol.Path) } + if vol.VolumeMountGroup != tc.expectedVolumeMountGroup { + t.Errorf("expected volume mount group %q, got: %q", tc.expectedVolumeMountGroup, vol.VolumeMountGroup) + } } wg.Wait() diff --git a/pkg/volume/csi/csi_block.go b/pkg/volume/csi/csi_block.go index 70837bb8303..3e68b7bb27a 100644 --- a/pkg/volume/csi/csi_block.go +++ b/pkg/volume/csi/csi_block.go @@ -193,7 +193,8 @@ func (m *csiBlockMapper) stageVolumeForBlock( accessMode, nodeStageSecrets, csiSource.VolumeAttributes, - nil /* MountOptions */) + nil, /* MountOptions */ + nil /* fsGroup */) if err != nil { return "", err diff --git a/pkg/volume/csi/csi_client.go b/pkg/volume/csi/csi_client.go index 404b299b431..c087a529d79 100644 --- a/pkg/volume/csi/csi_client.go +++ b/pkg/volume/csi/csi_client.go @@ -78,6 +78,7 @@ type csiClient interface { secrets map[string]string, volumeContext map[string]string, mountOptions []string, + fsGroup *int64, ) error NodeGetVolumeStats( @@ -384,6 +385,7 @@ func (c *csiDriverClient) NodeStageVolume(ctx context.Context, secrets map[string]string, volumeContext map[string]string, mountOptions []string, + fsGroup *int64, ) error { klog.V(4).Info(log("calling NodeStageVolume rpc [volid=%s,staging_target_path=%s]", volID, stagingTargetPath)) if volID == "" { @@ -425,11 +427,24 @@ func (c *csiDriverClient) NodeStageVolume(ctx context.Context, Block: &csipbv1.VolumeCapability_BlockVolume{}, } } else { + mountVolume := &csipbv1.VolumeCapability_MountVolume{ + FsType: fsType, + MountFlags: mountOptions, + } + + if utilfeature.DefaultFeatureGate.Enabled(features.DelegateFSGroupToCSIDriver) { + supported, err := c.NodeSupportsVolumeMountGroup(ctx) + if err != nil { + return err + } + + if supported && fsGroup != nil { + mountVolume.VolumeMountGroup = strconv.FormatInt(*fsGroup, 10 /* base */) + } + } + req.VolumeCapability.AccessType = &csipbv1.VolumeCapability_Mount{ - Mount: &csipbv1.VolumeCapability_MountVolume{ - FsType: fsType, - MountFlags: mountOptions, - }, + Mount: mountVolume, } } diff --git a/pkg/volume/csi/csi_client_test.go b/pkg/volume/csi/csi_client_test.go index 84ab3eb35cb..bf23b5f08cc 100644 --- a/pkg/volume/csi/csi_client_test.go +++ b/pkg/volume/csi/csi_client_test.go @@ -222,6 +222,7 @@ func (c *fakeCsiDriverClient) NodeStageVolume(ctx context.Context, secrets map[string]string, volumeContext map[string]string, mountOptions []string, + fsGroup *int64, ) error { c.t.Log("calling fake.NodeStageVolume...") req := &csipbv1.NodeStageVolumeRequest{ @@ -241,11 +242,24 @@ func (c *fakeCsiDriverClient) NodeStageVolume(ctx context.Context, Block: &csipbv1.VolumeCapability_BlockVolume{}, } } else { + mountVolume := &csipbv1.VolumeCapability_MountVolume{ + FsType: fsType, + MountFlags: mountOptions, + } + + if utilfeature.DefaultFeatureGate.Enabled(features.DelegateFSGroupToCSIDriver) { + supported, err := c.NodeSupportsVolumeMountGroup(ctx) + if err != nil { + return err + } + + if supported && fsGroup != nil { + mountVolume.VolumeMountGroup = strconv.FormatInt(*fsGroup, 10 /* base */) + } + } + req.VolumeCapability.AccessType = &csipbv1.VolumeCapability_Mount{ - Mount: &csipbv1.VolumeCapability_MountVolume{ - FsType: fsType, - MountFlags: mountOptions, - }, + Mount: mountVolume, } } @@ -572,6 +586,8 @@ func TestClientNodeUnpublishVolume(t *testing.T) { } func TestClientNodeStageVolume(t *testing.T) { + var testFSGroup int64 = 3000 + tmpDir, err := utiltesting.MkTmpdir("csi-test") if err != nil { t.Fatalf("can't create temp dir: %v", err) @@ -580,30 +596,78 @@ func TestClientNodeStageVolume(t *testing.T) { testPath := filepath.Join(tmpDir, "/test/path") testCases := []struct { - name string - volID string - stagingTargetPath string - fsType string - secrets map[string]string - mountOptions []string - mustFail bool - err error + name string + volID string + stagingTargetPath string + fsType string + secrets map[string]string + mountOptions []string + fsGroup *int64 + delegateFSGroupFeatureGate bool + driverSupportsVolumeMountGroup bool + expectedFSGroupInNodeStage string + mustFail bool + err error }{ {name: "test ok", volID: "vol-test", stagingTargetPath: testPath, fsType: "ext4", mountOptions: []string{"unvalidated"}}, {name: "missing volID", stagingTargetPath: testPath, mustFail: true}, {name: "missing target path", volID: "vol-test", mustFail: true}, {name: "bad fs", volID: "vol-test", stagingTargetPath: testPath, fsType: "badfs", mustFail: true}, {name: "grpc error", volID: "vol-test", stagingTargetPath: testPath, mustFail: true, err: errors.New("grpc error")}, + + { + name: "fsgroup provided, DelegateFSGroupToCSIDriver feature enabled, driver supports volume mount group; expect fsgroup to be passed to NodeStageVolume", + volID: "vol-test", + stagingTargetPath: testPath, + fsType: "ext4", + fsGroup: &testFSGroup, + delegateFSGroupFeatureGate: true, + driverSupportsVolumeMountGroup: true, + expectedFSGroupInNodeStage: "3000", + }, + { + name: "fsgroup not provided, DelegateFSGroupToCSIDriver feature enabled, driver supports volume mount group; expect fsgroup not to be passed to NodeStageVolume", + volID: "vol-test", + stagingTargetPath: testPath, + fsType: "ext4", + fsGroup: nil, + delegateFSGroupFeatureGate: true, + driverSupportsVolumeMountGroup: true, + expectedFSGroupInNodeStage: "", + }, + { + name: "fsgroup provided, DelegateFSGroupToCSIDriver feature enabled, driver does not support volume mount group; expect fsgroup not to be passed to NodeStageVolume", + volID: "vol-test", + stagingTargetPath: testPath, + fsType: "ext4", + fsGroup: &testFSGroup, + delegateFSGroupFeatureGate: true, + driverSupportsVolumeMountGroup: false, + expectedFSGroupInNodeStage: "", + }, + { + name: "fsgroup provided, DelegateFSGroupToCSIDriver feature disabled, driver supports volume mount group; expect fsgroup not to be passed to NodeStageVolume", + volID: "vol-test", + stagingTargetPath: testPath, + fsType: "ext4", + fsGroup: &testFSGroup, + delegateFSGroupFeatureGate: false, + driverSupportsVolumeMountGroup: true, + expectedFSGroupInNodeStage: "", + }, } for _, tc := range testCases { t.Logf("Running test case: %s", tc.name) + + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DelegateFSGroupToCSIDriver, tc.delegateFSGroupFeatureGate)() + + nodeClient := fake.NewNodeClientWithVolumeMountGroup(true /* stagingCapable */, tc.driverSupportsVolumeMountGroup) + nodeClient.SetNextError(tc.err) fakeCloser := fake.NewCloser(t) client := &csiDriverClient{ driverName: "Fake Driver Name", nodeV1ClientCreator: func(addr csiAddr, m *MetricsManager) (csipbv1.NodeClient, io.Closer, error) { - nodeClient := fake.NewNodeClient(false /* stagingCapable */) - nodeClient.SetNextError(tc.err) return nodeClient, fakeCloser, nil }, } @@ -618,9 +682,15 @@ func TestClientNodeStageVolume(t *testing.T) { tc.secrets, map[string]string{"attr0": "val0"}, tc.mountOptions, + tc.fsGroup, ) checkErr(t, tc.mustFail, err) + volumeMountGroup := nodeClient.GetNodeStagedVolumes()[tc.volID].VolumeMountGroup + if volumeMountGroup != tc.expectedFSGroupInNodeStage { + t.Errorf("expected VolumeMountGroup parameter in NodePublishVolumeRequest to be %q, got: %q", tc.expectedFSGroupInNodeStage, volumeMountGroup) + } + if !tc.mustFail { fakeCloser.Check() } diff --git a/pkg/volume/csi/fake/fake_client.go b/pkg/volume/csi/fake/fake_client.go index 6f6cf516307..5c95417421a 100644 --- a/pkg/volume/csi/fake/fake_client.go +++ b/pkg/volume/csi/fake/fake_client.go @@ -280,6 +280,7 @@ func (f *NodeClient) NodeStageVolume(ctx context.Context, req *csipb.NodeStageVo if mounted != nil { fsType = mounted.GetFsType() csiVol.MountFlags = mounted.GetMountFlags() + csiVol.VolumeMountGroup = mounted.VolumeMountGroup } if !strings.Contains(fsTypes, fsType) { return nil, errors.New("invalid fstype")