Skip to content

Commit

Permalink
driver: validate capabilities for volume creation
Browse files Browse the repository at this point in the history
We don't validate the capabilities during `CreateVolume()`. We should
start doing it to avoid creating volumes that needs to be in different
modes, such as `MULTI_NODE_MULTI_WRITER` (ReadWriteMany in k8s) or
`MULTI_NODE_READER_ONLY` (ReadOnlyMany in k8s)

Note that, there is a
[bug](kubernetes-csi/external-provisioner#117)
in the `external-provisioner` sidecar that passes down the Kubernetes
`accessModes` fields to the CSI plugin. It currently passes a hardcoded
value of `SINGLE_NODE_WRITER` (see:
https://github.com/kubernetes-csi/external-provisioner/blob/4d8a86cc54901d4d14b56c03a6ee7dfe6482c9b1/pkg/controller/controller.go#L89),
so this means currently it's not possible to create volumws with
`ReadWriteMany` or `ReadOnlyMany` nevertheless.

But it's good we have the check in place, so we'de protected once the
issue in the provisioner is fixed in the future.

closes #42
  • Loading branch information
fatih committed Aug 23, 2018
1 parent b73826b commit f1f265d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 36 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
## unreleased

* Validate volume capabilities during volume creation.
[[GH-68]](https://github.com/digitalocean/csi-digitalocean/pull/68)

## v0.1.4 - 2018.08.23

* Add logs to mount operations
Expand Down
81 changes: 45 additions & 36 deletions driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ const (
createdByDO = "Created by DigitalOcean CSI driver"
)

var (
// DO currently only support a single node to be attached to a single node
// in read/write mode. This corresponds to `accessModes.ReadWriteOnce` in a
// PVC resource on Kubernets
supportedAccessMode = &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
}
)

// CreateVolume creates a new volume from the given request. The function is
// idempotent.
func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
Expand All @@ -68,6 +77,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
"volume_name": volumeName,
"storage_size_giga_bytes": size / GB,
"method": "create_volume",
"volume_capabilities": req.VolumeCapabilities,
})
ll.Info("create volume called")

Expand Down Expand Up @@ -107,12 +117,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
SizeGigaBytes: size / GB,
}

ll.WithField("volume_req", volumeReq).Info("creating volume")
if !validateCapabilities(req.VolumeCapabilities) {
return nil, status.Error(codes.AlreadyExists, "invalid volume capabilities requested. Only SINGLE_NODE_WRITER is supported ('accessModes.ReadWriteOnce' on Kubernetes)")
}

// TODO(arslan): Currently DO only supports SINGLE_NODE_WRITER mode. In the
// future, if we support more modes, we need to make sure to create a
// volume that aligns with the incoming capability. We need to make sure to
// test req.VolumeCapabilities
ll.WithField("volume_req", volumeReq).Info("creating volume")
vol, _, err := d.doClient.Storage.CreateVolume(ctx, volumeReq)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
Expand Down Expand Up @@ -319,19 +328,10 @@ func (d *Driver) ValidateVolumeCapabilities(ctx context.Context, req *csi.Valida
return nil, status.Error(codes.InvalidArgument, "ValidateVolumeCapabilities Volume Capabilities must be provided")
}

var vcaps []*csi.VolumeCapability_AccessMode
for _, mode := range []csi.VolumeCapability_AccessMode_Mode{
// DO currently only support a single node to be attached to a single
// node in read/write mode
csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
} {
vcaps = append(vcaps, &csi.VolumeCapability_AccessMode{Mode: mode})
}

ll := d.log.WithFields(logrus.Fields{
"volume_id": req.VolumeId,
"volume_capabilities": req.VolumeCapabilities,
"supported_capabilities": vcaps,
"supported_capabilities": supportedAccessMode,
"method": "validate_volume_capabilities",
})
ll.Info("validate volume capabilities called")
Expand All @@ -345,28 +345,8 @@ func (d *Driver) ValidateVolumeCapabilities(ctx context.Context, req *csi.Valida
return nil, err
}

hasSupport := func(mode csi.VolumeCapability_AccessMode_Mode) bool {
for _, m := range vcaps {
if mode == m.Mode {
return true
}
}
return false
}

resp := &csi.ValidateVolumeCapabilitiesResponse{
Supported: false,
}

for _, cap := range req.VolumeCapabilities {
// cap.AccessMode.Mode
if hasSupport(cap.AccessMode.Mode) {
resp.Supported = true
} else {
// we need to make sure all capabilities are supported. Revert back
// in case we have a cap that is supported, but is invalidated now
resp.Supported = false
}
Supported: validateCapabilities(req.VolumeCapabilities),
}

ll.WithField("response", resp).Info("supported capabilities")
Expand Down Expand Up @@ -555,3 +535,32 @@ func (d *Driver) waitAction(ctx context.Context, volumeId string, actionId int)
}
}
}

// validateCapabilities validates the requested capabilities. It returns false
// if it doesn't satisfy the currently supported modes of DigitalOcean Block
// Storage
func validateCapabilities(caps []*csi.VolumeCapability) bool {
vcaps := []*csi.VolumeCapability_AccessMode{supportedAccessMode}

hasSupport := func(mode csi.VolumeCapability_AccessMode_Mode) bool {
for _, m := range vcaps {
if mode == m.Mode {
return true
}
}
return false
}

supported := false
for _, cap := range caps {
if hasSupport(cap.AccessMode.Mode) {
supported = true
} else {
// we need to make sure all capabilities are supported. Revert back
// in case we have a cap that is supported, but is invalidated now
supported = false
}
}

return supported
}

0 comments on commit f1f265d

Please sign in to comment.