diff --git a/plugins/csi/client.go b/plugins/csi/client.go index bfff2848d508..d9a19b20eb26 100644 --- a/plugins/csi/client.go +++ b/plugins/csi/client.go @@ -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: @@ -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 AccessMode %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 } @@ -442,6 +454,7 @@ NEXT_CAP: continue NEXT_CAP } } + return nil } return err.ErrorOrNil() diff --git a/plugins/csi/client_test.go b/plugins/csi/client_test.go index 265d234b83a9..5b846f2ed19b 100644 --- a/plugins/csi/client_test.go +++ b/plugins/csi/client_test.go @@ -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{}, @@ -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{}, @@ -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{}, @@ -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 { @@ -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"},