Convert existing CORS module into a policy #487
Conversation
The functionality is exactly the same.
apicast/src/policy/cors.lua
Outdated
|
|
||
| if not origin then return end | ||
|
|
||
| ngx.header['Access-Control-Allow-Headers'] = ngx.var.http_access_control_request_headers |
There was a problem hiding this comment.
I'm not sure about these 2. Should we return the headers and methods that apicast supports? Or should we just accept everything (like we do now) because we don't know that the upstream api accepts?
There was a problem hiding this comment.
I think this module should accept a configuration that would whitelist what HTTP methods / Headers are supported. And possibly what origins too and if to pass credentials or not.
But until we have that configuration we can't do much other than just allow all. And even with the configuration there should be a way to configure it as accept all that browser requests.
There was a problem hiding this comment.
Makes sense. We can use the "configuration" field of a policy to pass this information for now: https://github.com/3scale/apicast/blob/master/apicast/src/configuration.lua#L167
There was a problem hiding this comment.
Yep. That sounds good! Forgot it is there :)
davidor
changed the title
davidor
force-pushed
the
cors-policy
branch
4 times, most recently
from
5e75c0b to
768ad17
Compare
davidor
changed the title
apicast/src/policy/cors.lua
Outdated
| end | ||
|
|
||
| local function set_access_control_allow_headers(allow_headers) | ||
| local value = allow_headers and concat(allow_headers, ',') |
There was a problem hiding this comment.
I think it should be possible to assign a table to the header and nginx should take care of the serialization. That could simplify the code a bit.
There was a problem hiding this comment.
You're right. I didn't know that.
Changed.
apicast/src/policy/cors.lua
Outdated
|
|
||
| local concat = table.concat | ||
|
|
||
| local policy = require('policy') |
There was a problem hiding this comment.
This will produce a deprecation warning once #486 is merged. No action needed here, but that PR is going to have to fix this.
Closes #484
This PR simply converts the existing CORS module into a policy without changing its behavior.
I've also added integration tests based on the new
TestAPIcastBlackboxmodule.@mikz I have some questions about the current behavior of this CORS module. Please check my comments and let me know what you think.