From b763546f4725804d3890a40ab4969bf40d47cab8 Mon Sep 17 00:00:00 2001 From: gman Date: Sat, 3 Aug 2019 19:30:46 +0200 Subject: [PATCH 1/3] fixed incorrect behaviour discovered by csi-sanity test suite * implemented ValidateVolumeCapabilities in Controller Plugin * validateNode{Unstage,Unpublish}Request functions were missing checks for empty staging/target paths * verifySnapshotCompatibility's check for source volume ID was incorrect this commit fixes the issues mentioned above and cleans up the codebase a bit --- pkg/csi/manila/controllerserver.go | 63 +++++++++++++++++++++++++++++- pkg/csi/manila/nodeserver.go | 8 ++-- pkg/csi/manila/share.go | 7 ++-- pkg/csi/manila/snapshot.go | 10 ++--- pkg/csi/manila/util.go | 59 ++++++++++++++++++++++++---- pkg/csi/manila/volumesource.go | 3 +- 6 files changed, 126 insertions(+), 24 deletions(-) diff --git a/pkg/csi/manila/controllerserver.go b/pkg/csi/manila/controllerserver.go index 7c30b33387..66a99da841 100644 --- a/pkg/csi/manila/controllerserver.go +++ b/pkg/csi/manila/controllerserver.go @@ -229,6 +229,10 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS // Retrieve the source share if sourceShare, res.err = manilaClient.GetShareByID(req.GetSourceVolumeId()); res.err != nil { + if isManilaErrNotFound(res.err) { + return nil, status.Errorf(codes.InvalidArgument, "failed to create a snapshot (%s) for share %s because the share doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err) + } + return nil, status.Errorf(codes.Internal, "failed to retrieve source share %s when creating a snapshot (%s): %v", req.GetSourceVolumeId(), req.GetName(), res.err) } @@ -246,10 +250,14 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS return nil, status.Errorf(codes.DeadlineExceeded, "deadline exceeded while waiting for snapshot %s of share %s to become available", snapshot.ID, req.GetSourceVolumeId()) } + if isManilaErrNotFound(res.err) { + return nil, status.Errorf(codes.InvalidArgument, "failed to create a snapshot (%s) for share %s because the share doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err) + } + return nil, status.Errorf(codes.Internal, "failed to create a snapshot (%s) of share %s: %v", req.GetName(), req.GetSourceVolumeId(), res.err) } - if res.err = verifySnapshotCompatibility(sourceShare, snapshot, req); res.err != nil { + if res.err = verifySnapshotCompatibility(snapshot, req); res.err != nil { return nil, status.Errorf(codes.AlreadyExists, "a snapshot named %s already exists, but is incompatible with the request: %v", req.GetName(), res.err) } @@ -332,7 +340,58 @@ func (cs *controllerServer) ControllerGetCapabilities(ctx context.Context, req * } func (cs *controllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { - return nil, status.Error(codes.Unimplemented, "") + if err := validateValidateVolumeCapabilitiesRequest(req); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + osOpts, err := options.NewOpenstackOptions(req.GetSecrets()) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "invalid OpenStack secrets: %v", err) + } + + for _, volCap := range req.GetVolumeCapabilities() { + if volCap.GetBlock() != nil { + return &csi.ValidateVolumeCapabilitiesResponse{Message: "block access type is not allowed"}, nil + } + + if volCap.GetMount() == nil { + return &csi.ValidateVolumeCapabilitiesResponse{Message: "volume must be accessible via filesystem API"}, nil + } + + if volCap.GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_UNKNOWN { + return &csi.ValidateVolumeCapabilitiesResponse{Message: "unknown volume access mode"}, nil + } + } + + manilaClient, err := cs.d.manilaClientBuilder.New(osOpts) + if err != nil { + return nil, status.Errorf(codes.Unauthenticated, "failed to create Manila v2 client: %v", err) + } + + share, err := manilaClient.GetShareByID(req.GetVolumeId()) + if err != nil { + if isManilaErrNotFound(err) { + return nil, status.Errorf(codes.NotFound, "share %s not found: %v", req.GetVolumeId(), err) + } + + return nil, status.Errorf(codes.Internal, "failed to retrieve share %s: %v", req.GetVolumeId(), err) + } + + if share.Status != shareAvailable { + return nil, status.Errorf(codes.InvalidArgument, "share %s is in an unexpected state: wanted %s, got %s", share.ID, shareAvailable, share.Status) + } + + if !compareProtocol(share.ShareProto, cs.d.shareProto) { + return nil, status.Errorf(codes.InvalidArgument, "share protocol mismatch: wanted %s, got %s", cs.d.shareProto, share.ShareProto) + } + + return &csi.ValidateVolumeCapabilitiesResponse{ + Confirmed: &csi.ValidateVolumeCapabilitiesResponse_Confirmed{ + VolumeContext: req.GetVolumeContext(), + VolumeCapabilities: req.GetVolumeCapabilities(), + Parameters: req.GetParameters(), + }, + }, nil } func (cs *controllerServer) ControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) { diff --git a/pkg/csi/manila/nodeserver.go b/pkg/csi/manila/nodeserver.go index 41deb80754..68b0c5a2d0 100644 --- a/pkg/csi/manila/nodeserver.go +++ b/pkg/csi/manila/nodeserver.go @@ -156,12 +156,12 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis shareOpts, err := options.NewNodeVolumeContext(req.GetVolumeContext()) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "invalid volume parameters: %v", err) + return nil, status.Errorf(codes.InvalidArgument, "invalid volume context: %v", err) } osOpts, err := options.NewOpenstackOptions(req.GetSecrets()) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "invalid volume secret: %v", err) + return nil, status.Errorf(codes.InvalidArgument, "invalid OpenStack secrets: %v", err) } volID := volumeID(req.GetVolumeId()) @@ -240,12 +240,12 @@ func (ns *nodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol shareOpts, err := options.NewNodeVolumeContext(req.GetVolumeContext()) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "invalid volume parameters: %v", err) + return nil, status.Errorf(codes.InvalidArgument, "invalid volume context: %v", err) } osOpts, err := options.NewOpenstackOptions(req.GetSecrets()) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "invalid volume secret: %v", err) + return nil, status.Errorf(codes.InvalidArgument, "invalid OpenStack secrets: %v", err) } volID := volumeID(req.GetVolumeId()) diff --git a/pkg/csi/manila/share.go b/pkg/csi/manila/share.go index 84ee1754c2..1fd3479617 100644 --- a/pkg/csi/manila/share.go +++ b/pkg/csi/manila/share.go @@ -20,7 +20,6 @@ import ( "fmt" "time" - "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/shares" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient" @@ -51,7 +50,7 @@ func getOrCreateShare(shareName string, createOpts *shares.CreateOpts, manilaCli // First, check if the share already exists or needs to be created if share, err = manilaClient.GetShareByName(shareName); err != nil { - if _, ok := err.(gophercloud.ErrResourceNotFound); ok { + if isManilaErrNotFound(err) { // It doesn't exist, create it var createErr error @@ -77,7 +76,7 @@ func getOrCreateShare(shareName string, createOpts *shares.CreateOpts, manilaCli func deleteShare(shareID string, manilaClient manilaclient.Interface) error { if err := manilaClient.DeleteShare(shareID); err != nil { - if _, ok := err.(gophercloud.ErrResourceNotFound); ok { + if isManilaErrNotFound(err) { klog.V(4).Infof("share %s not found, assuming it to be already deleted", shareID) } else { return err @@ -121,7 +120,7 @@ func waitForShareStatus(shareID, currentStatus, desiredStatus string, successOnN share, err = manilaClient.GetShareByID(shareID) if err != nil { - if _, ok := err.(gophercloud.ErrDefault404); ok && successOnNotFound { + if isManilaErrNotFound(err) && successOnNotFound { return true, nil } diff --git a/pkg/csi/manila/snapshot.go b/pkg/csi/manila/snapshot.go index 9265855a7d..d677fe36de 100644 --- a/pkg/csi/manila/snapshot.go +++ b/pkg/csi/manila/snapshot.go @@ -18,12 +18,12 @@ package manila import ( "fmt" - "github.com/gophercloud/gophercloud" + "time" + "github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/snapshots" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient" "k8s.io/klog" - "time" ) const ( @@ -47,7 +47,7 @@ func getOrCreateSnapshot(snapName, sourceShareID string, manilaClient manilaclie // First, check if the snapshot already exists or needs to be created if snapshot, err = manilaClient.GetSnapshotByName(snapName); err != nil { - if _, ok := err.(gophercloud.ErrResourceNotFound); ok { + if isManilaErrNotFound(err) { // It doesn't exist, create it opts := snapshots.CreateOpts{ @@ -74,7 +74,7 @@ func getOrCreateSnapshot(snapName, sourceShareID string, manilaClient manilaclie func deleteSnapshot(snapID string, manilaClient manilaclient.Interface) error { if err := manilaClient.DeleteSnapshot(snapID); err != nil { - if _, ok := err.(gophercloud.ErrResourceNotFound); ok { + if isManilaErrNotFound(err) { klog.V(4).Infof("snapshot %s not found, assuming it to be already deleted", snapID) } else { return err @@ -118,7 +118,7 @@ func waitForSnapshotStatus(snapshotID, currentStatus, desiredStatus string, succ snapshot, err = manilaClient.GetSnapshotByID(snapshotID) if err != nil { - if _, ok := err.(gophercloud.ErrResourceNotFound); ok && successOnNotFound { + if isManilaErrNotFound(err) && successOnNotFound { return true, nil } diff --git a/pkg/csi/manila/util.go b/pkg/csi/manila/util.go index b4dd2a752b..668eb90445 100644 --- a/pkg/csi/manila/util.go +++ b/pkg/csi/manila/util.go @@ -22,6 +22,7 @@ import ( "strings" "github.com/container-storage-interface/spec/lib/go/csi" + "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/messages" "github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/shares" "github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/snapshots" @@ -176,6 +177,26 @@ func lastResourceError(resourceID string, manilaClient manilaclient.Interface) ( return manilaErrorMessage{message: "unknown error"}, nil } +func isManilaErrNotFound(err error) bool { + if err == nil { + return false + } + + if _, isDefault404 := err.(gophercloud.ErrDefault404); isDefault404 { + return true + } + + if _, isResourceNotFound := err.(gophercloud.ErrResourceNotFound); isResourceNotFound { + return true + } + + return false +} + +func compareProtocol(protoA, protoB string) bool { + return strings.ToUpper(protoA) == strings.ToUpper(protoB) +} + // // Controller service request validation // @@ -192,7 +213,7 @@ func validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error { for _, cap := range reqCaps { if cap.GetBlock() != nil { - return errors.New("block volume not supported") + return errors.New("block access type not allowed") } } @@ -257,11 +278,11 @@ func verifyVolumeCompatibility(sizeInGiB int, req *csi.CreateVolumeRequest, shar } if share.Size != sizeInGiB { - return fmt.Errorf("size mismatch: wanted %d, got %d", sizeInGiB, share.Size) + return fmt.Errorf("size mismatch: wanted %d, got %d", share.Size, sizeInGiB) } if share.ShareProto != shareOpts.Protocol { - return fmt.Errorf("share protocol mismatch: wanted %s, got %s", coalesceValue(shareOpts.Protocol), coalesceValue(share.ShareProto)) + return fmt.Errorf("share protocol mismatch: wanted %s, got %s", coalesceValue(share.ShareProto), coalesceValue(shareOpts.Protocol)) } // FIXME shareOpts.Type may be either type name or type ID @@ -272,7 +293,7 @@ func verifyVolumeCompatibility(sizeInGiB int, req *csi.CreateVolumeRequest, shar */ if share.ShareNetworkID != shareOpts.ShareNetworkID { - return fmt.Errorf("share network ID mismatch: wanted %s, got %s", coalesceValue(shareOpts.ShareNetworkID), coalesceValue(share.ShareNetworkID)) + return fmt.Errorf("share network ID mismatch: wanted %s, got %s", coalesceValue(share.ShareNetworkID), coalesceValue(shareOpts.ShareNetworkID)) } var reqSrcSnapID string @@ -287,9 +308,25 @@ func verifyVolumeCompatibility(sizeInGiB int, req *csi.CreateVolumeRequest, shar return nil } -func verifySnapshotCompatibility(sourceShare *shares.Share, snapshot *snapshots.Snapshot, req *csi.CreateSnapshotRequest) error { - if sourceShare.ID != req.SourceVolumeId { - return fmt.Errorf("source share ID mismatch: wanted %s, got %s", req.SourceVolumeId, sourceShare.ID) +func verifySnapshotCompatibility(snapshot *snapshots.Snapshot, req *csi.CreateSnapshotRequest) error { + if snapshot.ShareID != req.GetSourceVolumeId() { + return fmt.Errorf("source share ID mismatch: wanted %s, got %s", snapshot.ID, req.GetSourceVolumeId()) + } + + return nil +} + +func validateValidateVolumeCapabilitiesRequest(req *csi.ValidateVolumeCapabilitiesRequest) error { + if req.GetVolumeId() == "" { + return errors.New("volume ID missing in request") + } + + if req.GetVolumeCapabilities() == nil || len(req.GetVolumeCapabilities()) == 0 { + return errors.New("volume capabilities cannot be nil or empty") + } + + if req.GetSecrets() == nil || len(req.GetSecrets()) == 0 { + return errors.New("stage secrets cannot be nil or empty") } return nil @@ -320,6 +357,10 @@ func validateNodeStageVolumeRequest(req *csi.NodeStageVolumeRequest) error { } func validateNodeUnstageVolumeRequest(req *csi.NodeUnstageVolumeRequest) error { + if req.GetStagingTargetPath() == "" { + return errors.New("staging path missing in request") + } + if req.GetVolumeId() == "" { return errors.New("volume ID missing in request") } @@ -348,6 +389,10 @@ func validateNodePublishVolumeRequest(req *csi.NodePublishVolumeRequest) error { } func validateNodeUnpublishVolumeRequest(req *csi.NodeUnpublishVolumeRequest) error { + if req.GetTargetPath() == "" { + return errors.New("target path missing in request") + } + if req.GetVolumeId() == "" { return errors.New("volume ID missing in request") } diff --git a/pkg/csi/manila/volumesource.go b/pkg/csi/manila/volumesource.go index 593c83c7a0..c8ba78a21b 100644 --- a/pkg/csi/manila/volumesource.go +++ b/pkg/csi/manila/volumesource.go @@ -18,7 +18,6 @@ package manila import ( "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/shares" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -81,7 +80,7 @@ func (volumeFromSnapshot) create(req *csi.CreateVolumeRequest, shareName string, snapshot, err := manilaClient.GetSnapshotByID(snapshotSource.GetSnapshotId()) if err != nil { - if _, ok := err.(gophercloud.ErrResourceNotFound); ok { + if isManilaErrNotFound(err) { return nil, status.Errorf(codes.NotFound, "source snapshot %s not found: %v", snapshotSource.GetSnapshotId(), err) } From 725bf888c8a011aa1b3148cc8fd5ce4a61b5623c Mon Sep 17 00:00:00 2001 From: gman Date: Wed, 21 Aug 2019 11:17:37 +0200 Subject: [PATCH 2/3] addressed review comments: fixed error codes --- pkg/csi/manila/controllerserver.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/csi/manila/controllerserver.go b/pkg/csi/manila/controllerserver.go index 66a99da841..8e6ef4a1ed 100644 --- a/pkg/csi/manila/controllerserver.go +++ b/pkg/csi/manila/controllerserver.go @@ -230,7 +230,7 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS if sourceShare, res.err = manilaClient.GetShareByID(req.GetSourceVolumeId()); res.err != nil { if isManilaErrNotFound(res.err) { - return nil, status.Errorf(codes.InvalidArgument, "failed to create a snapshot (%s) for share %s because the share doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err) + return nil, status.Errorf(codes.NotFound, "failed to create a snapshot (%s) for share %s because the share doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err) } return nil, status.Errorf(codes.Internal, "failed to retrieve source share %s when creating a snapshot (%s): %v", req.GetSourceVolumeId(), req.GetName(), res.err) @@ -251,7 +251,7 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS } if isManilaErrNotFound(res.err) { - return nil, status.Errorf(codes.InvalidArgument, "failed to create a snapshot (%s) for share %s because the share doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err) + return nil, status.Errorf(codes.NotFound, "failed to create a snapshot (%s) for share %s because the share doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err) } return nil, status.Errorf(codes.Internal, "failed to create a snapshot (%s) of share %s: %v", req.GetName(), req.GetSourceVolumeId(), res.err) @@ -378,7 +378,7 @@ func (cs *controllerServer) ValidateVolumeCapabilities(ctx context.Context, req } if share.Status != shareAvailable { - return nil, status.Errorf(codes.InvalidArgument, "share %s is in an unexpected state: wanted %s, got %s", share.ID, shareAvailable, share.Status) + return nil, status.Errorf(codes.Internal, "share %s is in an unexpected state: wanted %s, got %s", share.ID, shareAvailable, share.Status) } if !compareProtocol(share.ShareProto, cs.d.shareProto) { From 4d0ef767cfb92359a7e42e2ecb7eb70e8d55caf7 Mon Sep 17 00:00:00 2001 From: gman Date: Wed, 21 Aug 2019 12:43:33 +0200 Subject: [PATCH 3/3] added explicit status checks for shares and snapshots --- pkg/csi/manila/controllerserver.go | 6 +++++- pkg/csi/manila/nodeserver.go | 6 +++++- pkg/csi/manila/volumesource.go | 8 ++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/pkg/csi/manila/controllerserver.go b/pkg/csi/manila/controllerserver.go index 8e6ef4a1ed..5828544cb0 100644 --- a/pkg/csi/manila/controllerserver.go +++ b/pkg/csi/manila/controllerserver.go @@ -378,7 +378,11 @@ func (cs *controllerServer) ValidateVolumeCapabilities(ctx context.Context, req } if share.Status != shareAvailable { - return nil, status.Errorf(codes.Internal, "share %s is in an unexpected state: wanted %s, got %s", share.ID, shareAvailable, share.Status) + if share.Status == shareCreating { + return nil, status.Errorf(codes.Unavailable, "share %s is in transient creating state", share.ID) + } + + return nil, status.Errorf(codes.FailedPrecondition, "share %s is in an unexpected state: wanted %s, got %s", share.ID, shareAvailable, share.Status) } if !compareProtocol(share.ShareProto, cs.d.shareProto) { diff --git a/pkg/csi/manila/nodeserver.go b/pkg/csi/manila/nodeserver.go index 68b0c5a2d0..d80bcc230a 100644 --- a/pkg/csi/manila/nodeserver.go +++ b/pkg/csi/manila/nodeserver.go @@ -79,7 +79,11 @@ func (ns *nodeServer) buildVolumeContext(volID volumeID, shareOpts *options.Node } if share.Status != shareAvailable { - return nil, nil, status.Errorf(codes.InvalidArgument, "invalid share status for volume %s (share ID %s): expected 'available', got '%s'", + if share.Status == shareCreating { + return nil, nil, status.Errorf(codes.Unavailable, "share %s for volume %s is in transient creating state", share.ID, volID) + } + + return nil, nil, status.Errorf(codes.FailedPrecondition, "invalid share status for volume %s (share ID %s): expected 'available', got '%s'", volID, share.ID, share.Status) } diff --git a/pkg/csi/manila/volumesource.go b/pkg/csi/manila/volumesource.go index c8ba78a21b..bff4c7ec02 100644 --- a/pkg/csi/manila/volumesource.go +++ b/pkg/csi/manila/volumesource.go @@ -87,6 +87,14 @@ func (volumeFromSnapshot) create(req *csi.CreateVolumeRequest, shareName string, return nil, status.Errorf(codes.Internal, "failed to retrieve snapshot %s: %v", snapshotSource.GetSnapshotId(), err) } + if snapshot.Status != snapshotAvailable { + if snapshot.Status == snapshotCreating { + return nil, status.Errorf(codes.Unavailable, "snapshot %s is in transient creating state", snapshot.ID) + } + + return nil, status.Errorf(codes.FailedPrecondition, "snapshot %s is in invalid state: expected 'available', got '%s'", snapshot.ID, snapshot.Status) + } + createOpts := &shares.CreateOpts{ SnapshotID: snapshot.ID, ShareProto: shareOpts.Protocol,