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

biakewe · 2022-10-31T17:01:12Z

…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 first)

…oute

spacewander

Could you add test in https://github.com/apache/apisix/blob/master/t/admin/stream-routes.t?
You can find some info at https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md

biakewe · 2022-11-01T14:20:02Z

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?

Can you give me some pointers? @spacewander

tzssangglass · 2022-11-02T07:54:07Z

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)

biakewe · 2022-11-02T10:08:06Z

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)

biakewe · 2022-11-02T10:14:56Z

Some problems have been changed； thank for the review， and hope to communicate @spacewander @tzssangglass

spacewander · 2022-11-03T08:21:16Z

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'

biakewe · 2022-11-03T08:53:42Z

please approve running workflows @spacewander

tzssangglass · 2022-11-04T10:38:41Z

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]
+


need three blank lines between test cases

tzssangglass · 2022-11-04T10:40:15Z

apisix/admin/stream_routes.lua

+    local referkey = "/stream_routes_refer/" .. id
+    local _, err = core.etcd.delete(referkey)
+    core.log.warn(err)
+    core.log.warn(items)


Suggested change

core.log.warn(items)

core.log.info("items:", core.json.delay_encode(items))

tzssangglass · 2022-11-04T10:41:16Z

apisix/admin/stream_routes.lua

+    local refer_list = {}
+    local referkey = "/stream_routes_refer/" .. id
+    local _, err = core.etcd.delete(referkey)
+    core.log.warn(err)


Is the err here only printed and not processed?

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

tzssangglass · 2022-11-04T10:44:12Z

apisix/admin/stream_routes.lua

+            else
+                core.log.error("In function check_router_refer  error: ", err)
+            end
+            local setres, err = core.etcd.set(setkey, data)


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?

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.

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?

apisix/apisix/admin/upstreams.lua

Lines 146 to 156 in a62d33f

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

Thanks for your suggestion I changed the implementation.

tzssangglass · 2022-11-04T10:45:39Z

apisix/admin/stream_routes.lua

+        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)


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.

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。

tzssangglass · 2022-11-07T02:11:44Z

apisix/admin/stream_routes.lua

+        if item.value == nil then
+            goto CONTINUE
+        end
+        local r_id = string.gsub(item["key"], "/", "_")


can we replace string.gsub with ngx.re.gsub?

biakewe · 2022-11-17T07:57:36Z

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

spacewander · 2022-11-17T09:21:24Z

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()


What about moving the init (stream_init_worker) to

apisix/apisix/admin/init.lua

Line 353 in 2f4a4db

?

We should only initialize it when stream_proxy is enabled:

apisix/apisix/admin/init.lua

Line 145 in 2f4a4db

if not local_conf.apisix.stream_proxy then

Modified，please have a look

tzssangglass · 2022-11-18T01:59:28Z

apisix/admin/init.lua

+        core.config.init()
+        local router_stream = require("apisix.stream.router.ip_port")
+        router_stream.stream_init_worker(filter)


here: #8219 (comment) means move router_stream.stream_init_worker(filter) to function _M.init_worker(), not local function run().

spacewander · 2022-11-18T06:40:53Z

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


We need to export filter first so we can use it:

apisix/apisix/router.lua

Line 30 in ae400b9

local function filter(route)

spacewander · 2022-11-21T07:02:21Z

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


Does module apisix.router export the function filter?
BTW, the require call should be put under local require = require.

spacewander · 2022-11-23T07:39:31Z

apisix/admin/init.lua

@@ -15,6 +15,7 @@
 -- limitations under the License.
 --
 local require = require
+local filter = require("apisix.router").filter


Does module "apisix.router" export the filter function?

…om/biakecw/apisix into check-the-subordinate-relationship

spacewander · 2022-11-25T05:56:18Z

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()


We need to export the filter and pass it into stream_init_worker, otherwise, the data structure may be different.

github-actions · 2023-01-24T10:04:38Z

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 [email protected] list. Thank you for your contributions.

github-actions · 2023-03-31T10:04:50Z

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.

validate the subordinate and check reference when deleting a stream r…

5d10f41

…oute

biakewe changed the title ~~validate the subordinate and check reference when deleting a stream …~~ feat: validate the subordinate and check reference when deleting a stream … Oct 31, 2022

caowang added 3 commits November 1, 2022 01:04

validate the subordinate and check reference when deleting a stream r…

734b43f

…oute

validate the subordinate and check reference when deleting a stream r…

4f13eed

…oute

validate the subordinate and check reference when deleting a stream r…

f0f6ff9

…oute

spacewander reviewed Nov 1, 2022

View reviewed changes

caowang added 2 commits November 1, 2022 18:47

add test cases to check res.refer

9168493

fix cache in refer

cc276f7

fix some bugs

7db5790

biakewe closed this Nov 2, 2022

biakewe reopened this Nov 2, 2022

fix (ci check fail)

34f79d2

caowang added 2 commits November 3, 2022 17:03

fix (ci check fail)

4dde2e9

format

cba2083

tzssangglass reviewed Nov 4, 2022

View reviewed changes

another imp no etcd

94cdf53

biakewe force-pushed the check-the-subordinate-relationship branch from dac6b2b to 94cdf53 Compare November 5, 2022 07:35

caowang added 2 commits November 5, 2022 15:43

another imp no etcd

b6cc11e

fix errors in CI stage

bb5a2a5

tzssangglass reviewed Nov 7, 2022

View reviewed changes

no-etcd implementation

63ff87f

biakewe dismissed tzssangglass’s stale review via 63ff87f November 16, 2022 16:02

caowang added 4 commits November 17, 2022 00:05

no-etcd implementation

11315d8

no-etcd implementation

23d4ace

update test case

5f4c836

update test case

aafaaf2

spacewander reviewed Nov 17, 2022

View reviewed changes

caowang added 3 commits November 17, 2022 19:11

update init

be47504

update init

9289dd7

update init

3f85069

tzssangglass reviewed Nov 18, 2022

View reviewed changes

update admin.init

d9f3a0a

tzssangglass previously approved these changes Nov 18, 2022

View reviewed changes

spacewander reviewed Nov 18, 2022

View reviewed changes

update admin.init

dbd0e02

biakewe dismissed tzssangglass’s stale review via dbd0e02 November 18, 2022 07:44

tzssangglass previously approved these changes Nov 21, 2022

View reviewed changes

spacewander reviewed Nov 21, 2022

View reviewed changes

update

1517c85

biakewe dismissed tzssangglass’s stale review via 1517c85 November 21, 2022 09:38

Merge branch 'master' into check-the-subordinate-relationship

cd17fcc

spacewander reviewed Nov 23, 2022

View reviewed changes

caowang added 2 commits November 24, 2022 17:43

update

401ba10

Merge branch 'check-the-subordinate-relationship' of https://github.c…

2c0a1b1

…om/biakecw/apisix into check-the-subordinate-relationship

spacewander reviewed Nov 25, 2022

View reviewed changes

github-actions bot added the stale label Jan 24, 2023

github-actions bot closed this Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

biakewe commented Oct 31, 2022 •

edited

Loading

spacewander left a comment

biakewe commented Nov 1, 2022 •

edited

Loading

tzssangglass commented Nov 2, 2022

biakewe commented Nov 2, 2022

biakewe commented Nov 2, 2022

spacewander commented Nov 3, 2022

biakewe commented Nov 3, 2022

tzssangglass Nov 4, 2022

biakewe Nov 4, 2022

tzssangglass Nov 4, 2022

biakewe Nov 4, 2022

tzssangglass Nov 4, 2022

biakewe Nov 4, 2022

tzssangglass Nov 4, 2022

biakewe Nov 4, 2022

tzssangglass Nov 4, 2022

biakewe Nov 5, 2022

tzssangglass Nov 4, 2022

biakewe Nov 4, 2022 •

edited

Loading

tzssangglass Nov 7, 2022

biakewe commented Nov 17, 2022 •

edited

Loading

spacewander Nov 17, 2022

biakewe Nov 17, 2022

tzssangglass Nov 18, 2022

biakewe Nov 18, 2022

spacewander Nov 18, 2022

spacewander Nov 21, 2022

spacewander Nov 23, 2022

biakewe Nov 24, 2022

spacewander Nov 25, 2022

github-actions bot commented Jan 24, 2023

github-actions bot commented Mar 31, 2023

	core.log.warn(items)
	core.log.info("items:", core.json.delay_encode(items))

	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

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

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

Conversation

biakewe commented Oct 31, 2022 • edited Loading

Description

Checklist

spacewander left a comment

Choose a reason for hiding this comment

biakewe commented Nov 1, 2022 • edited Loading

tzssangglass commented Nov 2, 2022

biakewe commented Nov 2, 2022

biakewe commented Nov 2, 2022

spacewander commented Nov 3, 2022

biakewe commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biakewe Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biakewe commented Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2023

github-actions bot commented Mar 31, 2023

biakewe commented Oct 31, 2022 •

edited

Loading

biakewe commented Nov 1, 2022 •

edited

Loading

biakewe Nov 4, 2022 •

edited

Loading

biakewe commented Nov 17, 2022 •

edited

Loading