From 4f54a633a2d67601153840cfe5002e94d8a4d3ca Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 14 May 2020 11:56:07 -0400 Subject: [PATCH] csi: refactor internal client field name to ExternalID (#7958) The CSI plugins RPCs require the use of the storage provider's volume ID, rather than the user-defined volume ID. Although changing the RPCs to use the field name `ExternalID` risks breaking backwards compatibility, we can use the `ExternalID` name internally for the client and only use `VolumeID` at the RPC boundaries. --- client/pluginmanager/csimanager/volume.go | 2 +- client/structs/csi.go | 8 +++--- plugins/csi/client_test.go | 28 ++++++++++----------- plugins/csi/plugin.go | 30 +++++++++++------------ 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/client/pluginmanager/csimanager/volume.go b/client/pluginmanager/csimanager/volume.go index 4d9c2a4d1..2f4a5c782 100644 --- a/client/pluginmanager/csimanager/volume.go +++ b/client/pluginmanager/csimanager/volume.go @@ -203,7 +203,7 @@ func (v *volumeManager) publishVolume(ctx context.Context, vol *structs.CSIVolum // CSI NodePublishVolume errors for timeout, codes.Unavailable and // codes.ResourceExhausted are retried; all other errors are fatal. err = v.plugin.NodePublishVolume(ctx, &csi.NodePublishVolumeRequest{ - VolumeID: vol.RemoteID(), + ExternalID: vol.RemoteID(), PublishContext: publishContext, StagingTargetPath: pluginStagingPath, TargetPath: pluginTargetPath, diff --git a/client/structs/csi.go b/client/structs/csi.go index 8e59fc12a..070dc2382 100644 --- a/client/structs/csi.go +++ b/client/structs/csi.go @@ -91,7 +91,7 @@ func (c *ClientCSIControllerAttachVolumeRequest) ToCSIRequest() (*csi.Controller } return &csi.ControllerPublishVolumeRequest{ - VolumeID: c.VolumeID, + ExternalID: c.VolumeID, NodeID: c.ClientCSINodeID, VolumeCapability: caps, ReadOnly: c.ReadOnly, @@ -121,7 +121,7 @@ type ClientCSIControllerAttachVolumeResponse struct { } type ClientCSIControllerDetachVolumeRequest struct { - // The ID of the volume to be unpublished for the node + // The external ID of the volume to be unpublished for the node // This field is REQUIRED. VolumeID string @@ -143,8 +143,8 @@ func (c *ClientCSIControllerDetachVolumeRequest) ToCSIRequest() *csi.ControllerU } return &csi.ControllerUnpublishVolumeRequest{ - VolumeID: c.VolumeID, - NodeID: c.ClientCSINodeID, + ExternalID: c.VolumeID, + NodeID: c.ClientCSINodeID, } } diff --git a/plugins/csi/client_test.go b/plugins/csi/client_test.go index 1d9ae7b4f..4b5ae1334 100644 --- a/plugins/csi/client_test.go +++ b/plugins/csi/client_test.go @@ -387,13 +387,13 @@ func TestClient_RPC_ControllerPublishVolume(t *testing.T) { { Name: "Handles PublishContext == nil", - Request: &ControllerPublishVolumeRequest{VolumeID: "vol", NodeID: "node"}, + Request: &ControllerPublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, Response: &csipbv1.ControllerPublishVolumeResponse{}, ExpectedResponse: &ControllerPublishVolumeResponse{}, }, { Name: "Handles PublishContext != nil", - Request: &ControllerPublishVolumeRequest{VolumeID: "vol", NodeID: "node"}, + Request: &ControllerPublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, Response: &csipbv1.ControllerPublishVolumeResponse{ PublishContext: map[string]string{ "com.hashicorp/nomad-node-id": "foobar", @@ -450,7 +450,7 @@ func TestClient_RPC_ControllerUnpublishVolume(t *testing.T) { }, { Name: "Handles successful response", - Request: &ControllerUnpublishVolumeRequest{VolumeID: "vol", NodeID: "node"}, + Request: &ControllerUnpublishVolumeRequest{ExternalID: "vol", NodeID: "node"}, ExpectedErr: fmt.Errorf("missing NodeID"), ExpectedResponse: &ControllerUnpublishVolumeResponse{}, }, @@ -675,7 +675,7 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) { { Name: "handles underlying grpc errors", Request: &NodePublishVolumeRequest{ - VolumeID: "foo", + ExternalID: "foo", TargetPath: "/dev/null", VolumeCapability: &VolumeCapability{}, }, @@ -685,7 +685,7 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) { { Name: "handles success", Request: &NodePublishVolumeRequest{ - VolumeID: "foo", + ExternalID: "foo", TargetPath: "/dev/null", VolumeCapability: &VolumeCapability{}, }, @@ -695,10 +695,10 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) { { Name: "Performs validation of the publish volume request", Request: &NodePublishVolumeRequest{ - VolumeID: "", + ExternalID: "", }, ResponseErr: nil, - ExpectedErr: errors.New("missing VolumeID"), + ExpectedErr: errors.New("missing volume ID"), }, } @@ -722,7 +722,7 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) { func TestClient_RPC_NodeUnpublishVolume(t *testing.T) { cases := []struct { Name string - VolumeID string + ExternalID string TargetPath string ResponseErr error Response *csipbv1.NodeUnpublishVolumeResponse @@ -730,26 +730,26 @@ func TestClient_RPC_NodeUnpublishVolume(t *testing.T) { }{ { Name: "handles underlying grpc errors", - VolumeID: "foo", + ExternalID: "foo", TargetPath: "/dev/null", ResponseErr: fmt.Errorf("some grpc error"), ExpectedErr: fmt.Errorf("some grpc error"), }, { Name: "handles success", - VolumeID: "foo", + ExternalID: "foo", TargetPath: "/dev/null", ResponseErr: nil, ExpectedErr: nil, }, { - Name: "Performs validation of the request args - VolumeID", + Name: "Performs validation of the request args - ExternalID", ResponseErr: nil, - ExpectedErr: errors.New("missing VolumeID"), + ExpectedErr: errors.New("missing volume ID"), }, { Name: "Performs validation of the request args - TargetPath", - VolumeID: "foo", + ExternalID: "foo", ResponseErr: nil, ExpectedErr: errors.New("missing TargetPath"), }, @@ -763,7 +763,7 @@ func TestClient_RPC_NodeUnpublishVolume(t *testing.T) { nc.NextErr = c.ResponseErr nc.NextUnpublishVolumeResponse = c.Response - err := client.NodeUnpublishVolume(context.TODO(), c.VolumeID, c.TargetPath) + err := client.NodeUnpublishVolume(context.TODO(), c.ExternalID, c.TargetPath) if c.ExpectedErr != nil { require.Error(t, c.ExpectedErr, err) } else { diff --git a/plugins/csi/plugin.go b/plugins/csi/plugin.go index 90b5ead0a..d91df8a1f 100644 --- a/plugins/csi/plugin.go +++ b/plugins/csi/plugin.go @@ -79,8 +79,8 @@ type CSIPlugin interface { } type NodePublishVolumeRequest struct { - // The ID of the volume to publish. - VolumeID string + // The external ID of the volume to publish. + ExternalID string // If the volume was attached via a call to `ControllerPublishVolume` then // we need to provide the returned PublishContext here. @@ -122,7 +122,7 @@ func (r *NodePublishVolumeRequest) ToCSIRepresentation() *csipbv1.NodePublishVol } return &csipbv1.NodePublishVolumeRequest{ - VolumeId: r.VolumeID, + VolumeId: r.ExternalID, PublishContext: r.PublishContext, StagingTargetPath: r.StagingTargetPath, TargetPath: r.TargetPath, @@ -133,8 +133,8 @@ func (r *NodePublishVolumeRequest) ToCSIRepresentation() *csipbv1.NodePublishVol } func (r *NodePublishVolumeRequest) Validate() error { - if r.VolumeID == "" { - return errors.New("missing VolumeID") + if r.ExternalID == "" { + return errors.New("missing volume ID") } if r.TargetPath == "" { @@ -230,7 +230,7 @@ func NewControllerCapabilitySet(resp *csipbv1.ControllerGetCapabilitiesResponse) } type ControllerPublishVolumeRequest struct { - VolumeID string + ExternalID string NodeID string ReadOnly bool VolumeCapability *VolumeCapability @@ -244,7 +244,7 @@ func (r *ControllerPublishVolumeRequest) ToCSIRepresentation() *csipbv1.Controll } return &csipbv1.ControllerPublishVolumeRequest{ - VolumeId: r.VolumeID, + VolumeId: r.ExternalID, NodeId: r.NodeID, Readonly: r.ReadOnly, VolumeCapability: r.VolumeCapability.ToCSIRepresentation(), @@ -254,8 +254,8 @@ func (r *ControllerPublishVolumeRequest) ToCSIRepresentation() *csipbv1.Controll } func (r *ControllerPublishVolumeRequest) Validate() error { - if r.VolumeID == "" { - return errors.New("missing VolumeID") + if r.ExternalID == "" { + return errors.New("missing volume ID") } if r.NodeID == "" { return errors.New("missing NodeID") @@ -268,9 +268,9 @@ type ControllerPublishVolumeResponse struct { } type ControllerUnpublishVolumeRequest struct { - VolumeID string - NodeID string - Secrets structs.CSISecrets + ExternalID string + NodeID string + Secrets structs.CSISecrets } func (r *ControllerUnpublishVolumeRequest) ToCSIRepresentation() *csipbv1.ControllerUnpublishVolumeRequest { @@ -279,15 +279,15 @@ func (r *ControllerUnpublishVolumeRequest) ToCSIRepresentation() *csipbv1.Contro } return &csipbv1.ControllerUnpublishVolumeRequest{ - VolumeId: r.VolumeID, + VolumeId: r.ExternalID, NodeId: r.NodeID, Secrets: r.Secrets, } } func (r *ControllerUnpublishVolumeRequest) Validate() error { - if r.VolumeID == "" { - return errors.New("missing VolumeID") + if r.ExternalID == "" { + return errors.New("missing ExternalID") } if r.NodeID == "" { // the spec allows this but it would unpublish the