issue-2285: skip changing group ownership for readonly volumes

ydb-platform · Nov 7, 2024 · bbad92b · bbad92b
1 parent af5ff7a
commit bbad92b
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 13 deletions.
diff --git a/cloud/blockstore/tests/csi_driver/e2e_tests_part2/test.py b/cloud/blockstore/tests/csi_driver/e2e_tests_part2/test.py
@@ -1,3 +1,4 @@
+import logging
 import pytest
 import subprocess
 
@@ -17,11 +18,11 @@ def get_access_mode(mount_point: str) -> str:
     return ""
 
 
-@pytest.mark.parametrize('mount_path,access_type,vm_mode',
-                         [("/var/lib/kubelet/pods/123/volumes/kubernetes.io~csi/example-disk/mount", "mount", False),
-                          ("/var/lib/kubelet/plugins/kubernetes.io/csi/volumeDevices/publish/123/example-disk", "block", False),
-                          ("/var/lib/kubelet/pods/123/volumes/kubernetes.io~csi/example-disk/mount", "mount", True)])
-def test_readonly_volume(mount_path, access_type, vm_mode):
+@pytest.mark.parametrize('mount_path,access_type,vm_mode,gid',
+                         [("/var/lib/kubelet/pods/123/volumes/kubernetes.io~csi/example-disk/mount", "mount", False, "1010"),
+                          ("/var/lib/kubelet/plugins/kubernetes.io/csi/volumeDevices/publish/123/example-disk", "block", False, "1011"),
+                          ("/var/lib/kubelet/pods/123/volumes/kubernetes.io~csi/example-disk/mount", "mount", True, "1012")])
+def test_readonly_volume(mount_path, access_type, vm_mode, gid):
     env, run = csi.init(vm_mode)
     try:
         volume_name = "example-disk"
@@ -35,6 +36,26 @@ def test_readonly_volume(mount_path, access_type, vm_mode):
         env.csi.publish_volume(pod_id, volume_name, pod_name, access_type, readonly=True)
         assert "ro" == get_access_mode(mount_path)
 
+        env.csi.unpublish_volume(pod_id, volume_name, access_type)
+        result = subprocess.run(
+            ["groupadd", "-g", gid, "test_group_" + gid],
+            capture_output=True,
+        )
+
+        logging.info("Stdout: %s", result.stdout)
+        logging.info("Stderr: %s", result.stderr)
+        assert result.returncode == 0
+
+        env.csi.publish_volume(
+            pod_id,
+            volume_name,
+            pod_name,
+            access_type,
+            readonly=True,
+            volume_mount_group=gid
+        )
+        assert "ro" == get_access_mode(mount_path)
+
     except subprocess.CalledProcessError as e:
         csi.log_called_process_error(e)
         raise

diff --git a/cloud/blockstore/tests/csi_driver/lib/csi_runner.py b/cloud/blockstore/tests/csi_driver/lib/csi_runner.py
@@ -152,7 +152,8 @@ def publish_volume(
             pod_name: str,
             access_type: str,
             fs_type: str = "",
-            readonly: bool = False):
+            readonly: bool = False,
+            volume_mount_group: str = ""):
         args = [
             "publishvolume",
             "--pod-id",
@@ -168,6 +169,9 @@ def publish_volume(
         ]
         if readonly:
             args += ["--readonly"]
+
+        if len(volume_mount_group) != 0:
+            args += ["--volume-mount-group", volume_mount_group]
         return self._node_run(*args)
 
     def unpublish_volume(self, pod_id: str, volume_id: str, access_type: str):

diff --git a/cloud/blockstore/tools/csi_driver/client/main.go b/cloud/blockstore/tools/csi_driver/client/main.go
@@ -191,11 +191,14 @@ func newDeleteVolumeCommand(endpoint *string) *cobra.Command {
 
 func createVolumeCapability(
 	accessMode csi.VolumeCapability_AccessMode_Mode,
-	accessType string) *csi.VolumeCapability {
+	accessType string,
+	volumeMountGroup string) *csi.VolumeCapability {
 	volumeCapability := &csi.VolumeCapability{
 		AccessMode: &csi.VolumeCapability_AccessMode{Mode: accessMode},
 		AccessType: &csi.VolumeCapability_Mount{
-			Mount: &csi.VolumeCapability_MountVolume{},
+			Mount: &csi.VolumeCapability_MountVolume{
+				VolumeMountGroup: volumeMountGroup,
+			},
 		},
 	}
 	if accessType == "block" {
@@ -245,7 +248,7 @@ func newNodeStageVolumeCommand(endpoint *string) *cobra.Command {
 				&csi.NodeStageVolumeRequest{
 					VolumeId:          volumeId,
 					StagingTargetPath: stagingTargetPath,
-					VolumeCapability:  createVolumeCapability(accessMode, accessType),
+					VolumeCapability:  createVolumeCapability(accessMode, accessType, ""),
 					VolumeContext:     volumeContext,
 				},
 			)
@@ -286,6 +289,7 @@ func newPublishVolumeCommand(endpoint *string) *cobra.Command {
 	var volumeId, podId, stagingTargetPath, podName, fsType string
 	var accessType string
 	var readOnly bool
+	var volumeMountGroup string
 	cmd := cobra.Command{
 		Use:   "publishvolume",
 		Short: "Send publish volume request to the CSI node",
@@ -319,7 +323,7 @@ func newPublishVolumeCommand(endpoint *string) *cobra.Command {
 					PublishContext:    nil,
 					StagingTargetPath: stagingTargetPath,
 					TargetPath:        getTargetPath(podId, volumeId, accessType),
-					VolumeCapability:  createVolumeCapability(accessMode, accessType),
+					VolumeCapability:  createVolumeCapability(accessMode, accessType, volumeMountGroup),
 					Readonly:          readOnly,
 					Secrets:           nil,
 					VolumeContext:     volumeContext,
@@ -370,6 +374,12 @@ func newPublishVolumeCommand(endpoint *string) *cobra.Command {
 		"mount",
 		"mount or block access type",
 	)
+	cmd.Flags().StringVar(
+		&volumeMountGroup,
+		"volume-mount-group",
+		"",
+		"fs group id",
+	)
 
 	err := cmd.MarkFlagRequired("volume-id")
 	if err != nil {
@@ -565,7 +575,7 @@ func newNodeExpandVolumeCommand(endpoint *string) *cobra.Command {
 					CapacityRange: &csi.CapacityRange{
 						RequiredBytes: size,
 					},
-					VolumeCapability: createVolumeCapability(accessMode, accessType),
+					VolumeCapability: createVolumeCapability(accessMode, accessType, ""),
 				},
 			)
 			if err != nil {

diff --git a/cloud/blockstore/tools/csi_driver/internal/driver/node.go b/cloud/blockstore/tools/csi_driver/internal/driver/node.go
@@ -625,7 +625,7 @@ func (s *nodeService) nodePublishDiskAsFilesystemDeprecated(
 		return err
 	}
 
-	if mnt != nil && mnt.VolumeMountGroup != "" {
+	if mnt != nil && mnt.VolumeMountGroup != "" && !req.Readonly {
 		cmd := exec.Command("chown", "-R", ":"+mnt.VolumeMountGroup, req.TargetPath)
 		if out, err := cmd.CombinedOutput(); err != nil {
 			return fmt.Errorf("failed to chown %s to %q: %w, output %q",
@@ -682,7 +682,7 @@ func (s *nodeService) nodePublishDiskAsFilesystem(
 		return err
 	}
 
-	if mnt != nil && mnt.VolumeMountGroup != "" {
+	if mnt != nil && mnt.VolumeMountGroup != "" && !req.Readonly {
 		cmd := exec.Command("chown", "-R", ":"+mnt.VolumeMountGroup, req.TargetPath)
 		if out, err := cmd.CombinedOutput(); err != nil {
 			return fmt.Errorf("failed to chown %s to %q: %w, output %q",