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

feat(cli): VPC context provider looks up RouteTable IDs #3171

Merged
merged 10 commits into from
Jul 25, 2019

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Jul 2, 2019

Import RouteTableIds for subnets when looking up a VPC from the VpcContextProvider.
This associates the Subnet-specific route table ID if there is one, and falls back to the
VPC's main route table otherwise.

Fixes #2487
Fixes #2926


Please read the contribution guidelines and follow the pull-request checklist.

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

- Correctly detect VPC without any subnets & abort with specific message.
- Correctly validate that only valid aws-cdk:subnet-type values are present.
@RomainMuller RomainMuller requested a review from a team as a code owner July 2, 2019 14:18
@ghost ghost requested a review from eladb July 2, 2019 14:18
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

missing tests

@RomainMuller RomainMuller changed the title feat(cli): Improved validation on VPC Context Provider feat(cli): VPC context provider looks up RouteTable IDs Jul 3, 2019
@RomainMuller RomainMuller self-assigned this Jul 3, 2019
@RomainMuller RomainMuller requested a review from a team as a code owner July 12, 2019 12:06
@RomainMuller RomainMuller requested a review from skinny85 as a code owner July 24, 2019 13:33
@RomainMuller RomainMuller requested a review from eladb July 24, 2019 13:41
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Good job with the tests!

packages/@aws-cdk/alexa-ask/package-lock.json Show resolved Hide resolved
if (routeTableIds == null) {
// Maintaining backwards-compatibility - this used to not be provided by the VPC Context Provider!
// tslint:disable-next-line: max-line-length
this.warning = `No routeTableIds were provided for subnets ${this.subnetIds.join(', ')}! Calling .routeTableId on these subnets will return undefined/null!`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it "throw" instead of returning undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be a breaking change...

packages/@aws-cdk/aws-ec2/lib/util.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/lib/util.ts Outdated Show resolved Hide resolved
@RomainMuller RomainMuller merged commit 6d762f9 into master Jul 25, 2019
@RomainMuller RomainMuller deleted the rmuller/improve-vpc-lookup branch July 25, 2019 06:59
jogold added a commit to jogold/aws-cdk that referenced this pull request Aug 1, 2019
Move `addGatewayEndpoint()` to `IVpc` now that imported VPCs can have route table IDs populated (aws#3171).

Closes aws#3472
mergify bot pushed a commit that referenced this pull request Aug 7, 2019
* fix(ec2): allow adding gateway endpoints to imported VPC

Move `addGatewayEndpoint()` to `IVpc` now that imported VPCs can have route table IDs populated (#3171).

Closes #3472

* deprecate addS3Endpoint() and addDynamoDbEndpoint()

* remove test on conveniance methods
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
? `at '${scope.node.path}/${id}'`
: `'${attrs.subnetId}'`;
// tslint:disable-next-line: max-line-length
scope.node.addWarning(`No routeTableId was provided to the subnet ${ref}. Attempting to read it's .routeTable.routeTableId will return null/undefined. (More info: https://github.com/aws/aws-cdk/pull/3171)`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: For anyone directed to this PR by the No routeTableId was provided to the subnet warning message: See this issue: #19786, which about making this warning only appear when necessary.

Choose a reason for hiding this comment

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

Thanks, but how do I fix the actual warning? I need the route table ids to be pulled back so I can make use of them. Do I have to hard-code them in my config and use from_attributes( ... routeTableId ...)?

Choose a reason for hiding this comment

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

I ended up fetching the subnets and route tables and marrying them via Subnet.fromSubnetAttributes. Not ideal, but I'm sure there's a reason...

Choose a reason for hiding this comment

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

From linked issue, do something like:

      cdk.Annotations.of(subnet).acknowledgeWarning(
        "@aws-cdk/aws-ec2:noSubnetRouteTableId",
        "Will not read route table ID for subnet",
      );

@JoshMcCullough
Copy link

Why, when fetching a subnet via lookup, is the routeTableId not populated??

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.
Projects
None yet
6 participants