You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Qiuqiu0505 (via GitHub)" <gi...@apache.org> on 2023/03/07 12:02:02 UTC
[GitHub] [apisix] Qiuqiu0505 opened a new pull request, #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Qiuqiu0505 opened a new pull request, #9028:
URL: https://github.com/apache/apisix/pull/9028
### Description
Fixes # (9027)
-- If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only.
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1475512789
https://github.com/apache/apisix/actions/runs/4446285829/jobs/7839374950?pr=9028
```
# Failed test 't/plugin/cors2.t TEST 5: regex specified match - header Vary ok'
# at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1011.
# got: 'Via'
# expected: 'Via, Origin'
# Looks like you failed 1 test of 35.
[02:10:05] t/plugin/cors2.t ..
```
Please make the CI pass, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] KID-G commented on pull request #9028: fix(cors): consider using `allow_origins_by_regex` only when it is not `nil`
Posted by "KID-G (via GitHub)" <gi...@apache.org>.
KID-G commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1484743939
Hi,
Thank you for your contribution to Apache APISIX!
Can you tell me your email address? We have prepared electronic certificates for new contributors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Qiuqiu0505 commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1136464300
##########
apisix/plugins/cors.lua:
##########
@@ -297,13 +297,13 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be matched to it only
local allow_origins
- allow_origins = process_with_allow_origins(conf.allow_origins, ctx, req_origin)
- if not match_origins(req_origin, allow_origins) then
- allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+ allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
Review Comment:
This function named process_with_allow_origins_by_regex has checked whether allow_origins_by_regex is nil.
Does it need to be checked again before the method invocation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1131906247
##########
t/plugin/cors.t:
##########
@@ -927,3 +927,88 @@ Access-Control-Allow-Headers: headr1,headr2
Access-Control-Expose-Headers: ex-headr1,ex-headr2
Access-Control-Max-Age: 50
Access-Control-Allow-Credentials: true
+
+
+
+=== TEST 36: set route ( regex specified and allow_origins is default value )
Review Comment:
Let's add new tests to the other test file.
> When the test file is too large, for example > 800 lines, you should split it to a new file. Please take a look at t/plugin/limit-conn.t and t/plugin/limit-conn2.t.
https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#check-code-style-and-test-case-style
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] qihaiyan commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "qihaiyan (via GitHub)" <gi...@apache.org>.
qihaiyan commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1128071257
##########
apisix/plugins/cors.lua:
##########
@@ -297,12 +297,15 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
local allow_origins
- allow_origins = process_with_allow_origins(conf.allow_origins, ctx, req_origin)
- if not match_origins(req_origin, allow_origins) then
- allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+ allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+ if not match_origins(req_origin, allow_origins) and conf.allow_origins_by_regex == nil then
Review Comment:
With cors plugin's default config, when a request doesn't have a origin header, the match_origins method will return true, then the following process_with_allow_origins method won't be executed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1138019353
##########
apisix/plugins/cors.lua:
##########
@@ -297,13 +297,13 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be matched to it only
local allow_origins
- allow_origins = process_with_allow_origins(conf.allow_origins, ctx, req_origin)
- if not match_origins(req_origin, allow_origins) then
- allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+ allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
Review Comment:
Yes. We can move it out, and avoid the repeated `and conf.allow_origins_by_regex == nil` check below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1134868440
##########
apisix/plugins/cors.lua:
##########
@@ -297,13 +297,13 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be matched to it only
local allow_origins
- allow_origins = process_with_allow_origins(conf.allow_origins, ctx, req_origin)
- if not match_origins(req_origin, allow_origins) then
- allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+ allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
Review Comment:
Look like we can skip this function call by checking the `conf.allow_origins_by_regex == nil` first. For example,
```
if conf.allow_origins_by_regex ~= nil then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] chenjinfeng0505 commented on pull request #9028: fix(cors): consider using `allow_origins_by_regex` only when it is not `nil`
Posted by "chenjinfeng0505 (via GitHub)" <gi...@apache.org>.
chenjinfeng0505 commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1484767503
@KID-G Hi, my email address is chenjinfeng4@hisense.com.
I am so lucky to receive certificate. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Qiuqiu0505 commented on pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1475540007
> https://github.com/apache/apisix/actions/runs/4446285829/jobs/7839374950?pr=9028
>
> ```
> # Failed test 't/plugin/cors2.t TEST 5: regex specified match - header Vary ok'
> # at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1011.
> # got: 'Via'
> # expected: 'Via, Origin'
> # Looks like you failed 1 test of 35.
> [02:10:05] t/plugin/cors2.t ..
> ```
>
> Please make the CI pass, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Qiuqiu0505 closed pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 closed pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
URL: https://github.com/apache/apisix/pull/9028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Qiuqiu0505 commented on pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1475541784
@spacewander I have just updated it. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1473033675
Let's merge master to make CI pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang merged pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang merged PR #9028:
URL: https://github.com/apache/apisix/pull/9028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1131906247
##########
t/plugin/cors.t:
##########
@@ -927,3 +927,88 @@ Access-Control-Allow-Headers: headr1,headr2
Access-Control-Expose-Headers: ex-headr1,ex-headr2
Access-Control-Max-Age: 50
Access-Control-Allow-Credentials: true
+
+
+
+=== TEST 36: set route ( regex specified and allow_origins is default value )
Review Comment:
> When the test file is too large, for example > 800 lines, you should split it to a new file. Please take a look at t/plugin/limit-conn.t and t/plugin/limit-conn2.t.
https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#check-code-style-and-test-case-style
##########
apisix/plugins/cors.lua:
##########
@@ -297,13 +297,13 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Review Comment:
Let's update docs for the new try order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] qihaiyan commented on pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "qihaiyan (via GitHub)" <gi...@apache.org>.
qihaiyan commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1467194738
Is this PR ready to merge? @spacewander
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Qiuqiu0505 commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1131999392
##########
apisix/plugins/cors.lua:
##########
@@ -297,13 +297,13 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Review Comment:
> Let's update docs for the new try order.
Thank you for reminding. I have modified it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Qiuqiu0505 commented on a diff in pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 commented on code in PR #9028:
URL: https://github.com/apache/apisix/pull/9028#discussion_r1138043065
##########
apisix/plugins/cors.lua:
##########
@@ -297,13 +297,13 @@ end
function _M.header_filter(conf, ctx)
local req_origin = ctx.original_request_origin
- -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+ -- If allow_origins_by_regex is not nil, should be matched to it only
local allow_origins
- allow_origins = process_with_allow_origins(conf.allow_origins, ctx, req_origin)
- if not match_origins(req_origin, allow_origins) then
- allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+ allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
Review Comment:
Thanks and I has changed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] qihaiyan commented on pull request #9028: fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only
Posted by "qihaiyan (via GitHub)" <gi...@apache.org>.
qihaiyan commented on PR #9028:
URL: https://github.com/apache/apisix/pull/9028#issuecomment-1482139500
Merging needs another approving of reviewer with write access. @spacewander
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org