[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9044: fix: check upstream reference in traffic-split plugin when delete upstream

["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