-
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
fix(apigatewayv2): multiple http integrations are created for each route #12528
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR but I don't the follow problem being fixed here.
In the PR description, can you provide a brief description of the error faced, the root cause and fix.
This will also ensure healthy commit messages for changes in our repo history.
@nija-at Sorry for not specifying the details of the problem upfront. I have tried to provide as much details as possible now. Please take a look and let me know if we need to refine the same :) |
Thank you for submitting this PR, @ayush987goyal. This is the behavior I encountered while executing the code above. Please let me know if you need any additional information from me. I would think, creating integration beforehand and the use its handler in subsequent routes additions would make sense. And if creation of a new integration for each route is needed, it could be achieved by just creating new object right there… in other words: const apiGateway = new apigw.HttpApi(this, apiLableApiGateway);
// FOR EXISTING INTEGRATION:
const apiIntegration = new apigwint.HttpAlbIntegration({
listener : apiListener,
vpcLink : apiVpcLink
});
apiGateway.addRoutes({
methods : [apigw.HttpMethod.GET],
path : apiRoutePath,
integration : apiIntegration
});
// FOR NEW INTEGRATION FOR EACH ROUTE:
apiGateway.addRoutes({
methods : [apigw.HttpMethod.GET],
path : apiRoutePath,
integration : new apigwint.HttpAlbIntegration({
listener : apiListener,
vpcLink : apiVpcLink
});
}); |
@nija-at The PR is still showing changes requested since no commit was pushed post the ask. Could you please take a look at the details now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's try this out and see how it works. Since this is all internal APIs, we can always change later, if we find a better approach.
Some small comments below.
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). |
Hi @nija-at , another approval would probably be needed since there was a new release in b/w and I had to manually merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got it.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Thank you for addressing this, guys! Very much appreciate it. Any idea when this change will get into the official release? |
…ute (aws#12528) Currently when we define any HTTP integration (like ALB, NLB etc.) explicitly and use it for multiple routes, the `HttpRoute` construct will create a new `HttpIntegration` resource for each of these routes. [Ref](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-apigatewayv2/lib/http/route.ts#L128) These extra HttpIntegration (and hence `CfnIntegration`) are not required since the same integration can be used for all the routes. This feature is supported in console as well (i.e. selecting same integration for multiple routes). A sample snippet: ```ts const apiGateway = new apigw.HttpApi(this, apiLableApiGateway); const apiIntegration = new apigwint.HttpAlbIntegration({ listener : apiListener, vpcLink : apiVpcLink }); const apiRoutes = [ { method: apigw.HttpMethod.ANY, path: '/' }, { method: apigw.HttpMethod.GET, path: '/status' }, { method: apigw.HttpMethod.GET, path: '/whateverelse' }, ]; for (const apiRouteIndex in apiRoutes) { const apiRouteMethod = apiRoutes[apiRouteIndex].method; const apiRoutePath = apiRoutes[apiRouteIndex].path; apiGateway.addRoutes({ methods : [apiRouteMethod], path : apiRoutePath, integration : apiIntegration, }); } ``` The root cause of this issue is that the current api resource does not keep track of any existing integrations with the same integration config. We are solving this issue by keeping track of these integrations in an internal map with key as stringified value of `HttpRouteIntegrationConfig` (which we get by calling the `bind` function) and value as the `HttpIntegration`. This ensures that we reuse any existing integration (with same config) instead of creating a new one for every route. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
I just redeployed my API Gateway and... the issue with multiple integrations has been fixed! Awesome, thanks guys! |
Currently when we define any HTTP integration (like ALB, NLB etc.) explicitly and use it for multiple routes, the
HttpRoute
construct will create a newHttpIntegration
resource for each of these routes. RefThese extra HttpIntegration (and hence
CfnIntegration
) are not required since the same integration can be used for all the routes. This feature is supported in console as well (i.e. selecting same integration for multiple routes). A sample snippet:The root cause of this issue is that the current api resource does not keep track of any existing integrations with the same integration config.
We are solving this issue by keeping track of these integrations in an internal map with key as stringified value of
HttpRouteIntegrationConfig
(which we get by calling thebind
function) and value as theHttpIntegration
. This ensures that we reuse any existing integration (with same config) instead of creating a new one for every route.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license