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

fix(apigatewayv2): integration class does not render an integration resource #17729

Merged
merged 3 commits into from
Nov 26, 2021

Conversation

nija-at
Copy link
Contributor

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

Routes on APIGateway V2 can integrate with different backends. This is
achieved by creating the CFN resource AWS::ApiGatewayV2::Integration
that is then referenced in the resource for the Route.

Currently, the IHttpRouteIntegration (and IWebSocketRouteIntegration)
interface represents a unique backend that a route can integrate with,
using the CDK "bind" pattern.
An integration can be bound to any number of routes but should be
rendered into a single instance of AWS::ApiGatewayV2::Integration
resource.
To achieve this currently, the HttpApi (and WebSocketApi) class
holds a cache of all integrations defined against its routes.

This is the wrong level of caching and causes a number of problems.

  1. We rely on the configuration of the AWS::ApiGateway::Integration
    resource to determine if one already exists.
    This means that two instances of IHttpRouteIntegration can result
    in rendering only one instance of AWS::ApiGateway::Integration
    resource.

    Users may want to intentionally generate multiple instances of
    AWS::ApiGateway::Integration classes with the same configuration.
    Taking away this power with CDK "magic" is just confusing.

  2. Currently, we allow using the same instance of
    IHttpRouteIntegration (or IWebSocketRouteIntegration) to be bound
    to routes in different HttpApi. When bound to the route, the CDK
    renders an instance of AWS::ApiGatewayV2::Integration for each API.

    This is another "magic" that has the potential for user confusion and
    bugs.

The solution is to KeepItSimple™.

Remove the API level caching and simply cache at the level of each
integration. This ensures that each instance of HttpRouteIntegration
(previously IHttpRouteIntegration) renders to exactly one instance of
AWS::ApiGatewayV2::Integration.

Disallow using the same instance of HttpRouteIntegration across
different instances of HttpApi.

fixes #13213

BREAKING CHANGE: The interface IHttpRouteIntegration is replaced by
the abstract class HttpRouteIntegration.

  • apigatewayv2: The interface IWebSocketRouteIntegration is now
    replaced by the abstract class WebSocketRouteIntegration.
  • apigatewayv2: Previously, we allowed the usage of integration
    classes to be used with routes defined in multiple HttpApi instances
    (or WebSocketApi instances). This is now disallowed, and separate
    instances must be created for each instance of HttpApi or
    WebSocketApi.

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

…esource

Routes on APIGateway V2 can integrate with different backends. This is
achieved by creating the CFN resource `AWS::ApiGatewayV2::Integration`
that is then referenced in the resource for the Route.

Currently, the `IHttpRouteIntegration` (and `IWebSocketRouteIntegration`)
interface represents a unique backend that a route can integrate with,
using the CDK "bind" pattern.
An integration can be bound to any number of routes but should be
rendered into a single instance of `AWS::ApiGatewayV2::Integration`
resource.
To achieve this currently, the `HttpApi` (and `WebSocketApi`) class
holds a cache of all integrations defined against its routes.

This is the wrong level of caching and causes a number of problems.

1. We rely on the configuration of the `AWS::ApiGateway::Integration`
   resource to determine if one already exists.
   This means that two instances of `IHttpRouteIntegration` can result
   in rendering only one instance of `AWS::ApiGateway::Integration`
   resource.

   Users may want to intentionally generate multiple instances of
   `AWS::ApiGateway::Integration` classes with the same configuration.
   Taking away this power with CDK "magic" is just confusing.

2. Currently, we allow using the same instance of
   `IHttpRouteIntegration` (or `IWebSocketRouteIntegration`) to be bound
   to routes in different `HttpApi`. When bound to the route, the CDK
   renders an instance of `AWS::ApiGatewayV2::Integration` for each API.

   This is another "magic" that has the potential for user confusion and
   bugs.

The solution is to KeepItSimple™.

Remove the API level caching and simply cache at the level of each
integration. This ensures that each instance of `HttpRouteIntegration`
(previously `IHttpRouteIntegration`) renders to exactly one instance of
`AWS::ApiGatewayV2::Integration`.

Disallow using the same instance of `HttpRouteIntegration` across
different instances of `HttpApi`.

fixes #13213

BREAKING CHANGE: The interface `IHttpRouteIntegration` is replaced by
the abstract class `HttpRouteIntegration`.
* **apigatewayv2:** The interface `IWebSocketRouteIntegration` is now
  replaced by the abstract class `WebSocketRouteIntegration`.
* **apigatewayv2:** Previously, we allowed the usage of integration
  classes to be used with routes defined in multiple `HttpApi` instances
  (or `WebSocketApi` instances). This is now disallowed, and separate
  instances must be created for each instance of `HttpApi` or
  `WebSocketApi`.
@nija-at nija-at requested review from otaviomacedo and a team November 26, 2021 10:28
@nija-at nija-at self-assigned this Nov 26, 2021
@gitpod-io
Copy link

gitpod-io bot commented Nov 26, 2021

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

nija-at commented Nov 26, 2021

cc @ayush987goyal

});

function hash(x: any) {
const stringifiedConfig = JSON.stringify(Stack.of(options.scope).resolve(x));
Copy link
Contributor

@otaviomacedo otaviomacedo Nov 26, 2021

Choose a reason for hiding this comment

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

I don't know if it can happen with Stacks (maybe resolve() already takes care of that), but if the order of the fields change or if the presence or absence of whitespaces change, the hash will be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace is not a problem here. Ordering maybe. I'll check.

Copy link
Contributor Author

@nija-at nija-at Nov 26, 2021

Choose a reason for hiding this comment

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

Ordering is actually a bigger problem here than I thought.

This PR is effectively not changing the logic of hashing. Just moving it to a different place (previously integration-cache.ts).

Do you mind if I take this on as a follow up PR? Easier to review as well.
I have a different solution in mind that effectively removes the need to hash.

@ayush987goyal
Copy link
Contributor

Previously, we allowed the usage of integration
classes to be used with routes defined in multiple HttpApi instances
(or WebSocketApi instances). This is now disallowed, and separate
instances must be created for each instance of HttpApi or
WebSocketApi.

@nija-at If I am remember correctly, this was an explicit request from customers. I think this is something that the API Gateway console allows to do. That is, a single integration can be bound to multiple APIs and is typically created out of the scope of an API. Do you think this change might create a problem with that and/or customer might ask for it again?

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

nija-at commented Nov 26, 2021

I think this is something that the API Gateway console allows to do. That is, a single integration can be bound to multiple APIs and is typically created out of the scope of an API

I don't know how that is possible since the AWS::ApiGatewayV2::Integration resource has ApiId as one of its properties.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigatewayv2-integration.html#cfn-apigatewayv2-integration-apiid

@ayush987goyal
Copy link
Contributor

I don't know how that is possible since the AWS::ApiGatewayV2::Integration resource has ApiId as one of its properties.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigatewayv2-integration.html#cfn-apigatewayv2-integration-apiid

Yeah that's true. Maybe I am remembering something else. The change LGTM!

@nija-at nija-at removed the pr/do-not-merge This PR should not be merged at this time. label Nov 26, 2021
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: f0288a6
  • 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 3b5b97a into master Nov 26, 2021
@mergify mergify bot deleted the nija-at/apigwv2-integrations branch November 26, 2021 14:10
@mergify
Copy link
Contributor

mergify bot commented Nov 26, 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).

beezly pushed a commit to beezly/aws-cdk that referenced this pull request Nov 29, 2021
…esource (aws#17729)

Routes on APIGateway V2 can integrate with different backends. This is
achieved by creating the CFN resource `AWS::ApiGatewayV2::Integration`
that is then referenced in the resource for the Route.

Currently, the `IHttpRouteIntegration` (and `IWebSocketRouteIntegration`)
interface represents a unique backend that a route can integrate with,
using the CDK "bind" pattern.
An integration can be bound to any number of routes but should be
rendered into a single instance of `AWS::ApiGatewayV2::Integration`
resource.
To achieve this currently, the `HttpApi` (and `WebSocketApi`) class
holds a cache of all integrations defined against its routes.

This is the wrong level of caching and causes a number of problems.

1. We rely on the configuration of the `AWS::ApiGateway::Integration`
   resource to determine if one already exists.
   This means that two instances of `IHttpRouteIntegration` can result
   in rendering only one instance of `AWS::ApiGateway::Integration`
   resource.

   Users may want to intentionally generate multiple instances of
   `AWS::ApiGateway::Integration` classes with the same configuration.
   Taking away this power with CDK "magic" is just confusing.

2. Currently, we allow using the same instance of
   `IHttpRouteIntegration` (or `IWebSocketRouteIntegration`) to be bound
   to routes in different `HttpApi`. When bound to the route, the CDK
   renders an instance of `AWS::ApiGatewayV2::Integration` for each API.

   This is another "magic" that has the potential for user confusion and
   bugs.

The solution is to KeepItSimple™.

Remove the API level caching and simply cache at the level of each
integration. This ensures that each instance of `HttpRouteIntegration`
(previously `IHttpRouteIntegration`) renders to exactly one instance of
`AWS::ApiGatewayV2::Integration`.

Disallow using the same instance of `HttpRouteIntegration` across
different instances of `HttpApi`.

fixes aws#13213

BREAKING CHANGE: The interface `IHttpRouteIntegration` is replaced by
the abstract class `HttpRouteIntegration`.
* **apigatewayv2:** The interface `IWebSocketRouteIntegration` is now
  replaced by the abstract class `WebSocketRouteIntegration`.
* **apigatewayv2:** Previously, we allowed the usage of integration
  classes to be used with routes defined in multiple `HttpApi` instances
  (or `WebSocketApi` instances). This is now disallowed, and separate
  instances must be created for each instance of `HttpApi` or
  `WebSocketApi`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
pedrosola pushed a commit to pedrosola/aws-cdk that referenced this pull request Dec 1, 2021
…esource (aws#17729)

Routes on APIGateway V2 can integrate with different backends. This is
achieved by creating the CFN resource `AWS::ApiGatewayV2::Integration`
that is then referenced in the resource for the Route.

Currently, the `IHttpRouteIntegration` (and `IWebSocketRouteIntegration`)
interface represents a unique backend that a route can integrate with,
using the CDK "bind" pattern.
An integration can be bound to any number of routes but should be
rendered into a single instance of `AWS::ApiGatewayV2::Integration`
resource.
To achieve this currently, the `HttpApi` (and `WebSocketApi`) class
holds a cache of all integrations defined against its routes.

This is the wrong level of caching and causes a number of problems.

1. We rely on the configuration of the `AWS::ApiGateway::Integration`
   resource to determine if one already exists.
   This means that two instances of `IHttpRouteIntegration` can result
   in rendering only one instance of `AWS::ApiGateway::Integration`
   resource.

   Users may want to intentionally generate multiple instances of
   `AWS::ApiGateway::Integration` classes with the same configuration.
   Taking away this power with CDK "magic" is just confusing.

2. Currently, we allow using the same instance of
   `IHttpRouteIntegration` (or `IWebSocketRouteIntegration`) to be bound
   to routes in different `HttpApi`. When bound to the route, the CDK
   renders an instance of `AWS::ApiGatewayV2::Integration` for each API.

   This is another "magic" that has the potential for user confusion and
   bugs.

The solution is to KeepItSimple™.

Remove the API level caching and simply cache at the level of each
integration. This ensures that each instance of `HttpRouteIntegration`
(previously `IHttpRouteIntegration`) renders to exactly one instance of
`AWS::ApiGatewayV2::Integration`.

Disallow using the same instance of `HttpRouteIntegration` across
different instances of `HttpApi`.

fixes aws#13213

BREAKING CHANGE: The interface `IHttpRouteIntegration` is replaced by
the abstract class `HttpRouteIntegration`.
* **apigatewayv2:** The interface `IWebSocketRouteIntegration` is now
  replaced by the abstract class `WebSocketRouteIntegration`.
* **apigatewayv2:** Previously, we allowed the usage of integration
  classes to be used with routes defined in multiple `HttpApi` instances
  (or `WebSocketApi` instances). This is now disallowed, and separate
  instances must be created for each instance of `HttpApi` or
  `WebSocketApi`.

----

*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
…esource (aws#17729)

Routes on APIGateway V2 can integrate with different backends. This is
achieved by creating the CFN resource `AWS::ApiGatewayV2::Integration`
that is then referenced in the resource for the Route.

Currently, the `IHttpRouteIntegration` (and `IWebSocketRouteIntegration`)
interface represents a unique backend that a route can integrate with,
using the CDK "bind" pattern.
An integration can be bound to any number of routes but should be
rendered into a single instance of `AWS::ApiGatewayV2::Integration`
resource.
To achieve this currently, the `HttpApi` (and `WebSocketApi`) class
holds a cache of all integrations defined against its routes.

This is the wrong level of caching and causes a number of problems.

1. We rely on the configuration of the `AWS::ApiGateway::Integration`
   resource to determine if one already exists.
   This means that two instances of `IHttpRouteIntegration` can result
   in rendering only one instance of `AWS::ApiGateway::Integration`
   resource.

   Users may want to intentionally generate multiple instances of
   `AWS::ApiGateway::Integration` classes with the same configuration.
   Taking away this power with CDK "magic" is just confusing.

2. Currently, we allow using the same instance of
   `IHttpRouteIntegration` (or `IWebSocketRouteIntegration`) to be bound
   to routes in different `HttpApi`. When bound to the route, the CDK
   renders an instance of `AWS::ApiGatewayV2::Integration` for each API.

   This is another "magic" that has the potential for user confusion and
   bugs.

The solution is to KeepItSimple™.

Remove the API level caching and simply cache at the level of each
integration. This ensures that each instance of `HttpRouteIntegration`
(previously `IHttpRouteIntegration`) renders to exactly one instance of
`AWS::ApiGatewayV2::Integration`.

Disallow using the same instance of `HttpRouteIntegration` across
different instances of `HttpApi`.

fixes aws#13213

BREAKING CHANGE: The interface `IHttpRouteIntegration` is replaced by
the abstract class `HttpRouteIntegration`.
* **apigatewayv2:** The interface `IWebSocketRouteIntegration` is now
  replaced by the abstract class `WebSocketRouteIntegration`.
* **apigatewayv2:** Previously, we allowed the usage of integration
  classes to be used with routes defined in multiple `HttpApi` instances
  (or `WebSocketApi` instances). This is now disallowed, and separate
  instances must be created for each instance of `HttpApi` or
  `WebSocketApi`.

----

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

(apigatewayv2): AddRoutes caches lambda integrations incorrectly
4 participants