feat(ec2): subnet ipv4 cidr blocks on imported vpc #23317

schourode · 2022-12-12T12:00:43Z

When using Vpc.fromVpcAttributes() to import a VPC, it is possible to specify all subnet properties except for the IPv4 CIDR block. This means that any attempt to read the ipv4CidrBlock property of a subnet on such a VPC will throw with the message:

You cannot reference an imported Subnet's IPv4 CIDR if it was not supplied. Add the ipv4CidrBlock when importing using Subnet.fromSubnetAttributes()

This PR extends the VpcAttributes interface to allow for subnet IPv4 CIDR blocks to be passed in alongside the IDs, names and route table IDs for each subnet group (public, private, isolated).

I have added a unit test showing how the values passed into Vpc.fromVpcAttributes() end up in the correct positions across the ImportedSubnet instances. As far as I can tell, there are no existing integration tests for this type of import, leading me to assume an integration test is not needed for this PR either.

After checking the current README in the @aws-cdk/aws-ec2 module, I am of the impression that nothing needs to be added here. Let me know if you believe otherwise.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-12-12T12:00:52Z

aws-cdk-automation

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.

schourode · 2022-12-16T14:07:20Z

@mrgrain Sorry about pinging you directly. You were helpful towards getting my previous PR into a mergeable state, so I am hoping you are able to help with this one also 🙏

I submitted this PR early this week. It fails the PR linter due to lack of changes to README and integration tests, and I suspect the failing checks may be the reason why none from the aws-cdk-owners group have looked at the PR yet.

As per the PR description, it is my opinion that neither README nor integration tests needs updating for the changes I am proposing here. If you disagree, I am happy to comply with directions on what additions you find to be needed for this PR to be acceptable.

mrgrain · 2022-12-16T14:42:19Z

@mrgrain Sorry about pinging you directly. You were helpful towards getting my previous PR into a mergeable state, so I am hoping you are able to help with this one also 🙏

Hey hey, I don't mind, but FYI some really don't like this. As an explanation (not an excuse), the holiday season is coming up and for example I had very limited availability this week and am on Annual Leave from pretty much now-ish. It will be similar with other maintainers.

I submitted this PR early this week. It fails the PR linter due to lack of changes to README and integration tests, and I suspect the failing checks may be the reason why none from the aws-cdk-owners group have looked at the PR yet.

It's more that it makes the review more complicated. Now I'll have to go and check the documentation and existing tests first, taking me away from just reviewing the code. Without a README entry and without an issue attached, all I have is 3-4 paragraphs on what problem a feature is solving and how the proposed solution fits in. Sometimes that is enough, but sometimes it's not. And this will also vary a lot from who is looking at a PR and what their knowledge is (at the end of the day core maintainers are CDK experts and not all-around AWS experts).

I'd recommend you to always write a README entry. If you don't think it should be in the README, just put it in the PR description instead.

Generally speaking, the lack of an existing integration tests does not prove that a new/changed integ test is not needed. It's about whether a change can be tested (e.g. we don't currently have to ability to effectively test updates to a resource) and if it makes sense (e.g. I've exempted a PR changing some Docker stuff recently because it would only have tested if docker works as documented).

As per the PR description, it is my opinion that neither README nor integration tests needs updating for the changes I am proposing here. If you disagree, I am happy to comply with directions on what additions you find to be needed for this PR to be acceptable.

I disagree in this case. This section would very much benefit from a description on how to specify the cidr blocks and why someone would want to do this.

Edit: I'm okay with skipping the integ test here as there's inherently not much to test. If the wrong cidrs are passed, deployment will fail. If the correct ones are passed it will work.

packages/@aws-cdk/aws-ec2/lib/util.ts

packages/@aws-cdk/aws-ec2/lib/vpc.ts

mrgrain · 2022-12-16T14:58:30Z

@schourode Like I've said above, I'm on leave now. I'm happy to take this PR through to its completion, but expect significant lower response times from me. If that's not okay for you (I'd understand) I'm equally happy to remove my assignment and let someone else take over (although holiday season constraints still apply). Just let me know what you'd prefer.

schourode · 2022-12-16T20:03:29Z

@mrgrain Thanks a million for the elaborate answer and the review. I will get to work on addressing incorrect strings, readme, tests next week. I am in no hurry to have this merged and would be happy to have you still assigned even if you won't be picking this up again until you are back at work.

When I tagged you above, it was out of a worry that the PR might fall between the cracks due to that failing lint check and would never attract any human attention. From your explanation, I understand that it would have been noticed, only this time of year it may take a while. That's completely understandable.

Again, thanks for all of the feedback here – both that concerning the proposed change and that concerning the contribution process moreover. It is really helpful.

Happy holidays 🤗

mrgrain · 2022-12-16T20:06:06Z

All good! You're worries are real and I can understand where they are coming from. That's something we as maintainers need to work on.

When using `Vpc.fromVpcAttributes()` to import a VPC, it is possible to specify all subnet properties except for the IPv4 CIDR block. This means that any attempt to read the `ipv4CidrBlock` property of a subnet on such a VPC will throw with the message: > You cannot reference an imported Subnet's IPv4 CIDR if it was not supplied. Add the ipv4CidrBlock when importing using Subnet.fromSubnetAttributes() This commit extends the `VpcAttributes` interface to allow for subnet IPv4 CIDR blocks to be passed in alongside the IDs, names and route table IDs for each subnet group (public, private, isolated).

@default

When adding three new optional properies on the `VpcAttributes` interface in the previous commit, the linter required @default annotations in the documentation of each property. This commits improves documentation consistency within the inteface by adding @default annotations also to _existing_ optional properties.

Adding test cases for two existing parameters guards, improving test coverage parity between existing and newly added `VpcAttribute` props.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

packages/@aws-cdk/aws-ec2/README.md

mrgrain · 2023-01-04T10:00:54Z

packages/@aws-cdk/aws-ec2/README.md

+```ts
+const vpc = ec2.Vpc.fromVpcAttributes(this, 'VPC', {
+  vpcId: 'vpc-1234',
+  availabilityZones: ['us-east-1a', 'us-east-1b', 'us-east-1c'],
+  publicSubnetIds: ['s-12345', 's-34567', 's-56789'],
+  publicSubnetNames: ['Subnet A', 'Subnet B', 'Subnet C'],
+  publicSubnetRouteTableIds: ['rt-12345', 'rt-34567', 'rt-56789'],
+  publicSubnetIpv4CidrBlocks: ['10.0.0.0/24', '10.0.1.0/24', '10.0.2.0/24'],
+});
+```
+
+The above example will create an `IVpc` instance with three public subnets:
+
+| Subnet id | Availability zone | Subnet name | Route table id | IPv4 CIDR   |
+| --------- | ----------------- | ----------- | -------------- | ----------- |
+| s-12345   | us-east-1a        | Subnet A    | rt-12345       | 10.0.0.0/24 |
+| s-34567   | us-east-1b        | Subnet B    | rt-34567       | 10.0.1.0/24 |
+| s-56789   | us-east-1c        | Subnet B    | rt-56789       | 10.0.2.0/24 |
+


Yes! I love this 😍

Pull request has been modified.

mrgrain

🚀

mrgrain · 2023-01-04T10:19:10Z

@Mergifyio update

mergify · 2023-01-04T10:19:57Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2023-01-04T10:59:06Z

AWS CodeBuild CI Report

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

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

mergify · 2023-01-04T10:59:19Z

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).

github-actions bot added the p2 label Dec 12, 2022

aws-cdk-automation requested a review from a team December 12, 2022 12:00

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Dec 12, 2022

aws-cdk-automation previously requested changes Dec 12, 2022

View reviewed changes

mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Dec 16, 2022

mrgrain previously requested changes Dec 16, 2022

View reviewed changes

packages/@aws-cdk/aws-ec2/lib/util.ts Show resolved Hide resolved

packages/@aws-cdk/aws-ec2/lib/util.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-ec2/lib/vpc.ts Outdated Show resolved Hide resolved

mrgrain self-assigned this Dec 16, 2022

schourode added 3 commits December 27, 2022 11:12

test(ec2): parameter guards in Vpc.fromVpcAttributes()

d900b68

Adding test cases for two existing parameters guards, improving test coverage parity between existing and newly added `VpcAttribute` props.

schourode force-pushed the feat-ec2-subnet-cidr branch from 2762c60 to d900b68 Compare December 27, 2022 10:14

mrgrain reviewed Jan 4, 2023

View reviewed changes

packages/@aws-cdk/aws-ec2/README.md Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-ec2/README.md

d492c95

mrgrain reviewed Jan 4, 2023

View reviewed changes

mrgrain approved these changes Jan 4, 2023

View reviewed changes

Merge branch 'main' into feat-ec2-subnet-cidr

23e3a5f

mergify bot merged commit e0885db into aws:main Jan 4, 2023

schourode deleted the feat-ec2-subnet-cidr branch January 5, 2023 21:07

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(ec2): subnet ipv4 cidr blocks on imported vpc #23317

feat(ec2): subnet ipv4 cidr blocks on imported vpc #23317

Uh oh!

schourode commented Dec 12, 2022

Uh oh!

gitpod-io bot commented Dec 12, 2022

Uh oh!

aws-cdk-automation left a comment

Uh oh!

schourode commented Dec 16, 2022

Uh oh!

mrgrain commented Dec 16, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain commented Dec 16, 2022

Uh oh!

schourode commented Dec 16, 2022

Uh oh!

mrgrain commented Dec 16, 2022

Uh oh!

Uh oh!

mrgrain Jan 4, 2023

Uh oh!

mrgrain left a comment

Uh oh!

mrgrain commented Jan 4, 2023

Uh oh!

mergify bot commented Jan 4, 2023

Uh oh!

aws-cdk-automation commented Jan 4, 2023

Uh oh!

mergify bot commented Jan 4, 2023

Uh oh!

Uh oh!

feat(ec2): subnet ipv4 cidr blocks on imported vpc #23317

feat(ec2): subnet ipv4 cidr blocks on imported vpc #23317

Uh oh!

Conversation

schourode commented Dec 12, 2022

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

Uh oh!

gitpod-io bot commented Dec 12, 2022

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

schourode commented Dec 16, 2022

Uh oh!

mrgrain commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain commented Dec 16, 2022

Uh oh!

schourode commented Dec 16, 2022

Uh oh!

mrgrain commented Dec 16, 2022

Uh oh!

Uh oh!

mrgrain Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

mrgrain commented Jan 4, 2023

Uh oh!

mergify bot commented Jan 4, 2023

✅ Branch has been successfully updated

Uh oh!

aws-cdk-automation commented Jan 4, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 4, 2023

Uh oh!

Uh oh!

mrgrain commented Dec 16, 2022 •

edited

Loading