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

chore(apigatewayv2): integration api re-organization #17752

Merged
merged 7 commits into from
Nov 29, 2021

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Nov 29, 2021

There are three major changes.

HttpRouteIntegration (and its sibling WebSocketRouteIntegration)
creates a CDK construct (HttpIntegration and WebSocketIntegration)
as part of its bind operation. The id to this CDK construct is
determined by hashing the results of the bind.
Using hashes makes the construct id fragile/sensitive, consequently the
CFN resource's logical id fragile.
The fragility comes mainly from the question - have we hashed all of the
expected properties that should be hashed, and nothing extra?
If we have not hashed properties that should be there, or hashed too
much, we end up with a hash change, hence resource replacement that
is unexpected.

This commit changes this approach and asks the user to provide the
construct's id. This is more aligned with the current CDK expectation
that users provide an id when initializing constructs.
We just don't have a good way to validate that our hashing is accurate,
so let's not do it at all. This change makes the user provide a unique
name within a scope, which is already a standard requirement for CDK
constructs.

Secondly, the ergonomics of specific integration implementations, such
as, LambdaProxyIntegration, HttpAlbIntegration, etc. is modified so
that the integrating primitive is moved out of the 'props', and to the
constructor.
The API ergonomics of this feels much better than having to always
provide a 'props'.

Since this package contains constructs around both http api and
websocket api, the convention to follow is that all classes specific to
the former will be prefixed with Http and the latter will be prefixed
with WebSocket.
Bring the integration classes LambdaProxyIntegration and
HttpProxyIntegration in line with this convention. These are renamed
to HttpLambdaIntegration and HttpUrlIntegration respectively.

BREAKING CHANGE: The HttpIntegration and WebSocketIntegration
classes require an "id" parameter to be provided during its initialization.

  • apigatewayv2-integrations: The LambdaWebSocketIntegration is now
    renamed to WebSocketLambdaIntegration. The new class accepts the
    handler to the target lambda function directly in its constructor.
  • apigatewayv2-integrations: HttpProxyIntegration and
    HttpProxyIntegrationProps are now renamed to HttpUrlIntegration
    and HttpUrlIntegrationProps respectively. The new class accepts the
    target url directly in its constructor.
  • apigatewayv2-integrations: LambdaProxyIntegration and
    LambdaProxyIntegrationProps are now renamed to
    HttpLambdaIntegration and HttpLambdaIntegrationProps respectively.
    The new class accepts the lambda function handler directly in its
    constructor.
  • apigatewayv2-integrations: HttpAlbIntegration now accepts the
    ELB listener directly in its constructor.
  • apigatewayv2-integrations: HttpNlbIntegration now accepts the
    ELB listener directly in its constructor.
  • apigatewayv2-integrations: HttpServiceDiscoveryIntegration now
    accepts the service discovery Service directly in its constructor.
  • apigatewayv2-authorizers: UserPoolAuthorizerProps is now
    renamed to HttpUserPoolAuthorizerProps.

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

There are three major changes.

`HttpRouteIntegration` (and its sibling `WebSocketRouteIntegration`)
creates a CDK construct (`HttpIntegration` and `WebSocketIntegration`)
as part of its bind operation. The id to this CDK construct is
determined by hashing the results of the bind.
Using hashes makes the construct id fragile, consequently the CFN
resource's logical id fragile.

This commit changes this approach and asks the user to provide the
construct's id. This is more aligned with the current CDK expectation
that users provide an id when initializing constructs.

Secondly, the ergonomics of specific integration implementations, such
as, `LambdaProxyIntegration`, `HttpAlbIntegration`, etc. is modified so
that the integrating primitive is moved out of the 'props', and to the
constructor.
The API ergonomics of this feels much better than having to always
provide a 'props'.

Since this package contains constructs around both http api and
websocket api, the convention to follow is that all classes specific to
the former will be prefixed with `Http` and the latter will be prefixed
with `WebSocket`.
Bring the integration classes `LambdaProxyIntegration` and
`HttpProxyIntegration` in line with this convention. These are renamed
to `HttpLambdaIntegration` and `HttpUrlIntegration` respectively.

BREAKING CHANGE: The `HttpIntegration` and `WebSocketIntegration`
classes require an "id" parameter passed in during its initialization.
* **apigatewayv2-integrations:** The `LambdaWebSocketIntegration` is now
  renamed to `WebSocketLambdaIntegration`. The new class accepts the
  handler to the target lambda function directly in its constructor.
* **apigatewayv2-integrations:** `HttpProxyIntegration` and
  `HttpProxyIntegrationProps` are now renamed to `HttpUrlIntegration`
  and `HttpUrlIntegrationProps` respectively. The new class accepts the
  target url directly in its constructor.
* **apigatewayv2-integrations:** `LambdaProxyIntegration` and
  `LambdaProxyIntegrationProps` are now renamed to
  `HttpLambdaIntegration` and `HttpLambdaIntegrationProps` respectively.
  The new class accepts the lambda function handler directly in its
  constructor.
* **apigatewayv2-integrations:** `HttpAlbIntegration` now accepts the
  ELB listener directly in its constructor.
* **apigatewayv2-integrations:** `HttpNlbIntegration` now accepts the
  ELB listener directly in its constructor.
* **apigatewayv2-integrations:** `HttpServiceDiscoveryIntegration` now
  accepts the service discovery Service directly in its constructor.
@nija-at nija-at requested review from otaviomacedo and a team November 29, 2021 14:39
@gitpod-io
Copy link

gitpod-io bot commented Nov 29, 2021

@github-actions github-actions bot added the @aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 label Nov 29, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 29, 2021
@nija-at
Copy link
Contributor Author

nija-at commented Nov 29, 2021

cc @ayush987goyal

@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Nov 29, 2021
Copy link
Contributor

@ayush987goyal ayush987goyal left a comment

Choose a reason for hiding this comment

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

This change is so amazing! Really liking the new simpler and ergonomic API approach. It feels more natural in context of how other constructs and integrations work! Thanks a lot for cleaning up the old hashing based approach which was hard to maintain (as we saw during past couple of refactors!). All in all the changes LGTM! A couple of small comments.

Do you think it makes sense to still have the word Proxy in the integrations where API Gateway's documentation uses it? Like for the HTTP Lambda integration, the docs uses proxy everywhere (ref). Similarly for HTTP Proxy (ref)

@nija-at
Copy link
Contributor Author

nija-at commented Nov 29, 2021

@ayush987goyal - thanks for your comments.

Like for the HTTP Lambda integration, the docs uses proxy everywhere (ref). Similarly for HTTP Proxy (ref)

The links above point to REST API (v1) and not HTTP API (v2).
In the v2 docs, this is still present but its not stressed as much.

Nevertheless, I don't see the value of keeping the word "proxy" in the name for either of these cases. I don't see any other kind. Maybe I'm missing something?

@ayush987goyal
Copy link
Contributor

Nevertheless, I don't see the value of keeping the word "proxy" in the name for either of these cases. I don't see any other kind. Maybe I'm missing something?

The only value that I was seeing was to have a parity with the docs. Not sure if that is even a concern to be honest or something we try to ensure in L2/3 constructs. We might anyway be pointing to the docs in the README and that should suffice.

Niranjan Jayakar and others added 2 commits November 29, 2021 16:19
Co-authored-by: Ayush Goyal <ayush987goyal@gmail.com>
@nija-at nija-at removed the pr/do-not-merge This PR should not be merged at this time. label Nov 29, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2021

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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 2b45903
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 29039e8 into master Nov 29, 2021
@mergify mergify bot deleted the nija-at/apigw-integration-hash branch November 29, 2021 17:44
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2021

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

pedrosola pushed a commit to pedrosola/aws-cdk that referenced this pull request Dec 1, 2021
There are three major changes.

`HttpRouteIntegration` (and its sibling `WebSocketRouteIntegration`)
creates a CDK construct (`HttpIntegration` and `WebSocketIntegration`)
as part of its bind operation. The id to this CDK construct is
determined by hashing the results of the bind.
Using hashes makes the construct id fragile/sensitive, consequently the
CFN resource's logical id fragile.
The fragility comes mainly from the question - have we hashed all of the
expected properties that should be hashed, and nothing extra?
If we have not hashed properties that should be there, or hashed too
much, we end up with a hash change, hence resource replacement that
is unexpected.

This commit changes this approach and asks the user to provide the
construct's id. This is more aligned with the current CDK expectation
that users provide an id when initializing constructs.
We just don't have a good way to validate that our hashing is accurate,
so let's not do it at all. This change makes the user provide a unique
name within a scope, which is already a standard requirement for CDK
constructs.

Secondly, the ergonomics of specific integration implementations, such
as, `LambdaProxyIntegration`, `HttpAlbIntegration`, etc. is modified so
that the integrating primitive is moved out of the 'props', and to the
constructor.
The API ergonomics of this feels much better than having to always
provide a 'props'.

Since this package contains constructs around both http api and
websocket api, the convention to follow is that all classes specific to
the former will be prefixed with `Http` and the latter will be prefixed
with `WebSocket`.
Bring the integration classes `LambdaProxyIntegration` and
`HttpProxyIntegration` in line with this convention. These are renamed
to `HttpLambdaIntegration` and `HttpUrlIntegration` respectively.

BREAKING CHANGE: The `HttpIntegration` and `WebSocketIntegration`
classes require an "id" parameter to be provided during its initialization.
* **apigatewayv2-integrations:** The `LambdaWebSocketIntegration` is now
  renamed to `WebSocketLambdaIntegration`. The new class accepts the
  handler to the target lambda function directly in its constructor.
* **apigatewayv2-integrations:** `HttpProxyIntegration` and
  `HttpProxyIntegrationProps` are now renamed to `HttpUrlIntegration`
  and `HttpUrlIntegrationProps` respectively. The new class accepts the
  target url directly in its constructor.
* **apigatewayv2-integrations:** `LambdaProxyIntegration` and
  `LambdaProxyIntegrationProps` are now renamed to
  `HttpLambdaIntegration` and `HttpLambdaIntegrationProps` respectively.
  The new class accepts the lambda function handler directly in its
  constructor.
* **apigatewayv2-integrations:** `HttpAlbIntegration` now accepts the
  ELB listener directly in its constructor.
* **apigatewayv2-integrations:** `HttpNlbIntegration` now accepts the
  ELB listener directly in its constructor.
* **apigatewayv2-integrations:** `HttpServiceDiscoveryIntegration` now
  accepts the service discovery Service directly in its constructor.
* **apigatewayv2-authorizers:** `UserPoolAuthorizerProps` is now
  renamed to `HttpUserPoolAuthorizerProps`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
There are three major changes.

`HttpRouteIntegration` (and its sibling `WebSocketRouteIntegration`)
creates a CDK construct (`HttpIntegration` and `WebSocketIntegration`)
as part of its bind operation. The id to this CDK construct is
determined by hashing the results of the bind.
Using hashes makes the construct id fragile/sensitive, consequently the
CFN resource's logical id fragile.
The fragility comes mainly from the question - have we hashed all of the
expected properties that should be hashed, and nothing extra?
If we have not hashed properties that should be there, or hashed too
much, we end up with a hash change, hence resource replacement that
is unexpected.

This commit changes this approach and asks the user to provide the
construct's id. This is more aligned with the current CDK expectation
that users provide an id when initializing constructs.
We just don't have a good way to validate that our hashing is accurate,
so let's not do it at all. This change makes the user provide a unique
name within a scope, which is already a standard requirement for CDK
constructs.

Secondly, the ergonomics of specific integration implementations, such
as, `LambdaProxyIntegration`, `HttpAlbIntegration`, etc. is modified so
that the integrating primitive is moved out of the 'props', and to the
constructor.
The API ergonomics of this feels much better than having to always
provide a 'props'.

Since this package contains constructs around both http api and
websocket api, the convention to follow is that all classes specific to
the former will be prefixed with `Http` and the latter will be prefixed
with `WebSocket`.
Bring the integration classes `LambdaProxyIntegration` and
`HttpProxyIntegration` in line with this convention. These are renamed
to `HttpLambdaIntegration` and `HttpUrlIntegration` respectively.

BREAKING CHANGE: The `HttpIntegration` and `WebSocketIntegration`
classes require an "id" parameter to be provided during its initialization.
* **apigatewayv2-integrations:** The `LambdaWebSocketIntegration` is now
  renamed to `WebSocketLambdaIntegration`. The new class accepts the
  handler to the target lambda function directly in its constructor.
* **apigatewayv2-integrations:** `HttpProxyIntegration` and
  `HttpProxyIntegrationProps` are now renamed to `HttpUrlIntegration`
  and `HttpUrlIntegrationProps` respectively. The new class accepts the
  target url directly in its constructor.
* **apigatewayv2-integrations:** `LambdaProxyIntegration` and
  `LambdaProxyIntegrationProps` are now renamed to
  `HttpLambdaIntegration` and `HttpLambdaIntegrationProps` respectively.
  The new class accepts the lambda function handler directly in its
  constructor.
* **apigatewayv2-integrations:** `HttpAlbIntegration` now accepts the
  ELB listener directly in its constructor.
* **apigatewayv2-integrations:** `HttpNlbIntegration` now accepts the
  ELB listener directly in its constructor.
* **apigatewayv2-integrations:** `HttpServiceDiscoveryIntegration` now
  accepts the service discovery Service directly in its constructor.
* **apigatewayv2-authorizers:** `UserPoolAuthorizerProps` is now
  renamed to `HttpUserPoolAuthorizerProps`.

----

*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
@aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants