feat: Ec2Runner VPC selection

[fwerkmeister]

I just tried out to use the EC2 runners. Within our accounts we do not have a default VPC which leads to the stack can not be deployed.

const ec2Provider = new Ec2Runner(this, "ec2Runner", {
      subnet: subnets[0],
      spot: true,
    })

10:28:42 AM | CREATE_FAILED        | AWS::ImageBuilder::Image                       | Dev/GitHubRunners/...mage Builder/Image
Resource handler returned message: "Error occurred during operation 'No default VPC for this user. GroupName is only supported for EC2-Classic and default VPC.'." (RequestToken: ...., Handle
rErrorCode: GeneralServiceException)

Currently the Ec2RunnerProps also do not provide the option to set a VPC.

https://github.com/CloudSnorkel/cdk-github-runners/blob/main/src/providers/ec2.ts#L126-L186

[fwerkmeister]

After doing some more analysis it looks like the problem is related to the AmiBuilder where subnet and vpc would need to be passed into as properties if set.

https://github.com/CloudSnorkel/cdk-github-runners/blob/2aae0e6286cda81fb809aa3e02287767e29a6d28/src/providers/ec2.ts#L233

https://github.com/CloudSnorkel/cdk-github-runners/blob/main/src/providers/image-builders/ami.ts#L55-L74

[kichik]

I will accept a PR for the change you described. My only concern is implicitly giving the builder access to the runner VPC. It would be using the default security group, right? What if that security group gives unwanted access to some internal resources? I guess the same can be said about the default VPC.

[kichik]

Hmm this would also mean the other providers behave differently as they don't set the VPC for their default builders. I need to think about this some more.

[fwerkmeister]

Hi @kichik,

thanks for implementing. I just gave it a retry and now it fails due to missing security group in AMI builder.

Resource handler returned message: "The value supplied for parameter 'subnetId' is not valid. If a 'subnetId' is specified, 'securityGroupIds' must also be provided. (Service: Imagebuilder, Status Code: 400, R
equest ID: ba25f5dd-442f-4b4d-9291-af87e9083f52)" (RequestToken: ..., HandlerErrorCode: GeneralServiceException)

So we would rather need to pass on the sg used within the ec2 instance, or create a new one to use with the ami-builder if no sg is set.

https://github.com/CloudSnorkel/cdk-github-runners/blob/main/src/providers/ec2.ts#L245-L248

What so you think? Should we reopen the issue or should I create a new one?

[kichik]

I'll create a PR. No need for a new issue.

BTW, I appreciate the bug reports, but you know you can pass your own image builder to get unblocked, right?

    const ec2Provider = new Ec2Runner(this, "ec2Runner", {
      subnet: subnets[0],
      spot: true,
      amiBuilder: new AmiBuilder(this, 'Image Builder', {
        vpc: vpc,
        subnetSelection: vpc.selectSubnets({subnetType: SubnetType.PUBLIC}),
        securityGroup: sg,
      }),
    })