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/06/30 02:23:19 UTC
[GitHub] [apisix] arthur-zhang opened a new pull request #4504: [feat batch-request] check control port conflict when batch-requests is enabled
arthur-zhang opened a new pull request #4504:
URL: https://github.com/apache/apisix/pull/4504
batch-requests plugin is working based on routing all the the request to 127.0.0.1:9080, if enable-control is on and port is the same with node_listen port, it will add configuration file like this：
```
server {
listen 127.0.0.1:9080;
access_log off;
location / {
content_by_lua_block {
local prometheus = require("apisix.plugins.prometheus")
prometheus.export_metrics()
}
}
}
```
this will cause all the 127.0.0.1:9080/* request route to this control api server, and all the expected batch-requests url return 404.
I add some force check when batch-requests is enabled.
--
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 #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#discussion_r662080252
##########
File path: apisix/cli/ops.lua
##########
@@ -407,16 +407,53 @@ Please modify "admin_key" in conf/config.yaml .
util.die("missing apisix.proxy_cache for plugin proxy-cache\n")
end
+ local control_port = 9090
Review comment:
```suggestion
local control_port
```
--
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 #4504: feat(cli): check if control port conflicts with the node port
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#issuecomment-872675982
@arthur-zhang
Would you also submit a PR to check port conflict with prometheus one? https://github.com/apache/apisix/blob/cd8afe6ec4e043a188718c41fd5937d2cd88c59d/conf/config-default.yaml#L313
--
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] arthur-zhang commented on pull request #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
arthur-zhang commented on pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#issuecomment-871985453
> Could you add a test in https://github.com/apache/apisix/blob/master/t/cli/test_control.sh?
I read all the shell code, but till have no idea how to make a test case in this scenario
--
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] arthur-zhang commented on pull request #4504: feat(cli): check if control port conflicts with the node port
Posted by GitBox <gi...@apache.org>.
arthur-zhang commented on pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#issuecomment-872682173
> @arthur-zhang
> Would you also submit a PR to check port conflict with prometheus one?
>
> https://github.com/apache/apisix/blob/cd8afe6ec4e043a188718c41fd5937d2cd88c59d/conf/config-default.yaml#L313
OK，I will submit later
--
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 #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#discussion_r662676415
##########
File path: apisix/cli/ops.lua
##########
@@ -407,16 +407,53 @@ Please modify "admin_key" in conf/config.yaml .
util.die("missing apisix.proxy_cache for plugin proxy-cache\n")
end
+ local control_port
+ local control_server_addr
+ if yaml_conf.apisix.enable_control then
+ if not yaml_conf.apisix.control then
+ control_server_addr = "127.0.0.1:9090"
+ else
+ local ip = yaml_conf.apisix.control.ip
+ local port = tonumber(yaml_conf.apisix.control.port)
+
+ if ip == nil then
+ ip = "127.0.0.1"
+ end
+
+ if not port then
+ port = 9090
+ end
+
+ control_server_addr = ip .. ":" .. port
+ control_port = port
+ end
+ end
+
-- support multiple ports listen, compatible with the original style
if type(yaml_conf.apisix.node_listen) == "number" then
+
+ if yaml_conf.apisix.node_listen == control_port then
+ util.die("control port conflict with node_listen port\n")
Review comment:
```suggestion
util.die("control port conflicts with node_listen port\n")
```
##########
File path: apisix/cli/ops.lua
##########
@@ -407,16 +407,53 @@ Please modify "admin_key" in conf/config.yaml .
util.die("missing apisix.proxy_cache for plugin proxy-cache\n")
end
+ local control_port
+ local control_server_addr
+ if yaml_conf.apisix.enable_control then
+ if not yaml_conf.apisix.control then
+ control_server_addr = "127.0.0.1:9090"
+ else
+ local ip = yaml_conf.apisix.control.ip
+ local port = tonumber(yaml_conf.apisix.control.port)
+
+ if ip == nil then
+ ip = "127.0.0.1"
+ end
+
+ if not port then
+ port = 9090
+ end
+
+ control_server_addr = ip .. ":" .. port
+ control_port = port
+ end
+ end
+
-- support multiple ports listen, compatible with the original style
if type(yaml_conf.apisix.node_listen) == "number" then
+
+ if yaml_conf.apisix.node_listen == control_port then
+ util.die("control port conflict with node_listen port\n")
+ end
+
local node_listen = {{port = yaml_conf.apisix.node_listen}}
yaml_conf.apisix.node_listen = node_listen
elseif type(yaml_conf.apisix.node_listen) == "table" then
local node_listen = {}
for index, value in ipairs(yaml_conf.apisix.node_listen) do
if type(value) == "number" then
+
+ if value == control_port then
+ util.die("control port conflict with node_listen port\n")
Review comment:
ditto
--
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 #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#issuecomment-871067636
Could you add a test in https://github.com/apache/apisix/blob/master/t/cli/test_control.sh?
--
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 #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#issuecomment-872031157
You can set node_listen to 9090: https://github.com/apache/apisix/blob/047f5e7d503ea62eba28bae7386efb3e6d5b8268/t/cli/test_main.sh#L85
And then check the output of `make init`.
--
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 #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#discussion_r661101354
##########
File path: apisix/cli/ops.lua
##########
@@ -542,6 +543,29 @@ Please modify "admin_key" in conf/config.yaml .
end
end
+ -- check control port conflict when batch-requests is enabled
+ if enabled_plugins["batch-requests"] and yaml_conf.apisix.enable_control then
Review comment:
Although this only happens when batch-requests is enabled now, I think we can always check if the port conflicts even batch-requests is not enabled.
##########
File path: apisix/cli/ops.lua
##########
@@ -542,6 +543,29 @@ Please modify "admin_key" in conf/config.yaml .
end
end
+ -- check control port conflict when batch-requests is enabled
+ if enabled_plugins["batch-requests"] and yaml_conf.apisix.enable_control then
+ local split = pl_stringx.split
+ local ip_port = split(sys_conf.control_server_addr, ":")
+ local port = tonumber(ip_port[#ip_port])
+ if port == nil then
+ util.die("port not number, should not happen\n")
+ end
+
+ if type(yaml_conf.apisix.node_listen) == "number" then
Review comment:
Maybe we can move the control API stuff above: https://github.com/apache/apisix/blob/d0bc7238c912259e325f45ad6caea039d24e4292/apisix/cli/ops.lua#L410
And use a port denylist to check it when iterating the node_listen? So that we don't need to iterate it twice.
--
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] arthur-zhang commented on a change in pull request #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
arthur-zhang commented on a change in pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#discussion_r662217999
##########
File path: apisix/cli/ops.lua
##########
@@ -407,16 +407,53 @@ Please modify "admin_key" in conf/config.yaml .
util.die("missing apisix.proxy_cache for plugin proxy-cache\n")
end
+ local control_port = 9090
Review comment:
> You can set node_listen to 9090:
>
> https://github.com/apache/apisix/blob/047f5e7d503ea62eba28bae7386efb3e6d5b8268/t/cli/test_main.sh#L85
>
>
> And then check the output of `make init`.
Many thanks, test case added.
--
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] arthur-zhang commented on a change in pull request #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
arthur-zhang commented on a change in pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#discussion_r662034666
##########
File path: apisix/cli/ops.lua
##########
@@ -542,6 +543,29 @@ Please modify "admin_key" in conf/config.yaml .
end
end
+ -- check control port conflict when batch-requests is enabled
+ if enabled_plugins["batch-requests"] and yaml_conf.apisix.enable_control then
+ local split = pl_stringx.split
+ local ip_port = split(sys_conf.control_server_addr, ":")
+ local port = tonumber(ip_port[#ip_port])
+ if port == nil then
+ util.die("port not number, should not happen\n")
+ end
+
+ if type(yaml_conf.apisix.node_listen) == "number" then
Review comment:
I think so. I've commit the code.
--
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] arthur-zhang commented on a change in pull request #4504: feat(batch-requests): check control port conflict when batch-requests is enabled
Posted by GitBox <gi...@apache.org>.
arthur-zhang commented on a change in pull request #4504:
URL: https://github.com/apache/apisix/pull/4504#discussion_r662684090
##########
File path: apisix/cli/ops.lua
##########
@@ -407,16 +407,53 @@ Please modify "admin_key" in conf/config.yaml .
util.die("missing apisix.proxy_cache for plugin proxy-cache\n")
end
+ local control_port
+ local control_server_addr
+ if yaml_conf.apisix.enable_control then
+ if not yaml_conf.apisix.control then
+ control_server_addr = "127.0.0.1:9090"
+ else
+ local ip = yaml_conf.apisix.control.ip
+ local port = tonumber(yaml_conf.apisix.control.port)
+
+ if ip == nil then
+ ip = "127.0.0.1"
+ end
+
+ if not port then
+ port = 9090
+ end
+
+ control_server_addr = ip .. ":" .. port
+ control_port = port
+ end
+ end
+
-- support multiple ports listen, compatible with the original style
if type(yaml_conf.apisix.node_listen) == "number" then
+
+ if yaml_conf.apisix.node_listen == control_port then
+ util.die("control port conflict with node_listen port\n")
+ end
+
local node_listen = {{port = yaml_conf.apisix.node_listen}}
yaml_conf.apisix.node_listen = node_listen
elseif type(yaml_conf.apisix.node_listen) == "table" then
local node_listen = {}
for index, value in ipairs(yaml_conf.apisix.node_listen) do
if type(value) == "number" then
+
+ if value == control_port then
+ util.die("control port conflict with node_listen port\n")
Review comment:
done
--
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 #4504: feat(cli): check if control port conflicts with the node port
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4504:
URL: https://github.com/apache/apisix/pull/4504
--
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