Repository: kubernetes-csi/csi-driver-smb
Pull request #449 (2022-04-09 07:51:17)

fix: disallow block volume capability
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
fix: disallow block volume capability
should return error for block volume capability

**Which issue(s) this PR fixes**:
<!-- 
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Requirements**:
- [ ] uses [conventional commit messages](https://www.conventionalcommits.org/)
  <!-- Common commit types:
        build: Build 🏭
        chore: Maintenance 🔧
        ci: Continuous Integration 💜
        docs: Documentation 📘
        feat: Features 🌈
        fix: Bug Fixes 🐞
        perf: Performance Improvements 🚀
        refactor: Code Refactoring 💎
        revert: Revert Change ◀️
        style: Code Style 🎶
        security: Security Fix 🛡️
        test: Testing 💚 -->
- [ ] includes documentation
- [ ] adds unit tests
- [ ] tested upgrade from previous version

**Special notes for your reviewer**:


**Release note**:
```
fix: disallow block volume capability
```



Patch:
diff --git a/pkg/smb/controllerserver.go b/pkg/smb/controllerserver.go
index 619726265..9ab0c3980 100644
--- a/pkg/smb/controllerserver.go
+++ b/pkg/smb/controllerserver.go
@@ -63,13 +63,9 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		return nil, status.Error(codes.InvalidArgument, "CreateVolume name must be provided")
 	}
 
-	var volCap *csi.VolumeCapability
 	volumeCapabilities := req.GetVolumeCapabilities()
-	if len(volumeCapabilities) == 0 {
-		return nil, status.Error(codes.InvalidArgument, "CreateVolume Volume capabilities must be provided")
-	}
-	if len(volumeCapabilities) > 0 {
-		volCap = req.GetVolumeCapabilities()[0]
+	if err := isValidVolumeCapabilities(volumeCapabilities); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 
 	reqCapacity := req.GetCapacityRange().GetRequiredBytes()
@@ -88,7 +84,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 			klog.V(2).Infof("existing subDir(%s) is provided, skip subdirectory creation", smbVol.subDir)
 		} else {
 			// Mount smb base share so we can create a subdirectory
-			if err := d.internalMount(ctx, smbVol, volCap, secrets); err != nil {
+			if err := d.internalMount(ctx, smbVol, volumeCapabilities[0], secrets); err != nil {
 				return nil, status.Errorf(codes.Internal, "failed to mount smb server: %v", err.Error())
 			}
 			defer func() {
@@ -189,11 +185,10 @@ func (d *Driver) ValidateVolumeCapabilities(ctx context.Context, req *csi.Valida
 	if len(req.GetVolumeId()) == 0 {
 		return nil, status.Error(codes.InvalidArgument, "Volume ID missing in request")
 	}
-	if req.GetVolumeCapabilities() == nil {
-		return nil, status.Error(codes.InvalidArgument, "Volume capabilities missing in request")
+	if err := isValidVolumeCapabilities(req.GetVolumeCapabilities()); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 
-	// supports all AccessModes, no need to check capabilities here
 	return &csi.ValidateVolumeCapabilitiesResponse{
 		Confirmed: &csi.ValidateVolumeCapabilitiesResponse_Confirmed{
 			VolumeCapabilities: req.GetVolumeCapabilities(),
@@ -357,3 +352,16 @@ func getSmbVolFromID(id string) (*smbVolume, error) {
 	}
 	return vol, nil
 }
+
+// isValidVolumeCapabilities validates the given VolumeCapability array is valid
+func isValidVolumeCapabilities(volCaps []*csi.VolumeCapability) error {
+	if len(volCaps) == 0 {
+		return fmt.Errorf("volume capabilities missing in request")
+	}
+	for _, c := range volCaps {
+		if c.GetBlock() != nil {
+			return fmt.Errorf("block volume capability not supported")
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/pkg/smb/controllerserver_test.go b/pkg/smb/controllerserver_test.go
index 91daae0c2..7262b978a 100644
--- a/pkg/smb/controllerserver_test.go
+++ b/pkg/smb/controllerserver_test.go
@@ -56,6 +56,14 @@ func TestControllerGetCapabilities(t *testing.T) {
 func TestCreateVolume(t *testing.T) {
 	d := NewFakeDriver()
 
+	blockVolCap := []*csi.VolumeCapability{
+		{
+			AccessType: &csi.VolumeCapability_Block{
+				Block: &csi.VolumeCapability_BlockVolume{},
+			},
+		},
+	}
+
 	// Setup workingMountDir
 	workingMountDir, err := os.Getwd()
 	if err != nil {
@@ -134,6 +142,20 @@ func TestCreateVolume(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			name: "Volume capabilities missing",
+			req: &csi.CreateVolumeRequest{
+				VolumeCapabilities: []*csi.VolumeCapability{},
+			},
+			expectErr: true,
+		},
+		{
+			name: "block volume capability not supported",
+			req: &csi.CreateVolumeRequest{
+				VolumeCapabilities: blockVolCap,
+			},
+			expectErr: true,
+		},
 		{
 			name: "invalid create context",
 			req: &csi.CreateVolumeRequest{
@@ -269,7 +291,7 @@ func TestDeleteVolume(t *testing.T) {
 
 func TestValidateVolumeCapabilities(t *testing.T) {
 	d := NewFakeDriver()
-	stdVolCap := []*csi.VolumeCapability{
+	mountVolCap := []*csi.VolumeCapability{
 		{
 			AccessType: &csi.VolumeCapability_Mount{
 				Mount: &csi.VolumeCapability_MountVolume{},
@@ -280,6 +302,14 @@ func TestValidateVolumeCapabilities(t *testing.T) {
 		},
 	}
 
+	blockVolCap := []*csi.VolumeCapability{
+		{
+			AccessType: &csi.VolumeCapability_Block{
+				Block: &csi.VolumeCapability_BlockVolume{},
+			},
+		},
+	}
+
 	tests := []struct {
 		desc        string
 		req         csi.ValidateVolumeCapabilitiesRequest
@@ -293,13 +323,21 @@ func TestValidateVolumeCapabilities(t *testing.T) {
 		{
 			desc:        "Volume capabilities missing",
 			req:         csi.ValidateVolumeCapabilitiesRequest{VolumeId: "vol_1"},
-			expectedErr: status.Error(codes.InvalidArgument, "Volume capabilities missing in request"),
+			expectedErr: status.Error(codes.InvalidArgument, "volume capabilities missing in request"),
+		},
+		{
+			desc: "block volume capability not supported",
+			req: csi.ValidateVolumeCapabilitiesRequest{
+				VolumeId:           "vol_1",
+				VolumeCapabilities: blockVolCap,
+			},
+			expectedErr: status.Error(codes.InvalidArgument, "block volume capability not supported"),
 		},
 		{
 			desc: "Valid request",
 			req: csi.ValidateVolumeCapabilitiesRequest{
 				VolumeId:           "vol_1#f5713de20cde511e8ba4900#fileshare#diskname#",
-				VolumeCapabilities: stdVolCap,
+				VolumeCapabilities: mountVolCap,
 			},
 			expectedErr: nil,
 		},
@@ -308,7 +346,7 @@ func TestValidateVolumeCapabilities(t *testing.T) {
 	for _, test := range tests {
 		_, err := d.ValidateVolumeCapabilities(context.Background(), &test.req)
 		if !reflect.DeepEqual(err, test.expectedErr) {
-			t.Errorf("Unexpected error: %v", err)
+			t.Errorf("[test: %s] Unexpected error: %v, expected error: %v", test.desc, err, test.expectedErr)
 		}
 	}
 }
@@ -568,3 +606,46 @@ func TestNewSMBVolume(t *testing.T) {
 		}
 	}
 }
+
+func TestIsValidVolumeCapabilities(t *testing.T) {
+	mountVolumeCapabilities := []*csi.VolumeCapability{
+		{
+			AccessType: &csi.VolumeCapability_Mount{
+				Mount: &csi.VolumeCapability_MountVolume{},
+			},
+		},
+	}
+	blockVolumeCapabilities := []*csi.VolumeCapability{
+		{
+			AccessType: &csi.VolumeCapability_Block{
+				Block: &csi.VolumeCapability_BlockVolume{},
+			},
+		},
+	}
+
+	cases := []struct {
+		desc      string
+		volCaps   []*csi.VolumeCapability
+		expectErr error
+	}{
+		{
+			volCaps:   mountVolumeCapabilities,
+			expectErr: nil,
+		},
+		{
+			volCaps:   blockVolumeCapabilities,
+			expectErr: fmt.Errorf("block volume capability not supported"),
+		},
+		{
+			volCaps:   []*csi.VolumeCapability{},
+			expectErr: fmt.Errorf("volume capabilities missing in request"),
+		},
+	}
+
+	for _, test := range cases {
+		err := isValidVolumeCapabilities(test.volCaps)
+		if !reflect.DeepEqual(err, test.expectErr) {
+			t.Errorf("[test: %s] Unexpected error: %v, expected error: %v", test.desc, err, test.expectErr)
+		}
+	}
+}
diff --git a/test/external-e2e/run.sh b/test/external-e2e/run.sh
index 63fde8b4f..57da3f051 100755
--- a/test/external-e2e/run.sh
+++ b/test/external-e2e/run.sh
@@ -52,6 +52,6 @@ trap print_logs EXIT
 mkdir -p /tmp/csi
 cp deploy/example/storageclass-smb.yaml /tmp/csi/storageclass.yaml
 ginkgo -p --progress --v -focus='External.Storage' \
-       -skip='\[Disruptive\]|\[Slow\]|support two pods which share the same volume|volume contents ownership changed' kubernetes/test/bin/e2e.test  -- \
+       -skip='\[Disruptive\]|support two pods which share the same volume|volume contents ownership changed' kubernetes/test/bin/e2e.test  -- \
        -storage.testdriver=$PROJECT_ROOT/test/external-e2e/testdriver.yaml \
        --kubeconfig=$KUBECONFIG
diff --git a/test/integration/run-test.sh b/test/integration/run-test.sh
index 0f60b3275..f0d702e5e 100755
--- a/test/integration/run-test.sh
+++ b/test/integration/run-test.sh
@@ -56,7 +56,7 @@ sleep 5
 params='source=//0.0.0.0/share'
 # Begin to run CSI functions one by one
 echo 'Create volume test:'
-value=$("$CSC_BIN" controller new --endpoint "$endpoint" --cap 1,block "$volname" --req-bytes 2147483648 --params "$params")
+value=$("$CSC_BIN" controller new --endpoint "$endpoint" --cap MULTI_NODE_MULTI_WRITER,mount,xfs "$volname" --req-bytes 2147483648 --params "$params")
 sleep 2
 
 volumeid=$(echo "$value" | awk '{print $1}' | sed 's/"//g')
@@ -66,14 +66,14 @@ echo "Got volume id: $volumeid"
 export X_CSI_SECRETS=username=username,"password=test"
 
 echo "stage volume test:"
-"$CSC_BIN" node stage --endpoint "$endpoint" --cap 1,block  --vol-context=source="//0.0.0.0/share" --staging-target-path "$staging_target_path" "$volumeid"
+"$CSC_BIN" node stage --endpoint "$endpoint" --cap MULTI_NODE_MULTI_WRITER,mount,xfs  --vol-context=source="//0.0.0.0/share" --staging-target-path "$staging_target_path" "$volumeid"
 sleep 2
 
 # check cifs mount
 mount | grep cifs
 
 echo 'Mount volume test:'
-"$CSC_BIN" node publish --endpoint "$endpoint" --cap 1,block --staging-target-path "$staging_target_path" --target-path "$target_path" "$volumeid"
+"$CSC_BIN" node publish --endpoint "$endpoint" --cap MULTI_NODE_MULTI_WRITER,mount,xfs --staging-target-path "$staging_target_path" --target-path "$target_path" "$volumeid"
 sleep 2
 
 echo "node stats test:"