-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: check upstream reference in traffic-split plugin when delete upstream #9044
Conversation
apisix/admin/upstreams.lua
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can give it a try.
@spacewander Fixed all, thanks for reivew. |
apisix/admin/upstreams.lua
Outdated
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the extra blank line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
apisix/admin/upstreams.lua
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can give it a try.
apisix/admin/upstreams.lua
Outdated
only_check_plugin, resources_name) | ||
|
||
if resources and resources_ver then | ||
for _, resource in ipairs(resources) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use config_util.iterate_values
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I didn't have a deep enough understanding of the logic here before.
4ec4c55
to
867ed12
Compare
@@ -38,6 +38,12 @@ function _M.init_worker() | |||
end | |||
end | |||
|
|||
function _M.consumer_groups() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use two blanks between functions
apisix/admin/upstreams.lua
Outdated
local function check_resources_reference(resources, resources_ver, up_id, | ||
only_check_plugin, resources_name) | ||
|
||
if resources and resources_ver then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we use the resources_ver? It seems checking resources is enough.
@soulbird Hi, could help me reivew this PR? |
@monkeyDluffy6017 Hi, this PR still not be merged, could help me merge it? |
* upstream/master: (25 commits) fix: upgrade lua-resty-ldap to 0.2.2 (apache#9254) feat(cli): support bypassing Admin API Auth by configuration (apache#9147) fix(ci): write version into xds first (apache#9274) fix: skip warning log when apisix.data_encryption.enable is false (apache#9057) docs: add-api7-information (apache#9260) docs: Fixed typo (apache#9244) docs: clarify what is client.ca in client-to-apisix-mtls.md (apache#9221) docs: Corrected typos and grammatical errors (apache#9216) docs: updated ssl sni parameter requirement in admin-api.md (apache#9176) fix: check upstream reference in traffic-split plugin when delete upstream (apache#9044) docs: Update proxy-rewrite headers.add docs (apache#9220) feat: suppot header injection for fault-injection plugin (apache#9039) fix: upgrade lua-resty-etcd to 1.10.4 (apache#9235) docs: fix incorrect semantic.yml link (apache#9231) feat: Upstream status report (apache#9151) fix: host_hdr should not be false (apache#9150) docs: remove APISIX base instruction (apache#9117) fix(cli): prevent non-`127.0.0.0/24` to access admin api with empty admin_key (apache#9146) docs: fix 404 link (apache#9160) fix(cors): consider using `allow_origins_by_regex` only when it is not `nil` (apache#9028) ...
Description
Fixes #5339
Checklist