Skip to content
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

chore(codebuild): improve the doc for subnetSelection #26592

Merged
merged 6 commits into from
Aug 18, 2023

Conversation

pahud
Copy link
Contributor

@pahud pahud commented Aug 1, 2023

If vpc is specified with subnetSelection undefined, according to this:

if (placement.subnetType === undefined && placement.subnetGroupName === undefined && placement.subnets === undefined) {
// Return default subnet type based on subnets that actually exist
let subnetType = this.privateSubnets.length
? SubnetType.PRIVATE_WITH_EGRESS : this.isolatedSubnets.length ? SubnetType.PRIVATE_ISOLATED : SubnetType.PUBLIC;
placement = { ...placement, subnetType: subnetType };
}

CDK will look for PRIVATE_WITH_EGRESS, PRIVATE_ISOLATED, and PUBLIC in order. If customer does not have PRIVATE_WITH_EGRESS subnets, they will need to have vpc endpoints if they need to access AWS services such as AWS Secrets Manager or Amazon ECR.

This PR improves the doc to clarify.

Closes #.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team August 1, 2023 22:38
@github-actions github-actions bot added the p2 label Aug 1, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 1, 2023
@pahud pahud marked this pull request as ready for review August 1, 2023 22:39
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 1, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 3, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 3, 2023

If you have no PRIVATE_WITH_EGRESS subnets in the specified vpc, leaving this undefined will require VPC endpoints to access AWS services.

I think this is giving vague instructions but don't really explain the underlying reasons. How about:

To access AWS services, your CodeBuild project needs to be in one of the following types of subnets:

* Subnets with access to the internet (of type PRIVATE_WITH_EGRESS or PUBLIC).
* Private subnets unconnected to the internet, but with [VPC endpoints](link here) for the necessary services.

If you don't specify a subnet selection, the default behavior is to use PRIVATE_WITH_EGRESS subnets first if they exist, then PRIVATE_WITHOUT_EGRESS, and finally PUBLIC subnets. If your VPC doesn't have PRIVATE_WITH_EGRESS subnets but you need AWS service access, either select PUBLIC subnets explicitly or add VPC Endpoints to your private subnets.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

@pahud
Copy link
Contributor Author

pahud commented Aug 17, 2023

@rix0rrr

According to Best practices for VPCs,

You need a NAT gateway or NAT instance to use CodeBuild with your VPC so that CodeBuild can reach public endpoints (for example, to run CLI commands when running builds). You cannot use the internet gateway instead of a NAT gateway or a NAT instance because CodeBuild does not support assigning Elastic IP addresses to the network interfaces that it creates, and auto-assigning a public IP address is not supported by Amazon EC2 for any network interfaces created outside of Amazon EC2 instance launches.

I believe if user selects public subnets, codebuild will not be able to access the public endpoints. So the option would be either PRIVATE_WITH_EGRESS or enable vpc endpoints.

I have modified the suggested changes. Let me know if it works for you.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find on the CodeBuild exception, I didn't know that. Thanks!

@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: c23aaeb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit dbe5615 into aws:main Aug 18, 2023
8 checks passed
@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants