You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "jiangfucheng (via GitHub)" <gi...@apache.org> on 2023/03/09 14:37:10 UTC
[GitHub] [apisix] jiangfucheng opened a new pull request, #9044: fix: check upstream reference in traffic-split plugin when delete upstream
jiangfucheng opened a new pull request, #9044:
URL: https://github.com/apache/apisix/pull/9044
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes https://github.com/apache/apisix/issues/5339
### Checklist
- [x] 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
- [x] 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] jiangfucheng commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9044:
URL: https://github.com/apache/apisix/pull/9044#discussion_r1135562822
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +38,102 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
+ for _, resource in ipairs(resources) do
+ if type(resource) == "table" and resource.value then
+ if up_id_in_plugins(resource.value.plugins, up_id) then
+
Review Comment:
Fixed. Do you have any good tools for automated detection of code formatting errors? I use vscode as my IDE, but haven't found a suitable plugin to do this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9044:
URL: https://github.com/apache/apisix/pull/9044#discussion_r1137980108
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +37,101 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
Review Comment:
We can remove the blank line here?
##########
apisix/consumer_group.lua:
##########
@@ -38,6 +38,12 @@ function _M.init_worker()
end
end
+function _M.consumer_groups()
Review Comment:
Please use two blanks between functions
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +37,101 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
Review Comment:
Where do we use the resources_ver? It seems checking resources is enough.
--
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] monkeyDluffy6017 merged pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9044:
URL: https://github.com/apache/apisix/pull/9044
--
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] jiangfucheng commented on pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9044:
URL: https://github.com/apache/apisix/pull/9044#issuecomment-1482424747
@soulbird Hi, could help me reivew 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] spacewander commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9044:
URL: https://github.com/apache/apisix/pull/9044#discussion_r1134805613
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +38,102 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
+ for _, resource in ipairs(resources) do
Review Comment:
Why don't we use `config_util.iterate_values`?
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +38,102 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
+ for _, resource in ipairs(resources) do
+ if type(resource) == "table" and resource.value then
+ if up_id_in_plugins(resource.value.plugins, up_id) then
+
Review Comment:
Can we remove the extra blank line?
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +38,102 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
+ for _, resource in ipairs(resources) do
+ if type(resource) == "table" and resource.value then
+ if up_id_in_plugins(resource.value.plugins, up_id) then
+
+ return {error_msg = "can not delete this upstream,"
+ .. " plugin in "
+ .. resources_name .. " ["
+ .. resource.value.id
+ .. "] is still using it now"}
+ end
+
+ if not only_check_plugin and resource.value.upstream_id
+ and tostring(resource.value.upstream_id) == up_id then
+
+ return {error_msg = "can not delete this upstream, "
+ .. resources_name .. " [" .. resource.value.id
+ .. "] is still using it now"}
+ end
end
end
end
+end
+
+
+local function delete_checker(id)
+ local routes, routes_ver = get_routes()
+ local err_msg = check_resources_reference(routes, routes_ver, id, false, "route")
+ if err_msg then
+ return 400, err_msg
+ end
local services, services_ver = get_services()
core.log.info("services: ", core.json.delay_encode(services, true))
core.log.info("services_ver: ", services_ver)
- 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
+ local err_msg = check_resources_reference(services, services_ver, id, false, "service")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ local plugin_configs, plugin_configs_ver = get_plugin_configs()
+ local err_msg = check_resources_reference(plugin_configs, plugin_configs_ver,
+ id, true, "plugin_config")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ local consumers, consumers_ver = get_consumers()
+ local err_msg = check_resources_reference(consumers, consumers_ver, id, true, "consumer")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ local consumer_groups, consumer_groups_ver = get_consumer_groups()
+ local err_msg = check_resources_reference(consumer_groups, consumer_groups_ver,
+ id, true, "consumer_group")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ -- TODO: Refactor router.global_rules and then refactor the following code
+ local global_rules = router.global_rules
+ if global_rules and global_rules.values
+ and #global_rules.values > 0 then
+
+ for _, global_rule in config_util.iterate_values(global_rules.values) do
+ if global_rule and global_rule.value
+ and global_rule.value.plugins
+ and up_id_in_plugins(global_rule.value.plugins, id) then
+
return 400, {error_msg = "can not delete this upstream,"
- .. " service [" .. service.value.id
+ .. " plugin in global_rule ["
+ .. global_rule.value.id
.. "] is still using it now"}
end
Review Comment:
You can give it a try.
--
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] jiangfucheng commented on pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9044:
URL: https://github.com/apache/apisix/pull/9044#issuecomment-1465114547
@spacewander Fixed all, thanks for reivew.
--
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] jiangfucheng commented on pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9044:
URL: https://github.com/apache/apisix/pull/9044#issuecomment-1495649701
@monkeyDluffy6017 Hi, this PR still not be merged, could help me merge 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] jiangfucheng commented on pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9044:
URL: https://github.com/apache/apisix/pull/9044#issuecomment-1482425351
@soulbird Hi, could help me reivew 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] spacewander commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9044:
URL: https://github.com/apache/apisix/pull/9044#discussion_r1131887802
##########
apisix/admin/upstreams.lua:
##########
@@ -33,17 +35,43 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
+
+ for _, rule in ipairs (plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream and plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == tostring(up_id) then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
local function delete_checker(id)
local routes, routes_ver = get_routes()
if routes_ver and routes then
for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ if type(route) == "table" and route.value then
+ if up_id_in_plugins(route.value.plugins, id) == true then
+ return 400, {error_msg = "can not delete this upstream,"
+ .. " traffic-split plugin in route [" .. route.value.id
Review Comment:
Better not to mention a particular plugin name, as we might add other plugin check later
##########
apisix/admin/upstreams.lua:
##########
@@ -53,11 +81,35 @@ local function delete_checker(id)
core.log.info("services_ver: ", services_ver)
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
+ if type(service) == "table" and service.value then
+ if up_id_in_plugins(service.value.plugins, id) then
+ return 400, {error_msg = "can not delete this upstream,"
+ .. " traffic-split plugin in service ["
+ .. service.value.id
+ .. "] is still using it now"}
+ end
+
+ if 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
+ end
Review Comment:
We should also check plugin_config, consumer and consumer group.
##########
t/admin/upstream5.t:
##########
@@ -111,3 +111,266 @@ passed
}
--- response_body
passed
+
+
+
+=== TEST4: prepare upstream
Review Comment:
```suggestion
=== TEST 4: prepare upstream
```
##########
apisix/admin/upstreams.lua:
##########
@@ -33,17 +35,43 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
+
+ for _, rule in ipairs (plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream and plugin_upstream.upstream_id
Review Comment:
We don't need to check plugin_upstream as there is no hole in the array
##########
apisix/admin/upstreams.lua:
##########
@@ -33,17 +35,43 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
+
+ for _, rule in ipairs (plugins["traffic-split"].rules) do
Review Comment:
```suggestion
for _, rule in ipairs(plugins["traffic-split"].rules) do
```
##########
apisix/admin/upstreams.lua:
##########
@@ -33,17 +35,43 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
+
+ for _, rule in ipairs (plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream and plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == tostring(up_id) then
Review Comment:
The up_id is already a string
--
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] jiangfucheng commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9044:
URL: https://github.com/apache/apisix/pull/9044#discussion_r1133063086
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +38,102 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
+ for _, resource in ipairs(resources) do
+ if type(resource) == "table" and resource.value then
+ if up_id_in_plugins(resource.value.plugins, up_id) then
+
+ return {error_msg = "can not delete this upstream,"
+ .. " plugin in "
+ .. resources_name .. " ["
+ .. resource.value.id
+ .. "] is still using it now"}
+ end
+
+ if not only_check_plugin and resource.value.upstream_id
+ and tostring(resource.value.upstream_id) == up_id then
+
+ return {error_msg = "can not delete this upstream, "
+ .. resources_name .. " [" .. resource.value.id
+ .. "] is still using it now"}
+ end
end
end
end
+end
+
+
+local function delete_checker(id)
+ local routes, routes_ver = get_routes()
+ local err_msg = check_resources_reference(routes, routes_ver, id, false, "route")
+ if err_msg then
+ return 400, err_msg
+ end
local services, services_ver = get_services()
core.log.info("services: ", core.json.delay_encode(services, true))
core.log.info("services_ver: ", services_ver)
- 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
+ local err_msg = check_resources_reference(services, services_ver, id, false, "service")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ local plugin_configs, plugin_configs_ver = get_plugin_configs()
+ local err_msg = check_resources_reference(plugin_configs, plugin_configs_ver,
+ id, true, "plugin_config")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ local consumers, consumers_ver = get_consumers()
+ local err_msg = check_resources_reference(consumers, consumers_ver, id, true, "consumer")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ local consumer_groups, consumer_groups_ver = get_consumer_groups()
+ local err_msg = check_resources_reference(consumer_groups, consumer_groups_ver,
+ id, true, "consumer_group")
+ if err_msg then
+ return 400, err_msg
+ end
+
+ -- TODO: Refactor router.global_rules and then refactor the following code
+ local global_rules = router.global_rules
+ if global_rules and global_rules.values
+ and #global_rules.values > 0 then
+
+ for _, global_rule in config_util.iterate_values(global_rules.values) do
+ if global_rule and global_rule.value
+ and global_rule.value.plugins
+ and up_id_in_plugins(global_rule.value.plugins, id) then
+
return 400, {error_msg = "can not delete this upstream,"
- .. " service [" .. service.value.id
+ .. " plugin in global_rule ["
+ .. global_rule.value.id
.. "] is still using it now"}
end
Review Comment:
Perhaps we could refactor this code in a later pull request. We can move the `router.global_rules` to a separate file named `global_rule.lua` and use the new `global_rules()` method to replace the current `router.global_rules` field. This will ensure consistency with the style of other components. However, I am not very familiar with APISIX's code at present, so I am unsure if this is feasible.
--
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] jiangfucheng commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9044:
URL: https://github.com/apache/apisix/pull/9044#discussion_r1135556251
##########
apisix/admin/upstreams.lua:
##########
@@ -33,31 +38,102 @@ local function check_conf(id, conf, need_id)
return true
end
+local function up_id_in_plugins(plugins, up_id)
+ if plugins and plugins["traffic-split"]
+ and plugins["traffic-split"].rules then
-local function delete_checker(id)
- local routes, routes_ver = get_routes()
- if routes_ver and routes then
- for _, route in ipairs(routes) do
- if type(route) == "table" and route.value
- and route.value.upstream_id
- and tostring(route.value.upstream_id) == id then
- return 400, {error_msg = "can not delete this upstream,"
- .. " route [" .. route.value.id
- .. "] is still using it now"}
+ for _, rule in ipairs(plugins["traffic-split"].rules) do
+ local plugin_upstreams = rule.weighted_upstreams
+
+ for _, plugin_upstream in ipairs(plugin_upstreams) do
+ if plugin_upstream.upstream_id
+ and tostring(plugin_upstream.upstream_id) == up_id then
+
+ return true
+ end
+ end
+ end
+
+ return false
+ end
+end
+
+local function check_resources_reference(resources, resources_ver, up_id,
+ only_check_plugin, resources_name)
+
+ if resources and resources_ver then
+ for _, resource in ipairs(resources) do
Review Comment:
My mistake, I didn't have a deep enough understanding of the logic here before.
--
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