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

@@ -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

pkg/volume/csi/csi_attacher_test.go

@@ -1076,19 +1076,21 @@ func TestAttacherMountDevice(t *testing.T) {
 	transientError := volumetypes.NewTransientOperationFailure("")
 
 	testCases := []struct {
-		testName                 string
+		testName                       string
-		volName                  string
+		volName                        string
-		devicePath               string
+		devicePath                     string
-		deviceMountPath          string
+		deviceMountPath                string
-		stageUnstageSet          bool
+		stageUnstageSet                bool
-		fsGroup                  *int64
+		fsGroup                        *int64
-		expectedVolumeMountGroup string
+		expectedVolumeMountGroup       string
-		shouldFail               bool
+		delegateFSGroupFeatureGate     bool
-		createAttachment         bool
+		driverSupportsVolumeMountGroup bool
-		populateDeviceMountPath  bool
+		shouldFail                     bool
-		exitError                error
+		createAttachment               bool
-		spec                     *volume.Spec
+		populateDeviceMountPath        bool
-		watchTimeout             time.Duration
+		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",
+			testName:                       "fsgroup provided, DelegateFSGroupToCSIDriver feature enabled, driver supports volume mount group; expect fsgroup to be passed to NodeStageVolume",
-			volName:                  "test-vol1",
+			volName:                        "test-vol1",
-			devicePath:               "path1",
+			devicePath:                     "path1",
-			deviceMountPath:          "path2",
+			deviceMountPath:                "path2",
-			fsGroup:                  &testFSGroup,
+			fsGroup:                        &testFSGroup,
-			expectedVolumeMountGroup: "3000",
+			delegateFSGroupFeatureGate:     true,
-			stageUnstageSet:          true,
+			driverSupportsVolumeMountGroup: true,
-			createAttachment:         true,
+			expectedVolumeMountGroup:       "3000",
-			spec:                     volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), false),
+			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)

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 fsGroup != nil {
-		if utilfeature.DefaultFeatureGate.Enabled(features.DelegateFSGroupToCSIDriver) {
+			mountVolume.VolumeMountGroup = strconv.FormatInt(*fsGroup, 10 /* base */)
-			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: mountVolume,
 		}

pkg/volume/csi/csi_client_test.go

@@ -246,18 +246,9 @@ func (c *fakeCsiDriverClient) NodeStageVolume(ctx context.Context,
 			FsType:     fsType,
 			MountFlags: mountOptions,
 		}
-
+		if fsGroup != nil {
-		if utilfeature.DefaultFeatureGate.Enabled(features.DelegateFSGroupToCSIDriver) {
+			mountVolume.VolumeMountGroup = strconv.FormatInt(*fsGroup, 10 /* base */)
-			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: mountVolume,
 		}
@@ -596,73 +587,29 @@ func TestClientNodeStageVolume(t *testing.T) {
 	testPath := filepath.Join(tmpDir, "/test/path")
 
 	testCases := []struct {
-		name                           string
+		name                     string
-		volID                          string
+		volID                    string
-		stagingTargetPath              string
+		stagingTargetPath        string
-		fsType                         string
+		fsType                   string
-		secrets                        map[string]string
+		secrets                  map[string]string
-		mountOptions                   []string
+		mountOptions             []string
-		fsGroup                        *int64
+		fsGroup                  *int64
-		delegateFSGroupFeatureGate     bool
+		expectedVolumeMountGroup string
-		driverSupportsVolumeMountGroup bool
+		mustFail                 bool
-		expectedFSGroupInNodeStage     string
+		err                      error
-		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", volID: "vol-test", stagingTargetPath: testPath, fsGroup: &testFSGroup, expectedVolumeMountGroup: "3000"},
-		{
-			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 */, true /* volumeMountGroupCapable */)
-
-		nodeClient := fake.NewNodeClientWithVolumeMountGroup(true /* stagingCapable */, tc.driverSupportsVolumeMountGroup)
 		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 {
+		if volumeMountGroup != tc.expectedVolumeMountGroup {
-			t.Errorf("expected VolumeMountGroup parameter in NodePublishVolumeRequest to be %q, got: %q", tc.expectedFSGroupInNodeStage, volumeMountGroup)
+			t.Errorf("expected VolumeMountGroup parameter in NodePublishVolumeRequest to be %q, got: %q", tc.expectedVolumeMountGroup, volumeMountGroup)
 		}
 
 		if !tc.mustFail {

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
 		}
 	}