[CSI] Change volume ownership if root directory permissions mismatch

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

cloud/blockstore/tools/csi_driver/internal/driver/node.go

@@ -11,16 +11,17 @@ import (
 	"log"
 	"math"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
 	"sync"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	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)
 	}

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,4 +198,3 @@ func walk(path string, info os.FileInfo, walkFunc filepath.WalkFunc) error {
 	}
 	return walkFunc(path, info, nil)
 }
-

cloud/blockstore/tools/csi_driver/internal/volume/ya.make

@@ -0,0 +1,7 @@
+GO_LIBRARY()
+
+SRCS(
+    volume_linux.go
+)
+
+END()

cloud/blockstore/tools/csi_driver/internal/ya.make

@@ -1,4 +1,5 @@
 RECURSE(
     driver
     mounter
+    volume
 )