-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(codebuild): add custom instance type and VPC to Fleets #34572
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.
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
feat(codebuild): add custom instance type and VPC to Fleets #34572
Conversation
...sting/framework-integ/test/aws-codebuild/test/integ.project-fleet-attribute-based-compute.ts
Show resolved
Hide resolved
| // Incredibly, if you pass a SubnetSelection that produces more than 1 | ||
| // subnet, you currently get this error: | ||
| // > Resource handler returned message: "Invalid vpc config: the maximum number of subnets is 1 | ||
| // This seems like a terrible limitation from the CodeBuild team. | ||
| // maxAzs: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad. If anyone at AWS can talk to the CodeBuild team... I've never seen another service that demands a single subnet like this. The CloudFormation types claim you can use "up to 16" but anything more than 1 fails at deploy time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ozelalisen maybe you can forward this complaint internally 🥲.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will follow up on this
28828ff to
d7ab2f0
Compare
8256ca3 to
3a75586
Compare
|
@alvazjor why did |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
3a75586 to
b26bef7
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
badmintoncryer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work! I've added some minor comments.
Is there a reason for combining the two features - VPC and Instance Type support - into a single PR? I feel these should typically be split into one feature per PR.
Thank you very much for reviewing.
The reason I've combined them is that I want to use both of these features myself. I do not think they strictly depend on one another, but because they modify code in the same place, if I were to split them up, whichever branch merged first would conflict with the other. So, it was easier for me to develop them together. So, I hope this it's okay to do it like this. |
b26bef7 to
2966716
Compare
|
I have rebased and am rerunning the new integration tests in this PR, as main has changed the snapshots. It takes a really long time to run integration tests that involve fleets, because fleet instances must exist for at least an hour before they can be deleted. I will hopefully finish it tomorrow and then will push the fixed tests. |
2966716 to
99d2d1e
Compare
|
😕 the main build succeeds but I don't understand either of the other jobs that failed. Especially the security guardian one, seems all unrelated to my changes. |
badmintoncryer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the thorough deployment verification. As noted in the comments, while there is a discrepancy between the documentation and the IAM policy, I understand that the implementation is correct.
Since large PRs tend to get deprioritized for review, I think it would be better for both of us to limit future PRs to a single feature.
|
I don't think you need to worry much about CI processes like Security Guardian, as they are flaky and their failures don't affect the review status. |
|
@badmintoncryer do you know how to get the label added now that #35268 is fixed? |
|
@isker Normally, this issue should have been resolved by merging the main branch, but it seems it has not been fixed yet. |
99d2d1e to
8e64358
Compare
|
Still no label 🥲. |
@badmintoncryer is there an open issue for this? |
8e64358 to
6214d0b
Compare
|
Hi @ozelalisen, I see you assigned yourself a while ago. I would be grateful for a review. |
packages/@aws-cdk-testing/framework-integ/test/aws-codebuild/test/integ.project-fleet.ts
Outdated
Show resolved
Hide resolved
6214d0b to
03a3894
Compare
Pull request has been modified.
92dfd15 to
39ec36e
Compare
|
@ozelalisen I think I've addressed everything, please take another look. |
|
@isker There are some merge conflicts, once you resolve them, I'll approve the PR |
https://aws.amazon.com/about-aws/whats-new/2025/04/aws-codebuild-ec2-instance-type-configurable-storage-size/ CodeBuild now supports specifying specific EC2 instance types to serve as fleet compute. Add this support to the Fleet construct by way of adding CUSTOM_INSTANCE_TYPE to the FleetComputeType enum, and `instanceType` to `ComputeConfiguration`. Also, add VPC support to Fleet. This mirrors the VPC support in Project. When using Fleets, the VPC configured on the Project does nothing. Only the VPC on the Fleet applies. This required adding a Role to the Fleet to handle provisioning EC2 network interfaces in the configured VPC.
39ec36e to
aa23ae8
Compare
ozelalisen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, LGTM, just left minor comment regarding permissions for future reference
| 'ec2:CreateNetworkInterface', | ||
| 'ec2:DeleteNetworkInterface', | ||
| 'ec2:ModifyNetworkInterfaceAttribute', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IAM permissions in this PR appear to be correct and align with AWS CodeBuild's official documentation. I also encountered deployment errors related to The service role is not authorized to perform ec2:CreateNetworkInterface when scoping down to subnetArns. Given this limitation, it is okay to merge this PR
| // Incredibly, if you pass a SubnetSelection that produces more than 1 | ||
| // subnet, you currently get this error: | ||
| // > Resource handler returned message: "Invalid vpc config: the maximum number of subnets is 1 | ||
| // This seems like a terrible limitation from the CodeBuild team. | ||
| // maxAzs: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will follow up on this
|
Comments on closed issues and PRs are hard for our team to see. |
https://aws.amazon.com/about-aws/whats-new/2025/04/aws-codebuild-ec2-instance-type-configurable-storage-size/
CodeBuild now supports specifying specific EC2 instance types to serve as fleet compute.
Add this support to the Fleet construct by way of adding CUSTOM_INSTANCE_TYPE to the FleetComputeType enum, and
instanceTypetoComputeConfiguration.Also, add VPC support to Fleet. This mirrors the VPC support in Project. When using Fleets, the VPC configured on the Project is disallowed by CloudFormation. Only the VPC on the Fleet applies. VPC support required adding a Role to the Fleet to handle provisioning EC2 network interfaces in the configured VPC.
Describe any new or updated permissions being added
When configuring a VPC on a Fleet, IAM permissions are granted to a CodeBuild Role as described here.
Description of how you validated changes
Unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license