Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor re-use Access Point #1233

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Refactor re-use Access Point #1233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 9 additions & 26 deletions pkg/cloud/cloud.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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