Merge pull request #850 from RomanBednar/gid-allocator

Fix GID allocator

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/mitchellh/go-ps v0.0.0-20170309133038-4fdf99ab2936
 	github.com/onsi/ginkgo/v2 v2.9.0
 	github.com/onsi/gomega v1.27.1
+	golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63
 	google.golang.org/grpc v1.53.0
 	k8s.io/api v0.25.6
 	k8s.io/apimachinery v0.25.6
@@ -81,7 +82,7 @@ require (
 	golang.org/x/term v0.11.0 // indirect
 	golang.org/x/text v0.12.0 // indirect
 	golang.org/x/time v0.0.0-20220609170525-579cf78fd858 // indirect
-	golang.org/x/tools v0.6.0 // indirect
+	golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20230110181048-76db0878b65f // indirect
 	google.golang.org/protobuf v1.28.1 // indirect

go.sum

@@ -382,6 +382,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0
 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
+golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 h1:m64FZMko/V45gv0bNmrNYoDEq8U5YUhetc9cBWKS1TQ=
+golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63/go.mod h1:0v4NqG35kSWCMzLaMeX+IQrlSnVE/bqGSyC2cz/9Le8=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
 golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
@@ -404,6 +406,7 @@ golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3/go.mod h1:3p9vT2HGsQu2K1YbXdKPJLVgG5VJdoTa1poYQBtP1AY=
+golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
@@ -585,8 +588,8 @@ golang.org/x/tools v0.0.0-20201224043029-2b0845dc783e/go.mod h1:emZCQorbCU4vsT4f
 golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/tools v0.1.10/go.mod h1:Uh6Zz+xoGYZom868N8YTex3t7RhtHDBrE8Gzo9bV56E=
-golang.org/x/tools v0.6.0 h1:BOw41kyTf3PuCW1pVQf8+Cyg8pMlkYB1oo9iJ6D/lKM=
-golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU=
+golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846 h1:Vve/L0v7CXXuxUmaMGIEK/dEeq7uiqb5qBgQrZzIE7E=
+golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846/go.mod h1:Sc0INKfu04TlqNoRA1hgpFZbhYXHPr4V5DzpSBTPqQM=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

pkg/cloud/cloud.go

@@ -55,6 +55,12 @@ type AccessPoint struct {
 	// Capacity is used for testing purpose only
 	// EFS does not consider capacity while provisioning new file systems or access points
 	CapacityGiB int64
+	PosixUser   *PosixUser
+}
+
+type PosixUser struct {
+	Gid int64
+	Uid int64
 }
 
 type AccessPointOptions struct {
@@ -91,6 +97,7 @@ type Cloud interface {
 	CreateAccessPoint(ctx context.Context, volumeName string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error)
 	DeleteAccessPoint(ctx context.Context, accessPointId string) (err error)
 	DescribeAccessPoint(ctx context.Context, accessPointId string) (accessPoint *AccessPoint, err error)
+	ListAccessPoints(ctx context.Context, fileSystemId string) (accessPoints []*AccessPoint, err error)
 	DescribeFileSystem(ctx context.Context, fileSystemId string) (fs *FileSystem, err error)
 	DescribeMountTargets(ctx context.Context, fileSystemId, az string) (fs *MountTarget, err error)
 }
@@ -233,6 +240,37 @@ func (c *cloud) DescribeAccessPoint(ctx context.Context, accessPointId string) (
 	}, nil
 }
 
+func (c *cloud) ListAccessPoints(ctx context.Context, fileSystemId string) (accessPoints []*AccessPoint, err error) {
+	describeAPInput := &efs.DescribeAccessPointsInput{
+		FileSystemId: &fileSystemId,
+	}
+	res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput)
+	if err != nil {
+		if isAccessDenied(err) {
+			return
+		}
+		if isFileSystemNotFound(err) {
+			return
+		}
+		err = fmt.Errorf("List Access Points failed: %v", err)
+		return
+	}
+
+	for _, accessPointDescription := range res.AccessPoints {
+		accessPoint := &AccessPoint{
+			AccessPointId: *accessPointDescription.AccessPointId,
+			FileSystemId:  *accessPointDescription.FileSystemId,
+			PosixUser: &PosixUser{
+				Gid: *accessPointDescription.PosixUser.Gid,
+				Uid: *accessPointDescription.PosixUser.Gid,
+			},
+		}
+		accessPoints = append(accessPoints, accessPoint)
+	}
+
+	return
+}
+
 func (c *cloud) DescribeFileSystem(ctx context.Context, fileSystemId string) (fs *FileSystem, err error) {
 	describeFsInput := &efs.DescribeFileSystemsInput{FileSystemId: &fileSystemId}
 	klog.V(5).Infof("Calling DescribeFileSystems with input: %+v", *describeFsInput)

pkg/cloud/cloud_test.go

@@ -443,6 +443,125 @@ func TestDescribeAccessPoint(t *testing.T) {
 	}
 }
 
+func TestListAccessPoints(t *testing.T) {
+	var (
+		fsId                = "fs-abcd1234"
+		accessPointId       = "ap-abc123"
+		Gid           int64 = 1000
+		Uid           int64 = 1000
+	)
+	testCases := []struct {
+		name     string
+		testFunc func(t *testing.T)
+	}{
+		{
+			name: "Success",
+			testFunc: func(t *testing.T) {
+				mockctl := gomock.NewController(t)
+				mockEfs := mocks.NewMockEfs(mockctl)
+				c := &cloud{efs: mockEfs}
+
+				output := &efs.DescribeAccessPointsOutput{
+					AccessPoints: []*efs.AccessPointDescription{
+						{
+							AccessPointId: aws.String(accessPointId),
+							FileSystemId:  aws.String(fsId),
+							PosixUser: &efs.PosixUser{
+								Gid: aws.Int64(Gid),
+								Uid: aws.Int64(Uid),
+							},
+						},
+					},
+					NextToken: nil,
+				}
+
+				ctx := context.Background()
+				mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
+				res, err := c.ListAccessPoints(ctx, fsId)
+				if err != nil {
+					t.Fatalf("List Access Points failed: %v", err)
+				}
+
+				if res == nil {
+					t.Fatal("Result is nil")
+				}
+
+				if len(res) != 1 {
+					t.Fatalf("Expected only one AccessPoint in response but got: %v", res)
+				}
+
+				mockctl.Finish()
+			},
+		},
+		{
+			name: "Success - multiple access points",
+			testFunc: func(t *testing.T) {
+				mockctl := gomock.NewController(t)
+				mockEfs := mocks.NewMockEfs(mockctl)
+				c := &cloud{efs: mockEfs}
+
+				output := &efs.DescribeAccessPointsOutput{
+					AccessPoints: []*efs.AccessPointDescription{
+						{
+							AccessPointId: aws.String(accessPointId),
+							FileSystemId:  aws.String(fsId),
+							PosixUser: &efs.PosixUser{
+								Gid: aws.Int64(Gid),
+								Uid: aws.Int64(Uid),
+							},
+						},
+						{
+							AccessPointId: aws.String(accessPointId),
+							FileSystemId:  aws.String(fsId),
+							PosixUser: &efs.PosixUser{
+								Gid: aws.Int64(1001),
+								Uid: aws.Int64(1001),
+							},
+						},
+					},
+					NextToken: nil,
+				}
+
+				ctx := context.Background()
+				mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
+				res, err := c.ListAccessPoints(ctx, fsId)
+				if err != nil {
+					t.Fatalf("List Access Points failed: %v", err)
+				}
+
+				if res == nil {
+					t.Fatal("Result is nil")
+				}
+
+				if len(res) != 2 {
+					t.Fatalf("Expected two AccessPoints in response but got: %v", res)
+				}
+
+				mockctl.Finish()
+			},
+		},
+		{
+			name: "Fail - Access Denied",
+			testFunc: func(t *testing.T) {
+				mockctl := gomock.NewController(t)
+				mockEfs := mocks.NewMockEfs(mockctl)
+				c := &cloud{efs: mockEfs}
+				ctx := context.Background()
+				mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, awserr.New(AccessDeniedException, "Access Denied", errors.New("Access Denied")))
+				_, err := c.ListAccessPoints(ctx, fsId)
+				if err == nil {
+					t.Fatalf("List Access Points should have failed: %v", err)
+				}
+
+				mockctl.Finish()
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, tc.testFunc)
+	}
+}
+
 func TestDescribeFileSystem(t *testing.T) {
 	var (
 		fsId = "fs-abcd1234"

pkg/cloud/fakes.go

@@ -97,3 +97,10 @@ func (c *FakeCloudProvider) DescribeMountTargets(ctx context.Context, fileSystem
 
 	return nil, ErrNotFound
 }
+
+func (c *FakeCloudProvider) ListAccessPoints(ctx context.Context, fileSystemId string) ([]*AccessPoint, error) {
+	accessPoints := []*AccessPoint{
+		c.accessPoints[fileSystemId],
+	}
+	return accessPoints, nil
+}

pkg/driver/controller.go

@@ -99,13 +99,13 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		azName           string
 		basePath         string
 		err              error
-		gid              int
+		gid              int64
 		gidMin           int
 		gidMax           int
 		localCloud       cloud.Cloud
 		provisioningMode string
 		roleArn          string
-		uid              int
+		uid              int64
 	)
 
 	//Parse parameters
@@ -149,7 +149,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 
 	uid = -1
 	if value, ok := volumeParams[Uid]; ok {
-		uid, err = strconv.Atoi(value)
+		uid, err = strconv.ParseInt(value, 10, 64)
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Uid, err)
 		}
@@ -160,7 +160,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 
 	gid = -1
 	if value, ok := volumeParams[Gid]; ok {
-		gid, err = strconv.Atoi(value)
+		gid, err = strconv.ParseInt(value, 10, 64)
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Gid, err)
 		}
@@ -233,9 +233,9 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		return nil, status.Errorf(codes.Internal, "Failed to fetch File System info: %v", err)
 	}
 
-	var allocatedGid int
+	var allocatedGid int64
 	if uid == -1 || gid == -1 {
-		allocatedGid, err = d.gidAllocator.getNextGid(accessPointsOptions.FileSystemId, gidMin, gidMax)
+		allocatedGid, err = d.gidAllocator.getNextGid(ctx, accessPointsOptions.FileSystemId, gidMin, gidMax)
 		if err != nil {
 			return nil, err
 		}
@@ -283,15 +283,12 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	}
 	klog.Infof("Using %v as the access point directory.", rootDir)
 
-	accessPointsOptions.Uid = int64(uid)
-	accessPointsOptions.Gid = int64(gid)
+	accessPointsOptions.Uid = uid
+	accessPointsOptions.Gid = gid
 	accessPointsOptions.DirectoryPath = rootDir
 
 	accessPointId, err := localCloud.CreateAccessPoint(ctx, volName, accessPointsOptions)
 	if err != nil {
-		if allocatedGid != 0 {
-			d.gidAllocator.releaseGid(accessPointsOptions.FileSystemId, gid)
-		}
 		if err == cloud.ErrAccessDenied {
 			return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err)
 		}