-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(ec2): global vpc endpoint support #29563
Conversation
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 pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
@nmussy want to rebase this and i'll take a look? |
250a767
to
6874c9b
Compare
@msambol Should be good to go, thanks 👍 |
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.
LGTM
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.
NIce! Thanks.
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.
Thanks for making this change! Just one nit and question
Not sure I fully understand this part of the description, could you elaborate? Did you want to add prefix, port and global as one props like object? |
Ideally, yes. The Props interface we ended up with still leaves us with |
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). |
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). |
@Mergifyio refresh |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
@Mergifyio refresh |
✅ Pull request refreshed |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
@Mergifyio refresh |
✅ Pull request refreshed |
Issue # (if applicable)
Closes #29560.
Reason for this change
The service name generated by the CDK for global VPC endpoints was incorrect, as it contained the stack's region:
In addition, another global endpoint was missing from
InterfaceVpcEndpointAwsService
.Description of changes
InterfaceVpcEndpointAwsService
constructor was modified toprefix
,port
, and nowglobal
), but couldn't make a breaking change to a publicly accessible constructorInterfaceVpcEndpointAwsService.S3_MULTI_REGION_ACCESS_POINTS
was changed to be a global VPC endpointInterfaceVpcEndpointAwsService.CODECATALYST
was addedDescription of how you validated changes
I've added a unit test to check that the global endpoints' name were set correctly.
I also added an integration test for
InterfaceVpcEndpointAwsService.S3_MULTI_REGION_ACCESS_POINTS
.To test it, I created a publicly accessible EC2 instance on the VPC, connected to it and ran
nslookup accesspoint.s3-global.amazonaws.com
to make sure it was resolvable (see Configuring a Multi-Region Access Point for use with AWS PrivateLink):Without the
InterfaceVpcEndpointAwsService.S3_MULTI_REGION_ACCESS_POINTS
interface endpoint:With the
InterfaceVpcEndpointAwsService.S3_MULTI_REGION_ACCESS_POINTS
interface endpoint:Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license