diff --git a/pkg/nfs/controllerserver.go b/pkg/nfs/controllerserver.go index 162074e4..b14af9f0 100644 --- a/pkg/nfs/controllerserver.go +++ b/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) { - volRegex := regexp.MustCompile("^([^/]+)/(.*)/([^/]+)$") - tokens := volRegex.FindStringSubmatch(id) - if tokens == nil { - return nil, fmt.Errorf("Could not split %q into server, baseDir and subDir", id) +// sample volume Id: +// new volumeID: nfs-server.default.svc.cluster.local#share#pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64 +// old volumeID: nfs-server.default.svc.cluster.local/share/pvc-4bcbf944-b6f7-4bd0-b50f-3c3dd00efc64 +func getNfsVolFromID(id string) (*nfsVolume, error) { + 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], - baseDir: tokens[2], - subDir: tokens[3], + server: server, + baseDir: baseDir, + subDir: subDir, }, nil } diff --git a/pkg/nfs/controllerserver_test.go b/pkg/nfs/controllerserver_test.go index 6d1036a4..664529bc 100644 --- a/pkg/nfs/controllerserver_test.go +++ b/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" - testBaseDir = "test-base-dir" - testBaseDirNested = "test/base/dir" - testCSIVolume = "test-csi" - testVolumeID = "test-server/test-base-dir/test-csi" - testVolumeIDNested = "test-server/test/base/dir/test-csi" + testServer = "test-server" + testBaseDir = "test-base-dir" + testBaseDirNested = "test/base/dir" + testCSIVolume = "test-csi" + testVolumeID = "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 - req *csi.DeleteVolumeRequest - resp *csi.DeleteVolumeResponse - expectedErr error + desc string + testOnWindows bool + req *csi.DeleteVolumeRequest + resp *csi.DeleteVolumeResponse + expectedErr error }{ { - desc: "Volume ID missing", - req: &csi.DeleteVolumeRequest{}, - resp: nil, - expectedErr: status.Error(codes.InvalidArgument, "Volume ID missing in request"), + desc: "Volume ID missing", + testOnWindows: true, + req: &csi.DeleteVolumeRequest{}, + resp: nil, + expectedErr: status.Error(codes.InvalidArgument, "Volume ID missing in request"), }, { - desc: "Valid request", - req: &csi.DeleteVolumeRequest{VolumeId: testVolumeID}, - resp: &csi.DeleteVolumeResponse{}, - expectedErr: nil, + desc: "Valid request", + testOnWindows: false, + req: &csi.DeleteVolumeRequest{VolumeId: testVolumeID}, + 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", - req: testVolumeID, + name: "valid request single baseDir", + volumeID: testVolumeID, resp: &nfsVolume{ id: testVolumeID, server: testServer, @@ -399,8 +459,19 @@ func TestNfsVolFromId(t *testing.T) { expectErr: false, }, { - name: "valid request nested baseDir", - req: testVolumeIDNested, + name: "valid request single baseDir with newTestVolumeID", + 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 - cs := initTestController(t) + resp, err := getNfsVolFromID(test.volumeID) - // Run - resp, err := cs.getNfsVolFromID(test.req) - - // Verify if !test.expectErr && err != nil { t.Errorf("test %q failed: %v", test.name, err) }