Skip to content

Commit

Permalink
csi: refactor internal client field name to ExternalID (#7958)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tgross
tgross authored May 14, 2020
1 parent 8997286 commit c514a55
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 34 deletions.
2 changes: 1 addition & 1 deletion client/pluginmanager/csimanager/volume.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions client/structs/csi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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

Expand All @@ -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,
}
}

Expand Down
28 changes: 14 additions & 14 deletions plugins/csi/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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{},
},
Expand Down Expand Up @@ -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{},
},
Expand All @@ -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{},
},
Expand All @@ -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"),
},
}

Expand All @@ -722,34 +722,34 @@ 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
ExpectedErr error
}{
{
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"),
},
Expand All @@ -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 {
Expand Down
30 changes: 15 additions & 15 deletions plugins/csi/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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 == "" {
Expand Down Expand Up @@ -230,7 +230,7 @@ func NewControllerCapabilitySet(resp *csipbv1.ControllerGetCapabilitiesResponse)
}

type ControllerPublishVolumeRequest struct {
VolumeID string
ExternalID string
NodeID string
ReadOnly bool
VolumeCapability *VolumeCapability
Expand All @@ -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(),
Expand All @@ -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")
Expand All @@ -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 {
Expand All @@ -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
Expand Down

0 comments on commit c514a55

Please sign in to comment.