-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[APIGateway] LambdaIntegration: Add option to create a single trigger/permission with wildcards only instead of one for each ApiGateway Resource #9327
Comments
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? 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 |
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.
Hope that helps, happy to give some more information if necessary, |
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. |
Hi nija-at! This sounds great and would definitely solve our issue, much better than proposed by myself initially. Thanks, |
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 |
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 |
This blocks me as well :/ |
Please resolve this issue. This blocks me as well. |
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:
|
If you're looking for Python solution, here is my snippet translated from @rehanvdm
|
Expecting for this to be fixed also! |
Hope this will get fixed soon |
+1 |
Here's a solution that's similar to the ones above but for
|
That one just saved my life! It is so much smarter than the original solution and should be fixed upstream IMHO. |
Can some one give me a little hand in something ? |
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
|
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 🙂 |
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. |
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 GitHub<#9327 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AW5J4CZH7DADRAAPIRWUYD3VCTU6DANCNFSM4PLQYBXA>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
@peterwoodworth : Unfortunately still experiencing this behavior on aws-cdk-lib 2.21.1 |
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 |
@peterwoodworth : This issue still exists on aws-cdk 2.26.0. Please keep us posted. |
This is the best solution so far if any one is still facing the same issue |
==========================================================================
|
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
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) |
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 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 ApiGatewayV2 + HttpApi + Python Workaroundclass 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 Versions:cdk = 2.52.0 (build 096d2e0) |
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 :) |
👍 Would like to get this one fixed as well. |
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. |
So glad to see the workaround, saved me a whole lot of time! I hope this gets fixed sooon! |
Added ability to upload case documents i.e. files relevant to the case and view them. Added masonry via vue-masonry (warning: No TS support). Moved cases and appointment related components into their own folder appointments-cases. Changed all <pre> blocks to have regular sans-serif fonts not monospaced, via a class added to ech <pre> block. Replaced LambdaIntegration with a custom LambdaIntegrationNoPermission which does not individually add permissions for APIGateway integrations. A blanket permission is later added allowing any APIGW endpoint to call the lambda function. This is required because of error: The final policy size (vwxyz) is bigger than the limit (20480). Solution courtesy of aws/aws-cdk#9327 (comment)
I tested the solution from ghadimdallal in 2.84.0 and it is working |
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. |
here's a hacky solution for apigatev2 HttpApi
I just override completeBind, then you can just straight replace you HttpLambdaIntegration with this HttpLambdaIntegrationOnePermissionOnly. No other step required. |
any update on this? |
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:
The requested feature would allow to have something like that instead:
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
tofalse
. This cut the number of triggers/IntegrationPermissions in half and the policy didn't hit the limit anymore.However, we would prefer leaving
allowTestInvoke
totrue
.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 #5774 (comment) (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
boolean
optionsingleWildcardTrigger
orsingleWildcardIntegrationPermission
toaws_cdk.aws_apigateway.LambdaIntegrationOptions
.false
and everything works like as it does currently.true
, only a single trigger with wildcards is generated (see above).allowTestInvoke
option, there is already an option which works globally an all tiggers/integrationPermissions as well. So something very similar is already available.Other
There is a similar (duplicate?) issue which as been closed already #5774 (closed #5774 (comment) 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 #5774 (comment) which has been added after closing the issue. This comment describes the problem exactly the same as we see it.
This is a 🚀 Feature Request
The text was updated successfully, but these errors were encountered: