Skip to content

Commit 5e44f89

Browse files
authored
Merge pull request #850 from RomanBednar/gid-allocator
Fix GID allocator
2 parents 75cc1da + 3e32348 commit 5e44f89

File tree

24 files changed

+2531
-141
lines changed

24 files changed

+2531
-141
lines changed

go.mod

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ require (
99
github.com/mitchellh/go-ps v0.0.0-20170309133038-4fdf99ab2936
1010
github.com/onsi/ginkgo/v2 v2.9.0
1111
github.com/onsi/gomega v1.27.1
12+
golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63
1213
google.golang.org/grpc v1.53.0
1314
k8s.io/api v0.25.6
1415
k8s.io/apimachinery v0.25.6
@@ -81,7 +82,7 @@ require (
8182
golang.org/x/term v0.11.0 // indirect
8283
golang.org/x/text v0.12.0 // indirect
8384
golang.org/x/time v0.0.0-20220609170525-579cf78fd858 // indirect
84-
golang.org/x/tools v0.6.0 // indirect
85+
golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846 // indirect
8586
google.golang.org/appengine v1.6.7 // indirect
8687
google.golang.org/genproto v0.0.0-20230110181048-76db0878b65f // indirect
8788
google.golang.org/protobuf v1.28.1 // indirect

go.sum

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0
382382
golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
383383
golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
384384
golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
385+
golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 h1:m64FZMko/V45gv0bNmrNYoDEq8U5YUhetc9cBWKS1TQ=
386+
golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63/go.mod h1:0v4NqG35kSWCMzLaMeX+IQrlSnVE/bqGSyC2cz/9Le8=
385387
golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
386388
golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
387389
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=
404406
golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
405407
golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
406408
golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3/go.mod h1:3p9vT2HGsQu2K1YbXdKPJLVgG5VJdoTa1poYQBtP1AY=
409+
golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=
407410
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
408411
golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
409412
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
585588
golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
586589
golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
587590
golang.org/x/tools v0.1.10/go.mod h1:Uh6Zz+xoGYZom868N8YTex3t7RhtHDBrE8Gzo9bV56E=
588-
golang.org/x/tools v0.6.0 h1:BOw41kyTf3PuCW1pVQf8+Cyg8pMlkYB1oo9iJ6D/lKM=
589-
golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU=
591+
golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846 h1:Vve/L0v7CXXuxUmaMGIEK/dEeq7uiqb5qBgQrZzIE7E=
592+
golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846/go.mod h1:Sc0INKfu04TlqNoRA1hgpFZbhYXHPr4V5DzpSBTPqQM=
590593
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
591594
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
592595
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

pkg/cloud/cloud.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,12 @@ type AccessPoint struct {
5555
// Capacity is used for testing purpose only
5656
// EFS does not consider capacity while provisioning new file systems or access points
5757
CapacityGiB int64
58+
PosixUser *PosixUser
59+
}
60+
61+
type PosixUser struct {
62+
Gid int64
63+
Uid int64
5864
}
5965

6066
type AccessPointOptions struct {
@@ -91,6 +97,7 @@ type Cloud interface {
9197
CreateAccessPoint(ctx context.Context, volumeName string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error)
9298
DeleteAccessPoint(ctx context.Context, accessPointId string) (err error)
9399
DescribeAccessPoint(ctx context.Context, accessPointId string) (accessPoint *AccessPoint, err error)
100+
ListAccessPoints(ctx context.Context, fileSystemId string) (accessPoints []*AccessPoint, err error)
94101
DescribeFileSystem(ctx context.Context, fileSystemId string) (fs *FileSystem, err error)
95102
DescribeMountTargets(ctx context.Context, fileSystemId, az string) (fs *MountTarget, err error)
96103
}
@@ -233,6 +240,37 @@ func (c *cloud) DescribeAccessPoint(ctx context.Context, accessPointId string) (
233240
}, nil
234241
}
235242

243+
func (c *cloud) ListAccessPoints(ctx context.Context, fileSystemId string) (accessPoints []*AccessPoint, err error) {
244+
describeAPInput := &efs.DescribeAccessPointsInput{
245+
FileSystemId: &fileSystemId,
246+
}
247+
res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput)
248+
if err != nil {
249+
if isAccessDenied(err) {
250+
return
251+
}
252+
if isFileSystemNotFound(err) {
253+
return
254+
}
255+
err = fmt.Errorf("List Access Points failed: %v", err)
256+
return
257+
}
258+
259+
for _, accessPointDescription := range res.AccessPoints {
260+
accessPoint := &AccessPoint{
261+
AccessPointId: *accessPointDescription.AccessPointId,
262+
FileSystemId: *accessPointDescription.FileSystemId,
263+
PosixUser: &PosixUser{
264+
Gid: *accessPointDescription.PosixUser.Gid,
265+
Uid: *accessPointDescription.PosixUser.Gid,
266+
},
267+
}
268+
accessPoints = append(accessPoints, accessPoint)
269+
}
270+
271+
return
272+
}
273+
236274
func (c *cloud) DescribeFileSystem(ctx context.Context, fileSystemId string) (fs *FileSystem, err error) {
237275
describeFsInput := &efs.DescribeFileSystemsInput{FileSystemId: &fileSystemId}
238276
klog.V(5).Infof("Calling DescribeFileSystems with input: %+v", *describeFsInput)

pkg/cloud/cloud_test.go

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,125 @@ func TestDescribeAccessPoint(t *testing.T) {
443443
}
444444
}
445445

446+
func TestListAccessPoints(t *testing.T) {
447+
var (
448+
fsId = "fs-abcd1234"
449+
accessPointId = "ap-abc123"
450+
Gid int64 = 1000
451+
Uid int64 = 1000
452+
)
453+
testCases := []struct {
454+
name string
455+
testFunc func(t *testing.T)
456+
}{
457+
{
458+
name: "Success",
459+
testFunc: func(t *testing.T) {
460+
mockctl := gomock.NewController(t)
461+
mockEfs := mocks.NewMockEfs(mockctl)
462+
c := &cloud{efs: mockEfs}
463+
464+
output := &efs.DescribeAccessPointsOutput{
465+
AccessPoints: []*efs.AccessPointDescription{
466+
{
467+
AccessPointId: aws.String(accessPointId),
468+
FileSystemId: aws.String(fsId),
469+
PosixUser: &efs.PosixUser{
470+
Gid: aws.Int64(Gid),
471+
Uid: aws.Int64(Uid),
472+
},
473+
},
474+
},
475+
NextToken: nil,
476+
}
477+
478+
ctx := context.Background()
479+
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
480+
res, err := c.ListAccessPoints(ctx, fsId)
481+
if err != nil {
482+
t.Fatalf("List Access Points failed: %v", err)
483+
}
484+
485+
if res == nil {
486+
t.Fatal("Result is nil")
487+
}
488+
489+
if len(res) != 1 {
490+
t.Fatalf("Expected only one AccessPoint in response but got: %v", res)
491+
}
492+
493+
mockctl.Finish()
494+
},
495+
},
496+
{
497+
name: "Success - multiple access points",
498+
testFunc: func(t *testing.T) {
499+
mockctl := gomock.NewController(t)
500+
mockEfs := mocks.NewMockEfs(mockctl)
501+
c := &cloud{efs: mockEfs}
502+
503+
output := &efs.DescribeAccessPointsOutput{
504+
AccessPoints: []*efs.AccessPointDescription{
505+
{
506+
AccessPointId: aws.String(accessPointId),
507+
FileSystemId: aws.String(fsId),
508+
PosixUser: &efs.PosixUser{
509+
Gid: aws.Int64(Gid),
510+
Uid: aws.Int64(Uid),
511+
},
512+
},
513+
{
514+
AccessPointId: aws.String(accessPointId),
515+
FileSystemId: aws.String(fsId),
516+
PosixUser: &efs.PosixUser{
517+
Gid: aws.Int64(1001),
518+
Uid: aws.Int64(1001),
519+
},
520+
},
521+
},
522+
NextToken: nil,
523+
}
524+
525+
ctx := context.Background()
526+
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
527+
res, err := c.ListAccessPoints(ctx, fsId)
528+
if err != nil {
529+
t.Fatalf("List Access Points failed: %v", err)
530+
}
531+
532+
if res == nil {
533+
t.Fatal("Result is nil")
534+
}
535+
536+
if len(res) != 2 {
537+
t.Fatalf("Expected two AccessPoints in response but got: %v", res)
538+
}
539+
540+
mockctl.Finish()
541+
},
542+
},
543+
{
544+
name: "Fail - Access Denied",
545+
testFunc: func(t *testing.T) {
546+
mockctl := gomock.NewController(t)
547+
mockEfs := mocks.NewMockEfs(mockctl)
548+
c := &cloud{efs: mockEfs}
549+
ctx := context.Background()
550+
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, awserr.New(AccessDeniedException, "Access Denied", errors.New("Access Denied")))
551+
_, err := c.ListAccessPoints(ctx, fsId)
552+
if err == nil {
553+
t.Fatalf("List Access Points should have failed: %v", err)
554+
}
555+
556+
mockctl.Finish()
557+
},
558+
},
559+
}
560+
for _, tc := range testCases {
561+
t.Run(tc.name, tc.testFunc)
562+
}
563+
}
564+
446565
func TestDescribeFileSystem(t *testing.T) {
447566
var (
448567
fsId = "fs-abcd1234"

pkg/cloud/fakes.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,3 +97,10 @@ func (c *FakeCloudProvider) DescribeMountTargets(ctx context.Context, fileSystem
9797

9898
return nil, ErrNotFound
9999
}
100+
101+
func (c *FakeCloudProvider) ListAccessPoints(ctx context.Context, fileSystemId string) ([]*AccessPoint, error) {
102+
accessPoints := []*AccessPoint{
103+
c.accessPoints[fileSystemId],
104+
}
105+
return accessPoints, nil
106+
}

pkg/driver/controller.go

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,13 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
9999
azName string
100100
basePath string
101101
err error
102-
gid int
102+
gid int64
103103
gidMin int
104104
gidMax int
105105
localCloud cloud.Cloud
106106
provisioningMode string
107107
roleArn string
108-
uid int
108+
uid int64
109109
)
110110

111111
//Parse parameters
@@ -149,7 +149,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
149149

150150
uid = -1
151151
if value, ok := volumeParams[Uid]; ok {
152-
uid, err = strconv.Atoi(value)
152+
uid, err = strconv.ParseInt(value, 10, 64)
153153
if err != nil {
154154
return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Uid, err)
155155
}
@@ -160,7 +160,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
160160

161161
gid = -1
162162
if value, ok := volumeParams[Gid]; ok {
163-
gid, err = strconv.Atoi(value)
163+
gid, err = strconv.ParseInt(value, 10, 64)
164164
if err != nil {
165165
return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Gid, err)
166166
}
@@ -233,9 +233,9 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
233233
return nil, status.Errorf(codes.Internal, "Failed to fetch File System info: %v", err)
234234
}
235235

236-
var allocatedGid int
236+
var allocatedGid int64
237237
if uid == -1 || gid == -1 {
238-
allocatedGid, err = d.gidAllocator.getNextGid(accessPointsOptions.FileSystemId, gidMin, gidMax)
238+
allocatedGid, err = d.gidAllocator.getNextGid(ctx, accessPointsOptions.FileSystemId, gidMin, gidMax)
239239
if err != nil {
240240
return nil, err
241241
}
@@ -283,15 +283,12 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
283283
}
284284
klog.Infof("Using %v as the access point directory.", rootDir)
285285

286-
accessPointsOptions.Uid = int64(uid)
287-
accessPointsOptions.Gid = int64(gid)
286+
accessPointsOptions.Uid = uid
287+
accessPointsOptions.Gid = gid
288288
accessPointsOptions.DirectoryPath = rootDir
289289

290290
accessPointId, err := localCloud.CreateAccessPoint(ctx, volName, accessPointsOptions)
291291
if err != nil {
292-
if allocatedGid != 0 {
293-
d.gidAllocator.releaseGid(accessPointsOptions.FileSystemId, gid)
294-
}
295292
if err == cloud.ErrAccessDenied {
296293
return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err)
297294
}

0 commit comments

Comments
 (0)