From 7e01fec249c3aaf045434b8074c4bc703a557872 Mon Sep 17 00:00:00 2001 From: Anton Myagkov Date: Fri, 17 Jan 2025 16:34:37 +0000 Subject: [PATCH 1/3] [CSI] Change volume ownership if root directory permissions mismatch --- .../tests/csi_driver/e2e_tests_part2/test.py | 51 +++++++++++++------ .../tools/csi_driver/internal/driver/node.go | 25 ++++----- .../internal/volume/volume_linux.go | 1 - .../tools/csi_driver/internal/volume/ya.make | 7 +++ .../tools/csi_driver/internal/ya.make | 1 + 5 files changed, 55 insertions(+), 30 deletions(-) create mode 100644 cloud/blockstore/tools/csi_driver/internal/volume/ya.make 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 6e67be5d57b..2bd2d484673 100644 --- a/cloud/blockstore/tests/csi_driver/e2e_tests_part2/test.py +++ b/cloud/blockstore/tests/csi_driver/e2e_tests_part2/test.py @@ -1,5 +1,6 @@ import pytest import subprocess +import os from pathlib import Path @@ -63,14 +64,21 @@ def test_readonly_volume(mount_path, access_type, vm_mode, gid): def test_mount_volume_group(): # Scenario - # 1. create volume and publish volume without mount volume group - # 2. create directory and file - # 3. unpublish volume + # 1. create volume and stage it + # 2. create directory and file in the staging directory # 4. create new group with specified GID # 5. publish volume with mount volume group GID # 6. check that mounted dir and existing files have specified GID # 7. create new directory and file # 8. check that new directory and file have specified GID + # 9. unpublish volume + # 10. create new file in staging directory and change ownership + # 11. publish volume with mount volume group GID + # 12. Verify that the new file doesn't have the specified GID. + # The change won't take effect because the GID of the mount directory + # matches the GID of the volume group. + + stage_path = Path("/var/lib/kubelet/plugins/kubernetes.io/csi/nbs.csi.nebius.ai/a/globalmount") env, run = csi.init() try: volume_name = "example-disk" @@ -81,6 +89,11 @@ def test_mount_volume_group(): env.csi.create_volume(name=volume_name, size=volume_size) env.csi.stage_volume(volume_name, access_type) + stage_test_dir1 = stage_path / "testdir1" + stage_test_dir1.mkdir() + stage_test_file1 = stage_test_dir1 / "testfile1" + stage_test_file1.touch() + gid = 1013 result = subprocess.run( ["groupadd", "-g", str(gid), "test_group_" + str(gid)], @@ -92,23 +105,13 @@ def test_mount_volume_group(): pod_id, volume_name, pod_name, - access_type + access_type, + volume_mount_group=str(gid) ) mount_path = Path("/var/lib/kubelet/pods") / pod_id / "volumes/kubernetes.io~csi" / volume_name / "mount" test_dir1 = mount_path / "testdir1" - test_dir1.mkdir() test_file1 = test_dir1 / "testfile1" - test_file1.touch() - - env.csi.unpublish_volume(pod_id, volume_name, access_type) - env.csi.publish_volume( - pod_id, - volume_name, - pod_name, - access_type, - volume_mount_group=str(gid) - ) assert gid == mount_path.stat().st_gid assert gid == test_dir1.stat().st_gid @@ -122,6 +125,24 @@ def test_mount_volume_group(): test_dir2.mkdir() assert gid == test_dir2.stat().st_gid + env.csi.unpublish_volume(pod_id, volume_name, access_type) + + stage_test_file3 = stage_test_dir1 / "testfile3" + stage_test_file3.touch() + os.chown(stage_test_file3, os.getuid(), os.getgid()) + assert gid != stage_test_file3.stat().st_gid + + env.csi.publish_volume( + pod_id, + volume_name, + pod_name, + access_type, + volume_mount_group=str(gid) + ) + + test_file3 = test_dir1 / "testfile3" + assert gid != test_file3.stat().st_gid + except subprocess.CalledProcessError as e: csi.log_called_process_error(e) raise diff --git a/cloud/blockstore/tools/csi_driver/internal/driver/node.go b/cloud/blockstore/tools/csi_driver/internal/driver/node.go index 44c0572104b..8a72402a645 100644 --- a/cloud/blockstore/tools/csi_driver/internal/driver/node.go +++ b/cloud/blockstore/tools/csi_driver/internal/driver/node.go @@ -11,9 +11,9 @@ import ( "log" "math" "os" - "os/exec" "path/filepath" "regexp" + "strconv" "strings" "sync" @@ -21,6 +21,7 @@ import ( nbsapi "github.com/ydb-platform/nbs/cloud/blockstore/public/api/protos" nbsclient "github.com/ydb-platform/nbs/cloud/blockstore/public/sdk/go/client" "github.com/ydb-platform/nbs/cloud/blockstore/tools/csi_driver/internal/mounter" + "github.com/ydb-platform/nbs/cloud/blockstore/tools/csi_driver/internal/volume" nfsapi "github.com/ydb-platform/nbs/cloud/filestore/public/api/protos" nfsclient "github.com/ydb-platform/nbs/cloud/filestore/public/sdk/go/client" "golang.org/x/sys/unix" @@ -658,11 +659,15 @@ func (s *nodeService) nodePublishDiskAsFilesystem( return err } - 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", - mnt.VolumeMountGroup, req.TargetPath, err, out) + if mnt != nil && mnt.VolumeMountGroup != "" { + fsGroup, err := strconv.ParseInt(mnt.VolumeMountGroup, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse volume mount group: %w", err) + } + + err = volume.SetVolumeOwnership(req.TargetPath, &fsGroup, req.Readonly) + if err != nil { + return fmt.Errorf("failed to set volume ownership: %w", err) } } @@ -741,14 +746,6 @@ func (s *nodeService) nodeStageDiskAsFilesystem( return fmt.Errorf("failed to format or mount filesystem: %w", err) } - if mnt != nil && mnt.VolumeMountGroup != "" { - cmd := exec.Command("chown", "-R", ":"+mnt.VolumeMountGroup, req.StagingTargetPath) - if out, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("failed to chown %s to %q: %w, output %q", - mnt.VolumeMountGroup, req.StagingTargetPath, err, out) - } - } - if err := os.Chmod(req.StagingTargetPath, targetPerm); err != nil { return fmt.Errorf("failed to chmod target path: %w", err) } diff --git a/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go b/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go index 98a7c0ba09c..ec7f6da4bfe 100644 --- a/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go +++ b/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go @@ -208,4 +208,3 @@ func walk(path string, info os.FileInfo, walkFunc filepath.WalkFunc) error { } return walkFunc(path, info, nil) } - diff --git a/cloud/blockstore/tools/csi_driver/internal/volume/ya.make b/cloud/blockstore/tools/csi_driver/internal/volume/ya.make new file mode 100644 index 00000000000..f2df5539b7e --- /dev/null +++ b/cloud/blockstore/tools/csi_driver/internal/volume/ya.make @@ -0,0 +1,7 @@ +GO_LIBRARY() + +SRCS( + volume_linux.go +) + +END() diff --git a/cloud/blockstore/tools/csi_driver/internal/ya.make b/cloud/blockstore/tools/csi_driver/internal/ya.make index 5c9c1c7aaf1..6ba42a1a0f9 100644 --- a/cloud/blockstore/tools/csi_driver/internal/ya.make +++ b/cloud/blockstore/tools/csi_driver/internal/ya.make @@ -1,4 +1,5 @@ RECURSE( driver mounter + volume ) From c159d99916e2d92ece6e8249ce1fda4b2f5048c9 Mon Sep 17 00:00:00 2001 From: Anton Myagkov Date: Tue, 28 Jan 2025 11:00:19 +0100 Subject: [PATCH 2/3] fix build with SetVolumeOwnership --- .../internal/volume/volume_linux.go | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go b/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go index ec7f6da4bfe..1f1c18e7151 100644 --- a/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go +++ b/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go @@ -17,6 +17,10 @@ See the License for the specific language governing permissions and limitations under the License. */ +/* This file was edited for NBS. +* Original is from kubernetes. +*/ + package volume import ( @@ -26,9 +30,7 @@ import ( "os" "time" - v1 "k8s.io/api/core/v1" "k8s.io/klog/v2" - "k8s.io/kubernetes/pkg/volume/util/types" ) const ( @@ -40,7 +42,7 @@ const ( // SetVolumeOwnership modifies the given volume to be owned by // fsGroup, and sets SetGid so that newly created files are owned by // fsGroup. If fsGroup is nil nothing is done. -func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { +func SetVolumeOwnership(dir string, fsGroup *int64, readonly bool) error { if fsGroup == nil { return nil } @@ -50,7 +52,7 @@ func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChan }) defer timer.Stop() - if skipPermissionChange(mounter, dir, fsGroup, fsGroupChangePolicy) { + if !requiresPermissionChange(dir, fsGroup, readonly) { klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", dir) return nil } @@ -59,13 +61,9 @@ func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChan if err != nil { return err } - return changeFilePermission(path, fsGroup, mounter.GetAttributes().ReadOnly, info) + return changeFilePermission(path, fsGroup, readonly, info) }) - if completeFunc != nil { - completeFunc(types.CompleteFuncParam{ - Err: &err, - }) - } + return err } @@ -104,14 +102,6 @@ func changeFilePermission(filename string, fsGroup *int64, readonly bool, info o return nil } -func skipPermissionChange(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) bool { - if fsGroupChangePolicy == nil || *fsGroupChangePolicy != v1.FSGroupChangeOnRootMismatch { - klog.V(4).InfoS("Perform recursive ownership change for directory", "path", dir) - return false - } - return !requiresPermissionChange(dir, fsGroup, mounter.GetAttributes().ReadOnly) -} - func requiresPermissionChange(rootDir string, fsGroup *int64, readonly bool) bool { fsInfo, err := os.Stat(rootDir) if err != nil { @@ -208,3 +198,4 @@ func walk(path string, info os.FileInfo, walkFunc filepath.WalkFunc) error { } return walkFunc(path, info, nil) } + From 154332ea91463c993978e502a12fa0cef1f45003 Mon Sep 17 00:00:00 2001 From: Anton Myagkov Date: Tue, 28 Jan 2025 14:52:04 +0100 Subject: [PATCH 3/3] fix format --- .../tools/csi_driver/internal/volume/volume_linux.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go b/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go index 1f1c18e7151..f8d17f2ba27 100644 --- a/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go +++ b/cloud/blockstore/tools/csi_driver/internal/volume/volume_linux.go @@ -19,7 +19,7 @@ limitations under the License. /* This file was edited for NBS. * Original is from kubernetes. -*/ + */ package volume @@ -198,4 +198,3 @@ func walk(path string, info os.FileInfo, walkFunc filepath.WalkFunc) error { } return walkFunc(path, info, nil) } -