From e5858c48b6a2659907f6b311316d228030da3db0 Mon Sep 17 00:00:00 2001 From: Anton Myagkov Date: Fri, 8 Nov 2024 16:11:27 +0100 Subject: [PATCH] issue-2285: skip changing group ownership for readonly volumes (#2443) --- .../tests/csi_driver/e2e_tests_part2/test.py | 27 +++++++++++++++---- .../tests/csi_driver/lib/csi_runner.py | 6 ++++- .../tools/csi_driver/client/main.go | 20 ++++++++++---- .../tools/csi_driver/internal/driver/node.go | 4 +-- 4 files changed, 44 insertions(+), 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 index 1014abfe222..b023802b7a3 100644 --- a/cloud/blockstore/tests/csi_driver/e2e_tests_part2/test.py +++ b/cloud/blockstore/tests/csi_driver/e2e_tests_part2/test.py @@ -17,11 +17,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 +35,23 @@ 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, + ) + 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 index 568b196d77b..ee10f0f3e75 100644 --- 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 index 12c347047bf..551f1134969 100644 --- 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 index 9940c4b019e..6bbbb251cf2 100644 --- a/cloud/blockstore/tools/csi_driver/internal/driver/node.go +++ b/cloud/blockstore/tools/csi_driver/internal/driver/node.go @@ -629,7 +629,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", @@ -686,7 +686,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",