aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.71k stars 3.94k forks source link

[APIGateway] LambdaIntegration: Add option to create a single trigger/permission with wildcards only instead of one for each ApiGateway Resource #9327

Open phrastnik opened 4 years ago

phrastnik commented 4 years ago

For the Lambda ApiGateway integration, add an option to prefer a single wildcard trigger/integrationPermission instead of multiple triggers/integrationPermissions for each URL/endpoint/resource defined in the ApiGateway.

Currently the created triggers in the AWS console looks like that:

arn:aws:execute-api:us-east-1:123:api_id/prod/GET/v1/parent_res_1/*
arn:aws:execute-api:us-east-1:123:api_id/prod/POST/v1/parent_res_1/*
arn:aws:execute-api:us-east-1:123:api_id/prod/GET/v1/parent_res_2/*
...

The requested feature would allow to have something like that instead:

arn:aws:execute-api:us-east-1:123:api_id/*

Use Case

In case of APIs with a larger amount of urls/endpoints/resources, it is likely to get a "The final policy size (XXX) is bigger than the limit (20480)" error.

In our case, we run into that for an API with around 15 resources and worked around temporarily by setting LambdaIntegrationOptions.allowTestInvoke to false. This cut the number of triggers/IntegrationPermissions in half and the policy didn't hit the limit anymore. However, we would prefer leaving allowTestInvoke to true. Moreover as the API grows over time, we will likely run into the same issue again later: the faster the API grows, the sooner. Implementing something like described in https://github.com/aws/aws-cdk/issues/5774#issuecomment-609583801 (also see below) currently seems to be something like a last resort for us.

Implication of the current state of CDK in this respect for us is that the CDK ApiGateway -> LambdaIntegration cannot be easily used for APIs with a considerable amount of endpoints because the CDK stack will break sooner or later when adding more resources to the APIGateway.

Proposed Solution

Other

There is a similar (duplicate?) issue which as been closed already https://github.com/aws/aws-cdk/issues/5774 (closed https://github.com/aws/aws-cdk/issues/5774#issuecomment-594902199 by AWS). The discussions in the end (after closing by AWS) are about workarounds (subclassing CDK) for something which seems to be missing as a feature, thus I created a new issue. Feel free to reopen the original one and add this as a duplicate.

Please also check https://github.com/aws/aws-cdk/issues/5774#issuecomment-609583801 which has been added after closing the issue. This comment describes the problem exactly the same as we see it.


This is a :rocket: Feature Request

nija-at commented 4 years ago

Thanks for filing this issue. I'd like to understand this issue a little more, before looking at the solution.

Which specific resource's policy size is reaching the limit? Is this the policy for the lambda function? Are you using the same lambda function for all your Rest API methods and resources?

Can you provide a code sample of relevant CDK code so I can see how you are using this? Specifically, how you are setting up the RestApi and LambdaIntegration.

phrastnik commented 4 years ago

Thanks nija-at for looking into that. Here the additional information:

This is the code, would be nice if it's possible to do something in different way to avoid hitting the policy limit.

    public void defineApiGateway(CisStackProps cisStackProps, FunctionBase lambda) {   
       //... some init code ...
       RestApi api = RestApi.Builder.create(this, cisStackProps.getApiGatewayInternalName())
               .restApiName(cisStackProps.getApiGatewayInternalName())
               .endpointConfiguration(
                       EndpointConfiguration.builder()
                       .types(List.of(EndpointType.PRIVATE))
                       .vpcEndpoints(List.of(vpcEndpoint))
                       .build()
                       )
               .deploy(Boolean.TRUE)
               .deployOptions(StageOptions.builder()
                       .stageName(cisStackProps.getEnvPostfix())
                       .loggingLevel(MethodLoggingLevel.INFO)
                       .tracingEnabled(Boolean.TRUE)
                       .dataTraceEnabled(Boolean.TRUE)
                       .accessLogDestination(
                               new LogGroupLogDestination(LogGroup.fromLogGroupName(this, "apiGatewayAccessLogGroup_" + cisStackProps.getEnvPostfix(), cisStackProps.getApiGatewayInternalLoggroupName()))
                        )
                       .accessLogFormat(AccessLogFormat.clf())
                       .build())
               .cloudWatchRole(Boolean.FALSE)
               .policy(PolicyDocument.fromJson(this.createResourcePolicy(vpcEndpoint.getVpcEndpointId())))
               .build();

       //... other configs like domainName, apiKey(s), usagePlans  ...

       LambdaIntegration lambdaIntegration = new LambdaIntegration(
               lambda, 
               LambdaIntegrationOptions.builder()
                   .allowTestInvoke(Boolean.FALSE) //This is the workaround in place, the policy size is reduced because no additional testInvoke policy entries are created
                   //.singleWildcardIntegrationPermission(Boolean.TRUE) This would be a proposal for the suggested additional feature, currently not working, of course                   
                   .build()
           );

       //LambdaIntegration lambdaIntegration = new LambdaIntegration(lambda); -> this caused the error as the Lambda policy would get too large with all the additional testInvokes for each resource

           IResource v1Res = api.getRoot().addResource("v1");
           IResource assetsRes = v1Res.addResource("assets");
           IResource videoRes = assetsRes.addResource("video");
           this.prepareResource(lambdaIntegration, videoRes, "POST");

       IResource videoByIdRes = videoRes.addResource("{guid}");
           this.prepareResource(lambdaIntegration, videoByIdRes, "GET");

           //... more resource defintions, the more we add, the more we fear that the policy size hits the limit and we have no acceptable workarounds anymore

    }

    private void prepareResource(Integration integration, IResource res, String method) {
        this.addIntegration(integration, res, method);
        this.addCorsOptions(res);  //this adds CORS responses to the resource, no LambdaIntegration but MockIntegration, so should not be relevant for the issue
    }

    private void addIntegration(Integration integration, IResource res, String method) {
        res.addMethod(method, integration,
                MethodOptions.builder()
                        .apiKeyRequired(Boolean.TRUE)
                        .build()
        );
    }

Hope that helps, happy to give some more information if necessary, Peter.

nija-at commented 4 years ago

Thanks Peter.

This is something we can improve in the CDK, however, I think we can do better than adding another property.

We should look into doing something more automatic - detect when the list of policies has reached a certain count and attempt to collapse them into a shorter prefix match, such as in your case.

phrastnik commented 4 years ago

Hi nija-at!

This sounds great and would definitely solve our issue, much better than proposed by myself initially.

Thanks, Peter.

kmbro commented 4 years ago

Is there an update on doing this work? I see the p2 was added on August 6. I am running into this error, and it is blocking my deployments right now

nija-at commented 4 years ago

Unfortunately, we've not gotten around to tackling this issue.

However, I have a workaround below -

const lambda = new lambda.Function(...);

const api = new apigateway.RestApi(...);

// define all resources, methods and integrations that use 'lambda'

class PermissionAspect implements core.IAspect {
  visit(construct: core.IConstruct) {
    if (construct instanceof apigateway.Method) {
      const permissions = construct.node.children.filter(c => c instanceof lambda.CfnPermission);
      permissions.forEach(p => construct.node.tryRemoveChild(p.node.id));
    }
  }
}

core.Aspects.of(api).add(new PermissionAspect());

lambda.addPermission('ApiPermissions', {
   // ...
})

The PermissionAspect model walks the construct tree starting from the RestApi and removes all permissions associated with the methods. Then, a compressed set of permissions with wildcards can be added.

Iku-turso commented 3 years ago

This blocks me as well :/

Priyaranjan1993 commented 3 years ago

Please resolve this issue. This blocks me as well.

rehanvdm commented 3 years ago

Author of the comment that is referenced in this ticket. It has been a few versions since that was posted and no longer works. I took some inspiration from @nija-at comment above. Rather extending the class as I did in the old method than using Aspects that apply it over all the whole API GW methods. So my alternative updated version that seems to work:

import lambda = require('@aws-cdk/aws-lambda');
import apigateway = require('@aws-cdk/aws-apigateway');

export class LambdaIntegrationNoPermission extends apigateway.LambdaIntegration {
  constructor(handler: lambda.IFunction, options?: apigateway.LambdaIntegrationOptions) {
    super(handler, options);
  }

  bind(method: apigateway.Method): apigateway.IntegrationConfig {
    const integrationConfig = super.bind(method);
    const permissions = method.node.children.filter(c => c instanceof lambda.CfnPermission);
    permissions.forEach(p => method.node.tryRemoveChild(p.node.id));
    return integrationConfig;
  }
}

const api = new apigateway.RestApi(this, id+"-api", {
            restApiName: id,
            deployOptions: { stageName: buildPros.Environment },
            defaultCorsPreflightOptions: {
                allowOrigins: apigateway.Cors.ALL_ORIGINS,
                allowMethods: apigateway.Cors.ALL_METHODS,
                allowHeaders: ["*"]
            },
            defaultIntegration: new LambdaIntegrationNoPermission(apiLambda, {proxy: true}),
        });

.... Add many methods and resources here ....

/* Manually add the permission, specifying with the API function arnForExecuteApi empty params means for all methods, paths and stages    */
apiLambda.addPermission(id + "ApiGWPermissions", {
           action: 'lambda:InvokeFunction',
           principal: new iam.ServicePrincipal('apigateway.amazonaws.com'),
           sourceArn: api.arnForExecuteApi()
       });
oleksii-donoha commented 3 years ago

If you're looking for Python solution, here is my snippet translated from @rehanvdm

from aws_cdk.aws_lambda import CfnPermission
from aws_cdk.aws_apigateway import LambdaIntegration, Method, RestApi

class CustomLambdaIntegration(LambdaIntegration):
    def __init__(self, handler, **kwargs):
        super().__init__(handler, **kwargs)

    def bind(self, method: Method):
        config = super().bind(method)
        permissions = filter(
            lambda x: isinstance(x, CfnPermission), method.node.children
        )
        for permission in permissions:
            method.node.try_remove_child(permission.node.id)
        return config

...
backend_integration = CustomLambdaIntegration(backend_function)

api = RestApi(
    self,
    "My API",
    default_integration=backend_integration,
)

backend_function.add_permission(
            id="API invoke permission",
            principal=ServicePrincipal("apigateway.amazonaws.com"),
            action="lambda:InvokeFunction",
            source_arn=api.arn_for_execute_api(),
        )
leantorres73 commented 3 years ago

Expecting for this to be fixed also!

cqthanh-zx commented 3 years ago

Hope this will get fixed soon

wyattisimo commented 3 years ago

+1

garrettsparks commented 2 years ago

Here's a solution that's similar to the ones above but for HttpApi from apigatewayv2 and the LambdaProxyIntegration

export class LambdaProxyIntegrationNoPermission extends LambdaProxyIntegration {
  constructor(props: LambdaProxyIntegrationProps) {
    super(props)
  }

  bind(options: HttpRouteIntegrationBindOptions): HttpRouteIntegrationConfig {
    const integrationConfig = super.bind(options)
    const permissions = options.route.node.children.filter(child => child instanceof CfnPermission)
    permissions.forEach(permission => options.route.node.tryRemoveChild(permission.node.id))
    return integrationConfig
  }
}
t3r commented 2 years ago

That one just saved my life! It is so much smarter than the original solution and should be fixed upstream IMHO.

ghadimdallal commented 2 years ago

Can some one give me a little hand in something ?

JPLemelin commented 2 years ago

Here's a solution that's similar to the ones of @rehanvdm but the lambda integration are also responsable to register the permission of the lambda:InvokeFunction

import * as iam from 'aws-cdk-lib/aws-iam';
import * as lambda from 'aws-cdk-lib/aws-lambda';
import * as apigateway from 'aws-cdk-lib/aws-apigateway';

export interface LambdaIntegrationOnePermissionOnlyOptions extends apigateway.LambdaIntegrationOptions {
  restApi: apigateway.IRestApi
}

export class LambdaIntegrationOnePermissionOnly extends apigateway.LambdaIntegration {

  constructor(handler: lambda.IFunction, options: LambdaIntegrationOnePermissionOnlyOptions) {
    super(handler, options);

    handler.addPermission('apigw-permissions', {
      principal: new iam.ServicePrincipal('apigateway.amazonaws.com'),
      action: 'lambda:InvokeFunction',
      sourceArn: options.restApi.arnForExecuteApi()
    });
  }

  bind(method: apigateway.Method): apigateway.IntegrationConfig {
    const integrationConfig = super.bind(method);

    // Remove all AWS::Lambda::Permission on methods
    const permissions = method.node.children.filter(c => c instanceof lambda.CfnPermission);
    permissions.forEach(p => method.node.tryRemoveChild(p.node.id));
    return integrationConfig;
  }
}
peterwoodworth commented 2 years ago

Hey, we've actually just recently merged a PR which aims to tackle all sorts of policy size limit issues. Look forward to the next v2 release and let me know if this is still an issue 🙂

https://github.com/aws/aws-cdk/pull/19114

github-actions[bot] commented 2 years ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

ghadimdallal commented 2 years ago

Hey there ! I actually have found a way to solve this issue , but I will check out your solution and come back to you !

From: github-actions[bot] @.> Sent: Thursday, March 31, 2022 3:14 AM To: aws/aws-cdk @.> Cc: Ghadi Mdallal @.>; Comment @.> Subject: Re: [aws/aws-cdk] [APIGateway] LambdaIntegration: Add option to create a single trigger/permission with wildcards only instead of one for each ApiGateway Resource (#9327)

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

— Reply to this email directly, view it on GitHubhttps://github.com/aws/aws-cdk/issues/9327#issuecomment-1083830184, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AW5J4CZH7DADRAAPIRWUYD3VCTU6DANCNFSM4PLQYBXA. You are receiving this because you commented.Message ID: @.**@.>>

robert-hanuschke commented 2 years ago

@peterwoodworth : Unfortunately still experiencing this behavior on aws-cdk-lib 2.21.1

peterwoodworth commented 2 years ago

Thanks for letting us know @robert-hanuschke, we're aware that our fix which went out before didn't fix as many of these issues as we would have liked. We're still working on figuring out how to best minimize the templates

naveenthontepu commented 2 years ago

@peterwoodworth : This issue still exists on aws-cdk 2.26.0.

Please keep us posted.

ghadimdallal commented 2 years ago

Here's a solution that's similar to the ones of @rehanvdm but the lambda integration are also responsable to register the permission of the lambda:InvokeFunction

import * as iam from 'aws-cdk-lib/aws-iam';
import * as lambda from 'aws-cdk-lib/aws-lambda';
import * as apigateway from 'aws-cdk-lib/aws-apigateway';

export interface LambdaIntegrationOnePermissionOnlyOptions extends apigateway.LambdaIntegrationOptions {
  restApi: apigateway.IRestApi
}

export class LambdaIntegrationOnePermissionOnly extends apigateway.LambdaIntegration {

  constructor(handler: lambda.IFunction, options: LambdaIntegrationOnePermissionOnlyOptions) {
    super(handler, options);

    handler.addPermission('apigw-permissions', {
      principal: new iam.ServicePrincipal('apigateway.amazonaws.com'),
      action: 'lambda:InvokeFunction',
      sourceArn: options.restApi.arnForExecuteApi()
    });
  }

  bind(method: apigateway.Method): apigateway.IntegrationConfig {
    const integrationConfig = super.bind(method);

    // Remove all AWS::Lambda::Permission on methods
    const permissions = method.node.children.filter(c => c instanceof lambda.CfnPermission);
    permissions.forEach(p => method.node.tryRemoveChild(p.node.id));
    return integrationConfig;
  }
}

This is the best solution so far if any one is still facing the same issue

ghadimdallal commented 2 years ago
class LambdaIntegrationNoPermission extends apigateway.LambdaIntegration {
  constructor(handler: lambda.IFunction, options?: apigateway.LambdaIntegrationOptions) {
    super(handler, options);
  }

  bind(method: apigateway.Method): apigateway.IntegrationConfig {
    const integrationConfig = super.bind(method);
    const permissions = method.node.children.filter(c => c instanceof lambda.CfnPermission);
    permissions.forEach(p => method.node.tryRemoveChild(p.node.id));
    return integrationConfig;
  }
}

========================================================================== Example from my code :

 getStartedAttributeRoute.addMethod("GET", new LambdaIntegrationNoPermission(attributeServiceLambda),
      {
        apiKeyRequired: true,
        authorizer: auth,
        authorizationType: apigateway.AuthorizationType.COGNITO,
        authorizationScopes: oAuthScope
      }
    )
enroly-mike commented 2 years ago

Note sure if its the case for anyone else but the above code didn't work with cdk 2.37.1 so I had to resort to the following

export class LambdaIntegrationNoPermission extends apigateway.LambdaIntegration {
  constructor(
    handler: lambda.IFunction,
    options?: apigateway.LambdaIntegrationOptions
  ) {
    super(handler, options);
  }

  bind(method: apigateway.Method): apigateway.IntegrationConfig {
    const integrationConfig = super.bind(method);
    const permissions = method.node.children.filter(
      // @ts-ignore
      (c) => c.node.host.action === "lambda:InvokeFunction"
    );

    console.log(permissions);

    permissions.forEach((p) => method.node.tryRemoveChild(p.node.id));
    return integrationConfig;
  }
}

I was told by AWS Premium support that this wasn't a CDK problem, it was a lambda problem, I'm somewhat unconvinced by that statement. Also forgive me for my TS escape, please feel free to recommend a more type safe way (or collapse the permissions properly)

rib commented 2 years ago

I'm new to using CDK since I thought it would be a good idea to switch my AWS project from using Serverless Framework to cdk

I immediately hit this issue which meant I couldn't deploy my HttpApi due to the per-route permissions making the policy too large to deploy.

After lots of head scratching, digging through aws-cdk code and borrowing ideas from the snippets above, here's my contribution to the growing collection of (hacky) workarounds:

ApiGatewayV2 + HttpApi + Python Workaround

class PermittedHttpLambdaIntegration(HttpLambdaIntegration):
    handler: aws_cdk.aws_lambda.IFunction = None
    permitted_apis = set()

    def __init__(self,
        id: str,
        handler: aws_cdk.aws_lambda.IFunction,
        *,
        parameter_mapping = None,
        payload_format_version = None,
    ) -> None:
        self.handler = handler
        super().__init__(id, handler, parameter_mapping=parameter_mapping, payload_format_version=payload_format_version)

    @jsii.member(jsii_name="bind")
    def bind(
        self,
        *,
        route: apigwv2.IHttpRoute,
        scope: constructs.Construct,
    ) -> apigwv2.HttpRouteIntegrationConfig:
        _ = apigwv2.HttpRouteIntegrationBindOptions(
            route=route, scope=scope
        )

        # The JSSI stuff all blows up if we try and call super().bind() but it seems to
        # work if we punch through and do what HttpLambdaIntegration.bind()
        # would do by calling jsii.invoke directly :/
        #
        # (Maybe someone familiar with jsii knows how to override methods properly?)
        return typing.cast(apigwv2.HttpRouteIntegrationConfig, jsii.invoke(self, "bind", [_]))

    @jsii.member(jsii_name="completeBind")
    def _complete_bind(
        self,
        *,
        route: apigwv2.IHttpRoute,
        scope: constructs.Construct,
    ) -> None:
        api_id = route.http_api.api_id
        if api_id not in self.permitted_apis:
            self.permitted_apis.add(api_id)

            handler = self.handler
            handler.add_permission("ApiGatewayPermissions",
              principal=aws_iam.ServicePrincipal("apigateway.amazonaws.com"),
              action='lambda:InvokeFunction',
              source_arn="arn:aws:execute-api:{region}:{account}:{api_id}/*".format(
                  region=handler.env.region,
                  account=handler.env.account,
                  api_id=api_id,
              ))

In terms of the comment above where @enroly-mike was told that this is not a CDK problem it seems fair to note here that this issue didn't exist with Serverless Framework which was creating a wildcard rule instead.

From my perspective as a first-time user of CDK this issue is a pretty big red flag for me currently. The issue is now over two years old and it seems pretty clear that this is a serious problem in situations where you have multiple routes that share a single function. (You can't deploy your stack without implementing a workaround)

In my case I'm using Rust to implement a native lambda with Runtime.PROVIDED_AL2 which handles multiple, related routes. I wouldn't have imagined this would be considered unusual, so it was surprising to hit a hurdle like this.

Versions:

cdk = 2.52.0 (build 096d2e0) aws-cdk-lib = 2.52.0 apigatewayv2-alpha = 2.52.0a0

sarahgancisonos commented 1 year ago

Just wanted to drop a comment to voice that I also feel this should be fixed. While there is a workaround, it is clear that this feature would be used if implemented :)

ognjen-andric commented 1 year ago

đź‘Ť Would like to get this one fixed as well. Currently we implemented a solution proposed by @rehanvdm (thanks for that), but feels like a dirty workaround which is prone to issues in future. AWS, please fix :)

mn-prp commented 1 year ago

I'd like to add my voice to the choir in support of cdk fixing this. The hacky workaround is, well, hacky. And it's also not obvious when first encountered.

muzeke commented 1 year ago

So glad to see the workaround, saved me a whole lot of time!

I hope this gets fixed sooon!

marcello0liveira commented 1 year ago

I tested the solution from ghadimdallal in 2.84.0 and it is working

sabornibhattacharya commented 9 months ago

I have published this pattern which creates an APIGW-Lambda integration with wildcard resource-based policy -

https://serverlessland.com/patterns/apigw-lambda-wildcard-resourcebasedpolicy-cdk

This is a python-based solution. Feel free to create your own based upon your choice of language for CDK.

tomhog commented 6 months ago

here's a hacky solution for apigatev2 HttpApi

import { Stack } from 'aws-cdk-lib';
import { IFunction, Function } from 'aws-cdk-lib/aws-lambda';
import { ServicePrincipal } from 'aws-cdk-lib/aws-iam';
import { HttpRouteIntegrationBindOptions } from 'aws-cdk-lib/aws-apigatewayv2';
import { HttpLambdaIntegration, HttpLambdaIntegrationProps } from 'aws-cdk-lib/aws-apigatewayv2-integrations';

export class HttpLambdaIntegrationOnePermissionOnly extends HttpLambdaIntegration {

  permissionAdded = false;

  constructor(id: string, handler: IFunction, props?: HttpLambdaIntegrationProps) {
    super(id, handler, props);
  }

  protected completeBind(options: HttpRouteIntegrationBindOptions) {
    if (this.permissionAdded) return;
    this.permissionAdded = true;
    const route = options.route;
    const nonReadonlyHandler = (this as any).handler as Function;
    const nonPrivateId = (this as any)._id as string;
    nonReadonlyHandler.addPermission(`${nonPrivateId}-Permission`, {
      scope: options.scope,
      principal: new ServicePrincipal('apigateway.amazonaws.com'),
      sourceArn: Stack.of(route).formatArn({
        service: 'execute-api',
        resource: route.httpApi.apiId,
        resourceName: `*/*`, // always use */*
      }),
    });
  }
}

I just override completeBind, then you can just straight replace you HttpLambdaIntegration with this HttpLambdaIntegrationOnePermissionOnly. No other step required. It is mega hacky though and prone to breaking in the future.