fix: remove unsupported options for openapi3.0 by ShreyaGangishetty · Pull Request #1316 · aws/serverless-application-model

ShreyaGangishetty · 2019-12-11T17:21:33Z

Issue #, if available:
#1161

Description of changes:
removed consumes and produces from options for openapi 3.
nested the type for CORS headers under schema as per openapi3 spec https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#headerObject

Description of how you validated changes:
Verified the open api definition output of SAM matches APIGW output for open api

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected (Verified openapi definition of SAM matches with APIGW)
Add/update example to examples/2016-10-31

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

praneetap · 2019-12-11T17:53:47Z

samtranslator/model/api/api_generator.py

                                                            "'AllowOrigin' is \"'*'\" or not set")

        editor = SwaggerEditor(self.definition_body)
+        openapi_flag = True if self.definition_body.get("openapi") else False


I think it's safer to remove produces and consumes in _openapi_postprocess than doing it in swagger. So instead of passing the flag to swagger, just add this to the method _openapi_postprocess here so we don't have too many openapi/swagger if checks in swagger.py, and we also make sure we chack not only for the presence of the flag but slso that the version is 3.x.x

codecov-io · 2019-12-11T19:33:16Z

Codecov Report

Merging #1316 into develop will decrease coverage by 0.09%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           develop    #1316     +/-   ##
==========================================
- Coverage    94.44%   94.34%   -0.1%     
==========================================
  Files           78       78             
  Lines         4353     4334     -19     
  Branches       860      870     +10     
==========================================
- Hits          4111     4089     -22     
- Misses         115      116      +1     
- Partials       127      129      +2

Impacted Files	Coverage Δ
samtranslator/model/api/api_generator.py	`95.13% <92.85%> (-0.09%)`	⬇️
samtranslator/model/iam.py	`86.36% <0%> (-2.53%)`	⬇️
samtranslator/plugins/exceptions.py	`83.33% <0%> (-2.39%)`	⬇️
samtranslator/translator/arn_generator.py	`86.95% <0%> (-0.55%)`	⬇️
samtranslator/model/sqs.py	`93.33% <0%> (-0.42%)`	⬇️
samtranslator/validator/validator.py	`95.23% <0%> (-0.42%)`	⬇️
samtranslator/open_api/open_api.py	`93.6% <0%> (-0.29%)`	⬇️
samtranslator/swagger/swagger.py	`92.77% <0%> (-0.13%)`	⬇️
samtranslator/model/eventsources/push.py	`90.53% <0%> (-0.04%)`	⬇️
samtranslator/model/__init__.py	`98.11% <0%> (-0.04%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a062026...c9b56a5. Read the comment docs.

ShreyaGangishetty · 2019-12-11T20:35:38Z

samtranslator/model/api/api_generator.py

                definition_body['components'] = components
                del definition_body['definitions']
+
+            if definition_body.get("paths"):


The check for openapi3.x.x is already present in _openapi_postprocess so I haven't added that.
Updated the code with removing consumes and produces. Also, updated the code with schema for headers as per ApiGateway export output

keetonian

Looks good overall, just a few small comments

keetonian · 2019-12-11T21:24:42Z

samtranslator/model/api/api_generator.py

+                                del definition_body["paths"][path]["options"][field]
+                            # add schema for the headers in options section for openapi3
+                            if field in ["responses"]:
+                                headers = definition_body["paths"][path]["options"][field]['200']['headers']


Are 200 and headers guaranteed to exist?

Yes they are guranteed to exist if SAM generates the definition body/inline swagger. This is the part of the swagger definition sam generates: https://github.com/awslabs/serverless-application-model/blob/master/samtranslator/swagger/swagger.py#L361-L366

Can you verify that this method doesn't run if SAM didn't generate the swagger?

I will add a check for 200 and headers to make sure we won't throw 5xx errors if the openapi template is incorrect

keetonian · 2019-12-11T21:26:06Z

samtranslator/model/api/api_generator.py

                components['schemas'] = definition_body['definitions']
                definition_body['components'] = components
                del definition_body['definitions']
+


Please either add a comment here about what this section is trying to do or have this method call other methods with docstrings that explain what things we're changing and why.

I have added comments for the other if statements but adding at the top level makes more sense. Thank you! I have updated with comments for this part

keetonian · 2019-12-11T21:27:10Z

Have you checked the OpenApi spec to see if anything else in this section needs to change? (Not a whole audit of the spec, just specific to this change)

ShreyaGangishetty · 2019-12-11T21:46:41Z

I have checked openapi GH and covered related to definition body for CORS added by SAM.

JoseExposito · 2019-12-12T16:57:33Z

Hi @ShreyaGangishetty

Thank you very much for your pull request :)

I am facing issues to enabling CORS with SAM:

$ sam --version
SAM CLI, version 0.37.0

Even though this version doesn't generate unsupported OpenApi 3.0 code, I think there are still some pieces missing.

The OPTIONS method seem to be generated properly, however, the GET method doesn't seem to have the "Access-Control-Allow-Origin" header.

Also, It is not possible to enable CORS in Api Gateway responses (x-amazon-apigateway-gateway-responses).
Edit: Already reported #1178

Let me illustrate the issues with an example:

openapi: "3.0.1"

[...]
# Shouldn't CORS related headers be added here as well?
x-amazon-apigateway-gateway-responses:
  DEFAULT_4XX:
    responseTemplates:
      application/json: "{ ... }"
  DEFAULT_5XX:
    responseTemplates:
      application/json: "{ ... }"
[...]

paths:
  /api/example:
    get:
      responses:
        500:
          description: "500 response"
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Error"
        200:
          description: "200 response"
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Response"
      x-amazon-apigateway-integration:
        credentials: "arn:aws:iam:: ... :role/ApiGatewayRole"
        uri: "arn:aws:apigateway:eu-west-3:dynamodb:action/Query"
        responses:
          default:
            statusCode: "500"
            responseTemplates:
              application/json: "#set($root = $input.path('$')) { ... }"

            # I believe CORS headers are missing here
            # responseParameters:
            #   method.response.header.Access-Control-Allow-Methods: "'GET,OPTIONS'"
            #  method.response.header.Access-Control-Allow-Origin: "'*'"

          2\d{2}:
            statusCode: "200"
            responseTemplates:
              application/json: "#set($root = $input.path('$'))\n  { ... }"

            # I believe CORS headers are missing here
            # responseParameters:
            #   method.response.header.Access-Control-Allow-Methods: "'GET,OPTIONS'"
            #  method.response.header.Access-Control-Allow-Origin: "'*'"

        passthroughBehavior: "when_no_templates"
        httpMethod: "POST"
        requestTemplates:
          application/json: "#set($root = $input.path('$')) { [DynamoDB Query] }"
        type: "aws"

    options:
      responses:
        200:
          description: "200 response"
          headers:
            Access-Control-Allow-Origin:
              schema:
                type: "string"
            Access-Control-Allow-Methods:
              schema:
                type: "string"
          content: {}
      x-amazon-apigateway-integration:
        responses:
          default:
            statusCode: "200"
            responseParameters:
              method.response.header.Access-Control-Allow-Methods: "'GET,OPTIONS'"
              method.response.header.Access-Control-Allow-Origin: "'*'"
            responseTemplates:
              application/json: "{}\n"
        passthroughBehavior: "when_no_match"
        requestTemplates:
          application/json: "{\n  \"statusCode\" : 200\n}\n"
        type: "mock"

[...]

If the scope of this PR is not to fix this, let me know and I will open an issue please.

Thank you very much in advance,
Jose

ShreyaGangishetty · 2019-12-12T20:01:18Z

@JoseExposito Thank you for collecting the issues with CORS. The given example helped a lot.
Is the header Access-Control-Allow-Origin in GET mandatory for enabling CORS? As far as I know, adding this header to OPTIONS will setup CORS. I followed apigateway documentation for reference. Please let me know if I am missing anything here, I can create another issue for the missing pieces.

This PR is to make the SAM generated swagger document compatible for openapi3. In this PR I have removed the produces and consumes properties of swagger generated by SAM for open api and nested the type for CORS headers under schema which is the followed convention for openapi3

Snippet for swagger:

swagger: "2.0"
info:
  version: "0.0.1"
  title: "AwsSamExample"
host: "xxxx"
basePath: "/Stage"
schemes:
- "https"
paths:
  /mypath:
    get:
      responses:
        200:
          description: "200 response"
      x-amazon-apigateway-integration:
        uri: "xxxx"
        responses:
          default:
            statusCode: "200"
        passthroughBehavior: "when_no_match"
        httpMethod: "POST"
        type: "aws_proxy"
    options:
      consumes:
      - "application/json"
      produces:
      - "application/json"
      responses:
        200:
          description: "200 response"
          headers:
            Access-Control-Allow-Origin:
              type: "string"
            Access-Control-Allow-Methods:
              type: "string"
      x-amazon-apigateway-integration:
        responses:
          default:
            statusCode: "200"
            responseParameters:
              method.response.header.Access-Control-Allow-Methods: "'GET,OPTIONS'"
              method.response.header.Access-Control-Allow-Origin: "'www.example.com'"
            responseTemplates:
              application/json: "{}\n"
        passthroughBehavior: "when_no_match"
        requestTemplates:
          application/json: "{\n  \"statusCode\" : 200\n}\n"
        type: "mock"

snippet for openapi3 which looks similar to the one generated by SAM:

openapi: "3.0.1"
info:
  title: "AwsSamExample"
  version: "0.0.1"
servers:
- url: "xxxx"
  variables:
    basePath:
      default: "/Stage"
paths:
  /mypath:
    get:
      responses:
        200:
          description: "200 response"
          content: {}
      x-amazon-apigateway-integration:
        uri: "xxxx"
        responses:
          default:
            statusCode: "200"
        passthroughBehavior: "when_no_match"
        httpMethod: "POST"
        type: "aws_proxy"
    options:
      responses:
        200:
          description: "200 response"
          headers:
            Access-Control-Allow-Origin:  ## these headers are added
              schema:
                type: "string"
            Access-Control-Allow-Methods:
              schema:
                type: "string"
          content: {}
      x-amazon-apigateway-integration:
        responses:
          default:
            statusCode: "200"
            responseParameters:
              method.response.header.Access-Control-Allow-Methods: "'GET,OPTIONS'" ## these headers are added
              method.response.header.Access-Control-Allow-Origin: "'www.example.com'"
            responseTemplates:
              application/json: "{}\n"
        passthroughBehavior: "when_no_match"
        requestTemplates:
          application/json: "{\n  \"statusCode\" : 200\n}\n"
        type: "mock"
components: {}

It is a really cool feature to add x-amazon-apigateway-gateway-responses for CORS. Please create another issue for this feature request or +1 the existing issue #1178. Also, updating the code by following this #1178 (comment) will add x-amazon-apigateway-gateway-responses support in SAM. For any other feature requests related to CORS please create a seperate issue so we can track it and prioritize it.

ShreyaGangishetty · 2019-12-13T17:51:17Z

samtranslator/model/api/api_generator.py

+                            # add schema for the headers in options section for openapi3
+                            if field in ["responses"]:
+                                options_path = definition_body["paths"][path]["options"]
+                                if options_path and options_path.get(field).get('200') and options_path.get(field).\


Added this line to avoid throwing 5xx errors

fix: remove unsupported options for openapi3.0

3238be0

praneetap reviewed Dec 11, 2019

View reviewed changes

fix: refactor produces and consumes and modify headers

7490473

ShreyaGangishetty commented Dec 11, 2019

View reviewed changes

ShreyaGangishetty requested review from keetonian and praneetap December 11, 2019 20:36

keetonian reviewed Dec 11, 2019

View reviewed changes

fix: add comments

cc90145

keetonian approved these changes Dec 11, 2019

View reviewed changes

praneetap approved these changes Dec 12, 2019

View reviewed changes

fix: add checks for headers

c9b56a5

ShreyaGangishetty commented Dec 13, 2019

View reviewed changes

ShreyaGangishetty merged commit 9b791fe into aws:develop Dec 13, 2019

ShreyaGangishetty mentioned this pull request Dec 17, 2019

V1.20.0 Release Tracker #1212

Closed

Conversation

ShreyaGangishetty commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keetonian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keetonian commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShreyaGangishetty commented Dec 11, 2019

Uh oh!

JoseExposito commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShreyaGangishetty commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ShreyaGangishetty commented Dec 11, 2019 •

edited

Loading

codecov-io commented Dec 11, 2019 •

edited

Loading

keetonian commented Dec 11, 2019 •

edited

Loading

JoseExposito commented Dec 12, 2019 •

edited

Loading