diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index 67d2a04cf..da464ad53 100644 --- a/internal/rbd/controllerserver.go +++ b/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() diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index e310ba6d0..52da74dde 100644 --- a/internal/rbd/nodeserver.go +++ b/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 } diff --git a/internal/util/credentials.go b/internal/util/credentials.go index 122c24f69..ad8fa6d6e 100644 --- a/internal/util/credentials.go +++ b/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 } diff --git a/internal/util/credentials_test.go b/internal/util/credentials_test.go index 735042f08..27c22dbc9 100644 --- a/internal/util/credentials_test.go +++ b/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) } })