Enable CORS configuration for APIGW #1703

patoncrispy · 2016-07-28T23:06:23Z

PR for Issue #1596

Status:

In Development

Description:

I've added the origin headers to the methods that require it and the tests pass for those. Still need to add the preflight (OPTIONS) method as required. I was thinking that I should probably set this at the Resource level, as the preflight request needs to be able to assign the allow-methods setting, so will need to have a global view of all the methods that have CORS configurations applied. Feedback VERY welcome at this point! :)

Todos:

Add preflight.
Write tests for preflight.

pmuens · 2016-07-29T04:43:11Z

Really great work @patoncrispy! Thanks a lot!
Looking forward to get this into v1.0!

Could you please rebase your branch? We've made some changes in the past which seems to cause some conflicts.

@eahefnawy could you review this?

patoncrispy · 2016-07-29T06:59:03Z

Thanks @pmuens! However, it's not quite finished. I need to add the preflight (OPTIONS) request so that the allow-headers and allow-methods headers can be applied. Without this, CORS won't work properly. Also, I haven't done a proper test using a compiled binary, so still not 100% sure it works. I was planning on getting this done over the weekend.

pmuens · 2016-07-29T08:26:34Z

Awesome @patoncrispy! No hurry. Great to have you as a contributor!

codepreneur · 2016-07-29T11:43:24Z

For those of you who want to do this manually while this is being implemented, do this:

enable CORS via the AWS console
like so:

and then create a new deployment aws apigateway create-deployment --rest-api-id $api_id --stage-name $stage how to create deployment

--stage-name should be the same name as created by serverless and you can get --rest-api-id by typing aws apigateway get-rest-apis in your command line

flomotlik · 2016-08-01T09:39:52Z

@patoncrispy could you give us a quick update on the status here? We would love to include this in the next release this week and would jump onto it in case you don't have enough time for it in the next days. Thanks for the contribution so far!

patoncrispy · 2016-08-01T10:49:32Z

Hey @flomotlik! I've just about finished. I've got the preflight stuff done, just need to write some tests for it and then actually build the binary and make sure it works as expected. In a few hours I'll update my pull request so you can see it. I was really hoping to get it finished last night but weekend ended up being busy. :( When do you want to do the next release?

flomotlik · 2016-08-01T14:39:42Z

@patoncrispy awesome job. We're actually planning to release on Wednesday, so would like to merge tomorrow. Do you think this would work out? Happy to have someone from our team help out as well to make sure we can get this out the door by tomorrow.

patoncrispy · 2016-08-01T14:41:39Z

I think I'm just about good to go, but it's all theory until I can actually run it. I'm not sure how to use the binary in ./bin to test out what I've done. It can't see my global AWS credentials. Any tips on that?

flomotlik · 2016-08-01T14:44:13Z

@patoncrispy you mean deploying serverless with the CORS integration, or what exactly would you like to test? I'm reaching out via email as well so we can talk directly.

flomotlik · 2016-08-01T17:03:44Z

lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js

+            if (typeof event.http.cors === 'object') {
+              // TODO - Some validation of cors object...
+              cors = event.http.cors;
+              cors.methods = cors.methods.map((m) => (m.toUpperCase()));


Got an exception here:

TypeError: cors.methods.map is not a function
at /app/lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js:45:43
at Array.forEach (native)
at /app/lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js:11:29
at /app/node_modules/lodash/lodash.js:4411:15
at baseForOwn (/app/node_modules/lodash/lodash.js:2654:24)
at /app/node_modules/lodash/lodash.js:4380:18
at Function.forEach (/app/node_modules/lodash/lodash.js:8634:14)
at AwsCompileApigEvents.compileMethods (/app/lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js:10:7)
at AwsCompileApigEvents.tryCatcher (/app/node_modules/bluebird/js/release/util.js:16:23)
at Promise._settlePromiseFromHandler (/app/node_modules/bluebird/js/release/promise.js:504:31)
at Promise._settlePromise (/app/node_modules/bluebird/js/release/promise.js:561:18)
at Promise._settlePromise0 (/app/node_modules/bluebird/js/release/promise.js:606:10)
at Promise._settlePromises (/app/node_modules/bluebird/js/release/promise.js:685:18)
at Async._drainQueue (/app/node_modules/bluebird/js/release/async.js:138:16)
at Async._drainQueues (/app/node_modules/bluebird/js/release/async.js:148:10)
at Immediate.Async.drainQueues as _onImmediate

flomotlik · 2016-08-01T17:05:34Z

@patoncrispy tried to get it running, but it looks like there was an exception (added it).

regarding the AWS keys do you have a profile configured in your aws credentials? if you can run something like aws s3 ls the serverless should be able to do it as well.

One thing that is currently also missing are the docs, but if you don't have time for it we can take care of this. Thanks again for your time on this

patoncrispy · 2016-08-01T19:15:20Z

@flomotlik I've updated the docs and validation. I'm home on my Mac (work on Windows) so hopefully will have a better experience with testing. :)

patoncrispy · 2016-08-01T22:33:04Z

@flomotlik It all worked great on my Mac! :) Had one or two issues that I have now resolved and it all works nicely. Keen for someone to inspect it though. It's pretty late and I'm pretty wrecked. Will be around tomorrow, too.

flomotlik · 2016-08-02T08:24:22Z

@patoncrispy which commands did you use to test this? I've tried testing it with curl

curl -H "Origin: http://example.com" \
  -H "Access-Control-Request-Method: GET" \
  -H "Access-Control-Request-Headers: X-Requested-With" \
  -X OPTIONS --verbose \
  https://n0vbhzc8md.execute-api.us-east-1.amazonaws.com/dev/user/create

but I consistently get {"message": "Internal server error"}

Could you add the test commands you've used to test this?

patoncrispy · 2016-08-02T10:30:43Z

@flomotlik Erm, that's exactly what I ran. Unfortunately, I was too tired to notice the status code! It was because I hadn't added the Integration RequestTemplate, so that is now in there. This worked for me:

curl -H "Origin: http://example.com" -H "Access-Control-Request-Methods: GET" \
-H "Access-Control-Request-Headers: Content-T ype" -X OPTIONS \
--verbose https://77y6hjigof.execute-api.eu-west-1.amazonaws.com/dev/files/find

flomotlik · 2016-08-02T17:52:27Z

@patoncrispy I tried it today a few times as well and I can deploy it once, but when I update the endpoint (e.g update the method of the api endpoint or the headers or the allowed origins) it is always returning the same cached response. It is updated in API Gateway though, so the Mock itself is properly configured, its just cached somewhere along the lines. Tried different things from setting headers to other stuff but couldn't get out of the caching problem. Could you try this as well, deploy an endpoint, update something and run it again to check if you're actually getting an updated response or not?

Works totally fine after removing and deploying again. Here are the function descriptions I used:

functions:
  hello:
    handler: handler.hello
    events:
    - http:
       path: user/createsomething
       method: get
       cors:
         origins:
           - 'https://example.com'

functions:
  hello:
    handler: handler.hello
    events:
    - http:
       path: user/createsomething
       method: POST
       cors:
         origins:
           - 'https://example.com'
           - 'https://example2.com'

flomotlik · 2016-08-02T17:52:59Z

lib/plugins/aws/deploy/compile/events/apiGateway/README.md

+```yml
+functions:
+  hello:
+  events:


the function needs the handler config to be set: handler: handler.hello

patoncrispy · 2016-08-02T20:49:34Z

@flomotlik Ah weird. Looking into it now. Will let you know how I get on. Getting there though! :)

WIll update docs too. :)

patoncrispy · 2016-08-02T23:13:29Z

Hey @flomotlik! I've been looking into it and it seems that when I run serverless deploy, it's only pushing my configuration up (I can see the different Integration Response headers in the OPTIONS method if I change the endpoint from GET to POST). The deployment to the specified stage does not happen. I can manually deploy the changes I make after running serverless deploy and it works fine. This happens with both with the CORS setting and without. I updated the readme in my last commit, but it seems there is another test failing (I'm not sure why). I'll try and take another look at it tomorrow. :(

patoncrispy · 2016-08-04T11:15:16Z

@flomotlik It seems like it's all hands on deck, but if I can help in anyway, please let me know!

eahefnawy · 2016-08-04T18:19:03Z

@patoncrispy you've put a lot of effort into this! 👍 Thanks a lot for your contribution ☺️ 😊

patoncrispy · 2016-08-04T18:22:32Z

Thanks @eahefnawy! Gotta find another issue to work on now. :)

Also, any advice or criticism would be greatly appreciated!

eahefnawy · 2016-08-09T10:19:51Z

lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js

+          const corsMethodResponseTemplate = `${allowOrigin} : ${allowOrigin}`;
+          const corsMethodIntegrationTemplate =
+          corsEnabled ? `${allowOrigin}: "'${cors.origins.join(',')}'"` : '';
+


Seems like this code snippet runs whether there's cors configured or not. So even for endpoints that doesn't have cors enabled they'd have this ResponseParameters:

"method.response.header.Access-Control-Allow-Origin" : "method.response.header.Access-Control-Allow-Origin"

Maybe I'm missing something, but this looks a bit hacky. If no cors are configured, this header shouldn't exist in the response right?

eahefnawy · 2016-08-09T11:04:33Z

@patoncrispy I played with your branch today and it seems to be working great! Thank a lot ☺️ ... the only issue of course is removing endpoints/methods from the stage page in the console. But this is an overall AWS issue that we're trying to get some answers around, and has nothing to do with this PR.

I have just a tiny comment/question above about setting the response header. But other than that I think this PR is G2M 💥

pmuens · 2016-08-09T11:36:29Z

lib/plugins/aws/deploy/compile/events/apiGateway/README.md

+```yml
+functions:
+  hello:
+  handler: handler.hello


Could you nest the handler inside the function hello?

pmuens · 2016-08-09T11:41:17Z

@patoncrispy thanks for this PR. Really love it that we soon have CORS support thanks to you 💃 !

Added a few comments. Other than that it's GTM from my side

patoncrispy · 2016-08-09T11:46:55Z

@eahefnawy @pmuens Argh! Amateur hour. 😞 I'll get these tidied up this evening and push 'em. Clearly couldn't find my fine-toothed comb that day! :)

Updated readme Fixed RequestTemplates issue. Added statusCode response to Options Request template. Added X-Amz-Security-Token header and updated tests, too. Manual test works. Updated unit tests, too. Added documentation and some validation. Updated tests to check each header. All passing. Added preflight config. Need to write tests. Adds origin header to method. Tests pass. Need preflight added, too. Setup basic cors assignement. Experimenting with configuration setup.

patoncrispy · 2016-08-09T21:29:45Z

@pmuens @eahefnawy I've updated stuff! Would you mind taking a look and seeing if things are OK now? On the readme changes, I initially just copied one of the other configuration examples to keep with the readme style, so it may be worth checking that some of the other examples are as desired.

pmuens · 2016-08-10T05:03:41Z

lib/plugins/aws/deploy/compile/events/apiGateway/README.md

+    handler: handler.hello
+    events:
+      - http:
+        path: user/create


This needs another 2 spaces for indentation.
YAML is nasty sometimes (I faced the exact same problem recently) and IMHO I would like this syntax more than adding 2 more spaces.

Here's an example of the difference between the two different indentations.

- http: path: users/create method: get

translates to:

[ { "http": null, "path": "users/create", "method": "get" } ]

Whereas

- http: path: users/create method: get

translates to

[ { "http": { "path": "users/create", "method": "get" } } ]

pmuens · 2016-08-10T05:07:05Z

Hey @patoncrispy thank you so much for your hard work!
You really deliver great value to our community!

I've just added two comments regarding YAML indentation (sorry for being that pedantic...).

I'll pull your PR and test it the upcoming hours. Will report back once I'm ready.
However reading over it I'm excited and happy to play with it!

Great work 🎉

pmuens · 2016-08-10T06:09:30Z

@patoncrispy just noticed that it's planned to merge this PR into the v1.0.

We're now working directly on master so it would be awesome if you could open a PR which points to the master branch once you're done. Let me know if you need help there!

pmuens · 2016-08-10T06:23:57Z

lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js

          let path;
+          let corsEnabled = false;
+
+          const headers = [


Can you move this const headers and the let cors below into the if (!!event.http.cors) section below so that the variables are only created when CORS is enabled?

pmuens · 2016-08-10T06:31:13Z

@patoncrispy Just tested it locally.
Works like magic! Really awesome stuff you've done here! 👍

Just added a few more comments. If those are resolved and @flomotlik and @eahefnawy also took a look it's GTM (IMO) once opened for the master branch.

pmuens · 2016-08-10T06:32:18Z

lib/plugins/aws/deploy/compile/events/apiGateway/README.md

+    handler: handler.hello
+    events:
+      - http:
+        path: user/create


Same indentation fix here.

patoncrispy · 2016-08-10T08:22:32Z

@pmuens, to create the pull request on master, should I just force pull the latest serverless/master branch and then rebase my feature branch onto that and then create the pull request, or is there a smarter way of doing this?

pmuens · 2016-08-10T11:45:34Z

Yes, rebasing this branch on top of the master branch is the cleanest solution IMHO.

But I'm not sure if this won't introduce other commits from the history as well (I believe we had a similar issue recently where some PRs needed to be reopenend so that we can get rid of those creeped in commits).

Maybe @flomotlik can share his thought about this in short here as he's identified the problem recently.

flomotlik · 2016-08-10T13:18:10Z

@patoncrispy @pmuens rebase or merge master into this branch are both fine with me. Rebases and force pushes sometimes lead to issues with Github

patoncrispy · 2016-08-10T19:45:41Z

@pmuens @flomotlik OK. I've rebased onto latest master and it all seems OK. However, if I push this branch it will update this PR. Should I just close this PR and open a new one? What's the best way to proceed here? Thanks! 😃

pmuens · 2016-08-11T06:06:24Z

Great @patoncrispy! Yes, the best would be to open up a new one which points to the master branch rather than the v1.0 branch. 👍

pmuens added the in-progress label Aug 1, 2016

flomotlik added this to the v1.0.0-beta.1 milestone Aug 1, 2016

patoncrispy force-pushed the feature/enable-cors branch 3 times, most recently from baa026d to 51bf3f9 Compare August 1, 2016 14:17

flomotlik reviewed Aug 1, 2016
View reviewed changes

patoncrispy force-pushed the feature/enable-cors branch from 51bf3f9 to dbf257b Compare August 1, 2016 18:06

pmuens added the area/events label Aug 1, 2016

patoncrispy force-pushed the feature/enable-cors branch from dbf257b to 89a5cfc Compare August 1, 2016 22:31

pmuens mentioned this pull request Aug 2, 2016

Add support for CORS in APIG #1596

Closed

patoncrispy force-pushed the feature/enable-cors branch from 89a5cfc to c5a9592 Compare August 2, 2016 10:26

flomotlik reviewed Aug 2, 2016
View reviewed changes

patoncrispy force-pushed the feature/enable-cors branch from c5a9592 to 3f8fb4e Compare August 2, 2016 22:46

eahefnawy reviewed Aug 9, 2016
View reviewed changes

nicka mentioned this pull request Aug 9, 2016

Support caching for APIG #1597

Closed

pmuens reviewed Aug 9, 2016
View reviewed changes

patoncrispy force-pushed the feature/enable-cors branch from 3f8fb4e to a5587c2 Compare August 9, 2016 21:15

pmuens reviewed Aug 10, 2016
View reviewed changes

patoncrispy closed this Aug 11, 2016

patoncrispy mentioned this pull request Aug 11, 2016

Add CORS support to APIGW #1820

Merged

Enable CORS configuration for APIGW #1703

Enable CORS configuration for APIGW #1703

Uh oh!

Conversation

patoncrispy commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status:

Description:

Todos:

Uh oh!

pmuens commented Jul 29, 2016

Uh oh!

patoncrispy commented Jul 29, 2016

Uh oh!

pmuens commented Jul 29, 2016

Uh oh!

codepreneur commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flomotlik commented Aug 1, 2016

Uh oh!

patoncrispy commented Aug 1, 2016

Uh oh!

flomotlik commented Aug 1, 2016

Uh oh!

patoncrispy commented Aug 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flomotlik commented Aug 1, 2016

Uh oh!

flomotlik Aug 1, 2016

Choose a reason for hiding this comment

Uh oh!

flomotlik commented Aug 1, 2016

Uh oh!

patoncrispy commented Aug 1, 2016

Uh oh!

patoncrispy commented Aug 1, 2016

Uh oh!

flomotlik commented Aug 2, 2016

Uh oh!

patoncrispy commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flomotlik commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flomotlik Aug 2, 2016

Choose a reason for hiding this comment

Uh oh!

patoncrispy commented Aug 2, 2016

Uh oh!

patoncrispy commented Aug 2, 2016

Uh oh!

patoncrispy commented Aug 4, 2016

Uh oh!

eahefnawy commented Aug 4, 2016

Uh oh!

patoncrispy commented Aug 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eahefnawy Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

pmuens Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

eahefnawy commented Aug 9, 2016

Uh oh!

pmuens Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmuens commented Aug 9, 2016

Uh oh!

patoncrispy commented Aug 9, 2016

Uh oh!

patoncrispy commented Aug 9, 2016

Uh oh!

pmuens Aug 10, 2016

patoncrispy commented Jul 28, 2016 •

edited

Loading

codepreneur commented Jul 29, 2016 •

edited

Loading

patoncrispy commented Aug 1, 2016 •

edited

Loading

patoncrispy commented Aug 2, 2016 •

edited

Loading

flomotlik commented Aug 2, 2016 •

edited

Loading

patoncrispy commented Aug 4, 2016 •

edited

Loading

pmuens Aug 9, 2016 •

edited

Loading

pmuens Aug 10, 2016 •

edited

Loading

pmuens Aug 10, 2016 •

edited

Loading