From c50b3074fe876a91ac4a67d917b8f6c8caa922b2 Mon Sep 17 00:00:00 2001 From: Cheng Xing Date: Sat, 3 Jul 2021 16:22:08 -0700 Subject: [PATCH] Moved VOLUME_MOUNT_GROUP capability check from NodeStageVolume to MountDevice; added log message in SetupAt to indicate FSGroup is delegated to driver --- pkg/volume/csi/csi_attacher.go | 17 +++++- pkg/volume/csi/csi_attacher_test.go | 90 +++++++++++++++++++++-------- pkg/volume/csi/csi_client.go | 17 ++---- pkg/volume/csi/csi_client_test.go | 85 +++++---------------------- pkg/volume/csi/csi_mounter.go | 1 + 5 files changed, 106 insertions(+), 104 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index 355434a8b93..3528dc3b833 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -27,6 +27,7 @@ import ( "time" "k8s.io/apimachinery/pkg/util/clock" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog/v2" v1 "k8s.io/api/core/v1" @@ -38,6 +39,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" + "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/volume" volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) @@ -365,6 +367,19 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo mountOptions = spec.PersistentVolume.Spec.MountOptions } + var nodeStageFSGroupArg *int64 + if utilfeature.DefaultFeatureGate.Enabled(features.DelegateFSGroupToCSIDriver) { + driverSupportsCSIVolumeMountGroup, err := csi.NodeSupportsVolumeMountGroup(ctx) + if err != nil { + return volumetypes.NewTransientOperationFailure(log("attacher.MountDevice failed to determine if the node service has VOLUME_MOUNT_GROUP capability: %v", err)) + } + + if driverSupportsCSIVolumeMountGroup { + klog.V(3).Infof("Driver %s supports applying FSGroup (has VOLUME_MOUNT_GROUP node capability). Delegating FSGroup application to the driver through NodeStageVolume.", csiSource.Driver) + nodeStageFSGroupArg = deviceMounterArgs.FsGroup + } + } + fsType := csiSource.FSType err = csi.NodeStageVolume(ctx, csiSource.VolumeHandle, @@ -375,7 +390,7 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo nodeStageSecrets, csiSource.VolumeAttributes, mountOptions, - deviceMounterArgs.FsGroup) + nodeStageFSGroupArg) if err != nil { return err diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index d25b01752c8..a141f9a80f8 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -1076,19 +1076,21 @@ func TestAttacherMountDevice(t *testing.T) { transientError := volumetypes.NewTransientOperationFailure("") testCases := []struct { - 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 string + volName string + devicePath string + deviceMountPath string + stageUnstageSet bool + fsGroup *int64 + expectedVolumeMountGroup string + delegateFSGroupFeatureGate bool + driverSupportsVolumeMountGroup bool + shouldFail bool + createAttachment bool + populateDeviceMountPath bool + exitError error + spec *volume.Spec + watchTimeout time.Duration }{ { testName: "normal PV", @@ -1190,15 +1192,55 @@ func TestAttacherMountDevice(t *testing.T) { 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), + testName: "fsgroup provided, DelegateFSGroupToCSIDriver feature enabled, driver supports volume mount group; expect fsgroup to be passed to NodeStageVolume", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + fsGroup: &testFSGroup, + delegateFSGroupFeatureGate: true, + driverSupportsVolumeMountGroup: true, + expectedVolumeMountGroup: "3000", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + }, + { + testName: "fsgroup not provided, DelegateFSGroupToCSIDriver feature enabled, driver supports volume mount group; expect fsgroup not to be passed to NodeStageVolume", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + delegateFSGroupFeatureGate: true, + driverSupportsVolumeMountGroup: true, + expectedVolumeMountGroup: "", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + }, + { + testName: "fsgroup provided, DelegateFSGroupToCSIDriver feature enabled, driver does not support volume mount group; expect fsgroup not to be passed to NodeStageVolume", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + fsGroup: &testFSGroup, + delegateFSGroupFeatureGate: true, + driverSupportsVolumeMountGroup: false, + expectedVolumeMountGroup: "", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), + }, + { + testName: "fsgroup provided, DelegateFSGroupToCSIDriver feature disabled, driver supports volume mount group; expect fsgroup not to be passed to NodeStageVolume", + volName: "test-vol1", + devicePath: "path1", + deviceMountPath: "path2", + fsGroup: &testFSGroup, + delegateFSGroupFeatureGate: false, + driverSupportsVolumeMountGroup: true, + expectedVolumeMountGroup: "", + stageUnstageSet: true, + createAttachment: true, + spec: volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false), }, } @@ -1214,6 +1256,8 @@ func TestAttacherMountDevice(t *testing.T) { t.Run(tc.testName, func(t *testing.T) { t.Logf("Running test case: %s", tc.testName) + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DelegateFSGroupToCSIDriver, tc.delegateFSGroupFeatureGate)() + // Setup // Create a new attacher fakeClient := fakeclient.NewSimpleClientset() @@ -1225,7 +1269,7 @@ func TestAttacherMountDevice(t *testing.T) { t.Fatalf("failed to create new attacher: %v", err0) } csiAttacher := getCsiAttacherFromVolumeAttacher(attacher, tc.watchTimeout) - csiAttacher.csiClient = setupClientWithVolumeMountGroup(t, tc.stageUnstageSet, true /* volumeMountGroupSet */) + csiAttacher.csiClient = setupClientWithVolumeMountGroup(t, tc.stageUnstageSet, tc.driverSupportsVolumeMountGroup) if tc.deviceMountPath != "" { tc.deviceMountPath = filepath.Join(tmpDir, tc.deviceMountPath) diff --git a/pkg/volume/csi/csi_client.go b/pkg/volume/csi/csi_client.go index c087a529d79..cf4ba7a0d48 100644 --- a/pkg/volume/csi/csi_client.go +++ b/pkg/volume/csi/csi_client.go @@ -69,6 +69,10 @@ type csiClient interface { volID string, targetPath string, ) error + + // The caller is responsible for checking whether the driver supports + // applying FSGroup by calling NodeSupportsVolumeMountGroup(). + // If the driver does not, fsGroup must be set to nil. NodeStageVolume(ctx context.Context, volID string, publishVolumeInfo map[string]string, @@ -431,18 +435,9 @@ func (c *csiDriverClient) NodeStageVolume(ctx context.Context, 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 */) - } + if fsGroup != nil { + mountVolume.VolumeMountGroup = strconv.FormatInt(*fsGroup, 10 /* base */) } - req.VolumeCapability.AccessType = &csipbv1.VolumeCapability_Mount{ Mount: mountVolume, } diff --git a/pkg/volume/csi/csi_client_test.go b/pkg/volume/csi/csi_client_test.go index bf23b5f08cc..0e15943a056 100644 --- a/pkg/volume/csi/csi_client_test.go +++ b/pkg/volume/csi/csi_client_test.go @@ -246,18 +246,9 @@ func (c *fakeCsiDriverClient) NodeStageVolume(ctx context.Context, 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 */) - } + if fsGroup != nil { + mountVolume.VolumeMountGroup = strconv.FormatInt(*fsGroup, 10 /* base */) } - req.VolumeCapability.AccessType = &csipbv1.VolumeCapability_Mount{ Mount: mountVolume, } @@ -596,73 +587,29 @@ 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 - fsGroup *int64 - delegateFSGroupFeatureGate bool - driverSupportsVolumeMountGroup bool - expectedFSGroupInNodeStage string - mustFail bool - err error + name string + volID string + stagingTargetPath string + fsType string + secrets map[string]string + mountOptions []string + fsGroup *int64 + expectedVolumeMountGroup 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: "", - }, + {name: "fsgroup", volID: "vol-test", stagingTargetPath: testPath, fsGroup: &testFSGroup, expectedVolumeMountGroup: "3000"}, } 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 := fake.NewNodeClientWithVolumeMountGroup(true /* stagingCapable */, true /* volumeMountGroupCapable */) nodeClient.SetNextError(tc.err) fakeCloser := fake.NewCloser(t) client := &csiDriverClient{ @@ -687,8 +634,8 @@ func TestClientNodeStageVolume(t *testing.T) { 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 volumeMountGroup != tc.expectedVolumeMountGroup { + t.Errorf("expected VolumeMountGroup parameter in NodePublishVolumeRequest to be %q, got: %q", tc.expectedVolumeMountGroup, volumeMountGroup) } if !tc.mustFail { diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index bc5376fa6da..98a60ebcb06 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -244,6 +244,7 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error } if driverSupportsCSIVolumeMountGroup { + klog.V(3).Infof("Driver %s supports applying FSGroup (has VOLUME_MOUNT_GROUP node capability). Delegating FSGroup application to the driver through NodePublishVolume.", c.driverName) nodePublishFSGroupArg = mounterArgs.FsGroup } }