fix: disallow creating duplicate plugins in global rules#12800
Conversation
Signed-off-by: Abhishek Choudhary <[email protected]>
dosubot
Bot
added
size:L
Signed-off-by: Abhishek Choudhary <[email protected]>
dosubot
Bot
added
size:XL
Signed-off-by: Abhishek Choudhary <[email protected]>
Signed-off-by: Abhishek Choudhary <[email protected]>
Signed-off-by: Abhishek Choudhary <[email protected]>
Signed-off-by: Abhishek Choudhary <[email protected]>
4df44ed
Signed-off-by: Abhishek Choudhary <[email protected]>
|
|
||
| -- remove duplicate plugins | ||
| for plugin_name, _ in pairs(duplicate_plugins) do | ||
| all_plugins[plugin_name] = nil |
There was a problem hiding this comment.
We should warn which plugins are not working.
There was a problem hiding this comment.
you are right, thanks. I pushed a new commit that contains:
core.log.warn("found ", plugin_name, " configured across different global rules ",
" it won't get executed")There was a problem hiding this comment.
I think duplicate_plugins are redundant.
Can we merge three loops into one?
Signed-off-by: Abhishek Choudhary <[email protected]>
Signed-off-by: Abhishek Choudhary <[email protected]>
Signed-off-by: Abhishek Choudhary <[email protected]>
Signed-off-by: Abhishek Choudhary <[email protected]>
0e6c6ce
Signed-off-by: Abhishek Choudhary <[email protected]>
This reverts commit 6313730.
This reverts commit 0e6c6ce.
|
lots of CI failed |
Signed-off-by: Nic <[email protected]>
nic-6443
changed the title
| if global_rule.value and global_rule.value.plugins then | ||
| for plugin_name, plugin_conf in pairs(global_rule.value.plugins) do | ||
| if seen_plugin[plugin_name] then | ||
| core.log.warn("Found ", plugin_name, |
There was a problem hiding this comment.
it is an important error message
There was a problem hiding this comment.
if we used one plugin about 3 or 5 times
we should throw an error in this case
current way seems wrong
Signed-off-by: Abhishek Choudhary <[email protected]>
|
Maybe it's too late to consider, but I've been testing around this change, and it does not consider that multiple global rules may be defined with different _meta.filter configuration that do not overlap. On the place I work for, we use this behaviour to define multiple traversal limit-count configurations where this limit changes based on the request. It make sense to consider this situation to allow multiple definitions that ensure only one is executed? It would require to traverse and discard the plugins by checking it's filtering first. If it's doable and make sense to allow this, I'm willing to try an implementation. If it sounds interesting, I can open a new issue. Thank you all :) |
|
Hi @rgalex2034, welcome to create an issue with your use case and configuration examples, and we will continue the discussion in your issue. |
7 participants
Description
When the same plugin is configured across different global rules, there will be no guarantee on which plugin config will be executed first. Additionally, the current architecture does not guarantee plugin execution order according to the priority.
We address this problem by disallowing configuring same plugin across multiple global_rules and sorting the plugins to their priority and executing in order.
Fixes: #12704
Checklist