Merge pull request #265 from andyzhangx/change-volumeid-format

feat: change volume id format

pkg/nfs/controllerserver.go

@@ -66,6 +66,8 @@ const (
 	totalIDElements // Always last
 )
 
+const separator = "#"
+
 // CreateVolume create a volume
 func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
 	name := req.GetName()
@@ -115,7 +117,7 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 	if volumeID == "" {
 		return nil, status.Error(codes.InvalidArgument, "volume id is empty")
 	}
-	nfsVol, err := cs.getNfsVolFromID(volumeID)
+	nfsVol, err := getNfsVolFromID(volumeID)
 	if err != nil {
 		// An invalid ID should be treated as doesn't exist
 		klog.Warningf("failed to get nfs volume for volume id %v deletion: %v", volumeID, err)
@@ -260,7 +262,7 @@ func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, v
 		}
 	}
 
-	klog.V(4).Infof("internally mounting %v:%v at %v", vol.server, sharePath, targetPath)
+	klog.V(2).Infof("internally mounting %v:%v at %v", vol.server, sharePath, targetPath)
 	_, err := cs.Driver.ns.NodePublishVolume(ctx, &csi.NodePublishVolumeRequest{
 		TargetPath: targetPath,
 		VolumeContext: map[string]string{
@@ -359,21 +361,37 @@ func (cs *ControllerServer) getVolumeIDFromNfsVol(vol *nfsVolume) string {
 	idElements[idServer] = strings.Trim(vol.server, "/")
 	idElements[idBaseDir] = strings.Trim(vol.baseDir, "/")
 	idElements[idSubDir] = strings.Trim(vol.subDir, "/")
-	return strings.Join(idElements, "/")
+	return strings.Join(idElements, separator)
 }
 
 // Given a CSI volume id, return a nfsVolume
-func (cs *ControllerServer) getNfsVolFromID(id string) (*nfsVolume, error) {
+// sample volume Id:
-	volRegex := regexp.MustCompile("^([^/]+)/(.*)/([^/]+)$")
+//   new volumeID: nfs-server.default.svc.cluster.local#share#pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64
-	tokens := volRegex.FindStringSubmatch(id)
+//   old volumeID: nfs-server.default.svc.cluster.local/share/pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64
-	if tokens == nil {
+func getNfsVolFromID(id string) (*nfsVolume, error) {
-		return nil, fmt.Errorf("Could not split %q into server, baseDir and subDir", id)
+	var server, baseDir, subDir string
+	segments := strings.Split(id, separator)
+	if len(segments) < 3 {
+		klog.V(2).Infof("could not split %s into server, baseDir and subDir with separator(%s)", id, separator)
+		// try with separator "/""
+		volRegex := regexp.MustCompile("^([^/]+)/(.*)/([^/]+)$")
+		tokens := volRegex.FindStringSubmatch(id)
+		if tokens == nil {
+			return nil, fmt.Errorf("could not split %s into server, baseDir and subDir with separator(%s)", id, "/")
+		}
+		server = tokens[1]
+		baseDir = tokens[2]
+		subDir = tokens[3]
+	} else {
+		server = segments[0]
+		baseDir = segments[1]
+		subDir = segments[2]
 	}
 
 	return &nfsVolume{
 		id:      id,
-		server:  tokens[1],
+		server:  server,
-		baseDir: tokens[2],
+		baseDir: baseDir,
-		subDir:  tokens[3],
+		subDir:  subDir,
 	}, nil
 }

pkg/nfs/controllerserver_test.go

@@ -20,6 +20,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -33,12 +34,14 @@ import (
 )
 
 const (
-	testServer         = "test-server"
+	testServer            = "test-server"
-	testBaseDir        = "test-base-dir"
+	testBaseDir           = "test-base-dir"
-	testBaseDirNested  = "test/base/dir"
+	testBaseDirNested     = "test/base/dir"
-	testCSIVolume      = "test-csi"
+	testCSIVolume         = "test-csi"
-	testVolumeID       = "test-server/test-base-dir/test-csi"
+	testVolumeID          = "test-server/test-base-dir/test-csi"
-	testVolumeIDNested = "test-server/test/base/dir/test-csi"
+	newTestVolumeID       = "test-server#test-base-dir#test-csi"
+	testVolumeIDNested    = "test-server/test/base/dir/test-csi"
+	newTestVolumeIDNested = "test-server#test/base/dir#test-csi"
 )
 
 // for Windows support in the future
@@ -103,7 +106,36 @@ func TestCreateVolume(t *testing.T) {
 			},
 			resp: &csi.CreateVolumeResponse{
 				Volume: &csi.Volume{
-					VolumeId: testVolumeID,
+					VolumeId: newTestVolumeID,
+					VolumeContext: map[string]string{
+						paramServer: testServer,
+						paramShare:  testShare,
+					},
+				},
+			},
+		},
+		{
+			name: "valid defaults with newTestVolumeID",
+			req: &csi.CreateVolumeRequest{
+				Name: testCSIVolume,
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
+						},
+					},
+				},
+				Parameters: map[string]string{
+					paramServer: testServer,
+					paramShare:  testBaseDir,
+				},
+			},
+			resp: &csi.CreateVolumeResponse{
+				Volume: &csi.Volume{
+					VolumeId: newTestVolumeID,
 					VolumeContext: map[string]string{
 						paramServer: testServer,
 						paramShare:  testShare,
@@ -204,37 +236,47 @@ func TestCreateVolume(t *testing.T) {
 
 func TestDeleteVolume(t *testing.T) {
 	cases := []struct {
-		desc        string
+		desc          string
-		req         *csi.DeleteVolumeRequest
+		testOnWindows bool
-		resp        *csi.DeleteVolumeResponse
+		req           *csi.DeleteVolumeRequest
-		expectedErr error
+		resp          *csi.DeleteVolumeResponse
+		expectedErr   error
 	}{
 		{
-			desc:        "Volume ID missing",
+			desc:          "Volume ID missing",
-			req:         &csi.DeleteVolumeRequest{},
+			testOnWindows: true,
-			resp:        nil,
+			req:           &csi.DeleteVolumeRequest{},
-			expectedErr: status.Error(codes.InvalidArgument, "Volume ID missing in request"),
+			resp:          nil,
+			expectedErr:   status.Error(codes.InvalidArgument, "Volume ID missing in request"),
 		},
 		{
-			desc:        "Valid request",
+			desc:          "Valid request",
-			req:         &csi.DeleteVolumeRequest{VolumeId: testVolumeID},
+			testOnWindows: false,
-			resp:        &csi.DeleteVolumeResponse{},
+			req:           &csi.DeleteVolumeRequest{VolumeId: testVolumeID},
-			expectedErr: nil,
+			resp:          &csi.DeleteVolumeResponse{},
+			expectedErr:   nil,
+		},
+		{
+			desc:          "Valid request with newTestVolumeID",
+			testOnWindows: true,
+			req:           &csi.DeleteVolumeRequest{VolumeId: newTestVolumeID},
+			resp:          &csi.DeleteVolumeResponse{},
+			expectedErr:   nil,
 		},
 	}
 
 	for _, test := range cases {
 		test := test //pin
+		if runtime.GOOS == "windows" && !test.testOnWindows {
+			continue
+		}
 		t.Run(test.desc, func(t *testing.T) {
-			// Setup
 			cs := initTestController(t)
 			_ = os.MkdirAll(filepath.Join(cs.Driver.workingMountDir, testCSIVolume), os.ModePerm)
 			_, _ = os.Create(filepath.Join(cs.Driver.workingMountDir, testCSIVolume, testCSIVolume))
 
-			// Run
 			resp, err := cs.DeleteVolume(context.TODO(), test.req)
 
-			// Verify
 			if test.expectedErr == nil && err != nil {
 				t.Errorf("test %q failed: %v", test.desc, err)
 			}
@@ -288,6 +330,24 @@ func TestValidateVolumeCapabilities(t *testing.T) {
 			resp:        &csi.ValidateVolumeCapabilitiesResponse{Message: ""},
 			expectedErr: nil,
 		},
+		{
+			desc: "valid request with newTestVolumeID",
+			req: &csi.ValidateVolumeCapabilitiesRequest{
+				VolumeId: newTestVolumeID,
+				VolumeCapabilities: []*csi.VolumeCapability{
+					{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
+						},
+					},
+				},
+			},
+			resp:        &csi.ValidateVolumeCapabilitiesResponse{Message: ""},
+			expectedErr: nil,
+		},
 	}
 
 	for _, test := range cases {
@@ -371,25 +431,25 @@ func TestControllerGetCapabilities(t *testing.T) {
 func TestNfsVolFromId(t *testing.T) {
 	cases := []struct {
 		name      string
-		req       string
+		volumeID  string
 		resp      *nfsVolume
 		expectErr bool
 	}{
 		{
 			name:      "ID only server",
-			req:       testServer,
+			volumeID:  testServer,
 			resp:      nil,
 			expectErr: true,
 		},
 		{
 			name:      "ID missing subDir",
-			req:       strings.Join([]string{testServer, testBaseDir}, "/"),
+			volumeID:  strings.Join([]string{testServer, testBaseDir}, "/"),
 			resp:      nil,
 			expectErr: true,
 		},
 		{
-			name: "valid request single baseDir",
+			name:     "valid request single baseDir",
-			req:  testVolumeID,
+			volumeID: testVolumeID,
 			resp: &nfsVolume{
 				id:      testVolumeID,
 				server:  testServer,
@@ -399,8 +459,19 @@ func TestNfsVolFromId(t *testing.T) {
 			expectErr: false,
 		},
 		{
-			name: "valid request nested baseDir",
+			name:     "valid request single baseDir with newTestVolumeID",
-			req:  testVolumeIDNested,
+			volumeID: newTestVolumeID,
+			resp: &nfsVolume{
+				id:      newTestVolumeID,
+				server:  testServer,
+				baseDir: testBaseDir,
+				subDir:  testCSIVolume,
+			},
+			expectErr: false,
+		},
+		{
+			name:     "valid request nested baseDir",
+			volumeID: testVolumeIDNested,
 			resp: &nfsVolume{
 				id:      testVolumeIDNested,
 				server:  testServer,
@@ -409,18 +480,24 @@ func TestNfsVolFromId(t *testing.T) {
 			},
 			expectErr: false,
 		},
+		{
+			name:     "valid request nested baseDir with newTestVolumeIDNested",
+			volumeID: newTestVolumeIDNested,
+			resp: &nfsVolume{
+				id:      newTestVolumeIDNested,
+				server:  testServer,
+				baseDir: testBaseDirNested,
+				subDir:  testCSIVolume,
+			},
+			expectErr: false,
+		},
 	}
 
 	for _, test := range cases {
 		test := test //pin
 		t.Run(test.name, func(t *testing.T) {
-			// Setup
+			resp, err := getNfsVolFromID(test.volumeID)
-			cs := initTestController(t)
 
-			// Run
-			resp, err := cs.getNfsVolFromID(test.req)
-
-			// Verify
 			if !test.expectErr && err != nil {
 				t.Errorf("test %q failed: %v", test.name, err)
 			}