diff --git a/.github/workflows/container-image.yaml b/.github/workflows/container-image.yaml deleted file mode 100644 index 870012226..000000000 --- a/.github/workflows/container-image.yaml +++ /dev/null @@ -1,45 +0,0 @@ -name: Container Images - -on: push - -permissions: - contents: read - security-events: write - -jobs: - build: - # this is to prevent the job to run at forked projects - if: github.repository == 'kubernetes-sigs/aws-efs-csi-driver' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v1 - - name: Set up Docker Buildx - id: buildx - uses: crazy-max/ghaction-docker-buildx@v3 - with: - buildx-version: latest - qemu-version: latest - - name: Push to Dockerhub registry - run: | - BRANCH=$(echo $GITHUB_REF | cut -d'/' -f3) - SHORT_SHA=$(echo $GITHUB_SHA | cut -c -7) - REPO=amazon/aws-efs-csi-driver - if [ "$BRANCH" = "master" ]; then - TAG=$SHORT_SHA - else - TAG=$BRANCH - fi - docker login -u ${{ secrets.DOCKERHUB_USER }} -p ${{ secrets.DOCKERHUB_TOKEN }} - - docker buildx build \ - -t $REPO:$TAG \ - --platform=linux/amd64,linux/arm64 \ - --progress plain \ - --push . - if [ "$BRANCH" = "master" ]; then - docker buildx build \ - -t $REPO:master \ - --platform=linux/amd64,linux/arm64 \ - --progress plain \ - --push . - fi diff --git a/Dockerfile b/Dockerfile index 6d2dd3160..bda96a61b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -63,6 +63,8 @@ COPY --from=rpm-provider /tmp/rpms/* /tmp/download/ # cd, ls, cat, vim, tcpdump, are for debugging RUN clean_install amazon-efs-utils true && \ clean_install crypto-policies true && \ + clean_install openssl true && \ + clean_install openssl-libs true && \ install_binary \ /usr/bin/cat \ /usr/bin/cd \ diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 8053778f8..0fb13dfc7 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -201,11 +201,22 @@ func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, acces o.Retryer = c.rm.createAccessPointRetryer }) if err != nil { - if isAccessDenied(err) { - return nil, ErrAccessDenied - } - return nil, fmt.Errorf("Failed to create access point: %v", err) - } + if isAccessPointAlreadyExists(err) { + klog.V(4).Infof("Access point already exists for client token %s. Retrieving existing access point details.", clientToken) + existingAccessPoint, err := c.FindAccessPointByClientToken(ctx, clientToken, *createAPInput.FileSystemId) + if err != nil { + return nil, fmt.Errorf("Error attempting to retrieve existing access point: %v", err) + } + if existingAccessPoint == nil { + return nil, fmt.Errorf("No access point for client token %s was returned", clientToken) + } + return existingAccessPoint, nil + } else if isAccessDenied(err) { + return nil, ErrAccessDenied + } else { + return nil, fmt.Errorf("Failed to create access point: %v", err) + } + } klog.V(5).Infof("Create AP response : %+v", res) return &AccessPoint{ @@ -435,6 +446,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..77e09a214 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -103,6 +103,53 @@ func TestCreateAccessPoint(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Success: 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, + } + + // CreateAccessPoint call returns AccessPointAlreadyExists + mockEfs.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, + &smithy.GenericAPIError{ + Code: AccessPointAlreadyExists, + Message: "Access point already exists", + }) + + // DescribeAccessPoints call with client token to find existing access point succeeds + mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return( + &efs.DescribeAccessPointsOutput{ + AccessPoints: []types.AccessPointDescription{ + { + AccessPointId: aws.String(accessPointId), + FileSystemId: aws.String(fsId), + ClientToken: aws.String(clientToken), + RootDirectory: &types.RootDirectory{ + Path: aws.String(directoryPath), + }, + }, + }, + }, nil) + + res, err := c.CreateAccessPoint(context.Background(), clientToken, req) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + if res.AccessPointId != accessPointId { + t.Fatalf("Expected AccessPointId %s, got %s", accessPointId, res.AccessPointId) + } + mockCtl.Finish() + }, + }, { name: "Fail", testFunc: func(t *testing.T) { @@ -158,6 +205,79 @@ func TestCreateAccessPoint(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Fail: Access Point exists but wasn't returned", + 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, + } + + mockEfs.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, + &smithy.GenericAPIError{ + Code: AccessPointAlreadyExists, + Message: "Access point already exists", + }) + + mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return( + &efs.DescribeAccessPointsOutput{ + AccessPoints: []types.AccessPointDescription{}, + }, nil) + + _, err := c.CreateAccessPoint(context.Background(), clientToken, req) + if err == nil { + t.Fatal("Expected error but got nil") + } + expectedErr := "No access point for client token " + clientToken + " was returned" + if err.Error() != expectedErr { + t.Fatalf("Expected error %q, got %q", expectedErr, err.Error()) + } + mockCtl.Finish() + }, + }, + { + name: "Fail: Access Point exists but there was error retrieving it", + 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, + } + + mockEfs.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, + &smithy.GenericAPIError{ + Code: AccessPointAlreadyExists, + Message: "Access point already exists", + }) + + mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, + errors.New("internal error")) + + _, err := c.CreateAccessPoint(context.Background(), clientToken, req) + if err == nil { + t.Fatal("Expected error but got nil") + } + expectedErr := "Error attempting to retrieve existing access point: failed to list Access Points of efs = " + fsId + " : internal error" + if err.Error() != expectedErr { + t.Fatalf("Expected error %q, got %q", expectedErr, err.Error()) + } + mockCtl.Finish() + }, + }, + } for _, tc := range testCases { diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 4ecbef96b..7c64455ce 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -352,9 +352,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) 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) }