feat(vpc): allow Vpc.fromLookup() to discover asymmetric subnets #4544

skinny85 · 2019-10-17T00:30:15Z

Previously, Vpc.fromLookup() required every subnet group to be in the same Availability Zones,
and for all subnets in all groups to cover all of those Availability Zones.
This loosens this requirement, so that now Vpc.fromLookup() works for VPCs of all shapes.

This also add a way to customize which tag of the subnet is considered when grouping subnets into groups when calling fromLookup().

Fixes #3407

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

mergify · 2019-10-17T00:30:52Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-17T01:06:48Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr

Sweet, thanks Adam!

tools/cdk-integ-tools/lib/integ-helpers.ts

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

rix0rrr · 2019-10-21T12:38:25Z

packages/@aws-cdk/cx-api/lib/context/vpc.ts

@@ -1,4 +1,4 @@
-export const VPC_PROVIDER = 'vpc-provider';
+export const VPC_PROVIDER = 'vpc-provider2';


This seems dangerous. It means the CLI cannot possibly recognize both vpc-provider1 and vpc-provider2 requests at the same time, which means the CLI breaks backwards compatibility with old CX-api versions.

How do we make sure that the new CLI will still handle old frameworks?

Damn, you're right. That means we have to have a new provider, just for this case.

Will change.

Here's an interesting question.

We will have 2 very similar providers: the "old" VPC one, and the "new" VPC one that accepts asymmetric subnets. Their code will be quite similar. Should I:

a) have the new provider be completely separate from the old one - they should not share any code (as the old one is there only for backwards compatibility reasons); or

b) should I refactor the old one to have an abstract class, and then have both the old one and the new one inherit from this abstract class, to reduce code duplication?

@rix0rrr what do you think?

Maybe even have a single class and have them do something else based on the input format? Instead of adding a new provider you could also add a version number in the request.

But yes, otherwise let's share code. Maybe the V1 uses the V2, and applies additional restrictions on the output afterwards?

Also, we're going to need to bump the CX version number to tell people that they need to upgrade their CLI to use the new VPC provider (don't forget to add a "version upgrade" code path, which again can do nothing except push up the CX version number one).

Maybe even have a single class and have them do something else based on the input format? Instead of adding a new provider you could also add a version number in the request.

I like this idea. That's what I did in the newest version. There's a new optional property for the request, returnAsymmetricSubnets, which Vpc.fromLookup() now uses, and it triggers the new behavior in the (existing) VPC provider.

Also, we're going to need to bump the CX version number to tell people that they need to upgrade their CLI to use the new VPC provider (don't forget to add a "version upgrade" code path, which again can do nothing except push up the CX version number one).

Can you expand what needs to be done here? I don't think I understood your comment fully. Is it just changing this to 1.15.0, or is there something else to be done here?

aws-cdk/packages/@aws-cdk/cx-api/lib/versioning.ts

Line 19 in c0c0513

// (good), but UNLESS YOU ALSO IMPLEMENT 'upgradeAssemblyManifest' you will also

Done here: https://github.com/aws/aws-cdk/pull/4544/files?file-filters%5B%5D=.ts#diff-eacf6cc8a5af37f2f5e8202fca7d8ec3R67-R70 , I hope I did it correctly.

packages/aws-cdk/lib/context-providers/vpcs.ts

Make sure that the CLI will continue to be able to load old cloud assemblies. We do this by using the CLI feature that it can directly load Cloud Assemblies through its `--app` argument, hard copying a couple of cloud assemblies into the test set and trying to synth those. In addition to adding a test for loading v0.36.0 assemblies, also add tests for 2 common context provider requests (AZs and VPC), to make sure new CLIs will continue to be able to handle these context lookup requests. Fixes #4475, pre-emptively adds a regression test for #4544.

skinny85 · 2019-10-22T17:30:54Z

Updated with comments. As to the question in #4544 (comment) , for this iteration, I kept the two providers completely separate, not sharing any code.

aws-cdk-automation · 2019-10-22T18:15:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2019-10-23T09:28:16Z

jd-carroll is adding CIDR lookup to the VPC provider here: #4591.

Heads up that depending on which PR lands first, the other one will need to be updated to integrate this change into both VPC providers.

skinny85 · 2019-10-24T01:07:24Z

Updated with the latest comments.

jd-carroll is adding CIDR lookup to the VPC provider here: #4591.

Heads up that depending on which PR lands first, the other one will need to be updated to integrate this change into both VPC providers.

It should be fine. I looked at #4591, and it looks to me like the merge will be relatively straightforward. Feel free to merge that when it's ready, and I'll deal with resolving the conflicts with this PR.

aws-cdk-automation · 2019-10-24T01:51:30Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

jd-carroll · 2019-10-24T15:36:34Z

@skinny85 @rix0rrr - I question how much of my code in #4591 will actually land (the general purpose of my PR has been debunk'd).

However, there may be some useful pieces related to #2232. Happy to amend my PR to include only changes related to that or happy to have the changes consumed by this PR. Whatever folks think is best!

skinny85 · 2019-10-24T23:15:43Z

@rix0rrr can you re-review please? Thanks!

* fix(cli): add Cloud Assembly backwards compat tests Make sure that the CLI will continue to be able to load old cloud assemblies. We do this by using the CLI feature that it can directly load Cloud Assemblies through its `--app` argument, hard copying a couple of cloud assemblies into the test set and trying to synth those. In addition to adding a test for loading v0.36.0 assemblies, also add tests for 2 common context provider requests (AZs and VPC), to make sure new CLIs will continue to be able to handle these context lookup requests. Fixes #4475, pre-emptively adds a regression test for #4544. * Address review comments

aws-cdk-automation · 2019-10-28T18:44:21Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

skinny85 · 2019-10-28T18:48:34Z

Added the toolkit versioning part.

Previously, Vpc.fromLookup() required every subnet group to be in the same Availability Zones, and for all subnets in all groups to cover all of those Availability Zones. This loosens this requirement, so that now Vpc.fromLookup() works for VPCs of all shapes. This also add a way to customize which tag of the subnet is considered when grouping subnets into groups when calling fromLookup(). Fixes aws#3407

skinny85 · 2019-10-28T19:59:50Z

Fixed missing docs.

aws-cdk-automation · 2019-10-28T20:44:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2019-10-29T10:16:39Z

packages/@aws-cdk/cx-api/lib/versioning.ts

@@ -64,6 +64,11 @@ export function upgradeAssemblyManifest(manifest: AssemblyManifest): AssemblyMan
    manifest = justUpgradeVersion(manifest, '1.10.0');
  }

+  if (manifest.version === '1.10.0') {


Thank you for getting where I was going with this!

(Now that we've done it twice it will be a tradition ;) )

mergify · 2019-10-29T18:06:59Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-29T18:57:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

gsdwait · 2019-11-07T14:40:36Z

Any projections on when this might be included in a release. The vpc import restriction is preventing us from using CDK in our dev and production environments. I'll have to rebuild some code as CloudFormation if this isn't released soon. We're really excited about the CDK but for now this is a show stopper for us.

jk21 · 2019-11-08T11:19:37Z

also looking forward for this!

ranman · 2019-11-17T19:10:19Z

+1 also looking forward to this!

jk21 · 2019-11-17T19:13:42Z

@ranman it is already in the new version! Works nice 👍

ranman · 2019-11-17T19:30:27Z

ah strange, just updated thinking this was in it and couldn't get it working

…

On Sun, Nov 17, 2019 at 11:13 AM Jonas Kemper ***@***.***> wrote: @ranman <https://github.com/ranman> it is already in the new version! Works nice 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4544?email_source=notifications&email_token=AABKYO3WAEJBMY2T7U5VHODQUGJW5A5CNFSM4JBSTKWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEITPGI#issuecomment-554776473>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKYO52KAB5F34WOF26CSDQUGJW5ANCNFSM4JBSTKWA> .

skinny85 requested a review from rix0rrr October 17, 2019 00:30

skinny85 requested review from eladb and RomainMuller as code owners October 17, 2019 00:30

SomayaB added the contribution/core This is a PR that came from AWS. label Oct 17, 2019

SomayaB assigned rix0rrr Oct 17, 2019

rix0rrr reviewed Oct 21, 2019

View reviewed changes

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Oct 21, 2019

rix0rrr mentioned this pull request Oct 22, 2019

fix(cli): add Cloud Assembly backwards compat tests #4625

Merged

skinny85 force-pushed the fix/vpc-import-issues branch from 29a6125 to 4742344 Compare October 22, 2019 17:28

rix0rrr mentioned this pull request Oct 23, 2019

fix(ec2) Add Ingress rule for VPC Endpoints & CIDR Blocks #4591

Closed

skinny85 force-pushed the fix/vpc-import-issues branch from 4742344 to bf7ca5d Compare October 24, 2019 01:04

skinny85 force-pushed the fix/vpc-import-issues branch from bf7ca5d to 0d5c5c1 Compare October 28, 2019 18:42

skinny85 force-pushed the fix/vpc-import-issues branch from 0d5c5c1 to 97b47bf Compare October 28, 2019 19:59

rix0rrr approved these changes Oct 29, 2019

View reviewed changes

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Oct 29, 2019

Merge branch 'master' into fix/vpc-import-issues

284b381

mergify bot merged commit 2ccb745 into aws:master Oct 29, 2019

skinny85 deleted the fix/vpc-import-issues branch October 29, 2019 22:32

NetaNir mentioned this pull request Dec 27, 2019

fix(docs) VPC: fromLookup() allows asymmetric vpc import #5563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vpc): allow Vpc.fromLookup() to discover asymmetric subnets #4544

feat(vpc): allow Vpc.fromLookup() to discover asymmetric subnets #4544

skinny85 commented Oct 17, 2019

mergify bot commented Oct 17, 2019

aws-cdk-automation commented Oct 17, 2019

rix0rrr left a comment

rix0rrr Oct 21, 2019

skinny85 Oct 22, 2019

skinny85 Oct 22, 2019

rix0rrr Oct 23, 2019

skinny85 Oct 24, 2019 •

edited

Loading

rix0rrr Oct 28, 2019

skinny85 Oct 28, 2019

skinny85 commented Oct 22, 2019

aws-cdk-automation commented Oct 22, 2019

rix0rrr commented Oct 23, 2019

skinny85 commented Oct 24, 2019

aws-cdk-automation commented Oct 24, 2019

jd-carroll commented Oct 24, 2019

skinny85 commented Oct 24, 2019

aws-cdk-automation commented Oct 28, 2019

skinny85 commented Oct 28, 2019

skinny85 commented Oct 28, 2019

aws-cdk-automation commented Oct 28, 2019

rix0rrr Oct 29, 2019

mergify bot commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

gsdwait commented Nov 7, 2019

jk21 commented Nov 8, 2019

ranman commented Nov 17, 2019

jk21 commented Nov 17, 2019

ranman commented Nov 17, 2019 via email

		@@ -1,4 +1,4 @@
		export const VPC_PROVIDER = 'vpc-provider';
		export const VPC_PROVIDER = 'vpc-provider2';

feat(vpc): allow Vpc.fromLookup() to discover asymmetric subnets #4544

feat(vpc): allow Vpc.fromLookup() to discover asymmetric subnets #4544

Conversation

skinny85 commented Oct 17, 2019

mergify bot commented Oct 17, 2019

aws-cdk-automation commented Oct 17, 2019

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Oct 21, 2019

Choose a reason for hiding this comment

skinny85 Oct 22, 2019

Choose a reason for hiding this comment

skinny85 Oct 22, 2019

Choose a reason for hiding this comment

rix0rrr Oct 23, 2019

Choose a reason for hiding this comment

skinny85 Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

rix0rrr Oct 28, 2019

Choose a reason for hiding this comment

skinny85 Oct 28, 2019

Choose a reason for hiding this comment

skinny85 commented Oct 22, 2019

aws-cdk-automation commented Oct 22, 2019

AWS CodeBuild CI Report

rix0rrr commented Oct 23, 2019

skinny85 commented Oct 24, 2019

aws-cdk-automation commented Oct 24, 2019

AWS CodeBuild CI Report

jd-carroll commented Oct 24, 2019

skinny85 commented Oct 24, 2019

aws-cdk-automation commented Oct 28, 2019

AWS CodeBuild CI Report

skinny85 commented Oct 28, 2019

skinny85 commented Oct 28, 2019

aws-cdk-automation commented Oct 28, 2019

AWS CodeBuild CI Report

rix0rrr Oct 29, 2019

Choose a reason for hiding this comment

mergify bot commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

gsdwait commented Nov 7, 2019

jk21 commented Nov 8, 2019

ranman commented Nov 17, 2019

jk21 commented Nov 17, 2019

ranman commented Nov 17, 2019 via email

skinny85 Oct 24, 2019 •

edited

Loading