Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: validate the subordinate and check reference when deleting and adding… #8219

Closed
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
5d10f41
validate the subordinate and check reference when deleting a stream …
Oct 31, 2022
734b43f
validate the subordinate and check reference when deleting a stream …
Oct 31, 2022
4f13eed
validate the subordinate and check reference when deleting a stream …
Oct 31, 2022
f0f6ff9
validate the subordinate and check reference when deleting a stream …
Oct 31, 2022
9168493
add test cases to check res.refer
Nov 1, 2022
cc276f7
fix cache in refer
Nov 1, 2022
7db5790
fix some bugs
Nov 2, 2022
34f79d2
fix (ci check fail)
Nov 3, 2022
4dde2e9
fix (ci check fail)
Nov 3, 2022
cba2083
format
Nov 4, 2022
94cdf53
another imp no etcd
Nov 5, 2022
b6cc11e
another imp no etcd
Nov 5, 2022
bb5a2a5
fix errors in CI stage
Nov 6, 2022
f04b2c5
style:
Nov 7, 2022
475007e
revise tablepool
Nov 7, 2022
527110c
Update stream_routes.lua
biakewe Nov 8, 2022
9f08dbb
drop trailing whitespace
Nov 9, 2022
02709ab
fix test case
Nov 9, 2022
4ecd49f
fix test case
Nov 9, 2022
3c7d4d3
fix test case
Nov 9, 2022
dee8669
fix test case
Nov 9, 2022
5aec5cb
fix APISIX.PM
Nov 10, 2022
1ae67e6
fix APISIX.PM
Nov 10, 2022
9f56771
debug
Nov 10, 2022
71c0f89
Get stream_routes from etcd directly
Nov 11, 2022
2fc55c9
Get stream_routes from etcd directly
Nov 11, 2022
e1894f8
Update stream-routes.t
biakewe Nov 11, 2022
63f4d7b
update
Nov 14, 2022
775fff0
Merge branch 'check-the-subordinate-relationship' of https://github.c…
Nov 14, 2022
f322e4f
Validate if the protocol matches the subordinate only if the stream r…
Nov 14, 2022
b5dba07
Validate if the protocol matches the subordinate only if the stream r…
Nov 14, 2022
d69ce76
Validate if the protocol matches the subordinate only if the stream r…
Nov 14, 2022
f6d98e2
fix
Nov 14, 2022
a64bb1e
fix
Nov 14, 2022
2854be0
fix
Nov 14, 2022
ed245b5
fix
Nov 14, 2022
c15e3e6
fix
Nov 14, 2022
49609a7
fix
Nov 14, 2022
3205a75
fix
Nov 14, 2022
0cbd892
debug test case
Nov 14, 2022
573578d
debug test case
Nov 14, 2022
09e34e7
debug test case
Nov 14, 2022
8ab326c
update
Nov 14, 2022
9b1533d
fix indent
Nov 15, 2022
63ff87f
no-etcd implementation
Nov 16, 2022
11315d8
no-etcd implementation
Nov 16, 2022
23d4ace
no-etcd implementation
Nov 16, 2022
5f4c836
update test case
Nov 17, 2022
aafaaf2
update test case
Nov 17, 2022
be47504
update init
Nov 17, 2022
9289dd7
update init
Nov 17, 2022
3f85069
update init
Nov 17, 2022
d9f3a0a
update admin.init
Nov 18, 2022
dbd0e02
update admin.init
Nov 18, 2022
1517c85
update
Nov 21, 2022
cd17fcc
Merge branch 'master' into check-the-subordinate-relationship
biakewe Nov 23, 2022
401ba10
update
Nov 24, 2022
2c0a1b1
Merge branch 'check-the-subordinate-relationship' of https://github.c…
Nov 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion apisix/admin/stream_routes.lua
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
--
local core = require("apisix.core")
local utils = require("apisix.admin.utils")
local config_util = require("apisix.core.config_util")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
local config_util = require("apisix.core.config_util")
local iterate_values = require("apisix.core.config_util").iterate_values

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

Expand All @@ -25,6 +27,51 @@ local _M = {
need_v3_filter = true,
}

local function check_router_refer(items, key)
local refer_list = {}
for _, item in config_util.iterate_values(items) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for _, item in config_util.iterate_values(items) do
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 then
local data
local setkey="/stream_routes/"..route.protocol.superior_id.."/refer"
local res, err = core.etcd.get(setkey,false)
if res then
if #res.body.node.value == 0 then
local v = core.json.decode("{}")
local r_id = item["key"]
v[r_id]=1
data = core.json.encode(v)
else
local v = core.json.decode(res.body.node.value)
local r_id = item["key"]
v[r_id]=1
data = core.json.encode(v)
end
end
local setres, err = core.etcd.set(setkey, data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if services_ver and services then
for _, service in ipairs(services) do
if type(service) == "table" and service.value
and service.value.upstream_id
and tostring(service.value.upstream_id) == id then
return 400, {error_msg = "can not delete this upstream,"
.. " service [" .. service.value.id
.. "] is still using it now"}
end
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion I changed the implementation.

if not setres then
core.log.error("failed to put stream route[", key, "]: ", err)
end
end
::CONTINUE::
end
local referkey = key .. "/refer"
local rescheck, err = core.etcd.get(referkey,false)
if rescheck then
if rescheck.body.node ~= nil then
local refer_values=core.json.decode(rescheck.body.node.value)
for v,_ in pairs(refer_values) do
table.insert(refer_list,v)
end
end
end
return refer_list
end



local function check_conf(id, conf, need_id)
if not conf then
Expand Down Expand Up @@ -142,14 +189,23 @@ function _M.delete(id)
return 400, {error_msg = "missing stream route id"}
end

local items,_ = routes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
local items,_ = routes()
local items, _ = routes()

local key = "/stream_routes/" .. id
-- core.log.info("key: ", key)
local refer_list=check_router_refer(items,key)
local warn_message
if #refer_list >0 then
warn_message = key.." is refered by "..table.concat(refer_list,";;")
else
warn_message = key.." is refered by None"
end

local res, err = core.etcd.delete(key)
if not res then
core.log.error("failed to delete stream route[", key, "]: ", err)
return 503, {error_msg = err}
end

res.body["refer"]=warn_message
return res.status, res.body
end

Expand Down
1 change: 1 addition & 0 deletions apisix/cli/ngx_tpl.lua
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,7 @@ http {

init_worker_by_lua_block {
apisix.http_init_worker()
apisix.stream_init_worker()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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。

}

exit_worker_by_lua_block {
Expand Down
29 changes: 26 additions & 3 deletions apisix/stream/router/ip_port.lua
Original file line number Diff line number Diff line change
Expand Up @@ -63,23 +63,45 @@ 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("RPC procotol is different in stream_route:"..item.key.." and "..key)
end
goto CONTINUE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we don't need to modify here? I see that the test cases don't cover the changes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, no action here, just warn logging。In order to remind users, do you think it's ok to keep this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@biakewe biakewe Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it simple and clear enough in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

end

Expand Down Expand Up @@ -146,7 +168,7 @@ do
if router_ver ~= user_routes.conf_version then
local err = create_router(user_routes.values)
if err then
return false, "failed to create router: " .. err
return false, "failed to create router: " .. err
end

router_ver = user_routes.conf_version
Expand Down Expand Up @@ -236,6 +258,7 @@ function _M.stream_init_worker(filter)
checker = stream_route_checker,
filter = filter,
})


if not user_routes then
error("failed to create etcd instance for fetching /stream_routes : "
Expand Down