From ea7be3439616aa1a112b7dd2ff9896e3e1afaec0 Mon Sep 17 00:00:00 2001
From: Niels de Vos <ndevos@ibm.com>
Date: Thu, 24 Apr 2025 17:08:38 +0200
Subject: [PATCH] rbd: use helper functions from csi-common for
 VolumeCapability checking

The internal/csi-common package offers helper functions like
`IsReaderOnly()` and `IsBlockMultiNode()`. These should be used instead
of checking the VolumeCapability that is passed in a request in
different places.

This also suggested that adding the "ro" mount option in
`NodeServer.mountVolumeToStagePath()` is not appropriate, as the
csi-common helper `ConstructMountOptions()` can take care of that
already too.

Signed-off-by: Niels de Vos <ndevos@ibm.com>
---
 internal/csi-common/nodeserver-default.go |  8 +++++++
 internal/rbd/nodeserver.go                | 28 +++++------------------
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/internal/csi-common/nodeserver-default.go b/internal/csi-common/nodeserver-default.go
index e8e784412..5b2552f5c 100644
--- a/internal/csi-common/nodeserver-default.go
+++ b/internal/csi-common/nodeserver-default.go
@@ -93,6 +93,14 @@ func ConstructMountOptions(mountOptions []string, volCap *csi.VolumeCapability)
 		}
 	}
 
+	// add "ro" in case the capabilities indicate READER_ONLY
+	rOnly := "ro"
+	if IsReaderOnly([]*csi.VolumeCapability{volCap}) {
+		if !MountOptionContains(mountOptions, rOnly) {
+			mountOptions = append(mountOptions, rOnly)
+		}
+	}
+
 	return mountOptions
 }
 
diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go
index 0b13f62bf..eaa75d599 100644
--- a/internal/rbd/nodeserver.go
+++ b/internal/rbd/nodeserver.go
@@ -165,10 +165,12 @@ func (ns *NodeServer) populateRbdVol(
 ) (*rbdVolume, error) {
 	var err error
 	volID := req.GetVolumeId()
-	isBlock := req.GetVolumeCapability().GetBlock() != nil
+
+	isBlock, isMultiNode := csicommon.IsBlockMultiNode([]*csi.VolumeCapability{req.GetVolumeCapability()})
 	disableInUseChecks := false
+
 	// MULTI_NODE_MULTI_WRITER is supported by default for Block access type volumes
-	if req.GetVolumeCapability().GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER {
+	if isMultiNode {
 		if !isBlock {
 			log.WarningLog(
 				ctx,
@@ -216,6 +218,7 @@ func (ns *NodeServer) populateRbdVol(
 	}
 
 	rv.DisableInUseChecks = disableInUseChecks
+	rv.readOnly = csicommon.IsReaderOnly([]*csi.VolumeCapability{req.GetVolumeCapability()})
 
 	err = rv.Connect(cr)
 	if err != nil {
@@ -404,13 +407,6 @@ func (ns *NodeServer) stageTransaction(
 
 	var err error
 
-	// Allow image to be mounted on multiple nodes if it is ROX
-	if req.GetVolumeCapability().GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY {
-		log.ExtendedLog(ctx, "setting disableInUseChecks on rbd volume to: %v", req.GetVolumeId)
-		volOptions.DisableInUseChecks = true
-		volOptions.readOnly = true
-	}
-
 	err = flattenImageBeforeMapping(ctx, volOptions)
 	if err != nil {
 		return transaction, err
@@ -760,7 +756,6 @@ func (ns *NodeServer) mountVolumeToStagePath(
 	stagingPath, devicePath string,
 	fileEncryption bool,
 ) error {
-	readOnly := false
 	fsType := req.GetVolumeCapability().GetMount().GetFsType()
 	diskMounter := &mount.SafeFormatAndMount{Interface: ns.Mounter, Exec: utilexec.New()}
 	// rbd images are thin-provisioned and return zeros for unwritten areas.  A freshly created
@@ -784,18 +779,7 @@ func (ns *NodeServer) mountVolumeToStagePath(
 	opt = append(opt, "_netdev")
 	opt = csicommon.ConstructMountOptions(opt, req.GetVolumeCapability())
 	isBlock := req.GetVolumeCapability().GetBlock() != nil
-	rOnly := "ro"
-
-	mode := req.GetVolumeCapability().GetAccessMode().GetMode()
-	if mode == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY ||
-		mode == csi.VolumeCapability_AccessMode_SINGLE_NODE_READER_ONLY {
-		if !csicommon.MountOptionContains(opt, rOnly) {
-			opt = append(opt, rOnly)
-		}
-	}
-	if csicommon.MountOptionContains(opt, rOnly) {
-		readOnly = true
-	}
+	readOnly := csicommon.IsReaderOnly([]*csi.VolumeCapability{req.GetVolumeCapability()})
 
 	if existingFormat == "" && !staticVol && !readOnly && !isBlock {
 		args := ns.getMkfsArgs(fsType)