Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
membphis commented on PR #12800: URL: https://github.com/apache/apisix/pull/12800#issuecomment-3686046229 lots of CI failed -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
shreemaan-abhishek commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2632069029
##
apisix/plugin.lua:
##
@@ -1263,7 +1267,59 @@ function _M.set_plugins_meta_parent(plugins, parent)
end
-function _M.run_global_rules(api_ctx, global_rules, phase_name)
+local function merge_global_rules(global_rules)
+-- First pass: identify duplicate plugins across all global rules
+local plugin_count = {}
+local duplicate_plugins = {}
+local values = global_rules
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+for plugin_name, _ in pairs(global_rule.value.plugins) do
+plugin_count[plugin_name] = (plugin_count[plugin_name] or 0) +
1
+if plugin_count[plugin_name] > 1 then
+duplicate_plugins[plugin_name] = true
+end
+end
+end
+end
+
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
+end
+end
+end
+
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
AlinsRan commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2625806646
##
apisix/plugin.lua:
##
@@ -1263,7 +1267,59 @@ function _M.set_plugins_meta_parent(plugins, parent)
end
-function _M.run_global_rules(api_ctx, global_rules, phase_name)
+local function merge_global_rules(global_rules)
+-- First pass: identify duplicate plugins across all global rules
+local plugin_count = {}
+local duplicate_plugins = {}
+local values = global_rules
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+for plugin_name, _ in pairs(global_rule.value.plugins) do
+plugin_count[plugin_name] = (plugin_count[plugin_name] or 0) +
1
+if plugin_count[plugin_name] > 1 then
+duplicate_plugins[plugin_name] = true
+end
+end
+end
+end
+
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
+end
+end
+end
+
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
Review Comment:
I think `duplicate_plugins` are redundant.
Can we merge three loops into one?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
shreemaan-abhishek commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2625790130
##
apisix/plugin.lua:
##
@@ -1263,7 +1267,59 @@ function _M.set_plugins_meta_parent(plugins, parent)
end
-function _M.run_global_rules(api_ctx, global_rules, phase_name)
+local function merge_global_rules(global_rules)
+-- First pass: identify duplicate plugins across all global rules
+local plugin_count = {}
+local duplicate_plugins = {}
+local values = global_rules
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+for plugin_name, _ in pairs(global_rule.value.plugins) do
+plugin_count[plugin_name] = (plugin_count[plugin_name] or 0) +
1
+if plugin_count[plugin_name] > 1 then
+duplicate_plugins[plugin_name] = true
+end
+end
+end
+end
+
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
+end
+end
+end
+
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
Review Comment:
you are right, thanks. I pushed a new commit that contains:
```lua
core.log.warn("found ", plugin_name, " configured across different
global rules ",
" it won't get executed")
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
AlinsRan commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2625778572
##
apisix/plugin.lua:
##
@@ -1263,7 +1267,59 @@ function _M.set_plugins_meta_parent(plugins, parent)
end
-function _M.run_global_rules(api_ctx, global_rules, phase_name)
+local function merge_global_rules(global_rules)
+-- First pass: identify duplicate plugins across all global rules
+local plugin_count = {}
+local duplicate_plugins = {}
+local values = global_rules
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+for plugin_name, _ in pairs(global_rule.value.plugins) do
+plugin_count[plugin_name] = (plugin_count[plugin_name] or 0) +
1
+if plugin_count[plugin_name] > 1 then
+duplicate_plugins[plugin_name] = true
+end
+end
+end
+end
+
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
+end
+end
+end
+
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
Review Comment:
We should warn which plugins are not working.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
shreemaan-abhishek commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2622021870
##
apisix/admin/global_rules.lua:
##
@@ -31,6 +35,32 @@ local function check_conf(id, conf, need_id, schema)
return nil, {error_msg = err}
end
+-- Check for plugin conflicts with existing global rules
+if conf.plugins then
+local global_rules = global_rules_mod.global_rules()
+if global_rules then
+for _, existing_rule in ipairs(global_rules) do
+-- Skip checking against itself when updating
+if existing_rule.value and existing_rule.value.id and
+ tostring(existing_rule.value.id) ~= tostring(id) then
+
+if existing_rule.value.plugins then
+-- Check for any overlapping plugins
+for plugin_name, _ in pairs(conf.plugins) do
+if existing_rule.value.plugins[plugin_name] then
+return nil, {
+error_msg = "plugin '" .. plugin_name ..
+"' already exists in global rule with id
'" ..
+existing_rule.value.id .. "'"
+}
+end
+end
+end
+end
+end
+end
+end
Review Comment:
good idea, let me fix 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
bzp2010 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2616671520
##
apisix/admin/global_rules.lua:
##
@@ -31,6 +35,32 @@ local function check_conf(id, conf, need_id, schema)
return nil, {error_msg = err}
end
+-- Check for plugin conflicts with existing global rules
+if conf.plugins then
+local global_rules = global_rules_mod.global_rules()
+if global_rules then
+for _, existing_rule in ipairs(global_rules) do
+-- Skip checking against itself when updating
+if existing_rule.value and existing_rule.value.id and
+ tostring(existing_rule.value.id) ~= tostring(id) then
+
+if existing_rule.value.plugins then
+-- Check for any overlapping plugins
+for plugin_name, _ in pairs(conf.plugins) do
+if existing_rule.value.plugins[plugin_name] then
+return nil, {
+error_msg = "plugin '" .. plugin_name ..
+"' already exists in global rule with id
'" ..
+existing_rule.value.id .. "'"
+}
+end
+end
+end
+end
+end
+end
+end
Review Comment:
I suspect this might be a problem. If an operation has already been executed
but the etcd watch hasn't yet received the update event to refresh the
in-memory cache, this validation could be unexpectedly bypassed.
Shouldn't we call the API here to fetch the latest list of global rules?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2601529736
##
apisix/plugin.lua:
##
@@ -1263,7 +1267,59 @@ function _M.set_plugins_meta_parent(plugins, parent)
end
-function _M.run_global_rules(api_ctx, global_rules, phase_name)
+local function merge_global_rules(global_rules)
+-- First pass: identify duplicate plugins across all global rules
+local plugin_count = {}
+local duplicate_plugins = {}
+local values = global_rules
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+for plugin_name, _ in pairs(global_rule.value.plugins) do
+plugin_count[plugin_name] = (plugin_count[plugin_name] or 0) +
1
+if plugin_count[plugin_name] > 1 then
+duplicate_plugins[plugin_name] = true
+end
+end
+end
+end
+
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
+end
+end
+end
+
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
+end
+
+local dummy_global_rule = {
+key = "/apisix/global_rules/1",
Review Comment:
give it a special name for easy distinguish
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2601528839
##
apisix/plugin.lua:
##
@@ -1263,7 +1267,59 @@ function _M.set_plugins_meta_parent(plugins, parent)
end
-function _M.run_global_rules(api_ctx, global_rules, phase_name)
+local function merge_global_rules(global_rules)
+-- First pass: identify duplicate plugins across all global rules
+local plugin_count = {}
+local duplicate_plugins = {}
+local values = global_rules
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+for plugin_name, _ in pairs(global_rule.value.plugins) do
+plugin_count[plugin_name] = (plugin_count[plugin_name] or 0) +
1
+if plugin_count[plugin_name] > 1 then
+duplicate_plugins[plugin_name] = true
+end
+end
+end
+end
+
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
+for _, global_rule in config_util.iterate_values(values) do
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
+end
+end
+end
+
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
+end
+
+local dummy_global_rule = {
+key = "/apisix/global_rules/1",
Review Comment:
```suggestion
key = "/apisix/global_rules/dummy",
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2601506921
##
apisix/plugin.lua:
##
@@ -1273,22 +1329,26 @@ function _M.run_global_rules(api_ctx, global_rules,
phase_name)
api_ctx.global_rules = global_rules
end
+local dummy_global_rule = merge_global_rule_lrucache(conf_version,
global_rules, merge_global_rules, global_rules)
+if not dummy_global_rule then
+core.log.error("failed to get merged global rules form cache,
using merge_global_rules instead")
+dummy_global_rule = merge_global_rules(global_rules)
Review Comment:
This is unnecessary, lruchche will automatically execute
`merge_global_rules` when the request key does not exist.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2601218177
##
apisix/plugin.lua:
##
@@ -1288,37 +1288,52 @@ function _M.run_global_rules(api_ctx, global_rules,
phase_name)
end
end
-local plugins = core.tablepool.fetch("plugins", 32, 0)
-local route = api_ctx.matched_route
+local all_plugins = {}
+local createdIndex = 1
+local modifiedIndex = 1
+-- merge all plugins across all global rules to one dummy global_rule
structure
for _, global_rule in config_util.iterate_values(values) do
-api_ctx.conf_type = "global_rule"
-api_ctx.conf_version = global_rule.modifiedIndex
-api_ctx.conf_id = global_rule.value.id
-
-core.table.clear(plugins)
-plugins = _M.filter(api_ctx, global_rule, plugins, route)
-
--- Remove duplicate plugins from the plugins table
-local i = 1
-while i <= #plugins do
-local plugin = plugins[i]
-local plugin_name = plugin.name
-if duplicate_plugins[plugin_name] then
--- Remove duplicate plugin and its config
-core.table.remove(plugins, i)
-core.table.remove(plugins, i)
-else
--- Move to next plugin pair
-i = i + 2
+if global_rule.value and global_rule.value.plugins then
+core.table.merge(all_plugins, global_rule.value.plugins)
+if global_rule.modifiedIndex > modifiedIndex then
+modifiedIndex = global_rule.modifiedIndex
+createdIndex = global_rule.createdIndex
end
end
+end
-if phase_name == nil then
-_M.run_plugin("rewrite", plugins, api_ctx)
-_M.run_plugin("access", plugins, api_ctx)
-else
-_M.run_plugin(phase_name, plugins, api_ctx)
-end
+-- remove duplicate plugins
+for plugin_name, _ in pairs(duplicate_plugins) do
+all_plugins[plugin_name] = nil
+end
+
+local dummy_global_rule = {
+key = "/apisix/global_rules/1",
+value = {
+updated_time = ngx.time(),
+plugins = all_plugins,
+created_time = ngx.time(),
+id = 1,
+},
+createdIndex = createdIndex,
+modifiedIndex = modifiedIndex,
+clean_handlers = {},
+}
Review Comment:
I think we need to cache the result of this merge. We can use
`global_rules.conf_version` as the cache key and the final `modifiedIndex` to
avoid executing the merge code repeatedly for each request.
https://github.com/apache/apisix/blob/896d3c389196e7a06fddeb5c03006bf300fa4704/apisix/global_rules.lua#L45
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800: URL: https://github.com/apache/apisix/pull/12800#discussion_r2600673750 ## apisix/plugin.lua: ## @@ -1283,6 +1297,22 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name) core.table.clear(plugins) plugins = _M.filter(api_ctx, global_rule, plugins, route) + +-- Remove duplicate plugins from the plugins table +local i = 1 +while i <= #plugins do +local plugin = plugins[i] +local plugin_name = plugin.name +if duplicate_plugins[plugin_name] then +-- Remove duplicate plugin and its config +core.table.remove(plugins, i) +core.table.remove(plugins, i) +else +-- Move to next plugin pair +i = i + 2 +end +end + Review Comment: We not only need to remove duplicate plugins that appear in the global rule, but also merge the remaining plugins into a `plugins` table before executing them. Otherwise, the execution order of the plugins will depend on the order of the `values` array rather than plugin priority, which is unintended.。 ``` for _, global_rule in config_util.iterate_values(values) do ... end ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2600664530
##
apisix/admin/global_rules.lua:
##
@@ -31,6 +35,33 @@ local function check_conf(id, conf, need_id, schema)
return nil, {error_msg = err}
end
+-- Check for plugin conflicts with existing global rules
+if conf.plugins then
+local global_rules = global_rules_mod.global_rules()
+core.log.info("dibag global_rules: ", core.json.encode(global_rules))
Review Comment:
remove temporary debug log
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] change: disallow creating duplicate plugins in global rules [apisix]
nic-6443 commented on code in PR #12800:
URL: https://github.com/apache/apisix/pull/12800#discussion_r2600664530
##
apisix/admin/global_rules.lua:
##
@@ -31,6 +35,33 @@ local function check_conf(id, conf, need_id, schema)
return nil, {error_msg = err}
end
+-- Check for plugin conflicts with existing global rules
+if conf.plugins then
+local global_rules = global_rules_mod.global_rules()
+core.log.info("dibag global_rules: ", core.json.encode(global_rules))
Review Comment:
temporary debug log should be removed
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
