diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index b63fb03e4..5220ef51d 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -302,10 +302,10 @@ func (c *cloud) ListAccessPoints(ctx context.Context, fileSystemId string) (acce res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput) if err != nil { if isAccessDenied(err) { - return + return nil, ErrAccessDenied } if isFileSystemNotFound(err) { - return + return nil, ErrNotFound } err = fmt.Errorf("List Access Points failed: %v", err) return diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 6f701981f..031738208 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -38,8 +38,8 @@ const ( AccessPointMode = "efs-ap" AzName = "az" BasePath = "basePath" - DefaultGidMin = 50000 - DefaultGidMax = 7000000 + DefaultGidMin = int64(50000) + DefaultGidMax = DefaultGidMin + cloud.AccessPointPerFsLimit DefaultTagKey = "efs.csi.aws.com/cluster" DefaultTagValue = "true" DirectoryPerms = "directoryPerms" @@ -120,8 +120,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) azName string basePath string gid int64 - gidMin int - gidMax int + gidMin int64 + gidMax int64 localCloud cloud.Cloud provisioningMode string roleArn string @@ -189,7 +189,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } if value, ok := volumeParams[GidMin]; ok { - gidMin, err = strconv.Atoi(value) + gidMin, err = strconv.ParseInt(value, 10, 64) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMin, err) } @@ -203,7 +203,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) if gidMin == 0 { return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", GidMin) } - gidMax, err = strconv.Atoi(value) + gidMax, err = strconv.ParseInt(value, 10, 64) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMax, err) } @@ -241,20 +241,27 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return nil, err } - // Check if file system exists. Describe FS handles appropriate error codes - if _, err = localCloud.DescribeFileSystem(ctx, accessPointsOptions.FileSystemId); err != nil { + // Check if file system exists. Describe FS or List APs handle appropriate error codes + // With dynamic uid/gid provisioning we can save a call to describe FS, as list APs fails if FS ID does not exist + var accessPoints []*cloud.AccessPoint + if uid == -1 || gid == -1 { + accessPoints, err = localCloud.ListAccessPoints(ctx, accessPointsOptions.FileSystemId) + } else { + _, err = localCloud.DescribeFileSystem(ctx, accessPointsOptions.FileSystemId) + } + if err != nil { if err == cloud.ErrAccessDenied { return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err) } if err == cloud.ErrNotFound { return nil, status.Errorf(codes.InvalidArgument, "File System does not exist: %v", err) } - return nil, status.Errorf(codes.Internal, "Failed to fetch File System info: %v", err) + return nil, status.Errorf(codes.Internal, "Failed to fetch Access Points or Describe File System: %v", err) } var allocatedGid int64 if uid == -1 || gid == -1 { - allocatedGid, err = d.gidAllocator.getNextGid(ctx, localCloud, accessPointsOptions.FileSystemId, gidMin, gidMax) + allocatedGid, err = d.gidAllocator.getNextGid(accessPointsOptions.FileSystemId, accessPoints, gidMin, gidMax) if err != nil { return nil, err } diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 82a07a120..666ea4a05 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -203,9 +203,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -230,7 +227,6 @@ func TestCreateVolume(t *testing.T) { } var expectedGid int64 = 1003 //1001 and 1002 are taken, next available is 1003 - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(accessPoint, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -289,9 +285,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } ap1 := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -329,7 +322,6 @@ func TestCreateVolume(t *testing.T) { accessPoints := []*cloud.AccessPoint{ap1, ap2, ap3} var expectedGid int64 = 1004 // 1001-1003 is taken. - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap2, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -347,7 +339,6 @@ func TestCreateVolume(t *testing.T) { accessPoints = []*cloud.AccessPoint{} expectedGid = 1001 // 1001 is now free and lowest possible, if no GID return would happen allocator would pick 1005. - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap3, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -365,7 +356,6 @@ func TestCreateVolume(t *testing.T) { accessPoints = []*cloud.AccessPoint{ap1, ap4} expectedGid = 1002 // 1001 and 1004 are now taken, lowest available is 1002 - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap2, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -424,13 +414,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoints := []*cloud.AccessPoint{} - for i := 0; i < cloud.AccessPointPerFsLimit; i++ { - gidMin, err := strconv.Atoi(req.Parameters[GidMin]) + for i := int64(0); i < cloud.AccessPointPerFsLimit; i++ { + gidMin, err := strconv.ParseInt(req.Parameters[GidMin], 10, 64) if err != nil { t.Fatalf("Failed to convert GidMax Parameter to int.") } @@ -439,8 +426,8 @@ func TestCreateVolume(t *testing.T) { AccessPointId: apId, FileSystemId: fsId, PosixUser: &cloud.PosixUser{ - Gid: int64(userGid), - Uid: int64(userGid), + Gid: userGid, + Uid: userGid, }, } accessPoints = append(accessPoints, ap) @@ -456,7 +443,6 @@ func TestCreateVolume(t *testing.T) { } expectedGid := 2000 - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(lastAccessPoint, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -477,7 +463,6 @@ func TestCreateVolume(t *testing.T) { } accessPoints = append(accessPoints, lastAccessPoint) - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) // All 1000 GIDs are taken now, internal limit should take effect causing CreateVolume to fail. @@ -519,9 +504,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, @@ -529,7 +511,6 @@ func TestCreateVolume(t *testing.T) { } expectedGid := 1000 // Allocator should pick lowest available GID - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(accessPoint, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -583,9 +564,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -595,7 +573,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -644,9 +621,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -655,7 +629,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -706,9 +679,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -718,7 +688,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -769,9 +738,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -781,7 +747,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -836,9 +801,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, @@ -849,7 +811,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(get64LenHash(pvcNameVal)), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -908,14 +869,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -980,14 +937,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1055,14 +1008,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1130,14 +1079,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1204,14 +1149,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1273,14 +1214,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1343,14 +1280,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1415,14 +1348,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -2173,7 +2102,7 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: File system does not exist", + name: "Fail: File system does not exist with fixed uid/gid", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2198,6 +2127,8 @@ func TestCreateVolume(t *testing.T) { GidMin: "1000", GidMax: "2000", DirectoryPerms: "777", + Uid: "1000", + Gid: "1001", }, } @@ -2211,7 +2142,7 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: DescribeFileSystem Access Denied", + name: "Fail: File system does not exist", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2239,6 +2170,46 @@ func TestCreateVolume(t *testing.T) { }, } + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, cloud.ErrNotFound) + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + mockCtl.Finish() + }, + }, + { + name: "Fail: DescribeFileSystem Access Denied with fixed uid/gid", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + Uid: "1000", + Gid: "1001", + }, + } + ctx := context.Background() mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(nil, cloud.ErrAccessDenied) _, err := driver.CreateVolume(ctx, req) @@ -2249,7 +2220,45 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: Describe File system call fails", + name: "Fail: DescribeFileSystem Access Denied", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, cloud.ErrAccessDenied) + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + mockCtl.Finish() + }, + }, + { + name: "Fail: Describe File system call fails with fixed uid/gid", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2274,6 +2283,8 @@ func TestCreateVolume(t *testing.T) { GidMin: "1000", GidMax: "2000", DirectoryPerms: "777", + Uid: "1000", + Gid: "1001", }, } @@ -2287,7 +2298,7 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: Create Access Point call fails", + name: "Fail: List access points call fails", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2316,10 +2327,44 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, + mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, errors.New("ListAccessPoints failed")) + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + mockCtl.Finish() + }, + }, + { + name: "Fail: Create Access Point call fails", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + }, + } + + ctx := context.Background() mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{}, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("CreateAccessPoint call failed")) _, err := driver.CreateVolume(ctx, req) @@ -2359,10 +2404,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{}, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAccessDenied) _, err := driver.CreateVolume(ctx, req) @@ -2402,9 +2443,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } ap1 := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -2421,7 +2459,6 @@ func TestCreateVolume(t *testing.T) { Uid: 1001, }, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil).AnyTimes() mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{ap1, ap2}, nil).AnyTimes() mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(ap2, nil).AnyTimes() @@ -2513,11 +2550,6 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) _, err := driver.CreateVolume(ctx, req) @@ -2562,11 +2594,6 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) _, err := driver.CreateVolume(ctx, req) @@ -2611,11 +2638,6 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) _, err := driver.CreateVolume(ctx, req) diff --git a/pkg/driver/gid_allocator.go b/pkg/driver/gid_allocator.go index 71460fbf4..97348a2c4 100644 --- a/pkg/driver/gid_allocator.go +++ b/pkg/driver/gid_allocator.go @@ -1,7 +1,6 @@ package driver import ( - "context" "fmt" "sync" @@ -13,29 +12,26 @@ import ( ) type FilesystemID struct { - gidMin int - gidMax int + gidMin int64 + gidMax int64 } type GidAllocator struct { - fsIdGidMap map[string]*FilesystemID - mu sync.Mutex + mu sync.Mutex } func NewGidAllocator() GidAllocator { - return GidAllocator{ - fsIdGidMap: make(map[string]*FilesystemID), - } + return GidAllocator{} } // Retrieves the next available GID -func (g *GidAllocator) getNextGid(ctx context.Context, localCloud cloud.Cloud, fsId string, gidMin, gidMax int) (int64, error) { +func (g *GidAllocator) getNextGid(fsId string, accessPoints []*cloud.AccessPoint, gidMin, gidMax int64) (int64, error) { g.mu.Lock() defer g.mu.Unlock() - klog.V(5).Infof("Recieved getNextGid for fsId: %v, min: %v, max: %v", fsId, gidMin, gidMax) + klog.V(5).Infof("Received getNextGid for fsId: %v, min: %v, max: %v", fsId, gidMin, gidMax) - usedGids, err := g.getUsedGids(ctx, localCloud, fsId) + usedGids, err := g.getUsedGids(fsId, accessPoints) if err != nil { return 0, status.Errorf(codes.Internal, "Failed to discover used GIDs for filesystem: %v: %v ", fsId, err) } @@ -47,23 +43,11 @@ func (g *GidAllocator) getNextGid(ctx context.Context, localCloud cloud.Cloud, f "Please create a new storage class with a new file-system", fsId) } - return int64(gid), nil - + return gid, nil } -func (g *GidAllocator) removeFsId(fsId string) { - g.mu.Lock() - defer g.mu.Unlock() - delete(g.fsIdGidMap, fsId) -} - -func (g *GidAllocator) getUsedGids(ctx context.Context, localCloud cloud.Cloud, fsId string) (gids []int64, err error) { +func (g *GidAllocator) getUsedGids(fsId string, accessPoints []*cloud.AccessPoint) (gids []int64, err error) { gids = []int64{} - accessPoints, err := localCloud.ListAccessPoints(ctx, fsId) - if err != nil { - err = fmt.Errorf("failed to list access points: %v", err) - return - } if len(accessPoints) == 0 { return gids, nil } @@ -80,7 +64,7 @@ func (g *GidAllocator) getUsedGids(ctx context.Context, localCloud cloud.Cloud, return } -func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err error) { +func getNextUnusedGid(usedGids []int64, gidMin, gidMax int64) (nextGid int64, err error) { requestedRange := gidMax - gidMin if requestedRange > cloud.AccessPointPerFsLimit { @@ -92,7 +76,7 @@ func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err er var lookup func(usedGids []int64) lookup = func(usedGids []int64) { for gid := gidMin; gid <= gidMax; gid++ { - if !slices.Contains(usedGids, int64(gid)) { + if !slices.Contains(usedGids, gid) { nextGid = gid return }