feat(apigatewayv2): vpc link and private integrations #10531

ayush987goyal · 2020-09-25T08:51:54Z

VpcLink and HttpProxyPrivateIntegration.

References in AWS docs:
VpcLink
Integration-1
Integration-2

closes #10119

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

nija-at

Thanks for submitting this PR. First round of comments.

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

packages/@aws-cdk/aws-apigatewayv2/lib/http/vpc-link.ts

Pull request has been modified.

ayush987goyal · 2020-10-01T11:14:40Z

Hi @nija-at ,

I have modified the previous implementation to a more specific one by also adding the usage with HttpProxyPrivateIntegration.

Some open things that I need help on:

A better example in the readme.
A better way to collect targets in HttpProxyPrivateIntegration. Currently I have implemented it to take the ARNs directly.
A way to verify the integration test stack. (integration tested by deployment but verification was not done)

nija-at

Good job on the first iteration of this PR. Most of this code is conformant to the CDK style and how we model this!

I've not gone through the code in detail but some high level comments around code and class organization that will need to be first addressed.

packages/@aws-cdk/aws-apigatewayv2/lib/http/integrations/http-private.ts

packages/@aws-cdk/aws-apigatewayv2/lib/http/vpc-link.ts

packages/@aws-cdk/aws-apigatewayv2/lib/http/integrations/http-private.ts

packages/@aws-cdk/aws-apigatewayv2/lib/http/vpc-link.ts

Pull request has been modified.

ayush987goyal · 2020-10-09T09:23:18Z

There is still an open question on a way to verify the integration tests stack (which currently just tests the deployment)

nija-at · 2020-10-12T09:26:09Z

The above one will be a tricky implementation since the current modelling of integration themselves are
not as constructs. So we will have to create the VpcLink in the Route construct where the bind method of
integrations are called.

We do this in several places already. Here's one example - https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-lambda-destinations/lib/lambda.ts#L38-L57

We might then also have to touch the defaultIntegration and HttpApi for the new Route behaviour and
would need to see how we do this for non-private integration. Therefore I have decided to go ahead
without this change for now and we could take it as part of further enhancements.

I'm not sure I'm following this. Could you explain a bit better?

It's fine if you want this to be done later but I would like to understand what's preventing this better.

If it was simple, it could be a nice customer experience.

nija-at

See my comments below. The main one is about splitting off into its own package.

Also, take a look at this one - #10531 (comment)

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

packages/@aws-cdk/aws-apigatewayv2/lib/http/integration.ts

...igatewayv2/lib/http/integrations/private-integration/application-loadbalancer-integration.ts

packages/@aws-cdk/aws-apigatewayv2/lib/http/integrations/private-integration/http-private.ts

...igatewayv2/lib/http/integrations/private-integration/application-loadbalancer-integration.ts

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

nija-at · 2020-10-12T09:50:26Z

There is still an open question on a way to verify the integration tests stack (which currently just tests the deployment)

You can run the integ test using yarn integ integ.custom-domain.js. This will deploy and then destroy the stack. If all goes well, it will report success.

Credentials can be configured using any means that you do for the AWS CLI.

ayush987goyal · 2020-10-12T10:09:05Z

There is still an open question on a way to verify the integration tests stack (which currently just tests the deployment)

You can run the integ test using yarn integ integ.custom-domain.js. This will deploy and then destroy the stack. If all goes well, it will report success.

Credentials can be configured using any means that you do for the AWS CLI.

Yeah I was able to test the deployment successfully. I was specifically talking about the verification steps we mention at the top of the integ-tests file. I was looking for a way to verify these stacks basically.

nija-at · 2020-10-12T12:53:15Z

Yeah I was able to test the deployment successfully. I was specifically talking about the verification steps we mention at the top of the integ-tests file. I was looking for a way to verify these stacks basically.

We currently don't have a way to run those tests in the CDK. These should contain steps on verification beyond deployment.

As an example, in your case, it should deploy a stack that creates an ALB that returns '200' status code, which is then integrated to an HttpApi construct. The steps should simply be call curl on the API endpoint and it should return '200'.
This internally will verify that the request goes to the ALB successfully and the response is correctly received.

Hope that makes it clear.

gitpod-io · 2020-10-17T09:20:25Z

Pull request has been modified.

ayush987goyal · 2020-10-17T09:24:20Z

Hi @nija-at ,

I have implemented the integrations by taking into account your suggestions and separated out the packages as well.

The ServiceDiscoveryPrivateIntegration still requires a vpcLink to be supplied mandatorily (unlike the ALB and NLB integrations) since there is no way to extract out the VPC from the Serivce or the Namespace.

nija-at

Wow! IMPRESSIVE.

Thanks for all the work here. I'm sure you had a few hoops to jump through.

Mostly adjustments to README and naming conventions, besides one or two other comments. This has been great!

packages/@aws-cdk/aws-apigatewayv2-integrations/README.md

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/alb-integration.ts

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http-private.ts

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/alb-integration.ts

packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts

Pull request has been modified.

nija-at · 2020-10-22T09:05:25Z

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http/http-private.ts

+      if (!this.props.vpc) {
+        throw new Error('One of vpcLink or vpc should be provided for private integration');
+      }


we should also error when both are provided and they don't point to the same vpc

Is that validation required? We are currently ignoring the passed vpc if the vpcLink is supplied. Basically vpcLink trumps the vpc.

Also, the vpcLink in v2 can point to multiple VPCs in theory (since it takes subnets and securityGroups). In this case it might not make sense to compare the VPCs.

Thoughts?

Accepting a property and not using it is usually confusing. "Why was my vpc not used?" would be a complaint I expect to hear.

How about just failing if both are presented? It'll make the requirement that only one should be passed clear.

Yeah, sounds good.

This should not be required anymore since we are not accepting vpc as a prop (it is passed around internally by integrations)

How come?
In the case of HttpAlbIntegration, we accept an ALB and a VPCLink. If the ALB is in a different vpc from the vpc link, it's useful to error, no?

Oh that way.

But in that case the intent was explicit to use the passed vpcLink right? We are not explicitly taking the vpc as a prop. We are inferring.

Also, as mentioned before, the vpcLink in v2 is not bound to just one vpc in theory. It revolves around subnets and securityGroups. We have just provided a convenience to pass a vpc to create the vpcLink. So comparing the vpc of vpcLink and ALB listener might not be ideal.

Thoughts?

I see. Ok, let's leave it as is for now. See how this pans out.

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http-private.ts

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http/service-discovery-integration.ts

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/alb-integration.ts

gitpod-io · 2020-10-23T14:36:42Z

nija-at · 2020-10-23T14:37:16Z

I've taken the liberty to pare down the file names to make it more terse and moved the http-private.ts into a private/ folder, like we do in other parts of the CDK construct library. Let me know if you're ok with this.

The code looks great otherwise.

ayush987goyal · 2020-10-23T14:41:12Z

Thanks for these changes. They look good to me.

nija-at

All the good stuff 🙌

mergify · 2020-10-23T15:04:18Z

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 · 2020-10-23T15:32:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 3855ada
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-10-23T15:32:38Z

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

shivlaks · 2020-10-23T17:07:52Z

heads up: I believe this also requires aws/jsii#2172 as the usage of @internal for HttpPrivateIntegration and VpcLinkConfigurationOptions is causing downstream compilation failures currently.

may need to temporarily revert this.

ayush987goyal · 2020-10-23T17:15:03Z

@shivlaks @nija-at Please go ahead with the solution you seem fit.

This reverts commit 0537598.

shivlaks · 2020-10-23T17:26:56Z

@shivlaks @nija-at Please go ahead with the solution you seem fit.

@ayush987goyal perfect. we're going to temporarily revert this and merge this back in when the next release of jsii is consumed in the cdk. stay tuned!

…" (#11070) This reverts commit 0537598. This change requires aws/jsii#2172 to be merged and a jsii upgrade as the usage of `@internal` for `HttpPrivateIntegration` and `VpcLinkConfigurationOptions` is causing downstream compilation failures currently. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This reverts commit 65be3a0. Original PR: #10531 Co-authored-by: Ayush Goyal <goyalayu@amazon.com> This commit was previously reverted due to a bug in jsii - aws/jsii#1947 and aws/jsii#1830. This has been fixed in jsii version 1.14.0.

This reverts commit 65be3a0. Original PR: #10531 Co-authored-by: Ayush Goyal <goyalayu@amazon.com> relates #10119 This commit was previously reverted due to a bug in jsii - aws/jsii#1947 and aws/jsii#1830. This has been fixed in jsii version 1.14.0. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

ayush987goyal mentioned this pull request Sep 25, 2020

[apigatewayv2] http api - support vpc link #10119

Closed

SomayaB assigned nija-at Sep 25, 2020

nija-at previously requested changes Sep 30, 2020

View reviewed changes

nija-at changed the title ~~feat(apigatewayv2): Add support for VpcLink~~ feat(apigatewayv2): vpc link Sep 30, 2020

ayush987goyal changed the title ~~feat(apigatewayv2): vpc link~~ feat(apigatewayv2): VpcLink and HttpProxyPrivateIntegration Oct 1, 2020

ayush987goyal requested a review from nija-at October 1, 2020 11:14

nija-at previously requested changes Oct 7, 2020

View reviewed changes

ayush987goyal requested a review from nija-at October 9, 2020 09:23

nija-at previously requested changes Oct 12, 2020

View reviewed changes

ayush987goyal requested a review from nija-at October 17, 2020 09:21

nija-at previously requested changes Oct 21, 2020

View reviewed changes

ayush987goyal requested a review from nija-at October 21, 2020 17:18

nija-at reviewed Oct 22, 2020

View reviewed changes

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http/service-discovery-integration.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-apigatewayv2-integrations/lib/alb-integration.ts Outdated Show resolved Hide resolved

feat(apigatewayv2): VpcLink and HttpProxyPrivateIntegration

9e1b566

ayush987goyal requested a review from nija-at October 22, 2020 14:18

terser file names and private/ folder

9943204

nija-at approved these changes Oct 23, 2020

View reviewed changes

nija-at changed the title ~~feat(apigatewayv2): VpcLink and HttpProxyPrivateIntegration~~ feat(apigatewayv2): vpc link and private integrations Oct 23, 2020

Merge branch 'master' into pr/vpc-link

3855ada

mergify bot merged commit 0537598 into aws:master Oct 23, 2020

ayush987goyal deleted the pr/vpc-link branch October 23, 2020 15:33

shivlaks added a commit that referenced this pull request Oct 23, 2020

Revert "feat(apigatewayv2): vpc link and private integrations (#10531)"

acb84ed

This reverts commit 0537598.

shivlaks added a commit that referenced this pull request Oct 23, 2020

Revert "feat(apigatewayv2): vpc link and private integrations (#10531)"

1f9954f

This reverts commit 0537598.

nija-at mentioned this pull request Oct 29, 2020

feat(apigatewayv2): vpc link and private integrations #11198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigatewayv2): vpc link and private integrations #10531

feat(apigatewayv2): vpc link and private integrations #10531

ayush987goyal commented Sep 25, 2020 •

edited

Loading

nija-at left a comment

ayush987goyal commented Oct 1, 2020 •

edited

Loading

nija-at left a comment

ayush987goyal commented Oct 9, 2020

nija-at commented Oct 12, 2020

nija-at left a comment

nija-at commented Oct 12, 2020

ayush987goyal commented Oct 12, 2020

nija-at commented Oct 12, 2020

gitpod-io bot commented Oct 17, 2020 •

edited

Loading

ayush987goyal commented Oct 17, 2020

nija-at left a comment

nija-at Oct 22, 2020 •

edited

Loading

ayush987goyal Oct 22, 2020

nija-at Oct 22, 2020

ayush987goyal Oct 22, 2020

ayush987goyal Oct 22, 2020

nija-at Oct 23, 2020

ayush987goyal Oct 23, 2020

nija-at Oct 23, 2020

gitpod-io bot commented Oct 23, 2020 •

edited

Loading

nija-at commented Oct 23, 2020

ayush987goyal commented Oct 23, 2020

nija-at left a comment

mergify bot commented Oct 23, 2020

aws-cdk-automation commented Oct 23, 2020

mergify bot commented Oct 23, 2020

shivlaks commented Oct 23, 2020 •

edited

Loading

ayush987goyal commented Oct 23, 2020

shivlaks commented Oct 23, 2020

feat(apigatewayv2): vpc link and private integrations #10531

feat(apigatewayv2): vpc link and private integrations #10531

Conversation

ayush987goyal commented Sep 25, 2020 • edited Loading

nija-at left a comment

Choose a reason for hiding this comment

ayush987goyal commented Oct 1, 2020 • edited Loading

nija-at left a comment

Choose a reason for hiding this comment

ayush987goyal commented Oct 9, 2020

nija-at commented Oct 12, 2020

nija-at left a comment

Choose a reason for hiding this comment

nija-at commented Oct 12, 2020

ayush987goyal commented Oct 12, 2020

nija-at commented Oct 12, 2020

gitpod-io bot commented Oct 17, 2020 • edited Loading

ayush987goyal commented Oct 17, 2020

nija-at left a comment

Choose a reason for hiding this comment

nija-at Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

ayush987goyal Oct 22, 2020

Choose a reason for hiding this comment

nija-at Oct 22, 2020

Choose a reason for hiding this comment

ayush987goyal Oct 22, 2020

Choose a reason for hiding this comment

ayush987goyal Oct 22, 2020

Choose a reason for hiding this comment

nija-at Oct 23, 2020

Choose a reason for hiding this comment

ayush987goyal Oct 23, 2020

Choose a reason for hiding this comment

nija-at Oct 23, 2020

Choose a reason for hiding this comment

gitpod-io bot commented Oct 23, 2020 • edited Loading

nija-at commented Oct 23, 2020

ayush987goyal commented Oct 23, 2020

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 23, 2020

aws-cdk-automation commented Oct 23, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 23, 2020

shivlaks commented Oct 23, 2020 • edited Loading

ayush987goyal commented Oct 23, 2020

shivlaks commented Oct 23, 2020

ayush987goyal commented Sep 25, 2020 •

edited

Loading

ayush987goyal commented Oct 1, 2020 •

edited

Loading

gitpod-io bot commented Oct 17, 2020 •

edited

Loading

nija-at Oct 22, 2020 •

edited

Loading

gitpod-io bot commented Oct 23, 2020 •

edited

Loading

shivlaks commented Oct 23, 2020 •

edited

Loading