-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(apigatewayv2): http api - IAM authorizer support #17519
Conversation
Hi @ChenKuanSun can you take a look? |
Looking at the docs it is unclear to me how to just turn on IAM for the whole API. Also I don't understand why it's not an "authorizer", as in the Rest API gateway. Personally I think that the use case of bringing your own policy/role is more often the case then the "grantInvoke", but that's a gut feeling. Currently I'm solving this problem with:
|
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.
This is starting to look interesting. See my initial comments below.
Pull request has been modified.
@bracki Good point. Since my initial PR, Nija has suggested adding |
@@ -61,9 +85,14 @@ export class HttpRouteKey { | |||
if (path !== '/' && (!path.startsWith('/') || path.endsWith('/'))) { | |||
throw new Error('A route path must always start with a "/" and not end with a "/"'); | |||
} | |||
return new HttpRouteKey(`${method ?? HttpMethod.ANY} ${path}`, path); | |||
const keyMethod = method ?? HttpMethod.ANY; | |||
return new HttpRouteKey(keyMethod, `${keyMethod} ${path}`, path); |
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.
[minor] Now that key
can be derived from the other two attributes, I would remove it from the constructor to make it cleaner and avoid mistakes.
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.
I've now simplified this section so that it's cleaner and easier to avoid mistakes. As far as I can see, HTTP API has only the $default
and METHOD /path/here
formats for key
, so this should work. https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-routes.html
Let me know whether this change works for you.
Pull request has been modified.
@otaviomacedo I got a notification for the following message but I can't find it anywhere to reply to it. I'll respond here in the comments:
Right now I've allowed grantInvoke to configure IAM via authorizer by implementing the authorizer in the main package. But yes, I can see that this makes the IAM authorizer the odd one out. It seems to me that if the IAM authorizer must be in the other package, the circular dependency arises from adding the authorizer automatically when the user calls grantInvoke. If we resolve this conflict by removing the route's responsibility to configure IAM authorization, we can relocate the IAM authorizer and there will be no circular dependency problem. All things being equal, I'd prefer to remove the route's responsibility to configure IAM from grantInvoke as it could simplify the Lazy props. What do you think? |
I agree. So, two questions:
|
@otaviomacedo We'd require that the user specify the authorizer they want in the construct's props as they do now.
It would look the same as it does now in the README, however, it differs in one case: When the user omits the IAM authorizer and calls |
Yes, the only downside is that, if someone provides a different authorizer, |
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.
Well done! Thank you.
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). |
Fixes aws#15123 See also: [@nija-at's comments on `grantInvoke`](aws#14853 (comment)), aws#10534 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Fixes #15123
See also: @nija-at's comments on
grantInvoke
, #10534By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license