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/11/02 10:10:01 UTC
[GitHub] [apisix] biakecw opened a new pull request, #8219: feat: validate the subordinate and check reference when deleting a stream …
biakecw opened a new pull request, #8219:
URL: https://github.com/apache/apisix/pull/8219
…route
### Description
Add two logics related to subordinate route。
1 validate if the stream route with superior id exists and its protocol matches the subordinate
solve it by core.log.warn()
2 when deleting a stream route, check if it is referenced by another stream route
add the referenced relation in res.body
Fixes #6939
### 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] biakecw closed pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw closed pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
URL: https://github.com/apache/apisix/pull/8219
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013911648
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
Review Comment:
deleting a stream route, check if it is referenced by another stream route ; before query ，delete previous value and then generate new。 Just delete previous , I choose to ignore exceptions handle because I can't think of errors happening
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1031302041
##########
apisix/admin/init.lua:
##########
@@ -15,6 +15,7 @@
-- limitations under the License.
--
local require = require
+local filter = require("apisix.router").filter
Review Comment:
sorry
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1025083503
##########
apisix/admin/stream_routes.lua:
##########
@@ -78,6 +102,14 @@ local function check_conf(id, conf, need_id)
return need_id and id or true
end
+function _M.stream_routes()
Review Comment:
Modified，please have a look
--
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] github-actions[bot] closed pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
URL: https://github.com/apache/apisix/pull/8219
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1016738525
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,41 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local config_util = require("apisix.core.config_util")
+local routes = require("apisix.stream.router.ip_port").routes
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local ngx = ngx
+local table = table
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = ngx.re.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list,r_id)
+ end
+ ::CONTINUE::
+ end
+ if #refer_list > 0 then
+ local warn_message = "/stream_routes/" .. id .. "is referred by " .. table.concat(refer_list,";;")
+ core.tablepool.release("refer_list",refer_list)
+ return warn_message
+ end
Review Comment:
ok
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1015052122
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,8 +163,18 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local items,_ = routes()
local key = "/stream_routes/" .. id
-- core.log.info("key: ", key)
+ local refer_list = {}
+ if items ~= nil then
+ refer_list=check_router_refer(items,id)
+ end
+ if #refer_list >0 then
+ local warn_message = key.." is referred by "..table.concat(refer_list,";;")
+ return 400,warn_message
+ end
Review Comment:
Thanks for your comments about core.tablepool.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1308725586
> I have fixed the bug in my submitted test case， but no thoughts on other errors. need help
You can analyze the error test cases reported by the CI and then run them locally to reproduce the errors in the CI. I will see what happens as soon as possible.
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013917090
##########
t/admin/stream-routes.t:
##########
@@ -639,3 +639,30 @@ passed
{"error_msg":"property \"faults\" validation failed: wrong type: expected array, got string"}
--- no_error_log
[error]
+
Review Comment:
ok
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1016646077
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,41 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local config_util = require("apisix.core.config_util")
+local routes = require("apisix.stream.router.ip_port").routes
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local ngx = ngx
+local table = table
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = ngx.re.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list,r_id)
+ end
+ ::CONTINUE::
+ end
+ if #refer_list > 0 then
+ local warn_message = "/stream_routes/" .. id .. "is referred by " .. table.concat(refer_list,";;")
+ core.tablepool.release("refer_list",refer_list)
+ return warn_message
+ end
Review Comment:
add `core.tablepool.release("refer_list",refer_list)` before return nil
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021551951
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
ok
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1022219080
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,38 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local iterate_values = require("apisix.core.config_util").iterate_values
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
Review Comment:
```suggestion
local refer_list = core.tablepool.fetch("refer_list", #items, 0)
```
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,38 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local iterate_values = require("apisix.core.config_util").iterate_values
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
+ for _, item in iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list,item["key"])
Review Comment:
```suggestion
table.insert(refer_list, item["key"])
```
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,38 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local iterate_values = require("apisix.core.config_util").iterate_values
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
+ for _, item in iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list,item["key"])
+ end
+ ::CONTINUE::
+ end
+ if #refer_list > 0 then
+ warn_message = "/stream_routes/" .. id .. " is referred by "
+ .. table.concat(refer_list,";;")
+ end
+ core.tablepool.release("refer_list",refer_list)
Review Comment:
```suggestion
core.tablepool.release("refer_list", refer_list)
```
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,6 +165,15 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local status, body = _M.get()
+ local items = body.list
+ if tonumber(status) == 200 and #items > 1 then
+ local warn_message = check_router_refer(items,id)
Review Comment:
```suggestion
local warn_message = check_router_refer(items, id)
```
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014237924
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
+ if res then
+ if res.body.node == nil then
+ data = core.json.decode("{}")
+ data[r_id] = 1
+ else
+ data = res.body.node.value
+ data[r_id] = 1
+ end
+ else
+ core.log.error("In function check_router_refer error: ", err)
+ end
+ local setres, err = core.etcd.set(setkey, data)
Review Comment:
so at what point do we delete `/apisix/stream_routes_refer/12`?
Anyway, I don't think it's a good solution to write this relationship to etcd.
Can we refer to the logic for removing the upstream?https://github.com/apache/apisix/blob/a62d33f2f1fcbd62fdd25ea2295b0d6c4675bd4c/apisix/admin/upstreams.lua#L146-L156
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014600152
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
+ if res then
+ if res.body.node == nil then
+ data = core.json.decode("{}")
+ data[r_id] = 1
+ else
+ data = res.body.node.value
+ data[r_id] = 1
+ end
+ else
+ core.log.error("In function check_router_refer error: ", err)
+ end
+ local setres, err = core.etcd.set(setkey, data)
Review Comment:
Thanks for your suggestion I changed the implementation.
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013903931
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
+ if res then
+ if res.body.node == nil then
+ data = core.json.decode("{}")
+ data[r_id] = 1
+ else
+ data = res.body.node.value
+ data[r_id] = 1
+ end
+ else
+ core.log.error("In function check_router_refer error: ", err)
+ end
+ local setres, err = core.etcd.set(setkey, data)
Review Comment:
The keys stream_routes_refer/super_ids will be update rather than grow .
ex：
/apisix/stream_routes_refer/12
{"_apisix_stream_routes_22":1,"_apisix_stream_routes_23":1}
it meas the stream_route 22 and 23 have protocol.superior_id 12; if the stream_routes rule no change ,the value no change.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021045552
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
I would prefer to check this when adding the stream route and return it if it is wrong.
In addition, two for loops are used here, which is even less efficient.
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021047986
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
ok
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1310000017
> Do we have an docker image that contains the dependencies required by the test environment. Deploying these in my development environment is a bit troublesome
try to use `make ci-env-up project_compose_ci=ci/pod/docker-compose.plugin.yml`, ref: https://github.com/apache/apisix/tree/master/ci/pod
--
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] github-actions[bot] commented on pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by github-actions.
github-actions[bot] commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1401666293
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1311487544
```diff
diff --git apisix/admin/stream_routes.lua apisix/admin/stream_routes.lua
index 57af650e..5baccb35 100644
--- apisix/admin/stream_routes.lua
+++ apisix/admin/stream_routes.lua
@@ -17,11 +17,11 @@
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
local config_util = require("apisix.core.config_util")
-local routes = require("apisix.stream.router.ip_port").routes
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
local ngx = ngx
local table = table
+local tonumber = tonumber
local _M = {
@@ -169,16 +169,16 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
- local items, _ = routes()
- local key = "/stream_routes/" .. id
- -- core.log.info("key: ", key)
- if items ~= nil then
+ local status, body = _M.get()
+ local items = body.list
+ if tonumber(status) == 200 and #items > 1 then
local warn_message = check_router_refer(items,id)
if warn_message ~= nil then
- return 400,warn_message
+ return 400, warn_message
end
end
-
+ local key = "/stream_routes/" .. id
+ -- core.log.info("key: ", key)
local res, err = core.etcd.delete(key)
if not res then
core.log.error("failed to delete stream route[", key, "]: ", err)
diff --git apisix/cli/ngx_tpl.lua apisix/cli/ngx_tpl.lua
index 514bdd70..fd45d7f8 100644
--- apisix/cli/ngx_tpl.lua
+++ apisix/cli/ngx_tpl.lua
@@ -447,7 +447,6 @@ http {
init_worker_by_lua_block {
apisix.http_init_worker()
- apisix.stream_init_worker()
}
exit_worker_by_lua_block {
diff --git t/admin/stream-routes.t t/admin/stream-routes.t
index d362910b..6480fdef 100644
--- t/admin/stream-routes.t
+++ t/admin/stream-routes.t
@@ -643,14 +643,37 @@ passed
=== TEST 17: put reference route + delete
+--- stream_enable
--- extra_yaml_config
xrpc:
protocols:
- name: pingpong
--- config
- location /t {
+ location /test {
content_by_lua_block {
local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local code, body = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "remote_addr": "127.0.0.1",
+ "desc": "test-desc",
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:8080": 1
+ },
+ "type": "roundrobin"
+ },
+ "desc": "new route"
+ }]]
+ )
+ if code > 300 then
+ ngx.status = code
+ ngx.print(body)
+ return
+ end
+ ngx.sleep(0.5)
+
local code, body = t('/apisix/admin/stream_routes/12',
ngx.HTTP_PUT,
[[{
@@ -667,20 +690,21 @@ xrpc:
"superior_id": "1"
}
}]]
- )
+ )
if code > 300 then
ngx.status = code
ngx.print(body)
return
end
+ ngx.sleep(0.5)
- code2, message = t('/apisix/admin/stream_routes/1', ngx.HTTP_DELETE)
- ngx.say("[delete] code: ", code2, " message: ", message)
+ local code2, message = t('/apisix/admin/stream_routes/1', ngx.HTTP_DELETE)
+ ngx.say("code: ", code2, " message: ", message)
}
}
--- request
-GET /t
+GET /test
--- response_body
-[delete] code: 200 message: passed
+code: 400 message: /stream_routes/1 is referred by _apisix_stream_routes_12
--- no_error_log
[error]
```
Here is what I tried to modify, hope to help you.
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021034262
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
Yes, no action here, just warn logging。In order to remind users, do you think it's ok to keep 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] tzssangglass commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014950028
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,8 +163,18 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local items,_ = routes()
local key = "/stream_routes/" .. id
-- core.log.info("key: ", key)
+ local refer_list = {}
+ if items ~= nil then
+ refer_list=check_router_refer(items,id)
Review Comment:
```suggestion
refer_list = check_router_refer(items,id)
```
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1301771579
Please make the test & lint pass, like:
https://github.com/apache/apisix/actions/runs/3376629320/jobs/5619505907
```
Error: led test 't/admin/stream-routes.t TEST 17: return reference info, DELETE - pattern "[error]" should not match any line in error.log but matches line "2022/11/03 06:56:19 [error] 40724#40724: *55 lua entry thread aborted: runtime error: /home/runner/work/apisix/apisix/apisix/core/config_util.lua:38: attempt to index local 'tab' (a nil value)" (req 0)
# stack traceback:
# coroutine 0:
# /home/runner/work/apisix/apisix/apisix/core/config_util.lua: in function '(for generator)'
# ...runner/work/apisix/apisix/apisix/admin/stream_routes.lua:35: in function 'check_router_refer'
# ...runner/work/apisix/apisix/apisix/admin/stream_routes.lua:195: in function <...runner/work/apisix/apisix/apisix/admin/stream_routes.lua:187>
# /home/runner/work/apisix/apisix/apisix/admin/init.lua:188: in function 'handler'
# ...ork/apisix/apisix/deps/share/lua/5.1/resty/radixtree.lua:723: in function 'dispatch'
# /home/runner/work/apisix/apisix/apisix/init.lua:821: in function 'http_admin'
```
https://github.com/apache/apisix/actions/runs/3376629320/jobs/5619505955
```
Error: led test 't/stream-node/sni.t TEST 13: clean up routes - pattern "[error]" should not match any line in error.log but matches line "2022/11/03 06:55:54 [error] 44683#44683: *63 lua entry thread aborted: runtime error: /home/runner/work/apisix/apisix/apisix/core/config_util.lua:38: attempt to index local 'tab' (a nil value)" (req 0)
# stack traceback:
# coroutine 0:
# /home/runner/work/apisix/apisix/apisix/core/config_util.lua: in function '(for generator)'
# ...runner/work/apisix/apisix/apisix/admin/stream_routes.lua:35: in function 'check_router_refer'
# ...runner/work/apisix/apisix/apisix/admin/stream_routes.lua:195: in function <...runner/work/apisix/apisix/apisix/admin/stream_routes.lua:187>
# /home/runner/work/apisix/apisix/apisix/admin/init.lua:188: in function 'handler'
# ...ork/apisix/apisix/deps/share/lua/5.1/resty/radixtree.lua:723: in function 'dispatch'
# /home/runner/work/apisix/apisix/apisix/init.lua:821: in function 'http_admin'
```
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1314932376
please help to retry the failed ci check
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1024937749
##########
apisix/admin/stream_routes.lua:
##########
@@ -78,6 +102,14 @@ local function check_conf(id, conf, need_id)
return need_id and id or true
end
+function _M.stream_routes()
Review Comment:
What about moving the init (stream_init_worker) to https://github.com/apache/apisix/blob/2f4a4dba2c0cbc426ae99133c2546112ea71dba8/apisix/admin/init.lua#L353?
We should only initialize it when stream_proxy is enabled: https://github.com/apache/apisix/blob/2f4a4dba2c0cbc426ae99133c2546112ea71dba8/apisix/admin/init.lua#L145
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1299979157
> > core.etcd.set("/apisix/stream_routes/refer/12",{"/apisix/stream_routes/22":1})
> > ---->
> > /apisix/stream_routes/refer/12
> > "{"/apisix/stream_routes/22":1}"
>
> read more: [api7/lua-resty-etcd#180 (comment)](https://github.com/api7/lua-resty-etcd/issues/180#issuecomment-1286449776)
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1318231389
I successfully tested locally：
-----request
curl http://127.0.0.1:9180/apisix/admin/stream_routes/22 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
"server_port": 9100,
"upstream": {
"type": "none",
"nodes": {
"192.168.31.170:6379": 1
}
},
"protocol": {
"name": "redis",
"superior_id": "1",
"conf": {
"faults": [{
"commands": ["get", "ping"],
"delay": 5
}]
}
}
}
'
curl http://127.0.0.1:9180/apisix/admin/stream_routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X DELETE
-----
-----response
{"key":"\/apisix\/stream_routes\/22","value":{"update_time":1668671546,"upstream":{"nodes":{"192.168.31.170:6379":1},"scheme":"http","type":"none","hash_on":"vars","pass_host":"pass"},"protocol":{"name":"redis","superior_id":"1","conf":{"faults":[{"delay":5,"commands":["get","ping"]}]}},"id":"22","create_time":1668668955,"server_port":9100}}
/stream_routes/1 is referred by /apisix/stream_routes/22
-----
I didn't find the reason for the failure in stream_routes.t，please help to see what happened
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013860348
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
Review Comment:
```suggestion
core.log.info("items:", core.json.delay_encode(items))
```
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021047986
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
Add a logic that checking this when adding the stream route , there are two situations to judge：
1 validate if the stream route with superior id exists
The user needs to ensure the order when creating routes in batches --> "super is in front of sub "
2 validate if the stream route with superior id exists and its protocol matches the subordinate
Validate if the protocol matches the subordinate only if the stream route with superior id exists。
So I would add a logic that Validate if the protocol matches the subordinate only if the stream route with superior id exists。Do you think it's ok?
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1311503603
Thanks , it is easy to go in the wrong direction when walking alone, haha.
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1020063052
##########
t/admin/stream-routes.t:
##########
@@ -639,3 +639,48 @@ passed
{"error_msg":"property \"faults\" validation failed: wrong type: expected array, got string"}
--- no_error_log
[error]
+
+
+
+=== TEST 17: put reference route + delete
+--- extra_yaml_config
+xrpc:
+ protocols:
+ - name: pingpong
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/12',
+ ngx.HTTP_PUT,
+ [[{
+ "remote_addr": "127.0.0.1",
+ "desc": "test-refer",
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:8080": 1
+ },
+ "type": "roundrobin"
+ },
+ "protocol": {
+ "name": "pingpong",
+ "superior_id": "1"
+ }
+ }]]
+ )
+ if code > 300 then
+ ngx.status = code
+ ngx.print(body)
+ return
+ end
+
+ code2, message = t('/apisix/admin/stream_routes/1', ngx.HTTP_DELETE)
Review Comment:
ok， I will revise
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1311483825
> I made a mistake. admin api is run under http subsystem, so we can't get stream routes(user_routes) in `ip_port.lua`.
>
> So we can only get all the stream routes like `core.etcd.get("/stream_routes", not id)`
> I made a mistake. admin api is run under http subsystem, so we can't get stream routes(user_routes) in `ip_port.lua`.
>
> So we can only get all the stream routes like `core.etcd.get("/stream_routes", not id)`
ok I will try 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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1023718333
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,6 +165,15 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local status, body = _M.get()
Review Comment:
I have tried to use synced data from _M.routes() and it needs to apisix.stream_init_worker() before calling .
I agree with you that it is more efficient to use synced data from _M.routes() , but I get a lot of errors while adding apisix.stream_init_worker() in stream subsystem of nginx.conf
So after that I took the etcd implementation based on this suggestion [https://github.com/apache/apisix/pull/8219#issuecomment-1311464368](url).
--
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] github-actions[bot] commented on pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1491669928
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1309873358
Do we have an docker image that contains the dependencies required by the test environment. Deploying these in my development environment is a bit troublesome
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1311464368
I made a mistake. admin api is run under http subsystem, so we can't get stream routes(user_routes) in `ip_port.lua`.
So we can only get all the stream routes like `core.etcd.get("/stream_routes", not id)`
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021540158
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
Let's keep it simple and clear enough in this PR.
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1310745861
The previous error was because I added a line “apisix.stream_init_worker()” in APISIX.PM, now I removed it and adjusted the test case. Due to the need to traverse stream routes, I have to add apisix.stream_init_worker() . But I didn't expect it to have a great impact on ci checks, maybe lack a method “stream_exit_worker” . Hope to guide me ，thx.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1019937419
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -447,6 +447,7 @@ http {
init_worker_by_lua_block {
apisix.http_init_worker()
+ apisix.stream_init_worker()
Review Comment:
we don't need 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] tzssangglass commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021021577
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,42 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local config_util = require("apisix.core.config_util")
Review Comment:
```suggestion
local iterate_values = require("apisix.core.config_util").iterate_values
```
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,42 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local config_util = require("apisix.core.config_util")
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local ngx = ngx
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = ngx.re.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list,r_id)
+ end
Review Comment:
```suggestion
local route = item.value
if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
table.insert(refer_list, item["key"])
end
```
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
It seems we don't need to modify here? I see that the test cases don't cover the changes here.
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,42 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local config_util = require("apisix.core.config_util")
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local ngx = ngx
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list",#items,0)
+ for _, item in config_util.iterate_values(items) do
Review Comment:
```suggestion
for _, item in iterate_values(items) do
```
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1025961329
##########
apisix/admin/init.lua:
##########
@@ -148,6 +149,9 @@ local function run()
core.response.exit(400, {error_msg = "stream mode is disabled, " ..
"can not add stream routes"})
end
+ core.config.init()
+ local router_stream = require("apisix.stream.router.ip_port")
+ router_stream.stream_init_worker(filter)
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1023722675
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,38 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local iterate_values = require("apisix.core.config_util").iterate_values
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list", #items, 0)
+ for _, item in iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list, item["key"])
+ end
+ ::CONTINUE::
+ end
+ if #refer_list > 0 then
+ warn_message = "/stream_routes/" .. id .. " is referred by "
+ .. table.concat(refer_list,";;")
Review Comment:
No particular reason
If there are relevant specification requirements, I will update
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1027639167
##########
apisix/admin/init.lua:
##########
@@ -14,6 +14,7 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
+local filter = require("apisix.router").filter
Review Comment:
Does module `apisix.router` export the function `filter`?
BTW, the `require` call should be put under `local require = require`.
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1020061944
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -447,6 +447,7 @@ http {
init_worker_by_lua_block {
apisix.http_init_worker()
+ apisix.stream_init_worker()
Review Comment:
I want to explain my code design ：
This is using logic about stream_routes inside admin_api(**/apisix/admin**). Get stream_routes from apisix.stream.router.ip_port.routes() and apisix.stream.router.ip_port.routes() depends on stream_init_worker(). So under the scope of admin_api , I have to add stream_init_worker in advance。
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1314627462
fix some bad indents, others 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] spacewander commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1026076194
##########
apisix/admin/init.lua:
##########
@@ -19,6 +19,7 @@ local core = require("apisix.core")
local route = require("apisix.utils.router")
local plugin = require("apisix.plugin")
local v3_adapter = require("apisix.admin.v3_adapter")
+local filter = require("apisix.router").filter
Review Comment:
We need to export filter first so we can use it:
https://github.com/apache/apisix/blob/ae400b95b91240fb867c3cedc9fa65a6b5593f36/apisix/router.lua#L30
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1301800198
please approve running workflows @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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1299986763
Some problems have been changed； thank for the review， and hope to communicate @spacewander @tzssangglass
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013863667
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
+ if res then
+ if res.body.node == nil then
+ data = core.json.decode("{}")
+ data[r_id] = 1
+ else
+ data = res.body.node.value
+ data[r_id] = 1
+ end
+ else
+ core.log.error("In function check_router_refer error: ", err)
+ end
+ local setres, err = core.etcd.set(setkey, data)
Review Comment:
We should avoid writing this data to etcd. You set here, I don't see where we delete key. so `stream_routes_refer` will grow forever?
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013864922
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
Review Comment:
Do we have to filter the data by `superior_id` in etcd every time when we delete an item like this? That might be very inefficient.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1308326015
looks a little wrong
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1308536369
I have fixed the bug in my submitted test case， but no thoughts on other errors. need help
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1306853000
These check errors have nothing to do with what I submitted,I don't know how to solve 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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013916508
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
Review Comment:
For generate the reference-relationship while iterating over the items(stream_routes), only the data by superior_id exists reference-relationship
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1298582048
I want to ask a question :
core.etcd.set("/apisix/stream_routes/refer/12",{"\/apisix\/stream_routes\/22":1})
---->
/apisix/stream_routes/refer/12
"{\"\\\/apisix\\\/stream_routes\\\/22\":1}"
Is additional serialization required?
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013916508
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
+ local route = item.value
+ if route.protocol and route.protocol.superior_id then
+ local data
+ local setkey = "/stream_routes_refer/" .. route.protocol.superior_id
+ local res, err = core.etcd.get(setkey, false)
Review Comment:
For generate the reference-relationship while iterating over the items(stream_routes), only the data by superior_id exists reference-relationship.
thanks for your suggestion。
--
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] biakecw commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013916761
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
+ core.log.warn(items)
Review Comment:
ok
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014950285
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,8 +163,18 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local items,_ = routes()
local key = "/stream_routes/" .. id
-- core.log.info("key: ", key)
+ local refer_list = {}
Review Comment:
```suggestion
local refer_list
```
we don't need to init `refer_list` here? it would be inited in `check_router_refer`.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014949846
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,8 +163,18 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local items,_ = routes()
Review Comment:
```suggestion
local items, _ = routes()
```
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014951340
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,8 +163,18 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local items,_ = routes()
local key = "/stream_routes/" .. id
-- core.log.info("key: ", key)
+ local refer_list = {}
+ if items ~= nil then
+ refer_list=check_router_refer(items,id)
+ end
+ if #refer_list >0 then
+ local warn_message = key.." is referred by "..table.concat(refer_list,";;")
+ return 400,warn_message
+ end
Review Comment:
After thinking about it for a while, it seems that the `refer_list` lifecycle is only used here. Do we use `core.tablepool` to fetch and release `refer_list`?
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1311502029
> ```diff
> local key = "/stream_routes/" .. id
> ```
Thank you, it is easy to go in the wrong direction when walking alone ，haha.
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1021538808
##########
apisix/stream/router/ip_port.lua:
##########
@@ -63,23 +63,46 @@ local create_router
do
local sni_to_items = {}
local tls_routes = {}
+ local routeid_to_protocols = {}
function create_router(items)
local tls_routes_idx = 1
local other_routes_idx = 1
core.table.clear(tls_routes)
core.table.clear(other_routes)
core.table.clear(sni_to_items)
+ core.table.clear(routeid_to_protocols)
for _, item in config_util.iterate_values(items) do
if item.value == nil then
goto CONTINUE
end
+ local route = item.value
+ if route.protocol then
+ routeid_to_protocols[item.key]=route.protocol.name
+ else
+ routeid_to_protocols[item.key]="No-Protocol"
+ end
+ ::CONTINUE::
+ end
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
local route = item.value
if route.protocol and route.protocol.superior_id then
-- subordinate route won't be matched in the entry
- -- TODO: check the subordinate relationship in the Admin API
+ local key="/apisix/stream_routes/"..route.protocol.superior_id
+ if routeid_to_protocols[key] == nil then
+ core.log.warn("There is not exist stream_route: "..key)
+ elseif routeid_to_protocols[key] == "No-Protocol" then
+ core.log.warn("The stream_route: "..key.." may lacks procotol configuration")
+ elseif routeid_to_protocols[key] == route.protocol.name then
+ goto CONTINUE
+ else
+ core.log.warn("Different between stream_route:"..item.key.." and "..key)
+ end
goto CONTINUE
Review Comment:
> Add a logic that checking this when adding the stream route , there are two situations to judge： 1 validate if the stream route with superior id exists The user needs to ensure the order when creating routes in batches --> "super is in front of sub " 2 validate if the stream route with superior id exists and its protocol matches the subordinate Validate if the protocol matches the subordinate only if the stream route with superior id exists。
>
> So I would add a logic that Validate if the protocol matches the subordinate only if the stream route with superior id exists。Do you think it's ok?
I have a suggestion that in this PR we only finish checking refer when deleting the stream route.
You can open a new issue and PR to do what you said.
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1032027112
##########
apisix/admin/init.lua:
##########
@@ -354,7 +353,7 @@ function _M.init_worker()
if local_conf.apisix.stream_proxy then
local router_stream = require("apisix.stream.router.ip_port")
- router_stream.stream_init_worker(filter)
+ router_stream.stream_init_worker()
Review Comment:
We need to export the filter and pass it into `stream_init_worker`, otherwise, the data structure may be 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] spacewander commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1030109188
##########
apisix/admin/init.lua:
##########
@@ -15,6 +15,7 @@
-- limitations under the License.
--
local require = require
+local filter = require("apisix.router").filter
Review Comment:
Does module "apisix.router" export the filter 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] tzssangglass commented on a diff in pull request #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1025914834
##########
apisix/admin/init.lua:
##########
@@ -148,6 +149,9 @@ local function run()
core.response.exit(400, {error_msg = "stream mode is disabled, " ..
"can not add stream routes"})
end
+ core.config.init()
+ local router_stream = require("apisix.stream.router.ip_port")
+ router_stream.stream_init_worker(filter)
Review Comment:
here: https://github.com/apache/apisix/pull/8219#discussion_r1024937749 means move `router_stream.stream_init_worker(filter)` to `function _M.init_worker()`, not `local function run()`.
--
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 #8219: feat: validate the subordinate and check reference when deleting and adding…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1023592625
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,38 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local iterate_values = require("apisix.core.config_util").iterate_values
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local table = table
+local tonumber = tonumber
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local warn_message = nil
+ local refer_list = core.tablepool.fetch("refer_list", #items, 0)
+ for _, item in iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local route = item.value
+ if route.protocol and route.protocol.superior_id and route.protocol.superior_id == id then
+ table.insert(refer_list, item["key"])
+ end
+ ::CONTINUE::
+ end
+ if #refer_list > 0 then
+ warn_message = "/stream_routes/" .. id .. " is referred by "
+ .. table.concat(refer_list,";;")
Review Comment:
Is there a particular reason to use `;;` instead of `,` as the separator?
##########
apisix/admin/stream_routes.lua:
##########
@@ -142,6 +165,15 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end
+ local status, body = _M.get()
Review Comment:
Would be better to use synced data from https://github.com/apache/apisix/blob/2f4a4dba2c0cbc426ae99133c2546112ea71dba8/apisix/stream/router/ip_port.lua#L189.
It may be too expensive to read the whole stream routes from etcd each time we want to delete one.
It might be data race but I think it is acceptable. The current implementation still has a smaller data race since we don't use a transaction to deal with the deletion.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013861249
##########
apisix/admin/stream_routes.lua:
##########
@@ -25,23 +27,69 @@ local _M = {
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ local referkey = "/stream_routes_refer/" .. id
+ local _, err = core.etcd.delete(referkey)
+ core.log.warn(err)
Review Comment:
Is the `err` here only printed and not processed?
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1013859041
##########
t/admin/stream-routes.t:
##########
@@ -639,3 +639,30 @@ passed
{"error_msg":"property \"faults\" validation failed: wrong type: expected array, got string"}
--- no_error_log
[error]
+
Review Comment:
need three blank lines between test cases
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1014949174
##########
apisix/admin/stream_routes.lua:
##########
@@ -16,15 +16,36 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
+local config_util = require("apisix.core.config_util")
+local routes = require("apisix.stream.router.ip_port").routes
local stream_route_checker = require("apisix.stream.router.ip_port").stream_route_checker
local tostring = tostring
+local string = string
+local table = table
local _M = {
version = 0.1,
need_v3_filter = true,
}
+local function check_router_refer(items, id)
+ local refer_list = {}
+ for _, item in config_util.iterate_values(items) do
+ if item.value == nil then
+ goto CONTINUE
+ end
+ local r_id = string.gsub(item["key"], "/", "_")
Review Comment:
can we replace `string.gsub` with `ngx.re.gsub`?
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1307236604
> These check errors have nothing to do with what I submitted,I don't know how to solve it.
rerun
--
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] biakecw commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1299984026
Some questions have been changed, thank you for the review 。 @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] tzssangglass commented on pull request #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1299727274
> core.etcd.set("/apisix/stream_routes/refer/12",{"/apisix/stream_routes/22":1})
> ---->
> /apisix/stream_routes/refer/12
> "{"\/apisix\/stream_routes\/22":1}"
read more: https://github.com/api7/lua-resty-etcd/issues/180#issuecomment-1286449776
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8219:
URL: https://github.com/apache/apisix/pull/8219#issuecomment-1308398335
@biakecw If you encounter test cases that you cannot resolve, please ask for help here.
--
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 #8219: feat: validate the subordinate and check reference when deleting a stream …
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8219:
URL: https://github.com/apache/apisix/pull/8219#discussion_r1019948040
##########
t/admin/stream-routes.t:
##########
@@ -639,3 +639,48 @@ passed
{"error_msg":"property \"faults\" validation failed: wrong type: expected array, got string"}
--- no_error_log
[error]
+
+
+
+=== TEST 17: put reference route + delete
+--- extra_yaml_config
+xrpc:
+ protocols:
+ - name: pingpong
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/12',
+ ngx.HTTP_PUT,
+ [[{
+ "remote_addr": "127.0.0.1",
+ "desc": "test-refer",
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:8080": 1
+ },
+ "type": "roundrobin"
+ },
+ "protocol": {
+ "name": "pingpong",
+ "superior_id": "1"
+ }
+ }]]
+ )
+ if code > 300 then
+ ngx.status = code
+ ngx.print(body)
+ return
+ end
+
+ code2, message = t('/apisix/admin/stream_routes/1', ngx.HTTP_DELETE)
Review Comment:
```suggestion
local code2, message = t('/apisix/admin/stream_routes/1', ngx.HTTP_DELETE)
```
--
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