fix: split global rules phase execution for client-control compatibility#13345
Merged
Conversation
When a global rule plugin reads the request body in access phase (e.g., loggers with include_req_body), it triggers nginx's body size check before client-control on route/service can set the FFI override in rewrite phase, causing false 413 rejections. Split run_global_rules in http_access_phase so that: 1. Global rules rewrite runs first 2. Route/service plugins rewrite runs (client-control sets FFI override) 3. Global rules access runs (body reader uses correct limit) 4. Route/service plugins access runs Also add phase-specific tracer span names (run_global_rules.rewrite, run_global_rules.access) for better observability.
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Pull request overview
This PR fixes an execution-order bug where global rule plugins that read the request body in access could trigger Nginx body-size checks before route/service client-control had a chance to apply its max-body-size override, causing incorrect 413 responses.
Changes:
- Split global rules execution in
http_access_phaseto run global-rulerewritebefore route/servicerewrite, and global-ruleaccessbefore route/serviceaccess. - Add phase-specific tracing span names for global rules (
run_global_rules.rewrite/run_global_rules.access). - Add regression tests ensuring
client-controloverrides the body limit before a global rule reads the body, and that 413 still occurs withoutclient-control.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
apisix/init.lua |
Reorders/splits global rule execution to interleave correctly with route/service plugin phases. |
apisix/plugin.lua |
Adds phase-specific span naming and adjusts when api_ctx.global_rules is set. |
t/plugin/client-control.t |
Adds tests covering the regression scenario with global rules + client-control body-size override. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous approach tried to set nginx client_max_body_size via extra_yaml_config, but test-nginx hardcodes client_max_body_size 30M in its generated nginx.conf, ignoring the APISIX config.yaml value. Replace with a direct phase-ordering test using grep_error_log to verify: global-rewrite → route-rewrite → global-access → route-access. This proves client-control (route rewrite) runs before any global rule access-phase plugin reads the request body.
The grep_error_log regex was picking up the log marker from function
source code strings logged during config sync. Use string concatenation
('PO ' .. 'global-rewrite') so the full marker only exists at runtime,
not in the source code. Also use [a-z]+-[a-z]+ regex to avoid matching
trailing commas from nginx error log format.
…phase ordering Replace the grep_error_log-based phase ordering tests with actual client-control body size verification: - TEST 9: send 1MB+1 body with client-control(10MB) on route + body reader on global rule access → expect 200 (FFI override works) - TEST 11: same body without client-control → expect 413 (nginx default 1m limit enforced) This tests the real bug scenario: client-control must set the FFI body size override in route rewrite before a global rule access-phase plugin reads the body.
The APISIX server block shares test-nginx's server block which has client_max_body_size 30M hardcoded. Use a 31MB body to exceed this limit, with client-control setting the FFI override to 50MB. Also add upstream_server_config with client_max_body_size 0 so the upstream fake server accepts the large body when proxied.
nic-6443
requested review from
AlinsRan,
membphis,
nic-chen and
shreemaan-abhishek
May 11, 2026 01:05
AlinsRan
previously approved these changes
May 11, 2026
nic-chen
reviewed
May 11, 2026
membphis
reviewed
May 11, 2026
membphis
left a comment
membphis
left a comment
Member
There was a problem hiding this comment.
I think it time to remove route.value.script, service.value.script
anyway, this is another question, we can do it in a new PR
Split the no-route path from run_global_rules(nil) into separate rewrite and access calls, matching the matched-route path. This removes the nil phase_name handling from run_global_rules() entirely, making the function simpler and the call sites consistent.
shreemaan-abhishek
approved these changes
May 11, 2026
nic-chen
approved these changes
May 11, 2026
membphis
approved these changes
May 11, 2026
AlinsRan
approved these changes
May 11, 2026
wistefan
pushed a commit
to wistefan/apisix
that referenced
this pull request
Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Split global rules phase execution so that route/service rewrite plugins run between global rule rewrite and global rule access phases. This fixes a conflict where
client-control(route rewrite, priority 22000) cannot set the nginx body size override via FFI before a global rule access-phase plugin (e.g., a logger withinclude_req_body) callsngx.req.read_body().Before
A global rule logger reading the body in access phase triggers nginx body size check before
client-controlcan set the FFI override → 413.After
client-controlsets the FFI override in route rewrite, then the global rule access phase reads the body using the overridden limit.Changes
apisix/init.lua: Splitrun_global_rules(nil)into separate"rewrite"and"access"calls in both the no-route and matched-route pathsapisix/plugin.lua: Simplifiedrun_global_rules()— removed nil phase_name handling, always uses explicit phase name with phase-specific tracer span namingt/plugin/client-control.t: Added regression tests with 31MB body exceeding nginx's 30M limit, verifying client-control FFI override works when global rule reads body in access phaseBehavioral Changes
consumer-restrictionon global rule now works correctly (consumer info available after merge)logphase always runs