From 64c6a8db2251aeae0af2713be64fb55f6ceaee8c Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Mon, 18 May 2020 08:23:17 -0400 Subject: [PATCH] csi: improve plugin error messages and volume validation (#7984) Some CSI plugins don't return much for errors over the gRPC socket above and beyond the bare minimum error codes. This changeset improves the operator experience by unpacking the error codes when available and wrapping the error with some user-friendly direction. Improving these errors also revealed a bad comparison with `require.Error` when `require.EqualError` should be used in the test code for plugin errors. This defect in turn was hiding a bug in volume validation where we're being overly permissive in allowing mount flags, which is now fixed. --- plugins/csi/client.go | 110 +++++++++++++++++-- plugins/csi/client_test.go | 215 +++++++++++++++++++------------------ 2 files changed, 210 insertions(+), 115 deletions(-) diff --git a/plugins/csi/client.go b/plugins/csi/client.go index 930dffe5bc2..a8e06c71453 100644 --- a/plugins/csi/client.go +++ b/plugins/csi/client.go @@ -16,6 +16,8 @@ import ( "github.com/hashicorp/nomad/plugins/base" "github.com/hashicorp/nomad/plugins/shared/hclspec" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // PluginTypeCSI implements the CSI plugin interface @@ -269,6 +271,24 @@ func (c *client) ControllerPublishVolume(ctx context.Context, req *ControllerPub pbrequest := req.ToCSIRepresentation() resp, err := c.controllerClient.ControllerPublishVolume(ctx, pbrequest, opts...) if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q or node %q could not be found: %v", + req.ExternalID, req.NodeID, err) + case codes.AlreadyExists: + err = fmt.Errorf( + "volume %q is already published at node %q but with capabilities or a read_only setting incompatible with this request: %v", + req.ExternalID, req.NodeID, err) + case codes.ResourceExhausted: + err = fmt.Errorf("node %q has reached the maximum allowable number of attached volumes: %v", + req.NodeID, err) + case codes.FailedPrecondition: + err = fmt.Errorf("volume %q is already published on another node and does not have MULTI_NODE volume capability: %v", + req.ExternalID, err) + case codes.Internal: + err = fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } return nil, err } @@ -292,6 +312,14 @@ func (c *client) ControllerUnpublishVolume(ctx context.Context, req *ControllerU upbrequest := req.ToCSIRepresentation() _, err = c.controllerClient.ControllerUnpublishVolume(ctx, upbrequest, opts...) if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q or node %q could not be found: %v", + req.ExternalID, req.NodeID, err) + case codes.Internal: + err = fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } return nil, err } @@ -317,6 +345,13 @@ func (c *client) ControllerValidateCapabilities(ctx context.Context, req *Contro creq := req.ToCSIRepresentation() resp, err := c.controllerClient.ValidateVolumeCapabilities(ctx, creq, opts...) if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q could not be found: %v", req.ExternalID, err) + case codes.Internal: + err = fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } return err } @@ -348,6 +383,7 @@ func (c *client) ControllerValidateCapabilities(ctx context.Context, req *Contro // contain the 'expected' capability func compareCapabilities(expected *csipbv1.VolumeCapability, got []*csipbv1.VolumeCapability) error { var err multierror.Error +NEXT_CAP: for _, cap := range got { expectedMode := expected.GetAccessMode().GetMode() @@ -356,7 +392,7 @@ func compareCapabilities(expected *csipbv1.VolumeCapability, got []*csipbv1.Volu if expectedMode != capMode { multierror.Append(&err, fmt.Errorf("requested AccessMode %v, got %v", expectedMode, capMode)) - continue + continue NEXT_CAP } // AccessType Block is an empty struct even if set, so the @@ -371,20 +407,20 @@ func compareCapabilities(expected *csipbv1.VolumeCapability, got []*csipbv1.Volu } multierror.Append(&err, fmt.Errorf( "requested AccessType Block but got AccessType Mount")) - continue + continue NEXT_CAP } if capMount == nil { multierror.Append(&err, fmt.Errorf( "requested AccessType Mount but got AccessType Block")) - continue + continue NEXT_CAP } if expectedMount.FsType != capMount.FsType { multierror.Append(&err, fmt.Errorf( "requested AccessType mount filesystem type %v, got %v", expectedMount.FsType, capMount.FsType)) - continue + continue NEXT_CAP } for _, expectedFlag := range expectedMount.MountFlags { @@ -399,7 +435,7 @@ func compareCapabilities(expected *csipbv1.VolumeCapability, got []*csipbv1.Volu // mount flags can contain sensitive data, so we can't log details multierror.Append(&err, fmt.Errorf( "requested mount flags did not match available capabilities")) - continue + continue NEXT_CAP } } return nil @@ -469,7 +505,7 @@ func (c *client) NodeStageVolume(ctx context.Context, volumeID string, publishCo } // These errors should not be returned during production use but exist as aids - // during Nomad Development + // during Nomad development if volumeID == "" { return fmt.Errorf("missing volumeID") } @@ -488,6 +524,23 @@ func (c *client) NodeStageVolume(ctx context.Context, volumeID string, publishCo // NodeStageVolume's response contains no extra data. If err == nil, we were // successful. _, err := c.nodeClient.NodeStageVolume(ctx, req, opts...) + if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q could not be found: %v", volumeID, err) + case codes.AlreadyExists: + err = fmt.Errorf( + "volume %q is already staged to %q but with incompatible capabilities for this request: %v", + volumeID, stagingTargetPath, err) + case codes.FailedPrecondition: + err = fmt.Errorf("volume %q is already published on another node and does not have MULTI_NODE volume capability: %v", + volumeID, err) + case codes.Internal: + err = fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } + } + return err } @@ -499,7 +552,7 @@ func (c *client) NodeUnstageVolume(ctx context.Context, volumeID string, staging return fmt.Errorf("Client not initialized") } // These errors should not be returned during production use but exist as aids - // during Nomad Development + // during Nomad development if volumeID == "" { return fmt.Errorf("missing volumeID") } @@ -515,6 +568,16 @@ func (c *client) NodeUnstageVolume(ctx context.Context, volumeID string, staging // NodeUnstageVolume's response contains no extra data. If err == nil, we were // successful. _, err := c.nodeClient.NodeUnstageVolume(ctx, req, opts...) + if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q could not be found: %v", volumeID, err) + case codes.Internal: + err = fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } + } + return err } @@ -533,6 +596,22 @@ func (c *client) NodePublishVolume(ctx context.Context, req *NodePublishVolumeRe // NodePublishVolume's response contains no extra data. If err == nil, we were // successful. _, err := c.nodeClient.NodePublishVolume(ctx, req.ToCSIRepresentation(), opts...) + if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q could not be found: %v", req.ExternalID, err) + case codes.AlreadyExists: + err = fmt.Errorf( + "volume %q is already published at target path %q but with capabilities or a read_only setting incompatible with this request: %v", + req.ExternalID, req.TargetPath, err) + case codes.FailedPrecondition: + err = fmt.Errorf("volume %q is already published on another node and does not have MULTI_NODE volume capability: %v", + req.ExternalID, err) + case codes.Internal: + err = fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } + } return err } @@ -544,12 +623,13 @@ func (c *client) NodeUnpublishVolume(ctx context.Context, volumeID, targetPath s return fmt.Errorf("Client not initialized") } + // These errors should not be returned during production use but exist as aids + // during Nomad development if volumeID == "" { - return fmt.Errorf("missing VolumeID") + return fmt.Errorf("missing volumeID") } - if targetPath == "" { - return fmt.Errorf("missing TargetPath") + return fmt.Errorf("missing targetPath") } req := &csipbv1.NodeUnpublishVolumeRequest{ @@ -560,5 +640,15 @@ func (c *client) NodeUnpublishVolume(ctx context.Context, volumeID, targetPath s // NodeUnpublishVolume's response contains no extra data. If err == nil, we were // successful. _, err := c.nodeClient.NodeUnpublishVolume(ctx, req, opts...) + if err != nil { + code := status.Code(err) + switch code { + case codes.NotFound: + err = fmt.Errorf("volume %q could not be found: %v", volumeID, err) + case codes.Internal: + err = fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: %v", err) + } + } + return err } diff --git a/plugins/csi/client_test.go b/plugins/csi/client_test.go index 0fdfb92a9d8..04e574947f5 100644 --- a/plugins/csi/client_test.go +++ b/plugins/csi/client_test.go @@ -11,6 +11,8 @@ import ( "github.com/hashicorp/nomad/nomad/structs" fake "github.com/hashicorp/nomad/plugins/csi/testing" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func newTestClient() (*fake.IdentityClient, *fake.ControllerClient, *fake.NodeClient, CSIPlugin) { @@ -65,20 +67,20 @@ func TestClient_RPC_PluginProbe(t *testing.T) { }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { ic, _, _, client := newTestClient() defer client.Close() - ic.NextErr = c.ResponseErr - ic.NextPluginProbe = c.ProbeResponse + ic.NextErr = tc.ResponseErr + ic.NextPluginProbe = tc.ProbeResponse resp, err := client.PluginProbe(context.TODO()) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } - require.Equal(t, c.ExpectedResponse, resp) + require.Equal(t, tc.ExpectedResponse, resp) }) } @@ -117,21 +119,21 @@ func TestClient_RPC_PluginInfo(t *testing.T) { }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { ic, _, _, client := newTestClient() defer client.Close() - ic.NextErr = c.ResponseErr - ic.NextPluginInfo = c.InfoResponse + ic.NextErr = tc.ResponseErr + ic.NextPluginInfo = tc.InfoResponse name, version, err := client.PluginGetInfo(context.TODO()) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } - require.Equal(t, c.ExpectedResponseName, name) - require.Equal(t, c.ExpectedResponseVersion, version) + require.Equal(t, tc.ExpectedResponseName, name) + require.Equal(t, tc.ExpectedResponseVersion, version) }) } @@ -182,20 +184,20 @@ func TestClient_RPC_PluginGetCapabilities(t *testing.T) { }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { ic, _, _, client := newTestClient() defer client.Close() - ic.NextErr = c.ResponseErr - ic.NextPluginCapabilities = c.Response + ic.NextErr = tc.ResponseErr + ic.NextPluginCapabilities = tc.Response resp, err := client.PluginGetCapabilities(context.TODO()) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } - require.Equal(t, c.ExpectedResponse, resp) + require.Equal(t, tc.ExpectedResponse, resp) }) } } @@ -290,7 +292,7 @@ func TestClient_RPC_ControllerGetCapabilities(t *testing.T) { resp, err := client.ControllerGetCapabilities(context.TODO()) if tc.ExpectedErr != nil { - require.Error(t, tc.ExpectedErr, err) + require.EqualError(t, err, tc.ExpectedErr.Error()) } require.Equal(t, tc.ExpectedResponse, resp) @@ -355,7 +357,7 @@ func TestClient_RPC_NodeGetCapabilities(t *testing.T) { resp, err := client.NodeGetCapabilities(context.TODO()) if tc.ExpectedErr != nil { - require.Error(t, tc.ExpectedErr, err) + require.EqualError(t, err, tc.ExpectedErr.Error()) } require.Equal(t, tc.ExpectedResponse, resp) @@ -374,25 +376,26 @@ func TestClient_RPC_ControllerPublishVolume(t *testing.T) { }{ { Name: "handles underlying grpc errors", - Request: &ControllerPublishVolumeRequest{}, - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + Request: &ControllerPublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, + ResponseErr: status.Errorf(codes.Internal, "some grpc error"), + ExpectedErr: fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { - Name: "Handles missing NodeID", - Request: &ControllerPublishVolumeRequest{}, + Name: "handles missing NodeID", + Request: &ControllerPublishVolumeRequest{ExternalID: "vol"}, Response: &csipbv1.ControllerPublishVolumeResponse{}, ExpectedErr: fmt.Errorf("missing NodeID"), }, { - Name: "Handles PublishContext == nil", - Request: &ControllerPublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, + Name: "handles PublishContext == nil", + Request: &ControllerPublishVolumeRequest{ + ExternalID: "vol", NodeID: "node"}, Response: &csipbv1.ControllerPublishVolumeResponse{}, ExpectedResponse: &ControllerPublishVolumeResponse{}, }, { - Name: "Handles PublishContext != nil", + Name: "handles PublishContext != nil", Request: &ControllerPublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, Response: &csipbv1.ControllerPublishVolumeResponse{ PublishContext: map[string]string{ @@ -409,20 +412,20 @@ func TestClient_RPC_ControllerPublishVolume(t *testing.T) { }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, cc, _, client := newTestClient() defer client.Close() - cc.NextErr = c.ResponseErr - cc.NextPublishVolumeResponse = c.Response + cc.NextErr = tc.ResponseErr + cc.NextPublishVolumeResponse = tc.Response - resp, err := client.ControllerPublishVolume(context.TODO(), c.Request) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + resp, err := client.ControllerPublishVolume(context.TODO(), tc.Request) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } - require.Equal(t, c.ExpectedResponse, resp) + require.Equal(t, tc.ExpectedResponse, resp) }) } } @@ -437,39 +440,38 @@ func TestClient_RPC_ControllerUnpublishVolume(t *testing.T) { ExpectedErr error }{ { - Name: "Handles underlying grpc errors", - Request: &ControllerUnpublishVolumeRequest{}, - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + Name: "handles underlying grpc errors", + Request: &ControllerUnpublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, + ResponseErr: status.Errorf(codes.Internal, "some grpc error"), + ExpectedErr: fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { - Name: "Handles missing NodeID", - Request: &ControllerUnpublishVolumeRequest{}, + Name: "handles missing NodeID", + Request: &ControllerUnpublishVolumeRequest{ExternalID: "vol"}, ExpectedErr: fmt.Errorf("missing NodeID"), ExpectedResponse: nil, }, { - Name: "Handles successful response", + Name: "handles successful response", Request: &ControllerUnpublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, - ExpectedErr: fmt.Errorf("missing NodeID"), ExpectedResponse: &ControllerUnpublishVolumeResponse{}, }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, cc, _, client := newTestClient() defer client.Close() - cc.NextErr = c.ResponseErr - cc.NextUnpublishVolumeResponse = c.Response + cc.NextErr = tc.ResponseErr + cc.NextUnpublishVolumeResponse = tc.Response - resp, err := client.ControllerUnpublishVolume(context.TODO(), c.Request) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + resp, err := client.ControllerUnpublishVolume(context.TODO(), tc.Request) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } - require.Equal(t, c.ExpectedResponse, resp) + require.Equal(t, tc.ExpectedResponse, resp) }) } } @@ -484,8 +486,8 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { }{ { Name: "handles underlying grpc errors", - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + ResponseErr: status.Errorf(codes.Internal, "some grpc error"), + ExpectedErr: fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { Name: "handles empty success", @@ -534,8 +536,10 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { }, }, ResponseErr: nil, - ExpectedErr: fmt.Errorf("volume capability validation failed"), + // this is a multierror + ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* requested AccessMode MULTI_NODE_MULTI_WRITER, got SINGLE_NODE_WRITER\n\n"), }, + { Name: "handles validation failure mount flags", Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ @@ -557,12 +561,13 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { }, }, ResponseErr: nil, - ExpectedErr: fmt.Errorf("volume capability validation failed"), + // this is a multierror + ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* requested mount flags did not match available capabilities\n\n"), }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, cc, _, client := newTestClient() defer client.Close() @@ -582,14 +587,14 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { Context: map[string]string{}, } - cc.NextValidateVolumeCapabilitiesResponse = c.Response - cc.NextErr = c.ResponseErr + cc.NextValidateVolumeCapabilitiesResponse = tc.Response + cc.NextErr = tc.ResponseErr err := client.ControllerValidateCapabilities(context.TODO(), req) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err, c.Name) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } else { - require.NoError(t, err, c.Name) + require.NoError(t, err, tc.Name) } }) } @@ -605,8 +610,8 @@ func TestClient_RPC_NodeStageVolume(t *testing.T) { }{ { Name: "handles underlying grpc errors", - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + ResponseErr: status.Errorf(codes.AlreadyExists, "some grpc error"), + ExpectedErr: fmt.Errorf("volume \"foo\" is already staged to \"/path\" but with incompatible capabilities for this request: rpc error: code = AlreadyExists desc = some grpc error"), }, { Name: "handles success", @@ -615,18 +620,18 @@ func TestClient_RPC_NodeStageVolume(t *testing.T) { }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, _, nc, client := newTestClient() defer client.Close() - nc.NextErr = c.ResponseErr - nc.NextStageVolumeResponse = c.Response + nc.NextErr = tc.ResponseErr + nc.NextStageVolumeResponse = tc.Response - err := client.NodeStageVolume(context.TODO(), "foo", nil, "/foo", + err := client.NodeStageVolume(context.TODO(), "foo", nil, "/path", &VolumeCapability{}, structs.CSISecrets{}) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } else { require.Nil(t, err) } @@ -643,8 +648,8 @@ func TestClient_RPC_NodeUnstageVolume(t *testing.T) { }{ { Name: "handles underlying grpc errors", - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + ResponseErr: status.Errorf(codes.Internal, "some grpc error"), + ExpectedErr: fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { Name: "handles success", @@ -653,17 +658,17 @@ func TestClient_RPC_NodeUnstageVolume(t *testing.T) { }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, _, nc, client := newTestClient() defer client.Close() - nc.NextErr = c.ResponseErr - nc.NextUnstageVolumeResponse = c.Response + nc.NextErr = tc.ResponseErr + nc.NextUnstageVolumeResponse = tc.Response err := client.NodeUnstageVolume(context.TODO(), "foo", "/foo") - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } else { require.Nil(t, err) } @@ -686,8 +691,8 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) { TargetPath: "/dev/null", VolumeCapability: &VolumeCapability{}, }, - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + ResponseErr: status.Errorf(codes.Internal, "some grpc error"), + ExpectedErr: fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { Name: "handles success", @@ -705,21 +710,21 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) { ExternalID: "", }, ResponseErr: nil, - ExpectedErr: errors.New("missing volume ID"), + ExpectedErr: errors.New("validation error: missing volume ID"), }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, _, nc, client := newTestClient() defer client.Close() - nc.NextErr = c.ResponseErr - nc.NextPublishVolumeResponse = c.Response + nc.NextErr = tc.ResponseErr + nc.NextPublishVolumeResponse = tc.Response - err := client.NodePublishVolume(context.TODO(), c.Request) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + err := client.NodePublishVolume(context.TODO(), tc.Request) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } else { require.Nil(t, err) } @@ -739,8 +744,8 @@ func TestClient_RPC_NodeUnpublishVolume(t *testing.T) { Name: "handles underlying grpc errors", ExternalID: "foo", TargetPath: "/dev/null", - ResponseErr: fmt.Errorf("some grpc error"), - ExpectedErr: fmt.Errorf("some grpc error"), + ResponseErr: status.Errorf(codes.Internal, "some grpc error"), + ExpectedErr: fmt.Errorf("node plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { Name: "handles success", @@ -752,27 +757,27 @@ func TestClient_RPC_NodeUnpublishVolume(t *testing.T) { { Name: "Performs validation of the request args - ExternalID", ResponseErr: nil, - ExpectedErr: errors.New("missing volume ID"), + ExpectedErr: errors.New("missing volumeID"), }, { Name: "Performs validation of the request args - TargetPath", ExternalID: "foo", ResponseErr: nil, - ExpectedErr: errors.New("missing TargetPath"), + ExpectedErr: errors.New("missing targetPath"), }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { _, _, nc, client := newTestClient() defer client.Close() - nc.NextErr = c.ResponseErr - nc.NextUnpublishVolumeResponse = c.Response + nc.NextErr = tc.ResponseErr + nc.NextUnpublishVolumeResponse = tc.Response - err := client.NodeUnpublishVolume(context.TODO(), c.ExternalID, c.TargetPath) - if c.ExpectedErr != nil { - require.Error(t, c.ExpectedErr, err) + err := client.NodeUnpublishVolume(context.TODO(), tc.ExternalID, tc.TargetPath) + if tc.ExpectedErr != nil { + require.EqualError(t, err, tc.ExpectedErr.Error()) } else { require.Nil(t, err) }