Repository: kubernetes-sigs/gcp-filestore-csi-driver
Pull request #303 (2022-06-07 04:27:39)

Cherrypick #298, #301, #302 to Release 1.2
<!--  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. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. 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?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:

**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 #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Cherrypick #298, #301, #302 to Release 1.2
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/pull/303#" title="Author self-approved">saikat-royc</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fgcp-filestore-csi-driver).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/blob/release-1.2/OWNERS)~~ [saikat-royc]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/CHANGELOG/CHANGELOG-1.2.md b/CHANGELOG/CHANGELOG-1.2.md
index 44d0454b..ee22ba79 100644
--- a/CHANGELOG/CHANGELOG-1.2.md
+++ b/CHANGELOG/CHANGELOG-1.2.md
@@ -1,4 +1,11 @@
 
+# v1.2.4 - Changelog since v1.2.3
+
+### Bug or Regression
+
+- Harden the invalid args checks for multi-share ops and add unit tests ([#298](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/pull/298), [@saikat-royc](https://github.com/saikat-royc))
+- Fix typo in logging error of supported size ([#301](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/pull/301), [@saikat-royc](https://github.com/saikat-royc))
+
 # v1.2.3 - Changelog since v1.2.2
 
 ### Bug or Regression
@@ -7,6 +14,7 @@
 
 - Fail instance eligibility check if any error encountered during checks ([#292](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/pull/291), [@saikat-royc](https://github.com/saikat-royc))
 
+
 # v1.2.2 - Changelog since v1.2.1
 
 ## Changes by Kind
diff --git a/pkg/csi_driver/controller.go b/pkg/csi_driver/controller.go
index bb2ebbf5..e574985d 100644
--- a/pkg/csi_driver/controller.go
+++ b/pkg/csi_driver/controller.go
@@ -726,6 +726,9 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
 	if len(volumeID) == 0 {
 		return nil, status.Error(codes.InvalidArgument, "CreateSnapshot source volume ID must be provided")
 	}
+	if isMultishareVolId(volumeID) {
+		return nil, status.Error(codes.InvalidArgument, "CreateSnapshot is not supported for multishare backed volumes")
+	}
 
 	if acquired := s.config.volumeLocks.TryAcquire(volumeID); !acquired {
 		return nil, status.Errorf(codes.Aborted, util.VolumeOperationAlreadyExistsFmt, volumeID)
diff --git a/pkg/csi_driver/multishare_controller.go b/pkg/csi_driver/multishare_controller.go
index c334b7e6..af8a2ed4 100644
--- a/pkg/csi_driver/multishare_controller.go
+++ b/pkg/csi_driver/multishare_controller.go
@@ -63,6 +63,7 @@ func NewMultishareController(driver *GCFSDriver, fileService file.Service, cloud
 }
 
 func (m *MultishareController) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
+	klog.Infof("CreateVolume called for multishare with request %+v", req)
 	name := req.GetName()
 	if len(name) == 0 {
 		return nil, status.Error(codes.InvalidArgument, "CreateVolume name must be provided")
@@ -70,13 +71,19 @@ func (m *MultishareController) CreateVolume(ctx context.Context, req *csi.Create
 	if err := m.driver.validateVolumeCapabilities(req.GetVolumeCapabilities()); err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
+	if req.GetVolumeContentSource() != nil {
+		return nil, status.Error(codes.InvalidArgument, "Multishare backed volumes do not support volume content source")
+	}
 
 	instanceScPrefix, err := getInstanceSCPrefix(req)
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 
-	klog.Infof("CreateVolume called for multishare with request %+v", req)
+	_, err = getShareRequestCapacity(req.GetCapacityRange())
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
 	if acquired := m.volumeLocks.TryAcquire(name); !acquired {
 		return nil, status.Errorf(codes.Aborted, util.VolumeOperationAlreadyExistsFmt, name)
 	}
@@ -543,11 +550,11 @@ func getShareRequestCapacity(capRange *csi.CapacityRange) (int64, error) {
 
 	if rSet {
 		if rCap < util.MinShareSizeBytes {
-			return 0, status.Errorf(codes.InvalidArgument, "Request bytes %v is less than minimum share size bytes %v", lCap, util.MinShareSizeBytes)
+			return 0, status.Errorf(codes.InvalidArgument, "Request bytes %v is less than minimum share size bytes %v", rCap, util.MinShareSizeBytes)
 		}
 
 		if rCap > util.MaxShareSizeBytes {
-			return 0, status.Errorf(codes.InvalidArgument, "Request bytes %v is greater than maximum share size bytes %v", lCap, util.MaxShareSizeBytes)
+			return 0, status.Errorf(codes.InvalidArgument, "Request bytes %v is greater than maximum share size bytes %v", rCap, util.MaxShareSizeBytes)
 		}
 	}
 


Test patch:
diff --git a/pkg/csi_driver/controller_test.go b/pkg/csi_driver/controller_test.go
index 51e0af10..afaaa085 100644
--- a/pkg/csi_driver/controller_test.go
+++ b/pkg/csi_driver/controller_test.go
@@ -881,3 +881,29 @@ func ValidateExpectedError(t *testing.T, errResp <-chan error, operationUnblocke
 		t.Errorf("The operation should have been aborted, but was started")
 	}
 }
+
+func TestCreateSnapshot(t *testing.T) {
+	cases := []struct {
+		name      string
+		req       *csi.CreateSnapshotRequest
+		expectErr bool
+	}{
+		{
+			name: "Create snapshot request for multishare backed volumes",
+			req: &csi.CreateSnapshotRequest{
+				SourceVolumeId: "modeMultishare/mysc/myproject/location/instancename/sharename",
+			},
+			expectErr: true,
+		},
+	}
+	for _, test := range cases {
+		cs := initTestController(t)
+		_, err := cs.CreateSnapshot(context.TODO(), test.req)
+		if !test.expectErr && err != nil {
+			t.Errorf("test %q failed: %v", test.name, err)
+		}
+		if test.expectErr && err == nil {
+			t.Errorf("test %q failed; got success", test.name)
+		}
+	}
+}
diff --git a/pkg/csi_driver/multishare_controller_test.go b/pkg/csi_driver/multishare_controller_test.go
index d22e594c..b457c9d7 100644
--- a/pkg/csi_driver/multishare_controller_test.go
+++ b/pkg/csi_driver/multishare_controller_test.go
@@ -611,6 +611,122 @@ func TestMultishareCreateVolume(t *testing.T) {
 		errorExpected     bool
 		checkOnlyVolidFmt bool // for auto generated instance, the instance name is not known
 	}{
+		{
+			name: "create volume called with volume content source",
+			req: &csi.CreateVolumeRequest{
+				Name: testVolName,
+				CapacityRange: &csi.CapacityRange{
+					RequiredBytes: 100 * util.Gb,
+				},
+				Parameters: map[string]string{
+					paramMultishareInstanceScLabel: testInstanceScPrefix,
+				},
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+						},
+					},
+				},
+				VolumeContentSource: &csi.VolumeContentSource{},
+			},
+			errorExpected: true,
+		},
+		{
+			name: "create volume called with volume size < 100G in required bytes",
+			req: &csi.CreateVolumeRequest{
+				Name: testVolName,
+				CapacityRange: &csi.CapacityRange{
+					RequiredBytes: 99 * util.Gb,
+				},
+				Parameters: map[string]string{
+					paramMultishareInstanceScLabel: testInstanceScPrefix,
+				},
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+						},
+					},
+				},
+			},
+			errorExpected: true,
+		},
+		{
+			name: "create volume called with volume size < 100G in limit bytes",
+			req: &csi.CreateVolumeRequest{
+				Name: testVolName,
+				CapacityRange: &csi.CapacityRange{
+					LimitBytes: 99 * util.Gb,
+				},
+				Parameters: map[string]string{
+					paramMultishareInstanceScLabel: testInstanceScPrefix,
+				},
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+						},
+					},
+				},
+			},
+			errorExpected: true,
+		},
+		{
+			name: "create volume called with volume size > 1T in required bytes",
+			req: &csi.CreateVolumeRequest{
+				Name: testVolName,
+				CapacityRange: &csi.CapacityRange{
+					RequiredBytes: 2 * util.Tb,
+				},
+				Parameters: map[string]string{
+					paramMultishareInstanceScLabel: testInstanceScPrefix,
+				},
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+						},
+					},
+				},
+			},
+			errorExpected: true,
+		},
+		{
+			name: "create volume called with volume size > 1T in limit bytes",
+			req: &csi.CreateVolumeRequest{
+				Name: testVolName,
+				CapacityRange: &csi.CapacityRange{
+					LimitBytes: 2 * util.Tb,
+				},
+				Parameters: map[string]string{
+					paramMultishareInstanceScLabel: testInstanceScPrefix,
+				},
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+						},
+					},
+				},
+			},
+			errorExpected: true,
+		},
 		{
 			name: "no initial instances, create instance and share, success response",
 			req: &csi.CreateVolumeRequest{
@@ -885,7 +1001,7 @@ func TestMultishareDeleteVolume(t *testing.T) {
 		errorExpected bool
 	}{
 		{
-			name: "share not found, instance ready, instance deleted, succes response",
+			name: "share not found, instance not found, succes response",
 			req: &csi.DeleteVolumeRequest{
 				VolumeId: testVolId,
 			},
@@ -1079,6 +1195,22 @@ func TestMultishareControllerExpandVolume(t *testing.T) {
 		resp          *csi.ControllerExpandVolumeResponse
 		errorExpected bool
 	}{
+		{
+			name: "Target expansion < 100G in required bytes",
+			req: &csi.ControllerExpandVolumeRequest{
+				VolumeId:      testVolId,
+				CapacityRange: &csi.CapacityRange{RequiredBytes: 99 * util.Gb},
+			},
+			errorExpected: true,
+		},
+		{
+			name: "Target expansion > 1T in required bytes",
+			req: &csi.ControllerExpandVolumeRequest{
+				VolumeId:      testVolId,
+				CapacityRange: &csi.CapacityRange{RequiredBytes: 2 * util.Tb},
+			},
+			errorExpected: true,
+		},
 		{
 			name: "share not found, error respond",
 			req: &csi.ControllerExpandVolumeRequest{