util: improve static PV handling

- Clarify static PV must not specify controllerPublishSecretRef.
- Improve handling volumeHandle validation error.

The related discussion:
#6290

Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>

Author: satoru-takeuchi

docs/static-pvc.md

@@ -83,6 +83,9 @@ spec:
   volumeMode: Filesystem
 ```
 
+> [!note]
+> `controllerPublishSecretRef` must not be specified for static RBD PVs.
+
 ### RBD Volume Attributes in PV
 
 Below table explains the list of volume attributes can be set when creating a
@@ -278,6 +281,9 @@ spec:
   volumeMode: Filesystem
 ```
 
+> [!note]
+> `controllerPublishSecretRef` must not be specified for static CephFS PVs.
+
 ### Node stage secret ref in CephFS PV
 
 For static CephFS PV to work, userID and userKey needs to be specified in the

internal/cephfs/controllerserver.go

@@ -1155,21 +1155,12 @@ func (cs *ControllerServer) getServiceAccountRestriction(
 	req *csi.ControllerPublishVolumeRequest,
 ) (string, error) {
 	volumeID := req.GetVolumeId()
-	secrets := req.GetSecrets()
-
-	if secrets == nil {
-		secretName, secretNamespace, err := util.GetControllerPublishSecretRef(volumeID, util.CephFsType)
-		if err != nil {
-			log.WarningLog(ctx, "controller publish secret not found: %v", err)
-
-			return "", nil
-		}
-
-		secrets, err = k8s.GetSecret(secretName, secretNamespace)
-		if err != nil {
-			return "", status.Errorf(codes.Internal,
-				"failed to get controller publish secret from k8s: %v", err)
-		}
+	secrets, skip, err := util.GetControllerPublishSecrets(ctx, req.GetSecrets(), volumeID, util.CephFsType)
+	if skip {
+		return "", nil
+	}
+	if err != nil {
+		return "", status.Errorf(codes.Internal, "%v", err)
 	}
 
 	volOptions, _, err := store.NewVolumeOptionsFromVolID(ctx, volumeID, nil, secrets, cs.ClusterName)
@@ -1228,21 +1219,12 @@ func (cs *ControllerServer) ControllerUnpublishVolume(
 	}
 	defer cs.VolumeLocks.Release(volumeId)
 
-	secrets := req.GetSecrets()
-	if secrets == nil {
-		secretName, secretNamespace, err := util.GetControllerPublishSecretRef(volumeId, util.CephFsType)
-		if err != nil {
-			log.WarningLog(ctx, "controller publish secret not found: %v", err)
-
-			// If the secret is not found, return success to not break for older PVs
-			// without controller-publish secrets.
-			return &csi.ControllerUnpublishVolumeResponse{}, nil
-		}
-
-		secrets, err = k8s.GetSecret(secretName, secretNamespace)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get controller publish secret from k8s: %w", err)
-		}
+	secrets, skip, err := util.GetControllerPublishSecrets(ctx, req.GetSecrets(), volumeId, util.CephFsType)
+	if skip {
+		return &csi.ControllerUnpublishVolumeResponse{}, nil
+	}
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "%v", err)
 	}
 
 	volOptions, _, err := store.NewVolumeOptionsFromVolID(ctx, volumeId, nil, secrets, cs.ClusterName)

internal/rbd/controllerserver.go

@@ -1755,21 +1755,12 @@ func (cs *ControllerServer) getServiceAccountRestriction(
 	req *csi.ControllerPublishVolumeRequest,
 ) (string, error) {
 	volumeID := req.GetVolumeId()
-	secrets := req.GetSecrets()
-
-	if secrets == nil {
-		secretName, secretNamespace, err := util.GetControllerPublishSecretRef(volumeID, util.RBDType)
-		if err != nil {
-			log.WarningLog(ctx, "controller publish secret not found: %v", err)
-
-			return "", nil
-		}
-
-		secrets, err = k8s.GetSecret(secretName, secretNamespace)
-		if err != nil {
-			return "", status.Errorf(codes.Internal,
-				"failed to get controller publish secret from k8s: %v", err)
-		}
+	secrets, skip, err := util.GetControllerPublishSecrets(ctx, req.GetSecrets(), volumeID, util.RBDType)
+	if skip {
+		return "", nil
+	}
+	if err != nil {
+		return "", status.Errorf(codes.Internal, "%v", err)
 	}
 
 	cr, err := util.NewUserCredentials(secrets)
@@ -1827,21 +1818,12 @@ func (cs *ControllerServer) ControllerUnpublishVolume(
 	}
 	defer cs.VolumeLocks.Release(volumeId)
 
-	secrets := req.GetSecrets()
-	if secrets == nil {
-		secretName, secretNamespace, err := util.GetControllerPublishSecretRef(volumeId, util.RBDType)
-		if err != nil {
-			log.WarningLog(ctx, "controller publish secret not found: %v", err)
-
-			// If the secret is not found, return success to not break for older PVs
-			// without controller-publish secrets.
-			return &csi.ControllerUnpublishVolumeResponse{}, nil
-		}
-
-		secrets, err = k8s.GetSecret(secretName, secretNamespace)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get controller publish secret from k8s: %w", err)
-		}
+	secrets, skip, err := util.GetControllerPublishSecrets(ctx, req.GetSecrets(), volumeId, util.RBDType)
+	if skip {
+		return &csi.ControllerUnpublishVolumeResponse{}, nil
+	}
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "%v", err)
 	}
 
 	credentials, err := util.NewAdminCredentials(secrets)

internal/util/errors.go

@@ -36,4 +36,6 @@ var (
 	ErrMissingConfigForMonitor = errors.New("missing configuration of cluster ID for monitor")
 	// ErrConfigNotFound is returned when no configuration is found for a cluster ID.
 	ErrConfigNotFound = errors.New("missing configuration for cluster ID")
+	// ErrInvalidVolID is returned when the volume ID cannot be decomposed into a CSI identifier.
+	ErrInvalidVolID = errors.New("invalid volume ID")
 )

internal/util/util.go

@@ -34,6 +34,7 @@ import (
 	mount "k8s.io/mount-utils"
 
 	"github.com/ceph/ceph-csi/internal/util/k8s"
+	"github.com/ceph/ceph-csi/internal/util/log"
 )
 
 // Driver types to identify type of driver running.
@@ -349,7 +350,8 @@ func GetControllerPublishSecretRef(volumeId, driverType string) (string, string,
 	)
 	err := vi.DecomposeCSIID(volumeId)
 	if err != nil {
-		return secretName, secretNamespace, fmt.Errorf("failed to decode volume ID (%s): %w", volumeId, err)
+		return secretName, secretNamespace, fmt.Errorf("failed to decode volume ID (%s): %w",
+			volumeId, errors.Join(ErrInvalidVolID, err))
 	}
 
 	secretName, secretNamespace, err = getControllerPublishSecretRef(vi.ClusterID, driverType)
@@ -395,6 +397,35 @@ func GetControllerPublishSecretRef(volumeId, driverType string) (string, string,
 	return secretName, secretNamespace, nil
 }
 
+// GetControllerPublishSecrets resolves secrets for a controller publish/unpublish operation.
+// If reqSecrets is non-nil, return it. Otherwise secrets are fetched from the CSI config.
+// When the second return value is true the caller should return early with no error.
+func GetControllerPublishSecrets(
+	ctx context.Context,
+	reqSecrets map[string]string,
+	volumeID, driverType string,
+) (map[string]string, bool, error) {
+	if reqSecrets != nil {
+		return reqSecrets, false, nil
+	}
+	secretName, secretNamespace, err := GetControllerPublishSecretRef(volumeID, driverType)
+	if errors.Is(err, ErrInvalidVolID) || errors.Is(err, ErrConfigNotFound) {
+		// Possibly the volume is a static/older volume. In this case, we have nothing to do.
+		// Even if it's not a static/older volume, we should skip handling it because
+		// we have no way to process handling anyway.
+		log.WarningLog(ctx, "should skip handling this volume: %v", err)
+		return nil, true, nil
+	}
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to get controller publish secret ref: %w", err)
+	}
+	secrets, err := k8s.GetSecret(secretName, secretNamespace)
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to get controller publish secret from k8s: %w", err)
+	}
+	return secrets, false, nil
+}
+
 func getControllerPublishSecretRef(clusterId, driverType string) (string, string, error) {
 	var (
 		err              error