From 4ecddab72f6485e9216c7d240dd259f048dfaf90 Mon Sep 17 00:00:00 2001
From: Christian Huffman <chuffman@redhat.com>
Date: Wed, 23 Sep 2020 08:56:22 -0400
Subject: [PATCH] UPSTREAM: 550: Clarify error message when unsupported volume
 capabilities are provided

---
 pkg/driver/controller.go      | 10 ++++++--
 pkg/driver/controller_test.go | 46 +++++++++++++++++++++++++++++++++++
 pkg/util/util.go              | 12 +++++++++
 pkg/util/util_test.go         | 26 ++++++++++++++++++++
 4 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go
index 97a1888b28..1e37dc0003 100644
--- a/pkg/driver/controller.go
+++ b/pkg/driver/controller.go
@@ -107,7 +107,10 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 	}
 
 	if !isValidVolumeCapabilities(volCaps) {
-		return nil, status.Error(codes.InvalidArgument, "Volume capabilities not supported")
+		modes := util.GetAccessModes(volCaps)
+		stringModes := strings.Join(*modes, ", ")
+		errString := "Volume capabilities " + stringModes + " not supported. Only AccessModes[ReadWriteOnce] supported."
+		return nil, status.Error(codes.InvalidArgument, errString)
 	}
 
 	disk, err := d.cloud.GetDiskByName(ctx, volName, volSizeBytes)
@@ -256,7 +259,10 @@ func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *cs
 
 	caps := []*csi.VolumeCapability{volCap}
 	if !isValidVolumeCapabilities(caps) {
-		return nil, status.Error(codes.InvalidArgument, "Volume capability not supported")
+		modes := util.GetAccessModes(caps)
+		stringModes := strings.Join(*modes, ", ")
+		errString := "Volume capabilities " + stringModes + " not supported. Only AccessModes[ReadWriteOnce] supported."
+		return nil, status.Error(codes.InvalidArgument, errString)
 	}
 
 	if !d.cloud.IsExistInstance(ctx, nodeID) {
diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
index c1bf8df96b..a35f7ab8da 100644
--- a/pkg/driver/controller_test.go
+++ b/pkg/driver/controller_test.go
@@ -152,6 +152,13 @@ func TestCreateVolume(t *testing.T) {
 			},
 		},
 	}
+	invalidVolCap := []*csi.VolumeCapability{
+		{
+			AccessMode: &csi.VolumeCapability_AccessMode{
+				Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_SINGLE_WRITER,
+			},
+		},
+	}
 	stdVolSize := int64(5 * 1024 * 1024 * 1024)
 	stdCapRange := &csi.CapacityRange{RequiredBytes: stdVolSize}
 	stdParams := map[string]string{}
@@ -1181,6 +1188,45 @@ func TestCreateVolume(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "fail with invalid volume access modes",
+			testFunc: func(t *testing.T) {
+				req := &csi.CreateVolumeRequest{
+					Name:               "vol-test",
+					CapacityRange:      stdCapRange,
+					VolumeCapabilities: invalidVolCap,
+					Parameters: map[string]string{
+						VolumeTypeKey: cloud.VolumeTypeIO1,
+						"unknownKey":  "unknownValue",
+					},
+				}
+
+				ctx := context.Background()
+
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				mockCloud := mocks.NewMockCloud(mockCtl)
+
+				awsDriver := controllerService{
+					cloud:         mockCloud,
+					driverOptions: &DriverOptions{},
+				}
+
+				_, err := awsDriver.CreateVolume(ctx, req)
+				if err == nil {
+					t.Fatalf("Expected CreateVolume to fail but got no error")
+				}
+
+				srvErr, ok := status.FromError(err)
+				if !ok {
+					t.Fatalf("Could not get error status code from error: %v", srvErr)
+				}
+				if srvErr.Code() != codes.InvalidArgument {
+					t.Fatalf("Expect InvalidArgument but got: %s", srvErr.Code())
+				}
+			},
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/pkg/util/util.go b/pkg/util/util.go
index 7a51a90b10..7e5b79a3fa 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -23,6 +23,8 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
+	csi "github.com/container-storage-interface/spec/lib/go/csi"
 )
 
 const (
@@ -78,3 +80,13 @@ func ParseEndpoint(endpoint string) (string, string, error) {
 func roundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 {
 	return (volumeSizeBytes + allocationUnitBytes - 1) / allocationUnitBytes
 }
+
+// GetAccessModes returns a slice containing all of the access modes defined
+// in the passed in VolumeCapabilities.
+func GetAccessModes(caps []*csi.VolumeCapability) *[]string {
+	modes := []string{}
+	for _, c := range caps {
+		modes = append(modes, c.AccessMode.GetMode().String())
+	}
+	return &modes
+}
diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
index e560e5f895..fd285d6415 100644
--- a/pkg/util/util_test.go
+++ b/pkg/util/util_test.go
@@ -18,7 +18,10 @@ package util
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
+
+	csi "github.com/container-storage-interface/spec/lib/go/csi"
 )
 
 func TestRoundUpBytes(t *testing.T) {
@@ -125,3 +128,26 @@ func TestParseEndpoint(t *testing.T) {
 	}
 
 }
+
+func TestGetAccessModes(t *testing.T) {
+	testVolCap := []*csi.VolumeCapability{
+		{
+			AccessMode: &csi.VolumeCapability_AccessMode{
+				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+			},
+		},
+		{
+			AccessMode: &csi.VolumeCapability_AccessMode{
+				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_READER_ONLY,
+			},
+		},
+	}
+	expectedModes := []string{
+		"SINGLE_NODE_WRITER",
+		"SINGLE_NODE_READER_ONLY",
+	}
+	actualModes := GetAccessModes(testVolCap)
+	if !reflect.DeepEqual(expectedModes, *actualModes) {
+		t.Fatalf("Wrong values returned for volume capabilities. Expected %v, got %v", expectedModes, actualModes)
+	}
+}