Skip to content

Commit

Permalink
Refactor re-use Access Point
Browse files Browse the repository at this point in the history 
When reuse access point flag is set to true, there is no point
in doing a lot of work that is aimed at preparing the options for
the createAccessPoint call (e.g. allocating a GID). We can save
quite some time and quite some API calls if the AP with the same
clientToken is found. The check for the AP existence has been moved
from the CreateAccessPoint function one level up. The controller
in this case checks if there is an access point with such client
token, if so, it proceeds to return a CreateVolumeResponse. If it
is not found, then it follows the usual flow.
  • Loading branch information
@otorreno
otorreno committed Jul 6, 2024
1 parent b86f12c commit 58c8272
Show file tree
Hide file tree
Showing 6 changed files with 355 additions and 274 deletions.
35 changes: 9 additions & 26 deletions pkg/cloud/cloud.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,10 @@ type Efs interface {

type Cloud interface {
GetMetadata() MetadataService
CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions, reuseAccessPoint bool) (accessPoint *AccessPoint, err error)
CreateAccessPoint(ctx context.Context, clientToken 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)
FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId 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)
Expand Down Expand Up @@ -164,26 +165,8 @@ func (c *cloud) GetMetadata() MetadataService {
return c.metadata
}

func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions, reuseAccessPoint bool) (accessPoint *AccessPoint, err error) {
func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error) {
efsTags := parseEfsTags(accessPointOpts.Tags)

//if reuseAccessPoint is true, check for AP with same Root Directory exists in efs
// if found reuse that AP
if reuseAccessPoint {
existingAP, err := c.findAccessPointByClientToken(ctx, clientToken, accessPointOpts)
if err != nil {
return nil, fmt.Errorf("failed to find access point: %v", err)
}
if existingAP != nil {
//AP path already exists
klog.V(2).Infof("Existing AccessPoint found : %+v", existingAP)
return &AccessPoint{
AccessPointId: existingAP.AccessPointId,
FileSystemId: existingAP.FileSystemId,
CapacityGiB: accessPointOpts.CapacityGiB,
}, nil
}
}
createAPInput := &efs.CreateAccessPointInput{
ClientToken: &clientToken,
FileSystemId: &accessPointOpts.FileSystemId,
Expand Down Expand Up @@ -262,22 +245,22 @@ func (c *cloud) DescribeAccessPoint(ctx context.Context, accessPointId string) (
}, nil
}

func (c *cloud) findAccessPointByClientToken(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error) {
klog.V(5).Infof("AccessPointOptions to find AP : %+v", accessPointOpts)
func (c *cloud) FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId string) (accessPoint *AccessPoint, err error) {
klog.V(5).Infof("Filesystem ID to find AP : %+v", fileSystemId)
klog.V(2).Infof("ClientToken to find AP : %s", clientToken)
describeAPInput := &efs.DescribeAccessPointsInput{
FileSystemId: &accessPointOpts.FileSystemId,
FileSystemId: &fileSystemId,
MaxResults: aws.Int64(AccessPointPerFsLimit),
}
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("failed to list Access Points of efs = %s : %v", accessPointOpts.FileSystemId, err)
err = fmt.Errorf("failed to list Access Points of efs = %s : %v", fileSystemId, err)
return
}
for _, ap := range res.AccessPoints {
Expand Down
181 changes: 118 additions & 63 deletions pkg/cloud/cloud_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestCreateAccessPoint(t *testing.T) {
testFunc func(t *testing.T)
}{
{
name: "Success - AP does not exist",
name: "Success",
testFunc: func(t *testing.T) {
mockCtl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockCtl)
Expand Down Expand Up @@ -74,63 +74,9 @@ func TestCreateAccessPoint(t *testing.T) {
},
}

describeAPOutput := &efs.DescribeAccessPointsOutput{
AccessPoints: nil,
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(describeAPOutput, nil)
mockEfs.EXPECT().CreateAccessPointWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
res, err := c.CreateAccessPoint(ctx, clientToken, req, true)

if err != nil {
t.Fatalf("CreateAccessPointFailed is failed: %v", err)
}

if res == nil {
t.Fatal("Result is nil")
}

if accessPointId != res.AccessPointId {
t.Fatalf("AccessPointId mismatched. Expected: %v, Actual: %v", accessPointId, res.AccessPointId)
}

if fsId != res.FileSystemId {
t.Fatalf("FileSystemId mismatched. Expected: %v, Actual: %v", fsId, res.FileSystemId)
}
mockCtl.Finish()
},
},
{
name: "Success - AP already exists",
testFunc: func(t *testing.T) {
mockCtl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockCtl)
c := &cloud{
efs: mockEfs,
}

tags := make(map[string]string)
tags["cluster"] = "efs"

req := &AccessPointOptions{
FileSystemId: fsId,
Uid: uid,
Gid: gid,
DirectoryPerms: directoryPerms,
DirectoryPath: directoryPath,
Tags: tags,
}

describeAPOutput := &efs.DescribeAccessPointsOutput{
AccessPoints: []*efs.AccessPointDescription{
{AccessPointId: aws.String(accessPointId), FileSystemId: aws.String(fsId), ClientToken: aws.String(clientToken), RootDirectory: &efs.RootDirectory{Path: aws.String(directoryPath)}, Tags: []*efs.Tag{{Key: aws.String(PvcNameTagKey), Value: aws.String(volName)}}},
},
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(describeAPOutput, nil)
res, err := c.CreateAccessPoint(ctx, clientToken, req, true)
res, err := c.CreateAccessPoint(ctx, clientToken, req)

if err != nil {
t.Fatalf("CreateAccessPointFailed is failed: %v", err)
Expand Down Expand Up @@ -164,14 +110,10 @@ func TestCreateAccessPoint(t *testing.T) {
DirectoryPerms: directoryPerms,
DirectoryPath: directoryPath,
}
describeAPOutput := &efs.DescribeAccessPointsOutput{
AccessPoints: nil,
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(describeAPOutput, nil)
mockEfs.EXPECT().CreateAccessPointWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, errors.New("CreateAccessPointWithContext failed"))
_, err := c.CreateAccessPoint(ctx, clientToken, req, true)
_, err := c.CreateAccessPoint(ctx, clientToken, req)
if err == nil {
t.Fatalf("CreateAccessPoint did not fail")
}
Expand All @@ -195,7 +137,7 @@ func TestCreateAccessPoint(t *testing.T) {

ctx := context.Background()
mockEfs.EXPECT().CreateAccessPointWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, awserr.New(AccessDeniedException, "Access Denied", errors.New("Access Denied")))
_, err := c.CreateAccessPoint(ctx, clientToken, req, false)
_, err := c.CreateAccessPoint(ctx, clientToken, req)
if err == nil {
t.Fatalf("CreateAccessPoint did not fail")
}
Expand Down Expand Up @@ -551,6 +493,119 @@ func TestDescribeAccessPoint(t *testing.T) {
}
}

func TestFindAccessPointByClientToken(t *testing.T) {
var (
fsId = "fs-abcd1234"
accessPointId = "ap-abc123"
clientToken = "token"
path = "/myDir"
Gid int64 = 1000
Uid int64 = 1000
)
testCases := []struct {
name string
testFunc func(t *testing.T)
}{
{
name: "Success - clientToken found",
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),
ClientToken: aws.String(clientToken),
RootDirectory: &efs.RootDirectory{
Path: aws.String(path),
},
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.FindAccessPointByClientToken(ctx, clientToken, fsId)
if err != nil {
t.Fatalf("Find Access Point by Client Token failed: %v", err)
}

if res == nil {
t.Fatal("Result is nil")
}

mockctl.Finish()
},
},
{
name: "Success - nil result if clientToken is not found",
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),
ClientToken: aws.String("differentToken"),
RootDirectory: &efs.RootDirectory{
Path: aws.String(path),
},
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.FindAccessPointByClientToken(ctx, clientToken, fsId)
if err != nil {
t.Fatalf("Find Access Point by Client Token failed: %v", err)
}

if res != nil {
t.Fatal("Result should be nil. No access point with the specified token")
}

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.FindAccessPointByClientToken(ctx, clientToken, fsId)
if err == nil {
t.Fatalf("Find Access Point by Client Token should have failed: %v", err)
}

mockctl.Finish()
},
},
}
for _, tc := range testCases {
t.Run(tc.name, tc.testFunc)
}
}

func TestListAccessPoints(t *testing.T) {
var (
fsId = "fs-abcd1234"
Expand Down Expand Up @@ -1024,7 +1079,7 @@ func Test_findAccessPointByPath(t *testing.T) {
tt.prepare(mockEfs)
}

gotAccessPoint, err := c.findAccessPointByClientToken(ctx, tt.args.clientToken, tt.args.accessPointOpts)
gotAccessPoint, err := c.FindAccessPointByClientToken(ctx, tt.args.clientToken, tt.args.accessPointOpts.FileSystemId)
if (err != nil) != tt.wantErr {
t.Errorf("findAccessPointByClientToken() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
10 changes: 9 additions & 1 deletion pkg/cloud/fakes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (c *FakeCloudProvider) GetMetadata() MetadataService {
return c.m
}

func (c *FakeCloudProvider) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions, usePvcName bool) (accessPoint *AccessPoint, err error) {
func (c *FakeCloudProvider) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error) {
ap, exists := c.accessPoints[clientToken]
if exists {
if accessPointOpts.CapacityGiB == ap.CapacityGiB {
Expand Down Expand Up @@ -98,6 +98,14 @@ func (c *FakeCloudProvider) DescribeMountTargets(ctx context.Context, fileSystem
return nil, ErrNotFound
}

func (c *FakeCloudProvider) FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId string) (accessPoint *AccessPoint, err error) {
if ap, exists := c.accessPoints[clientToken]; exists {
return ap, nil
} else {
return nil, nil
}
}

func (c *FakeCloudProvider) ListAccessPoints(ctx context.Context, fileSystemId string) ([]*AccessPoint, error) {
accessPoints := []*AccessPoint{
c.accessPoints[fileSystemId],
Expand Down

0 comments on commit 58c8272

Please sign in to comment.