feat(aws-apigatewayv2): l2 construct for api gateway v2 #7097

julienlepine · 2020-03-31T18:09:59Z

Initial implementation of the API Gateway V2 Constructs.
Implements all base APIGW constructs as L2 Constructs.

Merge

Merge #2

Initial implementation of the API Gateway V2 Constructs. Implements all base APIGW constructs as L2 Constructs. fixes #5301 fixes #2872 fixes #212

Merge from remote master

Merge from master

julienlepine · 2020-03-31T20:19:17Z

@nija-at I finally found the time to tidy up my initial hacky API Gateway v2 L2 construct, happy to review.

adamnfish

I'm really excited to see this come to CDK. I noticed a couple of things in the README but I'm only looking at this from the perspective of a potential user.

Thanks heaps for the hard work involved in getting this PR up.

adamnfish · 2020-04-02T21:56:53Z

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

+
+```ts
+const api = new apigatewayv2.Api(this, 'books-api', {
+  protocolType: apigatewayv2.ProtocolType.HTTP


Based on the preceding paragraph, should this be using the apigatewayv2.ProtocolType.WEBSOCKET protocol?

Looks like the same may be true of a few other examples here - the code snippet doesn't match up to the description?

ranman · 2020-04-09T22:43:04Z

I'm also excited to see this soon. Is there anything I can help with?

Merge from master (20200418)

merge from master

nija-at · 2020-04-22T13:44:30Z

packages/@aws-cdk/aws-apigatewayv2/lib/api.ts

+    switch (props.protocolType) {
+      case ProtocolType.WEBSOCKET: {
+        if (props.basePath !== undefined) {
+          throw new Error('"basePath" is only supported with HTTP APIs');
+        }
+        if (props.body !== undefined) {
+          throw new Error('"body" is only supported with HTTP APIs');
+        }
+        if (props.bodyS3Location !== undefined) {
+          throw new Error('"bodyS3Location" is only supported with HTTP APIs');
+        }
+        if (props.corsConfiguration !== undefined) {
+          throw new Error('"corsConfiguration" is only supported with HTTP APIs');
+        }
+        if (props.routeSelectionExpression === undefined) {
+          throw new Error('"routeSelectionExpression" is required for Web Socket APIs');
+        }
+        break;
+      }
+      case ProtocolType.HTTP:
+      case undefined: {
+        if (props.apiKeySelectionExpression !== undefined) {
+          throw new Error('"apiKeySelectionExpression" is only supported with Web Socket APIs');
+        }
+        if (props.disableSchemaValidation !== undefined) {
+          throw new Error('"disableSchemaValidation" is only supported with Web Socket APIs');
+        }
+        if (props.routeSelectionExpression !== undefined && props.apiKeySelectionExpression !== KnownRouteSelectionExpression.METHOD_PATH) {
+          throw new Error('"routeSelectionExpression" has a single supported value for HTTP APIs: "${request.method} ${request.path}"');
+        }
+        break;
+      }
+    }


Because of the level of differences between HTTP and Websocket APIs, I'm wondering if it would be cleaner to split these up into separate L2s.

A customer is going to know right from the beginning if they're building an HTTP API or a Websocket API.
It seems to me that we could provide a richer experience if we define two separate L2s for these.
I don't think it's possible or makes sense to mix routes between them, does it?

Another example is routes.
Routes in websocket are fairly different from routes in HTTP API. They can both be modeled as a string, but there may be value in modeling them even more strongly in the CDK.

With integrations, while both websocket and HTTP support Lambda and HTTP integrations, websocket also supports AWS service integrations but the HTTP API supports private integrations.

And so it goes on...

We have another open PR for APIGWv2 support. Take a look at some API suggestions on how this could look instead - #6432 (comment).

Let me know what you think.

As a potential consumer of this I don't care if it is exposed via 2 different L2 APIs as long as I can use the functionality of both and get compile time errors if I have a mismatch between configurations and usage. If two different L2 APIs make that easier then I say go for it!

Really hoping to see this feature soon as the v2 APIGW APIs have some great features I want to use!

i really like @pahud 's view, will use his view and combine with this PR

Support ESLint rules: - [comma-dangle]: Missing trailing comma - [quotes]: Strings must use singlequote'

nija-at · 2020-04-28T08:13:02Z

@julienlepine - I'm working with @pahud's this week on his PR to try and get a minimum set of CDK code for APIGWv2, with the aim to iron out an ergonomic API.
Letting you know in case you want to wait to see how that pans out.

julienlepine · 2020-04-28T10:37:12Z

I have a near final implementation that I will push this afternoon. Julien

…

Le 28 avr. 2020 à 10:13, Niranjan Jayakar ***@***.***> a écrit : @julienlepine - I'm working with @pahud's this week on his PR to try and get a minimum set of CDK code for APIGWv2, with the aim to iron out an ergonomic API. Letting you know in case you want to wait to see how that pans out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

…e/aws-cdk into julienlepine/apigwv2

julienlepine · 2020-04-28T13:20:39Z

Pushed, checked with @nija-at he will look at it.

nija-at

The awesome thing about this PR is that it has implemented a significant number of APIGWv2 features, if not all. This is a great job @julienlepine! 😊

However, I feel that this is missing a few patterns to get to a CDK module - this is mainly around using union types and how we have layered Http and Websocket on top of Api. (see individual comments below)

Over the next few days I'm going to try and re-hash either this or the other PR to get the right ergonomic pattern, but with only a very minimal set of features.
Once we get the right pattern going, features here can be re-based onto it piecemeal.

This would be a good way to divide and conquer here.

nija-at · 2020-04-28T13:37:35Z

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

+   */
+  public addRoute(
+    key: HttpRouteName,
+    integration: Integration | LambdaIntegration | HttpIntegration | ServiceIntegration,


Unfortunately, we don't support union types in the CDK yet, so this is not going to work. Union types translate into Object in Java and C#.

You will have to use the bind integration pattern.

The best example for this is how we've done event sources in the lambda package. See https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-lambda/lib/event-source.ts and https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-lambda-event-sources/lib.

This applies in all other places you've done union types.

nija-at · 2020-04-28T13:42:10Z

packages/@aws-cdk/aws-apigatewayv2/lib/api.ts

+  /**
+   * The OpenAPI definition.
+   *
+   * To import an HTTP API, you must specify `body` or `bodyS3Location`.
+   *
+   * Supported only for HTTP APIs.
+   * @default - `bodyS3Location` if defined, or no import
+   */
+  readonly body?: string;
+
+  /**
+   * The S3 location of an OpenAPI definition.
+   *
+   * To import an HTTP API, you must specify `body` or `bodyS3Location`.
+   *
+   * Supported only for HTTP APIs.
+   * @default - `body` if defined, or no import
+   */
+  readonly bodyS3Location?: BodyS3Location;


I would drop these entirely for now. Let's add support for this in a separate PR.
They need to be modeled differently in the L2s. See #7372

nija-at · 2020-04-28T13:47:05Z

packages/@aws-cdk/aws-apigatewayv2/lib/api.ts

+/**
+ * Available protocols for ApiGateway V2 APIs (currently only 'WEBSOCKET' is supported)
+ */
+export enum ProtocolType {


I would change this to be a class instead, so as to support protocol types not yet supported by the cdk.

See https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-lambda/lib/runtime.ts#L25

nija-at · 2020-04-28T13:50:22Z

packages/@aws-cdk/aws-apigatewayv2/lib/api.ts

+ * By default, the API will automatically be deployed and accessible from a
+ * public endpoint.
+ */
+export class Api extends Resource implements IApi {


I don't quite see much value in having Api. It feels like it's just L1 + validation, and this validation is what CF already provides.

nija-at

@julienlepine - we now have a structure of the APIGatewayV2 package with the patterns well organized - https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-apigatewayv2.

It currently only has HTTP APIs but I hope it's clear on where the websocket APIs will go under.
I recommend following this same pattern and contribute on top of this change.

I would also highly appreciate it if we could features piecemeal in smaller PRs than one massive PR. This makes it much easier on the reviewer and yourself to iterate and add small changes in.

Merged the model to be compatible with the new Http Api Model

…es) to classes Update of the classes of API Gateway V2 to use classes instead of enums for open-ended value lists (known content type, routes, models, ...)

…rations Bind the IAM policies for Lambda functions ro the integration/route pair, instead of just to the API/Integration pair.

Pull request has been modified.

nija-at · 2020-07-02T11:14:45Z

I would also highly appreciate it if we could features piecemeal in smaller PRs than one massive PR. This makes it much easier on the reviewer and yourself to iterate and add small changes in.

Hey @julienlepine - As mentioned, can we break this up into reviewable chunks? Each chunk focused on adding a specific feature, starting from the most basic set of code needed to create a super simple Websocket endpoint, and then building up from there, one feature at a time.

nija-at

Moving to request changes. See #7097 (comment)

Pull request has been modified.

aws-cdk-automation · 2020-08-27T08:28:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: d6dcb02
Result: FAILED
Build Logs (available for 30 days)

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

adamdottv · 2020-08-27T17:17:06Z

@nija-at @julienlepine is this still being worked on? If not, I'd like to add an L2 for WebSocket APIs if all parties are 👌 with that!

nija-at · 2020-09-01T12:42:24Z

@adamelmore - feel free to contribute here.

We have limited amount of people and time to review external contributions, so keeping PRs small and easy to review will be highly appreciated. Unfortunately, large PRs are hard to review and unlikely to get completed.

I would begin by picking the simplest use case of building constructs that allow creating a very basic websocket endpoint with little configurability. That will set up the structure of the code, and subsequent PRs can make the feature set richer.

As an example, this is the approach we've been taking for HTTP APIs - https://github.com/aws/aws-cdk/commits/7b297749bbe5d75f29f1aeb2652d095e3f2630e1/packages/%40aws-cdk/aws-apigatewayv2/lib

nija-at · 2020-09-02T09:13:44Z

I'm closing this PR for the time being. Feel free to reopen when it's ready with the changes - #7097 (review)

It may also makes sense to open a separate PR when this is broken up into smaller, reviewable chunks.

julienlepine · 2020-09-02T11:48:38Z

I am not actively working on it, but I have a team member who will pick some of it up, you can use a big part of the code base, but it has to be split in multiple « bite-sized » PRs Julien

…

Le 2 sept. 2020 à 11:14, Niranjan Jayakar ***@***.***> a écrit : I'm closing this PR for the time being. Feel free to reopen when it's ready with the changes - #7097 (review) It may also makes sense to open a separate PR when this is broken up into smaller, reviewable chunks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

julienlepine added 5 commits March 24, 2020 21:26

Merge pull request #1 from aws/master

1acf23e

Merge

Merge pull request #2 from aws/master

cab59c8

Merge #2

feat(module): l2 construct for api gateway v2

615ebbb

Initial implementation of the API Gateway V2 Constructs. Implements all base APIGW constructs as L2 Constructs. fixes #5301 fixes #2872 fixes #212

Merge pull request #3 from aws/master

7b8aab8

Merge from remote master

Merge pull request #4 from julienlepine/master

45a5d6e

Merge from master

julienlepine closed this Mar 31, 2020

fix(aws-apigatewayv2): add missing api gateway fields for HTTP apis

338912e

julienlepine reopened this Mar 31, 2020

SomayaB assigned nija-at Mar 31, 2020

julienlepine added 3 commits April 1, 2020 13:30

Merge branch 'master' into julienlepine/apigwv2

eb1349c

Merge branch 'master' into julienlepine/apigwv2

84c43d2

Merge branch 'master' into julienlepine/apigwv2

c42c3b2

adamnfish reviewed Apr 2, 2020

View reviewed changes

julienlepine added 3 commits April 18, 2020 09:44

Merge pull request #5 from aws/master

fea1bac

Merge from master (20200418)

Merge branch 'julienlepine/apigwv2' into master

2dd1bcb

Merge pull request #6 from julienlepine/master

d4458ad

merge from master

nija-at reviewed Apr 22, 2020

View reviewed changes

julienlepine added 2 commits April 25, 2020 13:14

Merge remote-tracking branch 'upstream/master' into julienlepine/apigwv2

765f645

chore(aws-apigatewayv2): update to support new linter rules

fb59ea3

Support ESLint rules: - [comma-dangle]: Missing trailing comma - [quotes]: Strings must use singlequote'

julienlepine added 3 commits April 28, 2020 14:38

fix(aws-apigatewayv2): integrate HTTP APIs and WebSocket APIs

fd91919

Merge branch 'master' into julienlepine/apigwv2

b9eee5d

Merge branch 'julienlepine/apigwv2' of https://github.com/julienlepin…

bd6aec0

…e/aws-cdk into julienlepine/apigwv2

nija-at reviewed Apr 28, 2020

View reviewed changes

bdgmb2 mentioned this pull request Apr 30, 2020

feat(apigateway): create RestApi from an OpenAPI spec #7372

Merged

nija-at previously requested changes May 11, 2020

View reviewed changes

julienlepine added 5 commits June 29, 2020 14:13

Merge remote-tracking branch 'upstream/master' into julienlepine/apigwv2

2c7cfac

feat(aws-apigatewayv2): update to HTTP Api Model

3ec9a17

Merged the model to be compatible with the new Http Api Model

feat(aws-apigatewayv2): change open-ended enum parameters (known valu…

35f48b1

…es) to classes Update of the classes of API Gateway V2 to use classes instead of enums for open-ended value lists (known content type, routes, models, ...)

Merge remote-tracking branch 'upstream/master' into julienlepine/apigwv2

d27b12b

fix(aws-apigatewayv2): make rights granular per route in Lambda integ…

8b6f2bd

…rations Bind the IAM policies for Lambda functions ro the integration/route pair, instead of just to the API/Integration pair.

Merge branch 'master' into julienlepine/apigwv2

0a513d6

nija-at previously requested changes Jul 8, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into julienlepine/apigwv2

d6dcb02

nija-at closed this Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-apigatewayv2): l2 construct for api gateway v2 #7097

feat(aws-apigatewayv2): l2 construct for api gateway v2 #7097

julienlepine commented Mar 31, 2020

julienlepine commented Mar 31, 2020

adamnfish left a comment

adamnfish Apr 2, 2020

adamnfish Apr 2, 2020

ranman commented Apr 9, 2020

nija-at Apr 22, 2020

nija-at Apr 22, 2020

ranman Apr 23, 2020

julienlepine Apr 25, 2020

nija-at commented Apr 28, 2020

julienlepine commented Apr 28, 2020 via email

julienlepine commented Apr 28, 2020

nija-at left a comment

nija-at Apr 28, 2020

nija-at Apr 28, 2020

nija-at Apr 28, 2020

nija-at Apr 28, 2020

nija-at Apr 28, 2020

nija-at left a comment

nija-at commented Jul 2, 2020

nija-at left a comment

aws-cdk-automation commented Aug 27, 2020

adamdottv commented Aug 27, 2020

nija-at commented Sep 1, 2020

nija-at commented Sep 2, 2020

julienlepine commented Sep 2, 2020 via email

feat(aws-apigatewayv2): l2 construct for api gateway v2 #7097

feat(aws-apigatewayv2): l2 construct for api gateway v2 #7097

Conversation

julienlepine commented Mar 31, 2020

julienlepine commented Mar 31, 2020

adamnfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranman commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at commented Apr 28, 2020

julienlepine commented Apr 28, 2020 via email

julienlepine commented Apr 28, 2020

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

nija-at commented Jul 2, 2020

nija-at left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 27, 2020

AWS CodeBuild CI Report

adamdottv commented Aug 27, 2020

nija-at commented Sep 1, 2020

nija-at commented Sep 2, 2020

julienlepine commented Sep 2, 2020 via email