Skip to content
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): authorizer is not removed when HttpNoneAuthorizer is used #14424

Merged
merged 11 commits into from
May 12, 2021

Conversation

njlaw
Copy link
Contributor

@njlaw njlaw commented Apr 28, 2021

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:

const authorizationType = authBindResult?.authorizationType === HttpAuthorizerType.NONE ? undefined : authBindResult?.authorizationType;

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,

  APIGETintegrationgoogleapiregister1D8736BD:
    Type: AWS::ApiGatewayV2::Route
    Properties:
      ApiId:
        Ref: API62EA1CEE
      RouteKey: GET /integration/google-api/register
      Target: ...

does not have the same effect as

  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

- 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.
@gitpod-io
Copy link

gitpod-io bot commented Apr 28, 2021

@github-actions github-actions bot added the @aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 label Apr 28, 2021
@nija-at nija-at changed the title fix(apigatewayv2): set AuthorizationType explicitly to None fix(apigatewayv2): authorizer is not removed when HttpNoneAuthorizer is used May 6, 2021
nija-at
nija-at previously requested changes May 6, 2021
Copy link
Contributor

@nija-at nija-at left a 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. The approach sounds good.

One suggestion below

packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts Outdated Show resolved Hide resolved
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
@gitpod-io
Copy link

gitpod-io bot commented May 6, 2021

@mergify mergify bot dismissed nija-at’s stale review May 6, 2021 15:53

Pull request has been modified.

nija-at
nija-at previously requested changes May 6, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build has failed. The link to the build logs are in a comment above.

@mergify mergify bot dismissed nija-at’s stale review May 6, 2021 17:52

Pull request has been modified.

Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @njlaw - There build is still failing. Likely more snapshot tests that need to be updated.

@mergify mergify bot dismissed nija-at’s stale review May 11, 2021 18:34

Pull request has been modified.

@njlaw
Copy link
Contributor Author

njlaw commented May 11, 2021

Hi @njlaw - There build is still failing. Likely more snapshot tests that need to be updated.

Thank you @nija-at. I should have some time to get the outstanding test snapshot updates done in the next few days if I don't manage to today.

@mergify
Copy link
Contributor

mergify bot commented May 12, 2021

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: afa7ceb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 3698a91 into aws:master May 12, 2021
@mergify
Copy link
Contributor

mergify bot commented May 12, 2021

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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants