Skip to content

Conversation

@pahud
Copy link
Contributor

@pahud pahud commented Dec 11, 2019

initial Amazon API Gateway HTTP API support for HTTP and Lambda integrations. (fix #5301)

still WIP.

Sample

    // HTTP API on root route with Lambda integration
    const httpApi = new apigw.HttpApi(this, 'MyHttpApi', {
      handler: echoFunc
    });

    // HTTP ANY /demo with Lambda integration
    const demoRoute = httpApi.root.addRoute('demo', {
      integrationType: HttpApiIntegrationType.LAMBDA,
      target: greetingFunc
    });

    // same as above with the addLambdaRoute() shorthand
    // HTTP ANY /demo/demo2
    demoRoute.addLambdaRoute('demo2', {
      target: greetingFunc,
    });

    // HTTP ANY /checkip
    // HTTP ANY /checkip will proxy to HTTP GET https://checkip.amazonaws.com/
    httpApi.root.addRoute('checkip', {
      integrationType: HttpApiIntegrationType.HTTP,
      integrationMethod: HttpMethod.GET,
      targetUrl: 'https://checkip.amazonaws.com/'
    });

    // same as above with the addHttpRoute() shorthand
    // HTTP ANY /checkip2 will proxy to HTTP ANY https://checkip.amazonaws.com/
    httpApi.root.addHttpRoute('checkip2', {
      targetUrl: 'https://checkip.amazonaws.com/'
    });

圖片


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

…da integration

initial Amazon API Gateway HTTP API support
@mergify
Copy link
Contributor

mergify bot commented Dec 11, 2019

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@pahud
Copy link
Contributor Author

pahud commented Dec 11, 2019

Hi @nija-at

As I am still working on it, would you please check my initial implementation and let me know if I am on the right track before I continue this PR?

thanks

@hoegertn
Copy link
Contributor

Hi, I really like the approach. What do you think about having convenience methods in the RootRoute like addLambdaRoute(path, method, function) or addHttpRoute(path, method, targetUrl, targetMethod). This might be easier to write and understand if you have many integrations.

@pahud
Copy link
Contributor Author

pahud commented Dec 12, 2019

@hoegertn addHttpRoute() and addLambdaRoute() were implemented and added :-)

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@nija-at
Copy link
Contributor

nija-at commented Dec 12, 2019

Given that there is a good chunk of overlap between the resource type names in v1 vs v2, what's your strategy to avoid namespace collisions?

From a cursory look, Deployment, Authorizer, DomainName and Stage overlap. As API Gateway v2 continues to cover more features and types that are in v1 but not in v2, I suspect there'll be a few more.

@pahud
Copy link
Contributor Author

pahud commented Dec 12, 2019

@nija-at Do you mean we should start creating v2 constructs in @aws-cdk/aws-apigatewayv2?

Another option might be using the V2 as the suffix to avoid potential namespace overlapping such as DeploymentV2, AuthorizerV2, DomainNameV2 and StageV2 indicating they come from AWS::ApiGatewayV2 resources.

Thought?

@nija-at
Copy link
Contributor

nija-at commented Dec 16, 2019

@pahud - First of all, thanks for getting ahead of this and starting your contribution of APIGatewayV2 to the CDK.

A couple of things first -

When building out new constructs to our library, we want to make sure we provide the best customer experience we can. Due to this, I expect this review to be quite an involved process, both for you as a contributor and myself as a reviewer.
During this review, we'll likely go through several major iterations of the code to build this out correctly.
I'm not entirely familiar with the APIGatewayV2 offering as yet and will be familiarizing myself as we get into these changes. This is going to need a chunk of my time to be spent here. There are going to be times when I'll not be available right away to provide feedback, based on other commitments. So, I apologize in advance if you don't hear from me for a few days.

All in all, this review is likely going to take anywhere between a few weeks to a couple of months to get right and ship to customers.

I hope this clarifies the effort involved here and sets up the right expectations in terms of time commitments 😊.

Now back to this review -

@nija-at Do you mean we should start creating v2 constructs in @aws-cdk/aws-apigatewayv2?

Another option might be using the V2 as the suffix to avoid potential namespace overlapping such as DeploymentV2, AuthorizerV2, DomainNameV2 and StageV2 indicating they come from AWS::ApiGatewayV2 resources.

Thought?

I believe this would be cleaner if we start with a new set of constructs under @aws-cdk/aws-apigatewayv2.
To do this, we should gather a separate pull request which moves the generated apigatewayv2.generated.ts into the new package.
At the same time, take the current apigatewayv2.generated.ts and check it into its current location and mark them as @deprecated. This will ensure we don't break any customers that already use our Cfn* constructs but they will need to move to the new package for any newer updates.

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

5 similar comments
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

@bionicles
Copy link

@pahud @nija-at bump, follow up, I really just want a nice simple aws-cdk-example of this working, how do we finish this?

@pahud
Copy link
Contributor Author

pahud commented Jan 4, 2020

@bionicles I am reaching out to @nija-at to discuss how to move to next step.

@SomayaB SomayaB added draft pr/work-in-progress This PR is a draft and needs further work. and removed draft labels Jan 7, 2020
@bionicles
Copy link

What specific changes are needed to make this merge?

@nija-at
Copy link
Contributor

nija-at commented Feb 21, 2020

No update in 2 months. Closing this PR for now. Feel free to reopen when you're ready to pick this back up, or you can open a new one.

@nija-at nija-at closed this Feb 21, 2020
@ryan-mars
Copy link

@pahud I'm sorry this wasn't merged but thank you for writing it. Your code helped me to understand the HTTP API better so I could write my own L2 constructs.

@pahud
Copy link
Contributor Author

pahud commented Feb 22, 2020

Thanks folks. Glad to see we have apigatewayv2 L1 now. I will explore the L1 and try build L2 in the next few days. Probably will create a new PR if there's any progress.

@nija-at
Copy link
Contributor

nija-at commented Feb 22, 2020

Sounds good.
I suggest starting off by implementing a couple of L2s and aiming to get smaller but more PRs, so it's faster to iterate on and make progress.

@ryan-mars
Copy link

@pahud Given @nija-at recommendation to do more smaller PR's I'm happy to help. I'll follow your lead if you want to break down and split the work somehow. For context I'm somewhat familiar with using the L1 construct to import OAS for HTTP APIs in apigatewayv2.

@pahud
Copy link
Contributor Author

pahud commented Feb 23, 2020

Thanks @ryan-mars

I will rebuild this PR with the apigatewayv2 L1 with unit tests and create a new PR tomorrow. Hopefully we will have initial L2 support for some resources including Api, Route and Integration and will be able to create HTTP API either Lambda or HTTP Proxy with L2 very soon.

@pahud
Copy link
Contributor Author

pahud commented Feb 25, 2020

Have created the new PR here #6432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/work-in-progress This PR is a draft and needs further work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws-apigateway: support HttpApi

7 participants