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

(apigatewayv2): AddRoutes caches lambda integrations incorrectly #13213

Closed
ShadowDog007 opened this issue Feb 23, 2021 · 11 comments · Fixed by #17729
Closed

(apigatewayv2): AddRoutes caches lambda integrations incorrectly #13213

ShadowDog007 opened this issue Feb 23, 2021 · 11 comments · Fixed by #17729
Labels
@aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@ShadowDog007
Copy link

When adding multiple routes with lambda integrations on a single http api, all routes end up sharing the same integration, instead of being correctly bound to the lambda which was provided.

Reproduction Steps

httpApi.AddRoutes({
  path: '/service1',
  integration: new LambdaProxyIntegration({
    handler: lambda1,
  })
});
httpApi.AddRoutes({
  path: '/service2',
  integration: new LambdaProxyIntegration({
    handler: lambda2,
  })
});

What did you expect to happen?

Two routes with their own integrations created

What actually happened?

Correct routes/lambda permissions, but both routes share the same lambda integration (the first one which was added)

Environment

  • CDK CLI Version : 1.90.1
  • Framework Version: 1.90.1
  • Node.js Version: 14.7.0
  • OS : Windows
  • Language (Version): dotnetcore (3.1)

Other

It appears that the Lambda arn doesn't become part of the cache key?
https://github.com/aws/aws-cdk/blob/v1.90.1/packages/@aws-cdk/aws-apigatewayv2/lib/http/api.ts#L281


This is 🐛 Bug Report

@ShadowDog007 ShadowDog007 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 23, 2021
@github-actions github-actions bot added the @aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 label Feb 23, 2021
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Feb 23, 2021
@nija-at nija-at added p1 effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. and removed @aws-cdk/aws-lambda Related to AWS Lambda needs-triage This issue or PR still needs to be triaged. effort/small Small work item – less than a day of effort p1 labels Feb 25, 2021
@nija-at
Copy link
Contributor

nija-at commented Feb 25, 2021

@ayush987goyal - can you take a look?

@ayush987goyal
Copy link
Contributor

ayush987goyal commented Feb 25, 2021

Hi @ShadowDog007 ,

I am not able to reproduce the above issue with the below code:

import * as cdk from '@aws-cdk/core';
import { HttpApi } from '@aws-cdk/aws-apigatewayv2';
import * as integrations from '@aws-cdk/aws-apigatewayv2-integrations';
import * as lambda from '@aws-cdk/aws-lambda';

export class ApiIntegStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const lambda1 = new lambda.Function(this, 'AlwaysSuccess1', {
      runtime: lambda.Runtime.NODEJS_12_X,
      handler: 'index.handler',
      code: new lambda.InlineCode(
        'exports.handler = async function(event, context) { return { statusCode: 200, body: "success1" }; };'
      ),
    });
    const lambda2 = new lambda.Function(this, 'AlwaysSuccess2', {
      runtime: lambda.Runtime.NODEJS_12_X,
      handler: 'index.handler',
      code: new lambda.InlineCode(
        'exports.handler = async function(event, context) { return { statusCode: 200, body: "success2" }; };'
      ),
    });

    const httpApi = new HttpApi(this, 'LambdaProxyApi');
    httpApi.addRoutes({
      path: '/service1',
      integration: new integrations.LambdaProxyIntegration({
        handler: lambda1,
      }),
    });
    httpApi.addRoutes({
      path: '/service2',
      integration: new integrations.LambdaProxyIntegration({
        handler: lambda2,
      }),
    });

    new cdk.CfnOutput(this, 'Endpoint', {
      value: httpApi.url!,
    });
  }
}

Testing:

❯ curl https://<some-id>.execute-api.us-east-1.amazonaws.com/service1
success1%                                                                                                                                             
❯ curl https://<some-id>.execute-api.us-east-1.amazonaws.com/service2
success2%     

Could you please check if you are using the latest version of AWS CDK and also try out the above code? It would help if you could provide more details on you implementation to dive deep further.

@nija-at nija-at added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Feb 25, 2021
@ShadowDog007
Copy link
Author

Hey @ayush987goyal I did some testing and struggled for a bit to reproduce it in a brand new application.

The specific reason why it was happening in my application was because the id of my Function was the same in two stacks.

The id of my functions were both 'Function', but in different stacks
So the cache key doesn't consider the stack id??

class CdkAg2TestStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: cdk.StackProps, lambdaStack1: LambdaStack, lambdaStack2: LambdaStack) {
    super(scope, id, props);

    const api = new apigateway.HttpApi(this, 'api');

    api.addRoutes({
      path: '/lambda1',
      integration: new apigatewayIntegrations.LambdaProxyIntegration({
        handler: lambdaStack1.lambda,
        payloadFormatVersion: apigateway.PayloadFormatVersion.VERSION_2_0
      })
    });

    api.addRoutes({
      path: '/lambda2',
      integration: new apigatewayIntegrations.LambdaProxyIntegration({
        handler: lambdaStack2.lambda,
        payloadFormatVersion: apigateway.PayloadFormatVersion.VERSION_2_0
      })
    });
  }
}

class LambdaStack extends cdk.Stack {
  lambda: lambda.Function;
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);
    this.lambda = new lambda.Function(this, 'lambda', {
      runtime: lambda.Runtime.NODEJS_12_X,
      handler: 'index.handler',
      code: lambda.Code.fromInline('exports.handler = async function(event) { };'),
    });
  }
}

const app = new cdk.App();
const lambdaStack1 = new LambdaStack(app, 'LambdaStack1');
const lambdaStack2 = new LambdaStack(app, 'LambdaStack2');
new CdkAg2TestStack(app, 'CdkAg2TestStack', {}, lambdaStack1, lambdaStack2);

@ayush987goyal
Copy link
Contributor

It's mainly driven by the functionArn. So if the ARN for the lambda is same, then it will treat it as same integration.

@ShadowDog007
Copy link
Author

ShadowDog007 commented Feb 25, 2021

Right, but the ARN is based off the function name, but I'm letting CloudFormation generate one (which bases it off the stack name and resource name). So the ARN's are different? (Just not known at synth time)

@ayush987goyal
Copy link
Contributor

Yeah ideally they should be different if they are in different stacks. I am not really sure what specific scenario you have in your local workspace. If you could provide a reproduction then we could further look into it.

@ShadowDog007
Copy link
Author

ShadowDog007 commented Feb 25, 2021

@ayush987goyal
Copy link
Contributor

I have reproduced it locally and can confirm this is indeed a bug. We will look into the same.

@nija-at
Copy link
Contributor

nija-at commented Feb 25, 2021

To confirm, this happens when the function name is the same but defined in different stacks. Is that right?

@nija-at nija-at added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Feb 25, 2021
@ayush987goyal
Copy link
Contributor

Yes just confirmed that this only happens when the function name is same and defined in different stacks.

@nija-at nija-at changed the title (aws-apigatewayv2): AddRoutes caches lambda integrations incorrectly (apigatewayv2): AddRoutes caches lambda integrations incorrectly Mar 3, 2021
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Mar 3, 2021
@nija-at nija-at removed the @aws-cdk/aws-lambda Related to AWS Lambda label Nov 24, 2021
nija-at pushed a commit that referenced this issue Nov 26, 2021
…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`.
@mergify mergify bot closed this as completed in #17729 Nov 26, 2021
mergify bot pushed a commit that referenced this issue Nov 26, 2021
…esource (#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 #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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

beezly pushed a commit to beezly/aws-cdk that referenced this issue 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 issue 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 issue 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 bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants