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 2021/09/03 11:51:47 UTC
[GitHub] [apisix] Zheaoli opened a new pull request #4980: [WIP] replace ip match tools by using ipmatcher
Zheaoli opened a new pull request #4980:
URL: https://github.com/apache/apisix/pull/4980
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) 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] Zheaoli commented on a change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
Zheaoli commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r705895742
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +76,18 @@ do
end
local route = item.value
+ if item.value.remote_addr then
+ local remote_matcher = core_ip.create_ip_matcher({item.value.remote_addr})
+ if remote_matcher then
Review comment:
The check process in the check function just checks the address validated if server_addr or remote_addr is existed.
But in some circumstances, both the remote_address and server_address are not existed, but the configuration is validated. Here's the example in https://github.com/apache/apisix/blob/master/docs/zh/latest/stream-proxy.md#%E6%8E%A5%E6%94%B6-tls-over-tcp
```bash
curl http://127.0.0.1:9080/apisix/admin/stream_routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
"sni": "a.test.com",
"upstream": {
"nodes": {
"127.0.0.1:5991": 1
},
"type": "roundrobin"
}
}'
```
So we can use the nil-check code here to avoid the useless matcher-created processes.
--
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] Zheaoli commented on a change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
Zheaoli commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r702426387
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -33,14 +35,38 @@ local _M = {version = 0.1}
local function match_addrs(route, vars)
-- todo: use resty-ipmatcher to support multiple ip address
- if route.value.remote_addr and
- route.value.remote_addr ~= vars.remote_addr then
- return false
+ if route.value.remote_addr then
+ local ip = remote_addr_match_cache[route.value.remote_addr]
Review comment:
Make sense, I'll update mycode
--
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] Zheaoli commented on a change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
Zheaoli commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r705895742
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +76,18 @@ do
end
local route = item.value
+ if item.value.remote_addr then
+ local remote_matcher = core_ip.create_ip_matcher({item.value.remote_addr})
+ if remote_matcher then
Review comment:
The check process in the check function just checks the address if server_addr or remote_addr is validated.
But in some circumstances, both the remote_address and server_address are not existed, but the configuration is validated. Here's the example in https://github.com/apache/apisix/blob/master/docs/zh/latest/stream-proxy.md#%E6%8E%A5%E6%94%B6-tls-over-tcp
```bash
curl http://127.0.0.1:9080/apisix/admin/stream_routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
"sni": "a.test.com",
"upstream": {
"nodes": {
"127.0.0.1:5991": 1
},
"type": "roundrobin"
}
}'
```
So we can use the nil-check code here to avoid a useless matcher-created processes.
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +76,18 @@ do
end
local route = item.value
+ if item.value.remote_addr then
+ local remote_matcher = core_ip.create_ip_matcher({item.value.remote_addr})
+ if remote_matcher then
Review comment:
The check process in the check function just checks the address if server_addr or remote_addr is validated.
But in some circumstances, both the remote_address and server_address are not existed, but the configuration is validated. Here's the example in https://github.com/apache/apisix/blob/master/docs/zh/latest/stream-proxy.md#%E6%8E%A5%E6%94%B6-tls-over-tcp
```bash
curl http://127.0.0.1:9080/apisix/admin/stream_routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
"sni": "a.test.com",
"upstream": {
"nodes": {
"127.0.0.1:5991": 1
},
"type": "roundrobin"
}
}'
```
So we can use the nil-check code here to avoid the useless matcher-created processes.
--
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] Zheaoli commented on a change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
Zheaoli commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r702544669
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -33,14 +33,30 @@ local _M = {version = 0.1}
local function match_addrs(route, vars)
-- todo: use resty-ipmatcher to support multiple ip address
- if route.value.remote_addr and
- route.value.remote_addr ~= vars.remote_addr then
- return false
+ if route.value.remote_addr then
Review comment:
Sorry I don't get your means, you mean remove the match function?
--
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] Zheaoli commented on a change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
Zheaoli commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r703188711
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +96,20 @@ do
end
local route = item.value
+
+ if not item.value.remote_addr_matcher and item.value.remote_addr then
+ local ip = create_matcher(item.value.remote_addr)
+ if ip then
Review comment:
I think it's unnecessary to use `validate_cidr_or_ip` here, we just need to check the `remote_addr` and `server_addr` is nil or not. the `ipmatcher` will help to validate the address when create a new match object by using `new` method
--
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 #4980: feat(stream_route): support CIDR in ip match
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4980:
URL: https://github.com/apache/apisix/pull/4980
--
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] tokers commented on a change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r702393439
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -33,14 +35,38 @@ local _M = {version = 0.1}
local function match_addrs(route, vars)
-- todo: use resty-ipmatcher to support multiple ip address
- if route.value.remote_addr and
- route.value.remote_addr ~= vars.remote_addr then
- return false
+ if route.value.remote_addr then
+ local ip = remote_addr_match_cache[route.value.remote_addr]
Review comment:
1. It's dangerous to use a module-level table to cache the match result, since it may cause the memory leak;
2. The IP match overheads are not so high, the caching is unnecessary;
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r703127989
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +96,20 @@ do
end
local route = item.value
+
+ if not item.value.remote_addr_matcher and item.value.remote_addr then
+ local ip = create_matcher(item.value.remote_addr)
+ if ip then
Review comment:
We can use validate_cidr_or_ip to validate.
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r702740839
##########
File path: t/stream-node/sanity.t
##########
@@ -281,3 +281,41 @@ GET /t
hello world
--- no_error_log
[error]
+
+
+
+=== TEST 11: set stream route (id: 1) which uses upstream_id
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "remote_addr": "127.0.0.1/26",
+ "upstream_id": "1"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 12: hit route
+--- stream_enable
+--- stream_request eval
+mmm
+--- stream_response
+hello world
+--- no_error_log
+[error]
Review comment:
Add blank line at the 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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r706835721
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +76,18 @@ do
end
local route = item.value
+ if item.value.remote_addr then
+ local remote_matcher = core_ip.create_ip_matcher({item.value.remote_addr})
+ if remote_matcher then
Review comment:
I mean the returned value of create_ip_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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r704926146
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +82,20 @@ do
end
local route = item.value
+
+ if not item.value.remote_addr_matcher and validate_address(item.value.remote_addr) then
Review comment:
Please add the check to https://github.com/apache/apisix/blob/df0db95ce8f8a5a977481539b6af7e056d295680/apisix/stream/router/ip_port.lua#L185
--
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] Zheaoli commented on pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
Zheaoli commented on pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#issuecomment-912932464
https://github.com/apache/apisix/issues/4441
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r702537405
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -33,14 +33,30 @@ local _M = {version = 0.1}
local function match_addrs(route, vars)
-- todo: use resty-ipmatcher to support multiple ip address
- if route.value.remote_addr and
- route.value.remote_addr ~= vars.remote_addr then
- return false
+ if route.value.remote_addr then
Review comment:
Better to put the init in the `create_router` instead of the match
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -69,6 +69,7 @@ dependencies = {
"ext-plugin-proto = 0.3.0",
"casbin = 1.26.0",
"api7-snowflake = 2.0-1",
+ "lua-resty-ipmatcher = 0.6.1"
Review comment:
Don't add lua-resty-ipmatcher twice
##########
File path: t/stream-plugin/mqtt-proxy.t
##########
@@ -220,7 +220,56 @@ hello world
-=== TEST 9: set route with invalid host
+=== TEST 9: set route with IP CIDR
Review comment:
New test should be added from the bottom of the file. And it should be in stream-node/sanity.t
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r702676585
##########
File path: t/stream-node/sanity.t
##########
@@ -281,3 +281,41 @@ GET /t
hello world
--- no_error_log
[error]
+
+
+
+=== TEST 11: set stream route (id: 1) which uses upstream_id
Review comment:
The name should show the difference from the others, like `CIDR`
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r703110817
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +96,20 @@ do
end
local route = item.value
+
+ if not item.value.remote_addr_matcher and item.value.remote_addr then
+ local ip = create_matcher(item.value.remote_addr)
+ if ip then
Review comment:
We can write a new checker in https://github.com/apache/apisix/blob/df0db95ce8f8a5a977481539b6af7e056d295680/apisix/stream/router/ip_port.lua#L185 to ensure the matcher can be always created?
We can use the methods in https://github.com/apache/apisix/blob/master/apisix/core/ip.lua to do this.
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r705883779
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -71,6 +76,18 @@ do
end
local route = item.value
+ if item.value.remote_addr then
+ local remote_matcher = core_ip.create_ip_matcher({item.value.remote_addr})
+ if remote_matcher then
Review comment:
We don't need to check the returned value, it should be checked in the check function.
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -176,13 +193,31 @@ function _M.routes()
return user_routes.values, user_routes.conf_version
end
+local function address_checker(item)
Review comment:
This function doesn't only check address.
```suggestion
local function checker(item)
```
##########
File path: apisix/stream/router/ip_port.lua
##########
@@ -176,13 +193,31 @@ function _M.routes()
return user_routes.values, user_routes.conf_version
end
+local function address_checker(item)
+ if item.plugins then
+ return plugin_checker(item)
Review comment:
Need to check both plugins and address
--
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 change in pull request #4980: fix(stream_route): replace ip match tools by using ipmatcher
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4980:
URL: https://github.com/apache/apisix/pull/4980#discussion_r706983824
##########
File path: t/stream-node/sanity.t
##########
@@ -281,3 +281,41 @@ GET /t
hello world
--- no_error_log
[error]
+
+
+
+=== TEST 11: set stream route (id: 1) which uses upstream_id and remote address with IP CIDR
Review comment:
Let's add a test that the conf is rejected by validate_cidr_or_ip
--
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