rbd: add migration secret support to controllerserver functions

This commit adds the migration secret request validation to expand,
create controller functions.

Ref # https://github.com/ceph/ceph-csi/issues/2509

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

internal/rbd/controllerserver.go

@@ -94,6 +94,8 @@ func (cs *ControllerServer) validateVolumeReq(ctx context.Context, req *csi.Crea
 	return nil
 }
 
+// parseVolCreateRequest take create volume `request` argument and make use of the
+// request arguments for subsequent calls.
 func (cs *ControllerServer) parseVolCreateRequest(
 	ctx context.Context,
 	req *csi.CreateVolumeRequest) (*rbdVolume, error) {
@@ -262,18 +264,19 @@ func checkValidCreateVolumeRequest(rbdVol, parentVol *rbdVolume, rbdSnap *rbdSna
 func (cs *ControllerServer) CreateVolume(
 	ctx context.Context,
 	req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
-	if err := cs.validateVolumeReq(ctx, req); err != nil {
+	err := cs.validateVolumeReq(ctx, req)
+	if err != nil {
 		return nil, err
 	}
 
 	// TODO: create/get a connection from the the ConnPool, and do not pass
 	// the credentials to any of the utility functions.
-	cr, err := util.NewUserCredentials(req.GetSecrets())
+
+	cr, err := util.NewUserCredentialsWithMigration(req.GetSecrets())
 	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 	defer cr.DeleteCredentials()
-
 	rbdVol, err := cs.parseVolCreateRequest(ctx, req)
 	if err != nil {
 		return nil, err
@@ -382,7 +385,6 @@ func (cs *ControllerServer) repairExistingVolume(ctx context.Context, req *csi.C
 				return nil, status.Error(codes.Internal, err.Error())
 			}
 		}
-
 	// rbdVol is a restore from snapshot, rbdSnap is passed
 	case vcs.GetSnapshot() != nil:
 		// When restoring of a thick-provisioned volume was happening,
@@ -813,17 +815,9 @@ func (cs *ControllerServer) DeleteVolume(
 		return nil, status.Error(codes.InvalidArgument, "empty volume ID in request")
 	}
 
-	secrets := req.GetSecrets()
-	if util.IsMigrationSecret(secrets) {
-		secrets, err = util.ParseAndSetSecretMapFromMigSecret(secrets)
-		if err != nil {
-			return nil, status.Error(codes.InvalidArgument, err.Error())
-		}
-	}
-
-	cr, err := util.NewUserCredentials(secrets)
+	cr, err := util.NewUserCredentialsWithMigration(req.GetSecrets())
 	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 	defer cr.DeleteCredentials()
 
@@ -856,7 +850,7 @@ func (cs *ControllerServer) DeleteVolume(
 		return &csi.DeleteVolumeResponse{}, nil
 	}
 
-	rbdVol, err := GenVolFromVolID(ctx, volumeID, cr, secrets)
+	rbdVol, err := GenVolFromVolID(ctx, volumeID, cr, req.GetSecrets())
 	defer rbdVol.Destroy()
 	if err != nil {
 		return cs.checkErrAndUndoReserve(ctx, err, volumeID, rbdVol, cr)
@@ -1423,7 +1417,8 @@ func (cs *ControllerServer) DeleteSnapshot(
 func (cs *ControllerServer) ControllerExpandVolume(
 	ctx context.Context,
 	req *csi.ControllerExpandVolumeRequest) (*csi.ControllerExpandVolumeResponse, error) {
-	if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_EXPAND_VOLUME); err != nil {
+	err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_EXPAND_VOLUME)
+	if err != nil {
 		log.ErrorLog(ctx, "invalid expand volume req: %v", protosanitizer.StripSecrets(req))
 
 		return nil, err
@@ -1447,9 +1442,9 @@ func (cs *ControllerServer) ControllerExpandVolume(
 	}
 	defer cs.VolumeLocks.Release(volID)
 
-	cr, err := util.NewUserCredentials(req.GetSecrets())
+	cr, err := util.NewUserCredentialsWithMigration(req.GetSecrets())
 	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 	defer cr.DeleteCredentials()
 

internal/rbd/nodeserver.go

@@ -270,19 +270,11 @@ func (ns *NodeServer) NodeStageVolume(
 	}
 
 	volID := req.GetVolumeId()
-	secrets := req.GetSecrets()
-	if util.IsMigrationSecret(secrets) {
-		secrets, err = util.ParseAndSetSecretMapFromMigSecret(secrets)
-		if err != nil {
-			return nil, status.Error(codes.InvalidArgument, err.Error())
-		}
-	}
-	cr, err := util.NewUserCredentials(secrets)
+	cr, err := util.NewUserCredentialsWithMigration(req.GetSecrets())
 	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 	defer cr.DeleteCredentials()
-
 	if acquired := ns.VolumeLocks.TryAcquire(volID); !acquired {
 		log.ErrorLog(ctx, util.VolumeOperationAlreadyExistsFmt, volID)
 
@@ -315,7 +307,7 @@ func (ns *NodeServer) NodeStageVolume(
 		return nil, status.Error(codes.InvalidArgument, "missing required parameter imageFeatures")
 	}
 
-	rv, err := populateRbdVol(ctx, req, cr, secrets)
+	rv, err := populateRbdVol(ctx, req, cr, req.GetSecrets())
 	if err != nil {
 		return nil, err
 	}

internal/util/credentials.go

@@ -128,7 +128,7 @@ func GetMonValFromSecret(secrets map[string]string) (string, error) {
 func ParseAndSetSecretMapFromMigSecret(secretmap map[string]string) (map[string]string, error) {
 	newSecretMap := make(map[string]string)
 	// parse and set userKey
-	if !IsMigrationSecret(secretmap) {
+	if !isMigrationSecret(secretmap) {
 		return nil, errors.New("passed secret map does not contain user key or it is nil")
 	}
 	newSecretMap[credUserKey] = secretmap[migUserKey]
@@ -141,15 +141,35 @@ func ParseAndSetSecretMapFromMigSecret(secretmap map[string]string) (map[string]
 	return newSecretMap, nil
 }
 
-// IsMigrationSecret validates if the passed in secretmap is a secret
+// isMigrationSecret validates if the passed in secretmap is a secret
 // of a migration volume request. The migration secret carry a field
 // called `key` which is the equivalent of `userKey` which is what we
 // check here for identifying the secret.
-func IsMigrationSecret(passedSecretMap map[string]string) bool {
+func isMigrationSecret(secrets map[string]string) bool {
 	// the below 'nil' check is an extra measure as the request validators like
 	// ValidateNodeStageVolumeRequest() already does the nil check, however considering
 	// this function can be called independently with a map of secret values
 	// it is good to have this check in place, also it gives clear error about this
 	// was hit on migration request compared to general one.
-	return len(passedSecretMap) != 0 && passedSecretMap[migUserKey] != ""
+	return len(secrets) != 0 && secrets[migUserKey] != ""
+}
+
+// NewUserCredentialsWithMigration takes secret map from the request and validate it is
+// a migration secret, if yes, it continues to create CR from it after parsing the migration
+// secret. If it is not a migration it will continue the attempt to create credentials from it
+// without parsing the secret. This function returns credentials and error.
+func NewUserCredentialsWithMigration(secrets map[string]string) (*Credentials, error) {
+	if isMigrationSecret(secrets) {
+		migSecret, err := ParseAndSetSecretMapFromMigSecret(secrets)
+		if err != nil {
+			return nil, err
+		}
+		secrets = migSecret
+	}
+	cr, cErr := NewUserCredentials(secrets)
+	if cErr != nil {
+		return nil, cErr
+	}
+
+	return cr, nil
 }

internal/util/credentials_test.go

@@ -42,7 +42,7 @@ func TestIsMigrationSecret(t *testing.T) {
 		newtt := tt
 		t.Run(newtt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := IsMigrationSecret(newtt.vc); got != newtt.want {
+			if got := isMigrationSecret(newtt.vc); got != newtt.want {
 				t.Errorf("isMigrationSecret() = %v, want %v", got, newtt.want)
 			}
 		})