fix(apigatewayv2): authorizer is not removed when HttpNoneAuthorizer is used#14424
Merged
mergify[bot] merged 11 commits intoaws:masterfrom May 12, 2021
Merged
fix(apigatewayv2): authorizer is not removed when HttpNoneAuthorizer is used#14424mergify[bot] merged 11 commits intoaws:masterfrom
mergify[bot] merged 11 commits intoaws:masterfrom
Conversation
- CloudFormation will not remove an existing Authorizer
if AuthorizationType and AuthorizerId are simply removed.
The AuthorizationType must be explicitly set to NONE
for CloudFormation to remove the existing Authorizer.
|
|
nija-at
previously requested changes
May 6, 2021
Contributor
nija-at
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR. The approach sounds good.
One suggestion below
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
|
|
nija-at
previously requested changes
May 6, 2021
Contributor
nija-at
left a comment
There was a problem hiding this comment.
The build has failed. The link to the build logs are in a comment above.
Contributor
Author
nija-at
approved these changes
May 12, 2021
Contributor
|
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). |
Collaborator
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Contributor
|
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). |
hollanddd
pushed a commit
to hollanddd/aws-cdk
that referenced
this pull request
Aug 26, 2021
…is used (aws#14424) CloudFormation will not remove an existing Authorizer if AuthorizationType and AuthorizerId are simply removed. The AuthorizationType must be explicitly set to NONE for CloudFormation to remove the existing Authorizer. As such, I updated the HttpRoute constructor to include the AuthorizationType even if it is NONE; otherwise it is impossible to remove an authorizer in CDK. Some thought had obviously gone into this previously because of the following line: https://github.com/aws/aws-cdk/blob/2f5eeb08f8790c73db7305cc7f85116e2730267d/packages/%40aws-cdk/aws-apigatewayv2/lib/http/route.ts#L159 I did not manage to track down the reasoning for this in commit comments, so I would be interested to hear why this was done, since I may have overlooked a desired use case. I'm wondering if it was assumed that the default CloudFormation value for AuthorizationType is NONE, so to have a more compact template it was omitted. However, the behavior when AuthorizationType is not present, is to not change the existing Authorizer. Basically in the CloudFormation template, ```yaml APIGETintegrationgoogleapiregister1D8736BD: Type: AWS::ApiGatewayV2::Route Properties: ApiId: Ref: API62EA1CEE RouteKey: GET /integration/google-api/register Target: ... ``` does not have the same effect as ```yaml APIGETintegrationgoogleapiregister1D8736BD: Type: AWS::ApiGatewayV2::Route Properties: ApiId: Ref: API62EA1CEE RouteKey: GET /integration/google-api/register AuthorizationType: NONE Target: ... ``` Only the later will remove an existing Authorizer. If you think this is a bug in CloudFormation and not its intended behavior, please let me know. I am assuming that they would not change the behavior anyway since that could have unintended consequence for anyone who redeploys a template without the AuthorizationType set. BREAKING CHANGE: setting the authorizer of an API route to HttpNoneAuthorizer will now remove any existing authorizer on the route ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CloudFormation will not remove an existing Authorizer if AuthorizationType and AuthorizerId are simply removed. The AuthorizationType must be explicitly set to NONE for CloudFormation to remove the existing Authorizer.
As such, I updated the HttpRoute constructor to include the AuthorizationType even if it is NONE; otherwise it is impossible to remove an authorizer in CDK. Some thought had obviously gone into this previously because of the following line:
aws-cdk/packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts
Line 159 in 2f5eeb0
I did not manage to track down the reasoning for this in commit comments, so I would be interested to hear why this was done, since I may have overlooked a desired use case. I'm wondering if it was assumed that the default CloudFormation value for AuthorizationType is NONE, so to have a more compact template it was omitted. However, the behavior when AuthorizationType is not present, is to not change the existing Authorizer.
Basically in the CloudFormation template,
does not have the same effect as
Only the later will remove an existing Authorizer.
If you think this is a bug in CloudFormation and not its intended behavior, please let me know. I am assuming that they would not change the behavior anyway since that could have unintended consequence for anyone who redeploys a template without the AuthorizationType set.
BREAKING CHANGE: setting the authorizer of an API route to HttpNoneAuthorizer will now remove any existing authorizer on the route
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license