Allow explicit AWS::NoValue for Api Gateway Default Authorizer

[ethanmills]

Issue #, if available

#3313

Description of changes

Change to schema and implementation to allow an explicit No Value for the API gateway default authoriser.

Description of how you validated changes

To do

Checklist

• Adheres to the development guidelines
• Add/update transform tests
  • Using correct values
  • Using wrong values
• Add/update integration tests

Examples?

Please reach out in the comments if you want to add an example. Examples will be
added to sam init through aws/aws-sam-cli-app-templates.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[aaythapa]

Tests and schema stuff looks good. Looking at the request you say

It would be great if SAM could accept an explicit None in the DefaultAuthorizer when there are authorizers configured.

I thought this meant you like to do DefaultAuthorizer: None. Does AWS::NoValue solve that usecase and why that over just allowing NONE in DefaultAuthorizer? (I'm not too familiar with AWS::NoValue so might just be going over my head)

[ethanmills]

Tests and schema stuff looks good. Looking at the request you say

It would be great if SAM could accept an explicit None in the DefaultAuthorizer when there are authorizers configured.

I thought this meant you like to do DefaultAuthorizer: None. Does AWS::NoValue solve that usecase and why that over just allowing NONE in DefaultAuthorizer? (I'm not too familiar with AWS::NoValue so might just be going over my head)

That was my original plan, but I did this because of the discussion over in #3313 (comment). I think DefaultAuthorizer: None would technically be a breaking change, because there's nothing to stop someone from having defined an authorizer called None previously.

[ethanmills]

@aaythapa Sorry for leaving this for so long, I ran out of time to work on this. I looked at adding integration tests for this (and would be happy to do so if you'd like) but given the nature of the transformation I think the existing tests cover this entirely.

[ethanmills]

May be worth adding an example to document this, but I've updated the schema to reflect. It looks like this was already implemented in code for the HttpApi, but not in the schema (now fixed)

[ethanmills]

Am I right in thinking these test failures are unrelated to my change?

[aaythapa]

@ethanmills thanks for taking on this PR again. Yep, those failures are unrelated to your changes, I'm going to push a change soon to fix that and then review your code. Thanks again

[aaythapa]

This test file seems redundant. From what I can tell you use Ref: AWS::NoValue here and use !Ref AWS::NoValue in tests/translator/input/api_with_auth_explicit_inline_no_default.yaml. I think its ok if we only test one (if anything you can have a mix of both in one file). Let me know if there are more differences I didn't spot

[aaythapa]

A lot of these Events seem duplicated and add needless complexity to this test. Could we make this simpler somehow while still being thorough? For example I don't think we need to test for different paths since I don't believe that will make a difference in the output.

[aaythapa]

@aaythapa Sorry for leaving this for so long, I ran out of time to work on this. I looked at adding integration tests for this (and would be happy to do so if you'd like) but given the nature of the transformation I think the existing tests cover this entirely.

Thanks again for taking this on. Since your change uses intrinsics I would feel more comfortable if you added integration tests. Something simple should do, just to test the behavior is as expected. Other than that and the few comments i added the PR lgtm

[GavinZZ]

Thanks for the contribution. Really appreciate that you spend the time and effort to create it. However, I have one question. Have you tried out using DefaultAuthorizer: null?

I had a quick test and it seems to work for me without making any changes to the code. Here is my sample template:

Resources:
  HttpApiFunction:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: s3://sam-demo-bucket/todo_list.zip
      Handler: index.restapi
      Runtime: python3.7
      Events:
        DefaultAuth:
          Type: HttpApi
          Properties:
            Path: /default
            Method: post
            ApiId: !Ref MyApi
        SpecifiedAuth:
          Type: HttpApi
          Properties:
            Path: /specified
            Method: get
            ApiId: !Ref MyApi
            Auth:
              Authorizer: LambdaAuth
  MyAuthFn:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: s3://bucket/key
      Handler: index.handler
      Runtime: nodejs12.x

  MyApi:
    Type: AWS::Serverless::HttpApi
    Properties:
      Tags:
        Tag1: value1
        Tag2: value2
      Auth:
        Authorizers:
          LambdaAuth:
            FunctionArn: !GetAtt MyAuthFn.Arn
            AuthorizerPayloadFormatVersion: 1.0
        DefaultAuthorizer: null

The transformed template shows the following section where /specified shows the authorizer since we explicitly set authorizer in Lambda event source while /default doesn't have anything related to authorizer.

"paths": {
      "/specified": {
       "get": {
        "x-amazon-apigateway-integration": {
         "httpMethod": "POST",
         "type": "aws_proxy",
         "uri": {
          "Fn::Sub": "arn:${AWS::Partition}:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${HttpApiFunction.Arn}/invocations"
         },
         "payloadFormatVersion": "2.0"
        },
        "security": [
         {
          "LambdaAuth": []
         }
        ],
        "responses": {}
       }
      },
      "/default": {
       "post": {
        "x-amazon-apigateway-integration": {
         "httpMethod": "POST",
         "type": "aws_proxy",
         "uri": {
          "Fn::Sub": "arn:${AWS::Partition}:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${HttpApiFunction.Arn}/invocations"
         },
         "payloadFormatVersion": "2.0"
        },
        "responses": {}
       }
      }
     },