Add support for HttpApi cors configuration

[qswinson]

Description

Added support for HttpApi CORS configuration from the serverless.yml file. This supports serverless CORS settings (documentation found here: https://www.serverless.com/framework/docs/providers/aws/events/http-api#cors-setup). Either the default values will be provided if cors: true is set or the access control headers that are specified in the serverless.yml file will be used.

Motivation and Context

The current code base only supports passing in additional CLI arguments to support CORS. With the addition of HttpApi from AWS Api Gateway and its built in support for CORS, the new settings should be used from the serverless.yml file instead.

How Has This Been Tested?

I have added integration tests for both the default cors: true setting and integration tests for custom access control settings.

Screenshots (if appropriate):

[pgrzesik]

Thanks @qswinson - looks good overall, it seems to have a small conflict with current master though - do you think you could rebase your changes on top of current master so we could merge it? Thanks in advance 🙇

[bryanvaz]

Was just about to submit the PR for a similar CORS issue (#1213), when I found this. Very elegant solution, however is does not address the scenarios in #1213 - CORS headers getting applied when they shouldn't.

This is because the CORS headers are always being applied regardless of whether the cors options is defined in serverless-offline.

@qswinson I'll try and merge my changes into yours tonight, but I may need your help to confirm that my changes do not break the functionality you are trying to create.

Cheers,
Bryan

[bryanvaz]

@qswinson PR #1215 fixes headers when the cors option is not set, and PR #1215 includes your changes from this PR.

All of your new Jest tests from this PR still work in #1215, but if you have the time, can you try out the changes in PR #1215 (bryanvaz/serverless-offline#http-api-cors) and make sure the functionality you expect is still there.

Cheers,
Bryan

[qswinson]

@bryanvaz My concern with the way you fixed #1213 breaks existing functionality for current users.

If you have a REST API from Api Gateway the only way to specify CORS options is on the function level. Since there is no explicit route or backing function for these OPTIONS requests, work arounds had to be made to enable the correct CORS response. This is currently done with the cors... command options. The problem you are facing is that the default options are configured to always return CORS headers.

If you remove the ability to use these settings, then people using REST APIs or just a single HTTP API with a CORS definition will have no way to proceed until someone else solves the function level CORS issue.

[bryanvaz]

Hi @qswinson,

That was my initial concern as well, till I realized the HttpServer class was duplicated and reimplemented for each type of event. While from this breaks slightly from the old school, polymorphism, approach to coding, in this case, it was actually a benefit in this scenario because that means the code for the HttpServer class for httpApi events is not shared with any other event type (REST or Websockets) 👍 🥳 . Each of those event types have their own HttpServer class code that they use.

I laughed pretty hard when I realized each event type uses separate HttpServer code; even harder when I realized the CORS issue was because someone had just copied and pasted the entire HttpServer code from the REST API and never checked the behaviour against the actual AWS implementation; and finally laughing on the floor, to the point where my cat looked at me as if I was crazy, when I realized that fixing the CORS issue would be as easy as commenting out those legacy lines which should have never been in the file in the first place.

This also means, that the only scenario we have to be concerned with is whether cors functionality for httpApi events is impacted, as I've added new tests to ensure the headers (or lack of headers) matches what is observed from the AWS HTTP API. From my testing, it seemed that when you specify the cors options at either the provider or function level, the headers are added back in (thanks to your elegant code.)

However I'd hate to have accidentally broken your intended functionality without realizing it. Since you're obviously a heavy user of the cors functionality, I'd feel a lot more confident that the patch is non-breaking if you could run it through any scenarios that you have (and aren't already tested for by the new Jest tests that you created.)

Cheers,
Bryan

(The cats are now napping because they got mackerel for breakfast and couldn't be bothered what I'm up to)

[qswinson]

Can you provide a link to where the httpApi (HTTP API) and http (REST API) events have different implementations of the HttpServer, please? I've searched the code and run tests and I just don't think that is the case.

[bryanvaz]

Hi @qswinson ,

Websocket is https://github.com/dherault/serverless-offline/blob/master/src/events/websocket/HttpServer.js
HttpApi is https://github.com/dherault/serverless-offline/blob/master/src/events/http/HttpServer.js
Rest should be https://github.com/dherault/serverless-offline/blob/master/src/lambda/HttpServer.js

If I got that super wrong, let me know (that's why I need that second pair of eyes). If so, I'll have to move the the REST auto CORS logic to the createRoutes block which can then use the event.http.isHttpApi flag to disable the functionality, which means #1175 needs to be further rebased.

Regardless @pgrzesik, I think you should probably merge this PR; that way we are not trying to balance 3 pending PRs on the same file.

Cheers,
Bryan

[pgrzesik]

Thank you @qswinson - it looks really good 👍