You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/05/06 04:01:16 UTC
[GitHub] [apisix] crazyMonkey1995 opened a new pull request, #6988: Real ip recursive
crazyMonkey1995 opened a new pull request, #6988:
URL: https://github.com/apache/apisix/pull/6988
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
https://github.com/apache/apisix/issues/6602
### 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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r867283247
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not conf.matcher then
+ conf.matcher = core.ip.create_ip_matcher(conf.trusted_addresses)
+ end
Review Comment:
Changed it to local variable.
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870115014
##########
apisix/plugins/real-ip.lua:
##########
@@ -106,7 +106,7 @@ local function get_addr(conf, ctx)
if conf.recursive and conf.trusted_addresses then
local split_addrs = {}
- local match_itr = str_gmatch(addrs, "[^,%s*]+")
+ local match_itr = ngx.re.gmatch(addrs, "[^,%s*]+", "jo")
Review Comment:
And the return value is different.
--
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] tzssangglass commented on pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #6988:
URL: https://github.com/apache/apisix/pull/6988#issuecomment-1120177526
LGTM
--
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] tzssangglass commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r871312348
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +75,22 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ local matcher, err = lrucache(conf.trusted_addresses, nil,
+ core.ip.create_ip_matcher, conf.trusted_addresses)
Review Comment:
```suggestion
local ver = tostring(conf.trusted_addresses)
local matcher, err = lrucache(conf.trusted_addresses, ver,
core.ip.create_ip_matcher, conf.trusted_addresses)
```
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870117005
##########
apisix/plugins/real-ip.lua:
##########
@@ -106,7 +106,7 @@ local function get_addr(conf, ctx)
if conf.recursive and conf.trusted_addresses then
local split_addrs = {}
- local match_itr = str_gmatch(addrs, "[^,%s*]+")
+ local match_itr = ngx.re.gmatch(addrs, "[^,%s*]+", "jo")
Review Comment:
Sorry for that, I should have tested it first
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870879873
##########
apisix/plugins/real-ip.lua:
##########
@@ -84,6 +103,17 @@ local function get_addr(conf, ctx)
return addrs
end
+ if conf.recursive and conf.trusted_addresses then
+ local split_addrs, _ = ngx_re_split(addrs, ",\\s*", "jo")
Review Comment:
Thanks for the review, the above if condition has already dealt with the absence of commas, so there should be no need to deal with err here? I'm not very familiar with lua, but this `_` may indeed be removed.
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r871132810
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not matcher then
Review Comment:
Use lru cache instead, refer to the implementation of ip-restriction
--
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] tzssangglass commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r866934795
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not conf.matcher then
+ conf.matcher = core.ip.create_ip_matcher(conf.trusted_addresses)
+ end
Review Comment:
It is bad practice to add properties to conf, and we should keep conf pure. How about making `matcher` a module-level variable?
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870873211
##########
apisix/plugins/real-ip.lua:
##########
@@ -106,7 +106,7 @@ local function get_addr(conf, ctx)
if conf.recursive and conf.trusted_addresses then
local split_addrs = {}
- local match_itr = str_gmatch(addrs, "[^,%s*]+")
+ local match_itr = ngx.re.gmatch(addrs, "[^,%s*]+", "jo")
Review Comment:
I replaced it with ngx.re.split
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870919916
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not matcher then
Review Comment:
But I defined it as a local variable above.
--
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] tzssangglass commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r867330939
##########
apisix/plugins/real-ip.lua:
##########
@@ -84,6 +104,22 @@ local function get_addr(conf, ctx)
return addrs
end
+ if conf.recursive and conf.trusted_addresses then
+ local split_addrs = {}
+ local match_itr = str_gmatch(addrs, "[^,%s*]+")
Review Comment:
can we use `ngx.re.gmatch` to replace `string.gmatch`
##########
apisix/plugins/real-ip.lua:
##########
@@ -84,6 +104,22 @@ local function get_addr(conf, ctx)
return addrs
end
+ if conf.recursive and conf.trusted_addresses then
+ local split_addrs = {}
+ local match_itr = str_gmatch(addrs, "[^,%s*]+")
Review Comment:
can we use `ngx.re.gmatch` to replace `string.gmatch`?
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r872190628
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +75,22 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, ctx, addr)
+ if not conf.trusted_addresses then
Review Comment:
We already check the trusted_addresses before: https://github.com/apache/apisix/blob/025710564ac717f99fb96dd19dc1fdd1344bc7ef/apisix/plugins/real-ip.lua#L107?
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r869918479
##########
apisix/plugins/real-ip.lua:
##########
@@ -84,6 +104,22 @@ local function get_addr(conf, ctx)
return addrs
end
+ if conf.recursive and conf.trusted_addresses then
+ local split_addrs = {}
+ local match_itr = str_gmatch(addrs, "[^,%s*]+")
Review Comment:
Solved.
--
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] crazyMonkey1995 commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
crazyMonkey1995 commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870989643
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not matcher then
Review Comment:
I see, can I use lru cache to solve it? Or do you have any suggestions?
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r867293987
##########
apisix/plugins/real-ip.lua:
##########
@@ -84,6 +104,22 @@ local function get_addr(conf, ctx)
return addrs
end
+ if conf.recursive and conf.trusted_addresses then
+ local split_addrs = {}
+ local match_itr = str_gmatch(addrs, "[^,%s*]+")
+ for itr in match_itr do
+ tb_insert(split_addrs, itr)
+ end
+
+ for i = #split_addrs, 1, -1 do
Review Comment:
```suggestion
for i = #split_addrs, 2, -1 do
```
We don't care if the first one can be trusted.
##########
docs/en/latest/plugins/real-ip.md:
##########
@@ -41,10 +41,11 @@ This Plugin requires APISIX to run on [APISIX-OpenResty](../how-to-build.md#step
## Attributes
-| Name | Type | Required | Valid values | Description |
-|-------------------|---------------|----------|-----------------------------------------------------------------|-----------------------------------------------------------------------------------|
-| source | string | True | Any Nginx variable like `arg_realip` or `http_x_forwarded_for`. | Dynamically sets the client's IP address and an optional port from APISIX's view. |
-| trusted_addresses | array[string] | False | List of IPs or CIDR ranges. | Dynamically sets the `set_real_ip_from` field. |
+| Name | Type | Required | Valid values | Description |
+|-------------------|---------------|----------|-----------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| source | string | True | Any Nginx variable like `arg_realip` or `http_x_forwarded_for`. | Dynamically sets the client's IP address and an optional port from APISIX's view. |
+| trusted_addresses | array[string] | False | List of IPs or CIDR ranges. | Dynamically sets the `set_real_ip_from` field. |
+| recursive | boolean | False | True to enable, false to disable, default is false | If recursive search is disabled, the original client address that matches one of the trusted addresses is replaced by the last address sent in the request header field defined by the real_ip_header directive. If recursive search is enabled, the original client address that matches one of the trusted addresses is replaced by the last non-trusted address sent in the request header field. |
Review Comment:
```suggestion
| recursive | boolean | False | True to enable, false to disable, default is false | If recursive search is disabled, the original client address that matches one of the trusted addresses is replaced by the last address sent in the configured `source`. If recursive search is enabled, the original client address that matches one of the trusted addresses is replaced by the last non-trusted address sent in the configured `source`. |
```
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870915504
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not matcher then
Review Comment:
We should not use global matcher. Otherwise different configuration will use the same matcher.
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r871943884
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +75,22 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ local matcher, err = lrucache(conf.trusted_addresses, nil,
+ core.ip.create_ip_matcher, conf.trusted_addresses)
Review Comment:
Better to use https://github.com/apache/apisix/blob/4690feb421779f5b79e8dd990dc00f4d3f1052d0/apisix/core/lrucache.lua#L171 instead.
--
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] soulbird commented on a diff in pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870874491
##########
apisix/plugins/real-ip.lua:
##########
@@ -84,6 +103,17 @@ local function get_addr(conf, ctx)
return addrs
end
+ if conf.recursive and conf.trusted_addresses then
+ local split_addrs, _ = ngx_re_split(addrs, ",\\s*", "jo")
Review Comment:
`local split_addrs = ngx_re_split(addrs, ",\\s*", "jo")` better? Or deal with the error?
##########
apisix/plugins/real-ip.lua:
##########
@@ -105,12 +135,8 @@ function _M.rewrite(conf, ctx)
end
if conf.trusted_addresses then
- if not conf.matcher then
- conf.matcher = core.ip.create_ip_matcher(conf.trusted_addresses)
- end
-
local remote_addr = ctx.var.remote_addr
- local trusted = conf.matcher:match(remote_addr)
+ local trusted = addr_match(conf, remote_addr)
if not trusted then
return
end
Review Comment:
```lua
if not addr_match(conf, remote_addr) then
return
end
```
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870949710
##########
apisix/plugins/real-ip.lua:
##########
@@ -67,6 +73,19 @@ function _M.check_schema(conf)
end
+local function addr_match(conf, addr)
+ if not conf.trusted_addresses then
+ return false
+ end
+
+ if not matcher then
Review Comment:
Different requests with different configurations will share this as it is defined as a module variable.
--
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 #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6988:
URL: https://github.com/apache/apisix/pull/6988#discussion_r870112762
##########
apisix/plugins/real-ip.lua:
##########
@@ -106,7 +106,7 @@ local function get_addr(conf, ctx)
if conf.recursive and conf.trusted_addresses then
local split_addrs = {}
- local match_itr = str_gmatch(addrs, "[^,%s*]+")
+ local match_itr = ngx.re.gmatch(addrs, "[^,%s*]+", "jo")
Review Comment:
We should update the regex syntax to PCRE version?
--
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 merged pull request #6988: feat(real-ip): support search recursive
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #6988:
URL: https://github.com/apache/apisix/pull/6988
--
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