util: improve static PV handling

- Clarify static PV must not specify controllerPublishSecretRef.
- Improve handling volumeHandle validation error. This error
  is harmless for static volume and is a real error
  for other volumes.

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

@@ -1159,11 +1159,14 @@ func (cs *ControllerServer) getServiceAccountRestriction(
 
 	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 util.IsOlderOrStaticPV(err) {
+			log.WarningLog(ctx, "possibly older or static PV: %v", err)
 
 			return "", nil
 		}
+		if err != nil {
+			return "", status.Errorf(codes.Internal, "failed to get controller publish secret ref: %v", err)
+		}
 
 		secrets, err = k8s.GetSecret(secretName, secretNamespace)
 		if err != nil {
@@ -1231,13 +1234,14 @@ func (cs *ControllerServer) ControllerUnpublishVolume(
 	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 util.IsOlderOrStaticPV(err) {
+			log.WarningLog(ctx, "possibly older or static PV: %v", err)
 
-			// If the secret is not found, return success to not break for older PVs
-			// without controller-publish secrets.
 			return &csi.ControllerUnpublishVolumeResponse{}, nil
 		}
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "failed to get controller publish secret ref: %v", err)
+		}
 
 		secrets, err = k8s.GetSecret(secretName, secretNamespace)
 		if err != nil {

internal/nvmeof/controller/controllerserver.go

@@ -295,11 +295,14 @@ func (cs *Server) ControllerUnpublishVolume(
 	secrets := req.GetSecrets()
 	if secrets == nil {
 		secretName, secretNamespace, err := util.GetControllerPublishSecretRef(req.GetVolumeId(), util.RBDType)
-		if err != nil {
-			log.WarningLog(ctx, "controller publish secret not found: %v", err)
+		if util.IsOlderOrStaticPV(err) {
+			log.WarningLog(ctx, "possibly older or static PV: %v", err)
 
 			return &csi.ControllerUnpublishVolumeResponse{}, nil
 		}
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "failed to get controller publish secret ref: %v", err)
+		}
 
 		secrets, err = k8s.GetSecret(secretName, secretNamespace)
 		if err != nil {

internal/rbd/controllerserver.go

@@ -1719,11 +1719,14 @@ func (cs *ControllerServer) getServiceAccountRestriction(
 
 	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 util.IsOlderOrStaticPV(err) {
+			log.WarningLog(ctx, "possibly older or static PV: %v", err)
 
 			return "", nil
 		}
+		if err != nil {
+			return "", status.Errorf(codes.Internal, "failed to get controller publish secret ref: %v", err)
+		}
 
 		secrets, err = k8s.GetSecret(secretName, secretNamespace)
 		if err != nil {
@@ -1790,13 +1793,14 @@ func (cs *ControllerServer) ControllerUnpublishVolume(
 	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 util.IsOlderOrStaticPV(err) {
+			log.WarningLog(ctx, "possibly older or static PV: %v", err)
 
-			// If the secret is not found, return success to not break for older PVs
-			// without controller-publish secrets.
 			return &csi.ControllerUnpublishVolumeResponse{}, nil
 		}
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "failed to get controller publish secret ref: %v", err)
+		}
 
 		secrets, err = k8s.GetSecret(secretName, secretNamespace)
 		if err != nil {

internal/rbd/nodeserver.go

@@ -1572,7 +1572,7 @@ func (ns *NodeServer) blockNodeGetVolumeStats(
 	}
 
 	secretName, secretNamespace, err := util.GetControllerPublishSecretRef(volumeId, util.RBDType)
-	if errors.Is(err, util.ErrConfigNotFound) {
+	if util.IsOlderOrStaticPV(err) {
 		// diff iterate to measure usage is not possible without secrets.
 		isDiffPossible = false
 	} else if err != nil {

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

@@ -349,7 +349,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 +396,13 @@ func GetControllerPublishSecretRef(volumeId, driverType string) (string, string,
 	return secretName, secretNamespace, nil
 }
 
+// IsOlderOrStaticPV returns true when the error from GetControllerPublishSecretRef indicates
+// that the volume is either a static PV (non-CSI-formatted volume handle) or an older PV
+// provisioned without a controller-publish secret configured.
+func IsOlderOrStaticPV(err error) bool {
+	return errors.Is(err, ErrInvalidVolID) || errors.Is(err, ErrConfigNotFound)
+}
+
 func getControllerPublishSecretRef(clusterId, driverType string) (string, string, error) {
 	var (
 		err              error