-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(ec2-alpha): adding imports for SubnetV2 and VpcV2(WIP) #31765
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.
c9e2d27
to
e1b038c
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
f8a1645
to
00eebb9
Compare
35857d9
to
13d1458
Compare
subnetType: SubnetType.PRIVATE_ISOLATED, | ||
availabilityZone: 'us-west-2a', | ||
ipv4CidrBlock: '10.2.0.0/24', | ||
routeTableId: 'rtb-0871c310f98da2cbb', |
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.
what is the value of providing the route table id? if it is needed, is it better to provide the route table object ?
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.
it will be used to add routes when attaching gateways, since routeTable interface only has the routeTableId as the property, i think just providing the Id should be enough to modify the class field using routetable id
this.routeTable = {
routeTableId: props.routeTableId!,
};
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.
honestly, I do not have a strong opinion about it. The only thing I have in mind is passing an object will give us and customers other options to create the route table, and do more logic by invoking other functions.
In general in CDK, we prefer using strong typed Objects more than Ids.
@@ -9,7 +9,7 @@ | |||
"InstanceTenancy": "default" | |||
} | |||
}, | |||
"VPCintegtest1SecondaryAddress256BAC1D3": { | |||
"VPCintegtest1SecondaryAddress2B60D56E9": { |
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.
do you know why these logical ids got changed although you did not change anything in the testing stack
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.
we need to be careful about the logical ids changes, as it means that the old resources will be deleted, and create new resources, and this can be a breaking change for the customers.
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.
I believe it is related to the change in packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2.ts
when you replaced the CfnVPCCidrBlock
resource with the new L2 resource. Could you please check if this is a breaking change (I mean if it will cause any outage for the customers or not during the update). If yes, I think it is ok since this module is an alpha module, but we need to highlight the change as a breaking change.
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.
I verified that this is a breaking change. The stacks that got already deployed using the old construct will fail to be updated after this change and will get this error CIDR range conflicts with x.xx.xx.xx/xx with association ID vpc-cidr-assoc-ABCD
.. we should highlight this issue as a breaking change.
f516802
to
8e557c4
Compare
8e557c4
to
6ec70f6
Compare
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 Shikha, it looks good to me.
@Mergifyio update |
✅ Branch has been successfully updated |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@Mergifyio merge |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at d108a80 |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Tracking #30762.
Reason for this change
Allow users to define imports for a VPC or subnet defined outside current stack definition.
Description of changes
Added new methods under VpcV2 and Subnet
VpcV2.fromVpcV2Attributes()
andSubnetV2.fromSubnetV2Attributes()
Added new L2 for VPCCidrBlock to allow import of secondary addresses.
VPCCidrBlock
Added new integration test and unit test file to check import related functionality.
Updated Readme.
Fixed an earlier issue with subnet range check, fixed to include IPAM defined IPv4 address as well
Description of how you validated changes
Deployed and tested for below scenarios in account:
Checklist
BREAKING CHANGE: The new
VpcCidrBlock
L2 construct replacesCfnVPCCidrBlock
. This change alters the logical ID ofAWS::EC2::VPCCidrBlock
resources in CloudFormation templates. Existing deployments will see errors likeCIDR range conflicts with x.xx.xx.xx/xx with association ID vpc-cidr-assoc-ABCD
. To resolve this, you must recreate your existing stacks to use the new module.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license