-
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: should assign value to api_ctx.global_rules
before running global rules
#3595
Conversation
Is this a bug? Which PR introduced it? |
apisix/init.lua
Outdated
@@ -297,8 +297,8 @@ function _M.http_access_phase() | |||
router.router_http.match(api_ctx) | |||
|
|||
-- run global rule | |||
plugin.run_global_rules(api_ctx, router.global_rules, "access") | |||
api_ctx.global_rules = router.global_rules |
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.
Do it in run_global_rules?
Need to update the api_router.lua too
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.
@spacewander
Thanks for reviewing.
I still have some doubts. The original code also assign value to api_ctx.global_rules
after run_plugin
, but it does not cause a bug like this. .
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.
@spacewander
Thanks for reviewing.
I still have some doubts. The original code also assign value toapi_ctx.global_rules
afterrun_plugin
, but it does not cause a bug like this. .
I re-checked the original code, it filtered plugins of global rule and assigned them to api_ctx.plugins
, that is why..
yes, it's a bug. it's introduced by #3396 |
@nic-chen Could you tell me where we use |
use in: and define in: you could also have a look at the prev PR #3396 for more detail. |
Got it. |
@@ -689,6 +689,10 @@ function _M.run_global_rules(api_ctx, global_rules, phase_name) | |||
local orig_conf_version = api_ctx.conf_version |
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.
better style, we can fix it in a new PR
function _M.run_global_rules(api_ctx, global_rules, phase_name)
if not global_rules or not global_rules.values
and #global_rules.values == 0 then
return
end
-- your code
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.
better style, we can fix it in a new PR
function _M.run_global_rules(api_ctx, global_rules, phase_name) if not global_rules or not global_rules.values and #global_rules.values == 0 then return end -- your code end
OK
What this PR does / why we need it:
this bug introduced by #3396.
should assign value to
api_ctx.global_rules
before running global rules, otherwise if a plugin exits in the access phase, global rules will not be able to run in the subsequent phases.Pre-submission checklist: