set results count for listing access points

DescribeAccessPointsWithContext call returns only 100 results by default. Since this is used for discovering used GIDs on access points we must not allow listing only a subset of access points. This is because gid allocator relies on this call to filter out used GIDs and getting next *free* GID. If we don't make sure to list every access point it will cause allocator to pick GID that might already be taken. (cherry picked from commit 16557ce)
kubernetes-sigs · Jan 3, 2024 · d31495d · d31495d
1 parent 7d97ab2
commit d31495d
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 11 deletions.
diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go
@@ -38,6 +38,7 @@ const (
 	AccessDeniedException    = "AccessDeniedException"
 	AccessPointAlreadyExists = "AccessPointAlreadyExists"
 	PvcNameTagKey            = "pvcName"
+	AccessPointPerFsLimit    = 1000
 )
 
 var (
@@ -266,7 +267,7 @@ func (c *cloud) findAccessPointByClientToken(ctx context.Context, clientToken st
 	klog.V(2).Infof("ClientToken to find AP : %s", clientToken)
 	describeAPInput := &efs.DescribeAccessPointsInput{
 		FileSystemId: &accessPointOpts.FileSystemId,
-		MaxResults:   aws.Int64(1000),
+		MaxResults:   aws.Int64(AccessPointPerFsLimit),
 	}
 	res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput)
 	if err != nil {
@@ -296,6 +297,7 @@ func (c *cloud) findAccessPointByClientToken(ctx context.Context, clientToken st
 func (c *cloud) ListAccessPoints(ctx context.Context, fileSystemId string) (accessPoints []*AccessPoint, err error) {
 	describeAPInput := &efs.DescribeAccessPointsInput{
 		FileSystemId: &fileSystemId,
+		MaxResults:   aws.Int64(AccessPointPerFsLimit),
 	}
 	res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput)
 	if err != nil {

diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
@@ -215,8 +215,8 @@ func TestCreateVolume(t *testing.T) {
 						AccessPointId: apId,
 						FileSystemId:  fsId,
 						PosixUser: &cloud.PosixUser{
-							Gid: 1003,
-							Uid: 1003,
+							Gid: 1001,
+							Uid: 1001,
 						},
 					},
 					{
@@ -229,7 +229,7 @@ func TestCreateVolume(t *testing.T) {
 					},
 				}
 
-				var expectedGid int64 = 1001 //1003 and 1002 are taken, next available is 1001
+				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).
@@ -429,7 +429,7 @@ func TestCreateVolume(t *testing.T) {
 				}
 
 				accessPoints := []*cloud.AccessPoint{}
-				for i := 0; i < ACCESS_POINT_PER_FS_LIMIT; i++ {
+				for i := 0; i < cloud.AccessPointPerFsLimit; i++ {
 					gidMin, err := strconv.Atoi(req.Parameters[GidMin])
 					if err != nil {
 						t.Fatalf("Failed to convert GidMax Parameter to int.")
@@ -488,6 +488,69 @@ func TestCreateVolume(t *testing.T) {
 				mockCtl.Finish()
 			},
 		},
+		{
+			name: "Success: GID range exceeds EFS access point limit",
+			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,
+						DirectoryPerms:   "777",
+						BasePath:         "test",
+						GidMin:           "1000",
+						GidMax:           "1000000",
+					},
+				}
+
+				ctx := context.Background()
+				fileSystem := &cloud.FileSystem{
+					FileSystemId: fsId,
+				}
+
+				accessPoint := &cloud.AccessPoint{
+					AccessPointId: apId,
+					FileSystemId:  fsId,
+				}
+
+				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) {
+						if accessPointOpts.Uid != int64(expectedGid) {
+							t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid)
+						}
+						if accessPointOpts.Gid != int64(expectedGid) {
+							t.Fatalf("Gid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Gid)
+						}
+					})
+
+				var err error
+
+				_, err = driver.CreateVolume(ctx, req)
+				if err != nil {
+					t.Fatalf("CreateVolume failed unexpectedly: %v", err)
+				}
+
+				mockCtl.Finish()
+			},
+		},
 		{
 			name: "Success: Normal flow",
 			testFunc: func(t *testing.T) {

diff --git a/pkg/driver/gid_allocator.go b/pkg/driver/gid_allocator.go
@@ -12,8 +12,6 @@ import (
 	"k8s.io/klog/v2"
 )
 
-var ACCESS_POINT_PER_FS_LIMIT int = 1000
-
 type FilesystemID struct {
 	gidMin int
 	gidMax int
@@ -85,9 +83,10 @@ func (g *GidAllocator) getUsedGids(ctx context.Context, localCloud cloud.Cloud,
 func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err error) {
 	requestedRange := gidMax - gidMin
 
-	if requestedRange > ACCESS_POINT_PER_FS_LIMIT {
-		klog.Warningf("Requested GID range (%v:%v) exceeds EFS Access Point limit (%v) per Filesystem. Driver will not allocate GIDs outside of this limit.", gidMin, gidMax, ACCESS_POINT_PER_FS_LIMIT)
-		gidMin = gidMax - ACCESS_POINT_PER_FS_LIMIT
+	if requestedRange > cloud.AccessPointPerFsLimit {
+		overrideGidMax := gidMin + cloud.AccessPointPerFsLimit
+		klog.Warningf("Requested GID range (%v:%v) exceeds EFS Access Point limit (%v) per Filesystem. Driver will use limited GID range (%v:%v)", gidMin, gidMax, cloud.AccessPointPerFsLimit, gidMin, overrideGidMax)
+		gidMax = overrideGidMax
 	}
 
 	var lookup func(usedGids []int64)
@@ -97,7 +96,7 @@ func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err er
 				nextGid = gid
 				return
 			}
-			klog.V(5).Infof("Allocator found GID which is already in use: %v - trying next one.", nextGid)
+			klog.V(5).Infof("Allocator found GID which is already in use: %v, trying next one.", gid)
 		}
 		return
 	}