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(ec2): access a vpc's internet gateway #7939

Merged
merged 10 commits into from
Jul 19, 2020
Merged

feat(ec2): access a vpc's internet gateway #7939

merged 10 commits into from
Jul 19, 2020

Conversation

shearn89
Copy link
Contributor

@shearn89 shearn89 commented May 12, 2020

This commit introduces a new internetGateway attribute to the VPC
construct to allow for creative routing using the default IGW added
with when using a public subnet.

Resolves #5327


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

packages/@aws-cdk/aws-ec2/lib/vpc.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed rix0rrr’s stale review May 28, 2020 16:41

Pull request has been modified.

@shearn89
Copy link
Contributor Author

I've made that change, I'll try and run through the integration tests at some point to confirm that works as well.

@shearn89 shearn89 marked this pull request as ready for review May 31, 2020 15:37
@shearn89
Copy link
Contributor Author

Integration tests running now.

@shearn89
Copy link
Contributor Author

The integration tests all pass but 2: integ.vpc-endpoint-service.js which complains about InvalidNetworkLoadBalancerArn.NotFound, and also integ.vpc-endpoint-service-cn.js which complains about credentials.

These don't appear to be failures caused by my code, so I think this should be ready for review and merge.

@iliapolo iliapolo assigned iliapolo and unassigned rix0rrr Jun 8, 2020
@shearn89
Copy link
Contributor Author

Hi @iliapolo/@rix0rrr - any chance of a review soon?

@iliapolo
Copy link
Contributor

Hi @shearn89. Yes I will take a look at this soon. In the meantime notice you have a conflict...

@shearn89
Copy link
Contributor Author

shearn89 commented Jul 1, 2020

Brill, thanks - Ah, I hadn't spotted that since last commenting, will address those this evening!

Copy link
Contributor

@iliapolo iliapolo left a comment

Choose a reason for hiding this comment

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

Hi @shearn89 - Thanks for this PR!

Im not sure I understand why we need the whole IInternetGateway interface and its complexity.
It seems like all we want to do is expose the internetGatewayId of a newly created VPC - right?

So why not just have:

public readonly internetGatewayId: string;

Only on the VPC class, and assign its value in the constructor:

this.internetGatewayId = igw.ref;

This way all imported VPC's won't even have this attribute available to them, which is ok since its not supported anyway.

Am I missing something?

Also, The documentation doesn't really emphasize the feature you are adding, but rather shows a specific use-case. Lets be clear about what the feature is:

Accessing the Internet Gateway

<explain its only supported for VPC's created in the stack, not imported ones>
<give an example use-case - like you do, but see if you can find one without having to downcast objects).

@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 10, 2020
@github-actions
Copy link

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jul 11, 2020
@shearn89
Copy link
Contributor Author

Hi @iliapolo: okay, that makes sense! TypeScript is not my forte so I copied what was already there, slightly missing the point of the interface. I'll try to get the PR updated this weekend if I can!

@mergify mergify bot dismissed iliapolo’s stale review July 11, 2020 12:39

Pull request has been modified.

@shearn89
Copy link
Contributor Author

Okay! Apologies for the mess/notification spam, had a shocker when I tried to update from master. Should be all fixed now, with only the changes I actually made!

packages/@aws-cdk/aws-ec2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/lib/vpc.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/test/vpc.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/README.md Show resolved Hide resolved
@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jul 12, 2020
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 12, 2020
@shearn89
Copy link
Contributor Author

Cool, thanks for the comments. I'm busy today but will pick this up this week and get it updated.

@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 13, 2020
@mergify mergify bot dismissed iliapolo’s stale review July 19, 2020 11:36

Pull request has been modified.

@iliapolo iliapolo removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 19, 2020
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 06dbe28
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit cb5dad8 into aws:master Jul 19, 2020
Chriscbr pushed a commit to Chriscbr/aws-cdk that referenced this pull request Jul 23, 2020
This commit introduces a new `internetGateway` attribute to the VPC
construct to allow for creative routing using the default IGW added
with when using a public subnet.

Resolves aws#5327

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this pull request Aug 11, 2020
This commit introduces a new `internetGateway` attribute to the VPC
construct to allow for creative routing using the default IGW added
with when using a public subnet.

Resolves aws#5327

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access a Vpc's Internet Gateway
4 participants