diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 8053778f8..f0bd50add 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -204,6 +204,9 @@ func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, acces if isAccessDenied(err) { return nil, ErrAccessDenied } + if isAccessPointAlreadyExists(err) { + return nil, ErrAlreadyExists + } return nil, fmt.Errorf("Failed to create access point: %v", err) } klog.V(5).Infof("Create AP response : %+v", res) @@ -289,6 +292,10 @@ func (c *cloud) FindAccessPointByClientToken(ctx context.Context, clientToken, f AccessPointId: *ap.AccessPointId, FileSystemId: *ap.FileSystemId, AccessPointRootDir: *ap.RootDirectory.Path, + PosixUser: &PosixUser{ + Gid: *ap.PosixUser.Gid, + Uid: *ap.PosixUser.Uid, + }, }, nil } } @@ -435,6 +442,16 @@ func isAccessDenied(err error) bool { return false } +func isAccessPointAlreadyExists(err error) bool { + var apiErr smithy.APIError + if errors.As(err, &apiErr) { + if apiErr.ErrorCode() == AccessPointAlreadyExists { + return true + } + } + return false +} + func isDriverBootedInECS() bool { ecsContainerMetadataUri := os.Getenv(taskMetadataV4EnvName) return ecsContainerMetadataUri != "" diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index 2a1889778..bc833fd37 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -158,6 +158,38 @@ func TestCreateAccessPoint(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Error: Access Point already exists", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockEfs := mocks.NewMockEfs(mockCtl) + c := &cloud{efs: mockEfs} + + req := &AccessPointOptions{ + FileSystemId: fsId, + Uid: uid, + Gid: gid, + DirectoryPerms: directoryPerms, + DirectoryPath: directoryPath, + } + + ctx := context.Background() + mockEfs.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, + &smithy.GenericAPIError{ + Code: AccessPointAlreadyExists, + Message: "Access point already exists", + }) + + _, err := c.CreateAccessPoint(ctx, clientToken, req) + if err == nil { + t.Fatalf("CreateAccessPoint did not return error") + } + if err != ErrAlreadyExists { + t.Fatalf("Failed. Expected: %v, Actual:%v", ErrAlreadyExists, err) + } + mockCtl.Finish() + }, + }, } for _, tc := range testCases { @@ -1085,6 +1117,16 @@ func Test_findAccessPointByPath(t *testing.T) { FileSystemId: fsId, } + expectedSingleAPWithPosixUser := &AccessPoint{ + AccessPointId: "testApId", + AccessPointRootDir: dirPath, + PosixUser: &PosixUser{ + Uid: 1000, + Gid: 1000, + }, + FileSystemId: fsId, + } + type args struct { clientToken string accessPointOpts *AccessPointOptions @@ -1105,10 +1147,16 @@ func Test_findAccessPointByPath(t *testing.T) { mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return(&efs.DescribeAccessPointsOutput{ AccessPoints: []types.AccessPointDescription{ {FileSystemId: aws.String(fsId), ClientToken: diffClientToken, AccessPointId: aws.String("differentApId"), RootDirectory: &types.RootDirectory{Path: aws.String(expectedSingleAP.AccessPointRootDir)}}, - {FileSystemId: aws.String(fsId), ClientToken: &clientToken, AccessPointId: aws.String(expectedSingleAP.AccessPointId), RootDirectory: &types.RootDirectory{Path: aws.String(expectedSingleAP.AccessPointRootDir)}}, + { + FileSystemId: aws.String(fsId), + ClientToken: &clientToken, + AccessPointId: aws.String(expectedSingleAPWithPosixUser.AccessPointId), + RootDirectory: &types.RootDirectory{Path: aws.String(expectedSingleAPWithPosixUser.AccessPointRootDir)}, + PosixUser: &types.PosixUser{Gid: aws.Int64(1000), Uid: aws.Int64(1000)}, + }, }, }, nil) - }, wantAccessPoint: expectedSingleAP, wantErr: false}, + }, wantAccessPoint: expectedSingleAPWithPosixUser, wantErr: false}, {name: "Fail_DescribeAccessPoints", args: args{clientToken, &AccessPointOptions{FileSystemId: fsId, DirectoryPath: dirPath}}, prepare: func(mockEfs *mocks.MockEfs) { mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("access_denied")) }, wantAccessPoint: nil, wantErr: true}, diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 4ecbef96b..07666416c 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -351,11 +351,23 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) if err != nil { if err == cloud.ErrAccessDenied { return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err) + } else if err == cloud.ErrAlreadyExists { + klog.V(4).Infof("Access point already exists for client token %s. Retrieving existing access point details.", clientToken) + existingAccessPoint, err := localCloud.FindAccessPointByClientToken(ctx, clientToken, accessPointsOptions.FileSystemId) + if err != nil { + return nil, fmt.Errorf("Error attempting to retrieve existing access point for client token %s: %v", clientToken, err) + } + if existingAccessPoint == nil { + return nil, fmt.Errorf("No access point for client token %s was returned: %v", clientToken, err) + } + err = validateExistingAccessPoint(existingAccessPoint, basePath, gidMin, gidMax) + if err != nil { + return nil, status.Errorf(codes.AlreadyExists, "Invalid existing access point: %v", err) + } + accessPoint = existingAccessPoint + } else { + return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err) } - if err == cloud.ErrAlreadyExists { - return nil, status.Errorf(codes.AlreadyExists, "Access Point already exists") - } - return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err) } // Lock on the new access point to prevent accidental deletion before creation is done @@ -711,3 +723,26 @@ func get64LenHash(text string) string { h.Write([]byte(text)) return fmt.Sprintf("%x", h.Sum(nil)) } + +func validateExistingAccessPoint(existingAccessPoint *cloud.AccessPoint, basePath string, gidMin int64, gidMax int64) error { + + normalizedBasePath := strings.TrimPrefix(basePath, "/") + normalizedAccessPointPath := strings.TrimPrefix(existingAccessPoint.AccessPointRootDir, "/") + if !strings.HasPrefix(normalizedAccessPointPath, normalizedBasePath) { + return fmt.Errorf("Access point found but has different base path than what's specified in storage class") + } + + if existingAccessPoint.PosixUser == nil { + return fmt.Errorf("Access point found but PosixUser is nil") + } + + if existingAccessPoint.PosixUser.Gid < gidMin || existingAccessPoint.PosixUser.Gid > gidMax { + return fmt.Errorf("Access point found but its GID is outside the specified range") + } + + if existingAccessPoint.PosixUser.Uid < gidMin || existingAccessPoint.PosixUser.Uid > gidMax { + return fmt.Errorf("Access point found but its UID is outside the specified range") + } + + return nil +} diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 1629f7dd6..4413234f6 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -3052,6 +3052,304 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Success: Reuse existing access point", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + lockManager: NewLockManagerMap(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + BasePath: "/test", + DirectoryPerms: "777", + PvcNameKey: "test-pvc", + }, + } + + existingAP := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + AccessPointRootDir: "/test/directory", + PosixUser: &cloud.PosixUser{ + Uid: 1000, + Gid: 1500, + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Any(), gomock.Any()).Return(nil, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAlreadyExists) + mockCloud.EXPECT().FindAccessPointByClientToken(gomock.Any(), gomock.Any(), fsId).Return(existingAP, nil) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + expectedVolumeId := fsId + "::" + apId + if res.Volume.VolumeId != expectedVolumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", expectedVolumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, + { + name: "Success: Reuse existing access point no leading slash in base path", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + lockManager: NewLockManagerMap(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + BasePath: "test", + DirectoryPerms: "777", + PvcNameKey: "test-pvc", + }, + } + + existingAP := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + AccessPointRootDir: "/test/directory", + PosixUser: &cloud.PosixUser{ + Uid: 1000, + Gid: 1500, + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Any(), gomock.Any()).Return(nil, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAlreadyExists) + mockCloud.EXPECT().FindAccessPointByClientToken(gomock.Any(), gomock.Any(), fsId).Return(existingAP, nil) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + expectedVolumeId := fsId + "::" + apId + if res.Volume.VolumeId != expectedVolumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", expectedVolumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, + { + name: "Fail: Reuse existing access point with GID outside specified range", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + lockManager: NewLockManagerMap(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + BasePath: "/test", + DirectoryPerms: "777", + PvcNameKey: "test-pvc", + }, + } + + existingAP := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + AccessPointRootDir: "/test/directory", + PosixUser: &cloud.PosixUser{ + Uid: 1500, + Gid: 2500, // Outside the specified range + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Any(), gomock.Any()).Return(nil, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAlreadyExists) + mockCloud.EXPECT().FindAccessPointByClientToken(gomock.Any(), gomock.Any(), fsId).Return(existingAP, nil) + + _, err := driver.CreateVolume(ctx, req) + + if err == nil { + t.Fatal("CreateVolume should have failed due to invalid GID") + } + + mockCtl.Finish() + }, + }, + { + name: "Fail: Reuse existing access point with UID outside specified range", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + lockManager: NewLockManagerMap(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + BasePath: "/test", + DirectoryPerms: "777", + PvcNameKey: "test-pvc", + }, + } + + existingAP := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + AccessPointRootDir: "/test/directory", + PosixUser: &cloud.PosixUser{ + Uid: 2500, // Outside the specified range + Gid: 1500, + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Any(), gomock.Any()).Return(nil, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAlreadyExists) + mockCloud.EXPECT().FindAccessPointByClientToken(gomock.Any(), gomock.Any(), fsId).Return(existingAP, nil) + + _, err := driver.CreateVolume(ctx, req) + + if err == nil { + t.Fatal("CreateVolume should have failed due to invalid UID") + } + + mockCtl.Finish() + }, + }, + { + name: "Fail: Reuse existing access point with different basepath from storageclass", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + lockManager: NewLockManagerMap(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + BasePath: "/test", + DirectoryPerms: "777", + PvcNameKey: "test-pvc", + }, + } + + existingAP := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + AccessPointRootDir: "/wrong/directory", + PosixUser: &cloud.PosixUser{ + Uid: 1500, + Gid: 2500, // Outside the specified range + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Any(), gomock.Any()).Return(nil, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAlreadyExists) + mockCloud.EXPECT().FindAccessPointByClientToken(gomock.Any(), gomock.Any(), fsId).Return(existingAP, nil) + + _, err := driver.CreateVolume(ctx, req) + + if err == nil { + t.Fatal("CreateVolume should have failed due to invalid base path") + } + + mockCtl.Finish() + }, + }, } for _, tc := range testCases {