Open doapp-ryanp opened 8 years ago
Hi @doapp-ryanp! There is similar issue here: https://github.com/joostfarla/serverless-cors-plugin/issues/6
To reopen this discussion, I'll repeat one of my comments:
This is done intentionally because multiple functions with different HTTP methods can share the same path. The question I was struggling with was: If one of x functions with the same path is being deployed, should it deploy the preflight endpoint for all methods?
What do you think?
Thanks for the quick response.
I've read https://github.com/joostfarla/serverless-cors-plugin/issues/6 and I'm not quite sure I understand the issue (it is early in the am tho ;)
An endpoint can have one or more method (where methods are GET,POST,PUT, OPTIONS etc). So OPTIONS
are at the same level under and endpoint as other methods.
Here is how I would attack the problem. Disclaimer I'm not sure if the serverless plugin system allows hooks into these extension points...
If I define custom.cors
in my s-function.json
I would expect all endpoints
in the s-function.json
(except for OPTIONS
method
s) to create the CORS OPTIONS
method in APIG when I deploy the endpoint.
If multiple functions have endpoints
with the same path
that is ok - deploying the OPTIONS
method under that endpoint again will not hurt anything. You could write logic to check to see if an OPTIONS
method already exists for said endpoint, but might not be worth it (because someone may be updating their OPTIONS
config via your custom.cors
attrs (which are nice btw).
custom.cors
in my s-project.json
anytime I do sls endpoint deploy
I would expect your plugin to create an OPTIONS
method in APIG.To summarize: 2 functions can define the same endpoints.path
and even the same function can have 2 endpoints
with the same path
. But because OPTIONS
is just a method under the endpoint AND because options uses a MOCK
integration there is no harm in always creating the OPTIONS
method in APIG.
I rarely want to deploy all endpoints. I (and I think most users) only want to deploy a specific endpoint(s) they are working on and not touch any other endpoints. For ex: If I already have a users~GET
and I'm add a users~DELETE
I do not want to re-deploy users~GET
just to get my OPTIONS
method created for users~DELETE
. If something is working in production, I don't to touch it...
Does that make sense or am I missing something?
I agree with @doapp-ryanp that if it is possible (and I might not understand all the edge cases) that I would like to be able to only deploy the OPTIONS endpoint defined in the function for which the endpoint exists.
So I think we found a more straightforward way to handle CORS with just using serverless templates. Using the strategy below eliminates need for this plugin and npm module, and it allows tighter integration with other serverless tools (like it shows up in the dash) as well as more fine grain control.
s-templates.json
:
"apiDefault200Response": {
"statusCode": "200",
"responseParameters": {
"method.response.header.Access-Control-Allow-Origin": "'*'"
},
"responseModels": {},
"responseTemplates": {
"application/json": ""
}
},
"api400ErrorMatchResponse": {
"statusCode": "400",
"selectionPattern": "MlnError.*|Task timed out.*",
"responseTemplates": {
"application/json": ""
},
"responseParameters": {
"method.response.header.Access-Control-Allow-Origin": "'*'"
}
},
"apiCorsOptionsResponse": {
"default": {
"statusCode": "200",
"responseParameters": {
"method.response.header.Access-Control-Allow-Headers": "'Content-Type,X-Amz-Date,Authorization,X-Api-Key'",
"method.response.header.Access-Control-Allow-Methods": "'GET,OPTIONS,HEAD,DELETE,PATCH,POST,PUT'",
"method.response.header.Access-Control-Allow-Origin": "'*'",
"method.response.header.Access-Control-Max-Age": "'3600'"
},
"responseModels": {},
"responseTemplates": {
"application/json": ""
}
}
},
"apiCorsRequestTemplate": {
"application/json": {
"statusCode": 200
}
}
s-function.json
...
"endpoints": [
{
"path": "discover/wx/{version}/latlng",
"method": "GET",
"type": "AWS",
"authorizationType": "none",
"authorizerFunction": false,
"apiKeyRequired": false,
"requestParameters": {
"integration.request.querystring.lat": "method.request.querystring.lat",
"integration.request.querystring.lng": "method.request.querystring.lng"
},
"requestTemplates": "$${apiRequestTemplate}",
"responses": {
"400": "$${api400ErrorMatchResponse}",
"default": "$${apiDefault200Response}"
}
},
{
"path": "discover/wx/{version}/latlng",
"method": "OPTIONS",
"type": "MOCK",
"requestTemplates": "$${apiCorsRequestTemplate}",
"responses": "$${apiCorsOptionsResponse}"
}
],
...
As you can see the OPTIONS
method is super straight forward. If you want your endpoint to have OPTIONS
just add the path
and 4 attributes.
The only semi-tricky thing is the other methods (GET
in this example) must include "method.response.header.Access-Control-Allow-Origin": "'*'"
in their responseParameters
. You can see we put this in our response templates so they get auto-inherited in all endpoints that wish to use them.
Lastly, if you need Access-Control-Allow-Credentials
this must also be defined in your OPTIONS
and other methods. It is just not pictured here.
This method doesn't dynamically set the Access-Control-Allow-Methods
based on the other methods under your endpoint, however having methods in the Access-Control-Allow-Methods
that you do not implement does not have a negative impact.
@doapp-ryanp Thank you sooooo much. I got confused by the existence of this plugin - but really love your solution. No plugin and fine grain control at the place where it belongs.
I suggest the Readme of this Plugin should point your solution out in HUGE letters ! It is too easy to fall into the trap of using to many unnecessary plugins.
Of course you're free to implement CORS manually using templates, but I do think the plugin has added value:
method.response.header.Access-Control-Allow-Methods
in syncThe problem however is the current plugin architecture of the SLS framework. It's missing essential extension points to achieve a tighter integration with the framework. What we need are more strategic extension points, like for example a postProjectLoad
action/hook. Any change done here would have the desirable effect on all the CLI tools, like dash
or any custom built action.
One other issue is that the plugin handles merging of the templates at a very late stage. In my opinion, this should be done instantly on project loading. That is the issue which is causing the unwanted warnings on deployment.
I will make an effort to solve the mentioned issues with the core team. Furthermore, I don't think having small convenience plugins is a bad thing, we're building Node.js apps, right? ;)
To get back to the original question, I do want to implement partial deployments. (thanx @doapp-ryanp for your thoughts!). Will work on it asap.
@doapp-ryanp how do you avoid your OPTIONS endpoints from executing the handler of the GET/POST/etc endpoint?
@arieljake I back them with "type": "MOCK"
APIG implementations. Check out my example from April 5 above.
Just created two issues (https://github.com/serverless/serverless/issues/1133 & https://github.com/serverless/serverless/issues/1134) which will potentially solve the mentioned issues and make the Serverless framework more extensible.
Just as a side note: It looks like serverless will be moving to a complete CloudFormation based solution without _meta folders and all these complications. That would mean that @doapp-ryanp way of managing CORS is definitely the more desirable way since it is a native part of CloudFormation without the need of another artificial process.
@BerndWessels I disagree! If the framework would offer extension points on project loading, this would still be included in the CF template and offer lots of convenience! See https://github.com/serverless/serverless/issues/1133
@joostfarla I appreciate your work a lot.
I just come from a different angle. In big enterprise companies with huge amounts of developers and processed around access rights , Git and PRs it becomes very difficult.
Not every developer will be allowed to change resources, use plugins and so on. Everything will be restricted and has to follow processes. There are further limitations on CI processes and limitations.
That is why the pure CloudFormation based approach seems much more flexible to put these complicated restrictions on top.
For smaller companies with only a handful of developers who all have full access to all resources the convenience that plugins offer is absolutely fine.
@BerndWessels I understand your position, but that will probably count for all plugins then ;)
How do you just deploy the
OPTIONS
method for one endpoint? I was hoping when I deployed myGET
method this plugin would detect that noOPTIONS
method was present for endpoint, and it would automatically create theOPTIONS
method.I have added the following to
s-project.json
I have also tried adding to
s-function.json
.the
OPTIONS
does not show up underserverless dash deploy
and it gives me an error when I try to runserverless endpoint deploy "discover/wx/{version}/latlng~OPTIONS"
saying method not found.Lastly I added an
OPTIONS
to mys-function.json
endpoints
. I was able to deploy the method, however looking at--debug
I don't see your plugin getting invoked and the integration request is not using aMOCK
any ideas whats going on here? serverless
v0.5.1
. Thanks