diff --git a/Dockerfile b/Dockerfile index 71e10a507..719bffe4f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -38,7 +38,11 @@ RUN mkdir -p /tmp/rpms && \ then echo "Installing efs-utils from Amazon Linux 2 yum repo" && \ yum -y install --downloadonly --downloaddir=/tmp/rpms amazon-efs-utils-1.35.0-1.amzn2.noarch; \ else echo "Installing efs-utils from github using the latest git tag" && \ - yum -y install git rpm-build make rust cargo openssl-devel && \ + yum -y install git rpm-build make openssl-devel curl && \ + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \ + source $HOME/.cargo/env && \ + rustup update && \ + rustup default stable && \ git clone https://github.com/aws/efs-utils && \ cd efs-utils && \ git checkout $(git describe --tags $(git rev-list --tags --max-count=1)) && \ diff --git a/charts/aws-efs-csi-driver/templates/node-daemonset.yaml b/charts/aws-efs-csi-driver/templates/node-daemonset.yaml index 89259137c..aab2dd537 100644 --- a/charts/aws-efs-csi-driver/templates/node-daemonset.yaml +++ b/charts/aws-efs-csi-driver/templates/node-daemonset.yaml @@ -5,6 +5,9 @@ metadata: name: efs-csi-node labels: app.kubernetes.io/name: {{ include "aws-efs-csi-driver.name" . }} + {{- with .Values.node.additionalLabels }} + {{ toYaml . | nindent 4 }} + {{- end }} spec: selector: matchLabels: diff --git a/charts/aws-efs-csi-driver/values.yaml b/charts/aws-efs-csi-driver/values.yaml index f6f8f8cfe..51fae0856 100644 --- a/charts/aws-efs-csi-driver/values.yaml +++ b/charts/aws-efs-csi-driver/values.yaml @@ -142,6 +142,7 @@ node: # nameservers: # - 169.254.169.253 podAnnotations: {} + additionalLabels: {} resources: {} # limits: diff --git a/examples/kubernetes/statefulset/specs/example.yaml b/examples/kubernetes/statefulset/specs/example.yaml index 09cc38fdb..20a8dac66 100644 --- a/examples/kubernetes/statefulset/specs/example.yaml +++ b/examples/kubernetes/statefulset/specs/example.yaml @@ -1,4 +1,4 @@ -apiVersion: v1 +apiVersion: storage.k8s.io/v1 kind: StorageClass metadata: name: efs-sc diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 5220ef51d..240b80202 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -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) @@ -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, @@ -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 { diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index e16cc5c63..48651edbf 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -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) @@ -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) @@ -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") } @@ -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") } @@ -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" @@ -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 diff --git a/pkg/cloud/fakes.go b/pkg/cloud/fakes.go index 49953665b..8f910ca88 100644 --- a/pkg/cloud/fakes.go +++ b/pkg/cloud/fakes.go @@ -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 { @@ -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], diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index be4630e63..86c6baaf1 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -144,21 +144,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", ProvisioningMode) } - // Create tags - tags := map[string]string{ - DefaultTagKey: DefaultTagValue, - } - - // Append input tags to default tag - if len(d.tags) != 0 { - for k, v := range d.tags { - tags[k] = v - } - } - accessPointsOptions := &cloud.AccessPointOptions{ CapacityGiB: volSize, - Tags: tags, } if value, ok := volumeParams[FsId]; ok { @@ -170,162 +157,197 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", FsId) } - uid = -1 - if value, ok := volumeParams[Uid]; ok { - uid, err = strconv.ParseInt(value, 10, 64) - if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Uid, err) - } - if uid < 0 { - return nil, status.Errorf(codes.InvalidArgument, "%v must be greater or equal than 0", Uid) - } + localCloud, roleArn, crossAccountDNSEnabled, err = getCloud(req.GetSecrets(), d) + if err != nil { + return nil, err } - gid = -1 - if value, ok := volumeParams[Gid]; ok { - gid, err = strconv.ParseInt(value, 10, 64) + var accessPoint *cloud.AccessPoint + //if reuseAccessPoint is true, check for AP with same Root Directory exists in efs + // if found reuse that AP + if reuseAccessPoint { + existingAP, err := localCloud.FindAccessPointByClientToken(ctx, clientToken, accessPointsOptions.FileSystemId) if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Gid, err) + return nil, fmt.Errorf("failed to find access point: %v", err) } - if gid < 0 { - return nil, status.Errorf(codes.InvalidArgument, "%v must be greater or equal than 0", Gid) + if existingAP != nil { + //AP path already exists + klog.V(2).Infof("Existing AccessPoint found : %+v", existingAP) + accessPoint = &cloud.AccessPoint{ + AccessPointId: existingAP.AccessPointId, + FileSystemId: existingAP.FileSystemId, + CapacityGiB: accessPointsOptions.CapacityGiB, + } } } - if value, ok := volumeParams[GidMin]; ok { - gidMin, err = strconv.ParseInt(value, 10, 64) - if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMin, err) - } - if gidMin <= 0 { - return nil, status.Errorf(codes.InvalidArgument, "%v must be greater than 0", GidMin) + if accessPoint == nil { + // Create tags + tags := map[string]string{ + DefaultTagKey: DefaultTagValue, } - } - if value, ok := volumeParams[GidMax]; ok { - // Ensure GID min is provided with GID max - if gidMin == 0 { - return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", GidMin) + // Append input tags to default tag + if len(d.tags) != 0 { + for k, v := range d.tags { + tags[k] = v + } } - gidMax, err = strconv.ParseInt(value, 10, 64) - if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMax, err) + + accessPointsOptions.Tags = tags + + uid = -1 + if value, ok := volumeParams[Uid]; ok { + uid, err = strconv.ParseInt(value, 10, 64) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Uid, err) + } + if uid < 0 { + return nil, status.Errorf(codes.InvalidArgument, "%v must be greater or equal than 0", Uid) + } } - if gidMax <= gidMin { - return nil, status.Errorf(codes.InvalidArgument, "%v must be greater than %v", GidMax, GidMin) + + gid = -1 + if value, ok := volumeParams[Gid]; ok { + gid, err = strconv.ParseInt(value, 10, 64) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", Gid, err) + } + if uid < 0 { + return nil, status.Errorf(codes.InvalidArgument, "%v must be greater or equal than 0", Gid) + } } - } else { - // Ensure GID max is provided with GID min - if gidMin != 0 { - return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", GidMax) + + if value, ok := volumeParams[GidMin]; ok { + gidMin, err = strconv.ParseInt(value, 10, 64) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMin, err) + } + if gidMin <= 0 { + return nil, status.Errorf(codes.InvalidArgument, "%v must be greater than 0", GidMin) + } } - } - // Assign default GID ranges if not provided - if gidMin == 0 && gidMax == 0 { - gidMin = DefaultGidMin - gidMax = DefaultGidMax - } + if value, ok := volumeParams[GidMax]; ok { + // Ensure GID min is provided with GID max + if gidMin == 0 { + return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", GidMin) + } + gidMax, err = strconv.ParseInt(value, 10, 64) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMax, err) + } + if gidMax <= gidMin { + return nil, status.Errorf(codes.InvalidArgument, "%v must be greater than %v", GidMax, GidMin) + } + } else { + // Ensure GID max is provided with GID min + if gidMin != 0 { + return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", GidMax) + } + } - if value, ok := volumeParams[DirectoryPerms]; ok { - accessPointsOptions.DirectoryPerms = value - } + // Assign default GID ranges if not provided + if gidMin == 0 && gidMax == 0 { + gidMin = DefaultGidMin + gidMax = DefaultGidMax + } - // Storage class parameter `az` will be used to fetch preferred mount target for cross account mount. - // If the `az` storage class parameter is not provided, a random mount target will be picked for mounting. - // This storage class parameter different from `az` mount option provided by efs-utils https://github.com/aws/efs-utils/blob/v1.31.1/src/mount_efs/__init__.py#L195 - // The `az` mount option provided by efs-utils is used for cross az mount or to provide az of efs one zone file system mount within the same aws-account. - // To make use of the `az` mount option, add it under storage class's `mountOptions` section. https://kubernetes.io/docs/concepts/storage/storage-classes/#mount-options - if value, ok := volumeParams[AzName]; ok { - azName = value - } + if value, ok := volumeParams[DirectoryPerms]; ok { + accessPointsOptions.DirectoryPerms = value + } - localCloud, roleArn, crossAccountDNSEnabled, err = getCloud(req.GetSecrets(), d) - if err != nil { - return nil, err - } + // Storage class parameter `az` will be used to fetch preferred mount target for cross account mount. + // If the `az` storage class parameter is not provided, a random mount target will be picked for mounting. + // This storage class parameter different from `az` mount option provided by efs-utils https://github.com/aws/efs-utils/blob/v1.31.1/src/mount_efs/__init__.py#L195 + // The `az` mount option provided by efs-utils is used for cross az mount or to provide az of efs one zone file system mount within the same aws-account. + // To make use of the `az` mount option, add it under storage class's `mountOptions` section. https://kubernetes.io/docs/concepts/storage/storage-classes/#mount-options + if value, ok := volumeParams[AzName]; ok { + azName = value + } - // Check if file system exists. Describe FS or List APs handle appropriate error codes - // With dynamic uid/gid provisioning we can save a call to describe FS, as list APs fails if FS ID does not exist - var accessPoints []*cloud.AccessPoint - if uid == -1 || gid == -1 { - accessPoints, err = localCloud.ListAccessPoints(ctx, accessPointsOptions.FileSystemId) - } else { - _, err = localCloud.DescribeFileSystem(ctx, accessPointsOptions.FileSystemId) - } - 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) + // Check if file system exists. Describe FS or List APs handle appropriate error codes + // With dynamic uid/gid provisioning we can save a call to describe FS, as list APs fails if FS ID does not exist + var accessPoints []*cloud.AccessPoint + if uid == -1 || gid == -1 { + accessPoints, err = localCloud.ListAccessPoints(ctx, accessPointsOptions.FileSystemId) + } else { + _, err = localCloud.DescribeFileSystem(ctx, accessPointsOptions.FileSystemId) } - if err == cloud.ErrNotFound { - return nil, status.Errorf(codes.InvalidArgument, "File System does not exist: %v", err) + 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) + } + if err == cloud.ErrNotFound { + return nil, status.Errorf(codes.InvalidArgument, "File System does not exist: %v", err) + } + return nil, status.Errorf(codes.Internal, "Failed to fetch Access Points or Describe File System: %v", err) } - return nil, status.Errorf(codes.Internal, "Failed to fetch Access Points or Describe File System: %v", err) - } - var allocatedGid int64 - if uid == -1 || gid == -1 { - allocatedGid, err = d.gidAllocator.getNextGid(accessPointsOptions.FileSystemId, accessPoints, gidMin, gidMax) - if err != nil { - return nil, err + var allocatedGid int64 + if uid == -1 || gid == -1 { + allocatedGid, err = d.gidAllocator.getNextGid(accessPointsOptions.FileSystemId, accessPoints, gidMin, gidMax) + if err != nil { + return nil, err + } + } + if uid == -1 { + uid = allocatedGid + } + if gid == -1 { + gid = allocatedGid } - } - if uid == -1 { - uid = allocatedGid - } - if gid == -1 { - gid = allocatedGid - } - if value, ok := volumeParams[BasePath]; ok { - basePath = value - } + if value, ok := volumeParams[BasePath]; ok { + basePath = value + } - rootDirName := volName - // Check if a custom structure should be imposed on the access point directory - if value, ok := volumeParams[SubPathPattern]; ok { - // Try and construct the root directory and check it only contains supported components - val, err := interpolateRootDirectoryName(value, volumeParams) - if err == nil { - klog.Infof("Using user-specified structure for access point directory.") - rootDirName = val - if value, ok := volumeParams[EnsureUniqueDirectory]; ok { - if ensureUniqueDirectory, err := strconv.ParseBool(value); !ensureUniqueDirectory && err == nil { - klog.Infof("Not appending PVC UID to path.") + rootDirName := volName + // Check if a custom structure should be imposed on the access point directory + if value, ok := volumeParams[SubPathPattern]; ok { + // Try and construct the root directory and check it only contains supported components + val, err := interpolateRootDirectoryName(value, volumeParams) + if err == nil { + klog.Infof("Using user-specified structure for access point directory.") + rootDirName = val + if value, ok := volumeParams[EnsureUniqueDirectory]; ok { + if ensureUniqueDirectory, err := strconv.ParseBool(value); !ensureUniqueDirectory && err == nil { + klog.Infof("Not appending PVC UID to path.") + } else { + klog.Infof("Appending PVC UID to path.") + rootDirName = fmt.Sprintf("%s-%s", val, uuid.New().String()) + } } else { klog.Infof("Appending PVC UID to path.") rootDirName = fmt.Sprintf("%s-%s", val, uuid.New().String()) } } else { - klog.Infof("Appending PVC UID to path.") - rootDirName = fmt.Sprintf("%s-%s", val, uuid.New().String()) + return nil, err } } else { - return nil, err + klog.Infof("Using PV name for access point directory.") } - } else { - klog.Infof("Using PV name for access point directory.") - } - rootDir := path.Join("/", basePath, rootDirName) - if ok, err := validateEfsPathRequirements(rootDir); !ok { - return nil, err - } - klog.Infof("Using %v as the access point directory.", rootDir) + rootDir := path.Join("/", basePath, rootDirName) + if ok, err := validateEfsPathRequirements(rootDir); !ok { + return nil, err + } + klog.Infof("Using %v as the access point directory.", rootDir) - accessPointsOptions.Uid = uid - accessPointsOptions.Gid = gid - accessPointsOptions.DirectoryPath = rootDir + accessPointsOptions.Uid = uid + accessPointsOptions.Gid = gid + accessPointsOptions.DirectoryPath = rootDir - accessPointId, err := localCloud.CreateAccessPoint(ctx, clientToken, accessPointsOptions, reuseAccessPoint) - 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) - } - if err == cloud.ErrAlreadyExists { - return nil, status.Errorf(codes.AlreadyExists, "Access Point already exists") + accessPoint, err = localCloud.CreateAccessPoint(ctx, clientToken, accessPointsOptions) + 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) + } + 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) } - return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err) } volContext := map[string]string{} @@ -352,7 +374,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return &csi.CreateVolumeResponse{ Volume: &csi.Volume{ CapacityBytes: volSize, - VolumeId: accessPointsOptions.FileSystemId + "::" + accessPointId.AccessPointId, + VolumeId: accessPointsOptions.FileSystemId + "::" + accessPoint.AccessPointId, VolumeContext: volContext, }, }, nil diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 3725ce026..b625f508b 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -79,8 +79,8 @@ func TestCreateVolume(t *testing.T) { FileSystemId: fsId, } mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any(), gomock.Eq(false)).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointsOptions *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointsOptions *cloud.AccessPointOptions) { if accessPointsOptions.Uid != 1000 { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", 1000, accessPointsOptions.Uid) } @@ -146,8 +146,8 @@ func TestCreateVolume(t *testing.T) { FileSystemId: fsId, } mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != 1000 { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", 1000, accessPointOpts.Uid) } @@ -228,8 +228,8 @@ func TestCreateVolume(t *testing.T) { var expectedGid int64 = 1003 //1001 and 1002 are taken, next available is 1003 mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != expectedGid { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid) } @@ -323,8 +323,8 @@ func TestCreateVolume(t *testing.T) { var expectedGid int64 = 1004 // 1001-1003 is taken. mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap2, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(ap2, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != expectedGid { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid) } @@ -340,8 +340,8 @@ func TestCreateVolume(t *testing.T) { expectedGid = 1001 // 1001 is now free and lowest possible, if no GID return would happen allocator would pick 1005. mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap3, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(ap3, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != expectedGid { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid) } @@ -357,8 +357,8 @@ func TestCreateVolume(t *testing.T) { expectedGid = 1002 // 1001 and 1004 are now taken, lowest available is 1002 mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap2, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(ap2, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != expectedGid { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid) } @@ -444,8 +444,8 @@ func TestCreateVolume(t *testing.T) { expectedGid := 2000 mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(lastAccessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(lastAccessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != int64(expectedGid) { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid) } @@ -512,8 +512,8 @@ func TestCreateVolume(t *testing.T) { expectedGid := 1000 // Allocator should pick lowest available GID mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.Uid != int64(expectedGid) { t.Fatalf("Uid mismatched. Expected: %v, actual: %v", expectedGid, accessPointOpts.Uid) } @@ -574,7 +574,7 @@ func TestCreateVolume(t *testing.T) { } accessPoints := []*cloud.AccessPoint{accessPoint} mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any(), gomock.Any()).Return(accessPoint, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any()).Return(accessPoint, nil) res, err := driver.CreateVolume(ctx, req) @@ -630,7 +630,7 @@ func TestCreateVolume(t *testing.T) { } accessPoints := []*cloud.AccessPoint{accessPoint} mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil) res, err := driver.CreateVolume(ctx, req) @@ -689,7 +689,7 @@ func TestCreateVolume(t *testing.T) { } accessPoints := []*cloud.AccessPoint{accessPoint} mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil) res, err := driver.CreateVolume(ctx, req) @@ -748,7 +748,7 @@ func TestCreateVolume(t *testing.T) { } accessPoints := []*cloud.AccessPoint{accessPoint} mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil) res, err := driver.CreateVolume(ctx, req) @@ -810,9 +810,7 @@ func TestCreateVolume(t *testing.T) { Uid: 1000, }, } - accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(get64LenHash(pvcNameVal)), gomock.Any(), gomock.Any()).Return(accessPoint, nil) + mockCloud.EXPECT().FindAccessPointByClientToken(gomock.Eq(ctx), gomock.Any(), gomock.Eq(fsId)).Return(accessPoint, nil) res, err := driver.CreateVolume(ctx, req) @@ -875,8 +873,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPoint bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, @@ -943,8 +941,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, @@ -1014,8 +1012,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, @@ -1085,8 +1083,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.DirectoryPath != directoryCreated { t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", directoryCreated, @@ -1155,8 +1153,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, @@ -1220,8 +1218,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.DirectoryPath != "/" { t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", "/", @@ -1286,8 +1284,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if accessPointOpts.DirectoryPath != "/" { t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", "/", @@ -1354,8 +1352,8 @@ func TestCreateVolume(t *testing.T) { } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). - Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) { if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, @@ -2366,7 +2364,7 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{}, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("CreateAccessPoint call failed")) + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(nil, errors.New("CreateAccessPoint call failed")) _, err := driver.CreateVolume(ctx, req) if err == nil { t.Fatal("CreateVolume did not fail") @@ -2405,7 +2403,7 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{}, nil) - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAccessDenied) + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAccessDenied) _, err := driver.CreateVolume(ctx, req) if err == nil { t.Fatal("CreateVolume did not fail") @@ -2460,7 +2458,7 @@ func TestCreateVolume(t *testing.T) { }, } mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{ap1, ap2}, nil).AnyTimes() - mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(ap2, nil).AnyTimes() + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(ap2, nil).AnyTimes() var err error // All GIDs from available range are taken, CreateVolume should fail. diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index a3510beb7..6800384ad 100644 --- a/pkg/driver/driver.go +++ b/pkg/driver/driver.go @@ -20,6 +20,7 @@ import ( "context" "net" "strings" + "time" "github.com/container-storage-interface/spec/lib/go/csi" "google.golang.org/grpc" @@ -129,10 +130,9 @@ func (d *Driver) Run() error { // Remove taint from node to indicate driver startup success // This is done at the last possible moment to prevent race conditions or false positive removals - err = removeNotReadyTaint(cloud.DefaultKubernetesAPIClient) - if err != nil { - klog.ErrorS(err, "Unexpected failure when attempting to remove node taint(s)") - } + go tryRemoveNotReadyTaintUntilSucceed(time.Second, func() error { + return removeNotReadyTaint(cloud.DefaultKubernetesAPIClient) + }) klog.Infof("Listening for connections on address: %#v", listener.Addr()) return d.srv.Serve(listener) diff --git a/pkg/driver/efs_watch_dog.go b/pkg/driver/efs_watch_dog.go index 55f8339ae..e4083c8d6 100644 --- a/pkg/driver/efs_watch_dog.go +++ b/pkg/driver/efs_watch_dog.go @@ -64,9 +64,16 @@ stunnel_check_cert_hostname = true # Use OCSP to check certificate validity. This option is not supported by certain stunnel versions. stunnel_check_cert_validity = false +# Enable FIPS mode. stunnel complains if FIPS is available and enabled system-wide, but not set here. +{{if .FipsEnabled -}} +fips_mode_enabled = {{.FipsEnabled -}} +{{else -}} +#fips_mode_enabled = false +{{- end}} + # Define the port range that the TLS tunnel will choose from port_range_lower_bound = 20049 -port_range_upper_bound = 20449 +port_range_upper_bound = 21049 # Optimize read_ahead_kb for Linux 5.4+ optimize_readahead = true @@ -163,6 +170,7 @@ type execWatchdog struct { type efsUtilsConfig struct { EfsClientSource string Region string + FipsEnabled string } func newExecWatchdog(efsUtilsCfgPath, efsUtilsStaticFilesPath, cmd string, arg ...string) Watchdog { @@ -264,7 +272,8 @@ func (w *execWatchdog) updateConfig(efsClientSource string) error { defer f.Close() // used on Fargate, IMDS queries suffice otherwise region := os.Getenv("AWS_DEFAULT_REGION") - efsCfg := efsUtilsConfig{EfsClientSource: efsClientSource, Region: region} + fipsEnabled := os.Getenv("FIPS_ENABLED") + efsCfg := efsUtilsConfig{EfsClientSource: efsClientSource, Region: region, FipsEnabled: fipsEnabled} if err = efsCfgTemplate.Execute(f, efsCfg); err != nil { return fmt.Errorf("cannot update config %s for efs-utils. Error: %v", w.efsUtilsCfgPath, err) } diff --git a/pkg/driver/efs_watch_dog_test.go b/pkg/driver/efs_watch_dog_test.go index fac37a272..38525d7be 100644 --- a/pkg/driver/efs_watch_dog_test.go +++ b/pkg/driver/efs_watch_dog_test.go @@ -54,9 +54,12 @@ stunnel_check_cert_hostname = true # Use OCSP to check certificate validity. This option is not supported by certain stunnel versions. stunnel_check_cert_validity = false +# Enable FIPS mode. stunnel complains if FIPS is available and enabled system-wide, but not set here. +#fips_mode_enabled = false + # Define the port range that the TLS tunnel will choose from port_range_lower_bound = 20049 -port_range_upper_bound = 20449 +port_range_upper_bound = 21049 # Optimize read_ahead_kb for Linux 5.4+ optimize_readahead = true diff --git a/pkg/driver/mocks/mock_cloud.go b/pkg/driver/mocks/mock_cloud.go index eacf69fae..96cef7e69 100644 --- a/pkg/driver/mocks/mock_cloud.go +++ b/pkg/driver/mocks/mock_cloud.go @@ -162,18 +162,18 @@ func (m *MockCloud) EXPECT() *MockCloudMockRecorder { } // CreateAccessPoint mocks base method. -func (m *MockCloud) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, usePvcName bool) (*cloud.AccessPoint, error) { +func (m *MockCloud) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions) (*cloud.AccessPoint, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateAccessPoint", ctx, clientToken, accessPointOpts, usePvcName) + ret := m.ctrl.Call(m, "CreateAccessPoint", ctx, clientToken, accessPointOpts) ret0, _ := ret[0].(*cloud.AccessPoint) ret1, _ := ret[1].(error) return ret0, ret1 } // CreateAccessPoint indicates an expected call of CreateAccessPoint. -func (mr *MockCloudMockRecorder) CreateAccessPoint(ctx, clientToken, accessPointOpts, usePvcName interface{}) *gomock.Call { +func (mr *MockCloudMockRecorder) CreateAccessPoint(ctx, clientToken, accessPointOpts interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateAccessPoint", reflect.TypeOf((*MockCloud)(nil).CreateAccessPoint), ctx, clientToken, accessPointOpts, usePvcName) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateAccessPoint", reflect.TypeOf((*MockCloud)(nil).CreateAccessPoint), ctx, clientToken, accessPointOpts) } // DeleteAccessPoint mocks base method. @@ -235,6 +235,21 @@ func (mr *MockCloudMockRecorder) DescribeMountTargets(ctx, fileSystemId, az inte return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeMountTargets", reflect.TypeOf((*MockCloud)(nil).DescribeMountTargets), ctx, fileSystemId, az) } +// FindAccessPointByClientToken mocks base method. +func (m *MockCloud) FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId string) (*cloud.AccessPoint, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FindAccessPointByClientToken", ctx, clientToken, fileSystemId) + ret0, _ := ret[0].(*cloud.AccessPoint) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FindAccessPointByClientToken indicates an expected call of FindAccessPointByClientToken. +func (mr *MockCloudMockRecorder) FindAccessPointByClientToken(ctx, clientToken, fileSystemId interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindAccessPointByClientToken", reflect.TypeOf((*MockCloud)(nil).FindAccessPointByClientToken), ctx, clientToken, fileSystemId) +} + // GetMetadata mocks base method. func (m *MockCloud) GetMetadata() cloud.MetadataService { m.ctrl.T.Helper() @@ -250,16 +265,16 @@ func (mr *MockCloudMockRecorder) GetMetadata() *gomock.Call { } // ListAccessPoints mocks base method. -func (m *MockCloud) ListAccessPoints(arg0 context.Context, arg1 string) ([]*cloud.AccessPoint, error) { +func (m *MockCloud) ListAccessPoints(ctx context.Context, fileSystemId string) ([]*cloud.AccessPoint, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListAccessPoints", arg0, arg1) + ret := m.ctrl.Call(m, "ListAccessPoints", ctx, fileSystemId) ret0, _ := ret[0].([]*cloud.AccessPoint) ret1, _ := ret[1].(error) return ret0, ret1 } // ListAccessPoints indicates an expected call of ListAccessPoints. -func (mr *MockCloudMockRecorder) ListAccessPoints(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockCloudMockRecorder) ListAccessPoints(ctx, fileSystemId interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAccessPoints", reflect.TypeOf((*MockCloud)(nil).ListAccessPoints), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAccessPoints", reflect.TypeOf((*MockCloud)(nil).ListAccessPoints), ctx, fileSystemId) } diff --git a/pkg/driver/node.go b/pkg/driver/node.go index d080d0d47..b730bff0d 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -25,6 +25,7 @@ import ( "path/filepath" "strconv" "strings" + "time" "github.com/container-storage-interface/spec/lib/go/csi" "github.com/kubernetes-sigs/aws-efs-csi-driver/pkg/cloud" @@ -464,7 +465,7 @@ type JSONPatch struct { Value interface{} `json:"value"` } -// removeNotReadyTaint removes the taint ebs.csi.aws.com/agent-not-ready from the local node +// removeNotReadyTaint removes the taint efs.csi.aws.com/agent-not-ready from the local node // This taint can be optionally applied by users to prevent startup race conditions such as // https://github.com/kubernetes/kubernetes/issues/95911 func removeNotReadyTaint(k8sClient cloud.KubernetesAPIClient) error { @@ -524,3 +525,16 @@ func removeNotReadyTaint(k8sClient cloud.KubernetesAPIClient) error { klog.InfoS("Removed taint(s) from local node", "node", nodeName) return nil } + +// remove taint may failed, this keep retring until succeed, make sure the taint will eventually being removed +func tryRemoveNotReadyTaintUntilSucceed(interval time.Duration, removeFn func() error) { + for { + err := removeFn() + if err == nil { + return + } + + klog.ErrorS(err, "Unexpected failure when attempting to remove node taint(s)") + time.Sleep(interval) + } +} diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index 4e578fce9..f1b01cb73 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -18,6 +18,7 @@ package driver import ( "context" + "errors" "fmt" "os" "reflect" @@ -1012,3 +1013,32 @@ func getNodeMock(mockCtl *gomock.Controller, nodeName string, returnNode *corev1 return mockClient, mockNode } + +func TestTryRemoveNotReadyTaintUntilSucceed(t *testing.T) { + { + i := 0 + tryRemoveNotReadyTaintUntilSucceed(time.Second, func() error { + i++ + if i < 3 { + return errors.New("test") + } + + return nil + }) + + if i != 3 { + t.Fatalf("unexpected result") + } + } + { + i := 0 + tryRemoveNotReadyTaintUntilSucceed(time.Second, func() error { + i++ + return nil + }) + + if i != 1 { + t.Fatalf("unexpected result") + } + } +}