Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

let the path /apisix/admin support the CORS #982

Merged
merged 2 commits into from Dec 24, 2019
Merged

let the path /apisix/admin support the CORS #982

merged 2 commits into from Dec 24, 2019

Conversation

Miss-you
Copy link
Member

…ORS.

NOTE: Please read the Contributing.md guidelines before submitting your patch:

https://github.com/apache/incubator-apisix/blob/master/Contributing.md#how-to-add-a-new-feature-or-change-an-existing-one

Summary

fix bin/apisix, let the API /apisix/admin support the CORS

Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, but still need @membphis's check.

@lilien1010
Copy link
Member

lilien1010 commented Dec 19, 2019

compare to make /apisix/admin CORS enabled in a hardcode-way,

I think we can move those code into apisix.http_admin(),which I think It will make things more elegant

@lilien1010
Copy link
Member

here is my suggestion for the admin api CORS feature.

1、set a enable_cors switch in config.yaml just like admin_port
2、move those CORS hardcode into function apisix.http_admin()

@Miss-you
Copy link
Member Author

I think it is necessary to add a enable_admin_cors switch.
However, moving the code from nginx.conf to http_admin is still hard-coded and does not solve the problem you said, right? Should we add another admin API CORS configuration?

@moonming
Copy link
Member

@Miss-you Travis CI is broken.

lua/apisix.lua Outdated
@@ -446,6 +446,29 @@ function _M.http_balancer_phase()
load_balancer(api_ctx.matched_route, api_ctx)
end

local function cors_admin()
local local_conf = core.config.local_conf()
if local_conf.apisix or not local_conf.apisix.enable_admin_cors then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be always true, you should add test case for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this problem during my self-test, but forgot to submit the code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_M.init_worker() in lua/apisix/admin/init.lua also has the same problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_M.init_worker() in lua/apisix/admin/init.lua also has the same problem.

welcome create another PR to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

1、set a enable_cors switch in config.yaml just like admin_port
2、move those CORS hardcode into function apisix.http_admin()
@moonming moonming merged commit eff1ca7 into apache:master Dec 24, 2019
@moonming
Copy link
Member

merged, thx @Miss-you

SaberMaster pushed a commit to SaberMaster/incubator-apisix that referenced this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants