Skip to content

Commit

Permalink
csi: loosen ValidateVolumeCapability requirements
Browse files Browse the repository at this point in the history 
The CSI specification for `ValidateVolumeCapability` says that we shall
"reconcile successful capability-validation responses by comparing the
validated capabilities with those that it had originally requested" but leaves
the details of that reconcilation unspecified. This API is not implemented in
Kubernetes, so controller plugins don't have a real-world implementation to
verify their behavior against.

We have found that CSI plugins in the wild may return "successful" but
incomplete `VolumeCapability` responses, so we can't require that all
capabilities we expect have been validated, only that the ones that have been
validated match. This appears to violate the CSI specification but until
that's been resolved in upstream we have to loosen our validation
requirements. The tradeoff is that we're more likely to have runtime errors
during `NodeStageVolume` instead of at the time of volume registration.
  • Loading branch information
@tgross
tgross committed Oct 8, 2020
1 parent 024e27e commit 1fdb3f6
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 23 deletions.
47 changes: 30 additions & 17 deletions plugins/csi/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,8 +383,16 @@ func (c *client) ControllerValidateCapabilities(ctx context.Context, req *Contro
return nil
}

// compareCapabilities returns an error if the 'got' capabilities does not
// contain the 'expected' capability
// compareCapabilities returns an error if the 'got' capabilities aren't found
// within the 'expected' capability.
//
// Note that plugins in the wild are known to return incomplete
// VolumeCapability responses, so we can't require that all capabilities we
// expect have been validated, only that the ones that have been validated
// match. This appears to violate the CSI specification but until that's been
// resolved in upstream we have to loosen our validation requirements. The
// tradeoff is that we're more likely to have runtime errors during
// NodeStageVolume.
func compareCapabilities(expected *csipbv1.VolumeCapability, got []*csipbv1.VolumeCapability) error {
var err multierror.Error
NEXT_CAP:
Expand All @@ -393,36 +401,40 @@ NEXT_CAP:
expectedMode := expected.GetAccessMode().GetMode()
capMode := cap.GetAccessMode().GetMode()

if expectedMode != capMode {
multierror.Append(&err,
fmt.Errorf("requested AccessMode %v, got %v", expectedMode, capMode))
continue NEXT_CAP
// The plugin may not validate AccessMode, in which case we'll
// get UNKNOWN as our response
if capMode != csipbv1.VolumeCapability_AccessMode_UNKNOWN {
if expectedMode != capMode {
multierror.Append(&err,
fmt.Errorf("requested access mode %v, got %v", expectedMode, capMode))
continue NEXT_CAP
}
}

// AccessType Block is an empty struct even if set, so the
// only way to test for it is to check that the AccessType
// isn't Mount.
expectedMount := expected.GetMount()
capBlock := cap.GetBlock()
capMount := cap.GetMount()
expectedBlock := expected.GetBlock()
expectedMount := expected.GetMount()

if expectedMount == nil {
if capMount == nil {
return nil
}
if capBlock != nil && expectedBlock == nil {
multierror.Append(&err, fmt.Errorf(
"requested AccessType Block but got AccessType Mount"))
"'block-device' access type was not requested but was validated by the controller"))
continue NEXT_CAP
}

if capMount == nil {
continue NEXT_CAP
}

if capMount != nil && expectedMount == nil {
multierror.Append(&err, fmt.Errorf(
"requested AccessType Mount but got AccessType Block"))
"'file-system' access type was not requested but was validated by the controller"))
continue NEXT_CAP
}

if expectedMount.FsType != capMount.FsType {
multierror.Append(&err, fmt.Errorf(
"requested AccessType mount filesystem type %v, got %v",
"requested filesystem type %v, got %v",
expectedMount.FsType, capMount.FsType))
continue NEXT_CAP
}
Expand All @@ -442,6 +454,7 @@ NEXT_CAP:
continue NEXT_CAP
}
}

return nil
}
return err.ErrorOrNil()
Expand Down
112 changes: 106 additions & 6 deletions plugins/csi/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,23 +480,31 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) {

cases := []struct {
Name string
AccessType VolumeAccessType
AccessMode VolumeAccessMode
ResponseErr error
Response *csipbv1.ValidateVolumeCapabilitiesResponse
ExpectedErr error
}{
{
Name: "handles underlying grpc errors",
AccessType: VolumeAccessTypeMount,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
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",
Name: "handles success empty capabilities",
AccessType: VolumeAccessTypeMount,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{},
ResponseErr: nil,
ExpectedErr: nil,
},
{
Name: "handles validate success",
Name: "handles success exact match MountVolume",
AccessType: VolumeAccessTypeMount,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeContext: map[string]string{},
Expand All @@ -518,8 +526,33 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) {
ResponseErr: nil,
ExpectedErr: nil,
},

{
Name: "handles success exact match BlockVolume",
AccessType: VolumeAccessTypeBlock,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeCapabilities: []*csipbv1.VolumeCapability{
{
AccessType: &csipbv1.VolumeCapability_Block{
Block: &csipbv1.VolumeCapability_BlockVolume{},
},

AccessMode: &csipbv1.VolumeCapability_AccessMode{
Mode: csipbv1.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
},
},
},
},
},
ResponseErr: nil,
ExpectedErr: nil,
},

{
Name: "handles validation failure block mismatch",
Name: "handles failure AccessMode mismatch",
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeContext: map[string]string{},
Expand All @@ -541,7 +574,9 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) {
},

{
Name: "handles validation failure mount flags",
Name: "handles failure MountFlags mismatch",
AccessType: VolumeAccessTypeMount,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeContext: map[string]string{},
Expand All @@ -564,6 +599,71 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) {
// 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"),
},

{
Name: "handles failure MountFlags with Block",
AccessType: VolumeAccessTypeBlock,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeContext: map[string]string{},
VolumeCapabilities: []*csipbv1.VolumeCapability{
{
AccessType: &csipbv1.VolumeCapability_Mount{
Mount: &csipbv1.VolumeCapability_MountVolume{
FsType: "ext4",
MountFlags: []string{},
},
},
AccessMode: &csipbv1.VolumeCapability_AccessMode{
Mode: csipbv1.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
},
},
},
},
},
ResponseErr: nil,
// this is a multierror
ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* 'file-system' access type was not requested but was validated by the controller\n\n"),
},

{
Name: "handles success incomplete no AccessType",
AccessType: VolumeAccessTypeMount,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeCapabilities: []*csipbv1.VolumeCapability{
{
AccessMode: &csipbv1.VolumeCapability_AccessMode{
Mode: csipbv1.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
},
},
},
},
},
ResponseErr: nil,
ExpectedErr: nil,
},

{
Name: "handles success incomplete no AccessMode",
AccessType: VolumeAccessTypeBlock,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
Response: &csipbv1.ValidateVolumeCapabilitiesResponse{
Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{
VolumeCapabilities: []*csipbv1.VolumeCapability{
{
AccessType: &csipbv1.VolumeCapability_Block{
Block: &csipbv1.VolumeCapability_BlockVolume{},
},
},
},
},
},
ResponseErr: nil,
ExpectedErr: nil,
},
}

for _, tc := range cases {
Expand All @@ -572,8 +672,8 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) {
defer client.Close()

requestedCaps := &VolumeCapability{
AccessType: VolumeAccessTypeMount,
AccessMode: VolumeAccessModeMultiNodeMultiWriter,
AccessType: tc.AccessType,
AccessMode: tc.AccessMode,
MountVolume: &structs.CSIMountOptions{ // should be ignored
FSType: "ext4",
MountFlags: []string{"noatime", "errors=remount-ro"},
Expand Down

0 comments on commit 1fdb3f6

Please sign in to comment.