Repository navigation
fix: preserve client-control body limit for gRPC upstreams - #14036
ruanweihong-aaa wants to merge 3 commits into
Conversation
| apisix_ngx_client.enable_mirror() | ||
| end | ||
|
|
||
| if api_ctx.client_max_body_size and has_mod then |
There was a problem hiding this comment.
The judgment of has_mod is redundant, and this judgment does not exist in the code of apisix/plugins/client-control.lua.
There was a problem hiding this comment.
Thanks for pointing this out. I removed the redundant has_mod check in 3eccc1e8. client-control saves the limit in the request context only after set_client_max_body_size succeeds.
| === TEST: client-control rejects a gRPC request without Content-Length after internal redirect | ||
| --- http2 | ||
| --- apisix_yaml | ||
| routes: | ||
| - id: grpc-body-limit | ||
| uri: /helloworld.Greeter/SayHello | ||
| plugins: | ||
| client-control: | ||
| max_body_size: 1 |
There was a problem hiding this comment.
gRPC tests lack a runtime gate
Without apisix-nginx-module, client-control returns 501 before the new gRPC tests reach their assertions. The existing client-control suite skips that environment; these cases need the same gate.
There was a problem hiding this comment.
Thanks for catching this. I moved the new cases to t/node/grpc-client-control.t and added the same apisix-nginx-module availability check used by the existing client-control test suite. The new cases are skipped when the module is unavailable, while the existing gRPC proxy tests remain runnable.
| === TEST: client-control raises the gRPC body limit above the global limit | ||
| --- http2 | ||
| --- extra_yaml_config | ||
| nginx_config: | ||
| http: | ||
| client_max_body_size: 1 | ||
| --- apisix_yaml | ||
| routes: | ||
| - id: grpc-larger-body-limit | ||
| uri: /helloworld.Greeter/SayHello | ||
| plugins: | ||
| client-control: | ||
| max_body_size: 5242880 | ||
| upstream: | ||
| scheme: grpc | ||
| nodes: | ||
| "127.0.0.1:10051": 1 | ||
| type: roundrobin | ||
| #END | ||
| --- exec | ||
| grpcurl -import-path ./t/grpc_server_example/proto -proto helloworld.proto -plaintext -d '{"name":"apisix"}' 127.0.0.1:1984 helloworld.Greeter.SayHello | ||
| --- response_body_like: Hello apisix |
There was a problem hiding this comment.
Raised-limit cases use tiny requests
The zero and 5 MiB cases send a small request, so both pass even if the original Nginx limit remains in force. The test fixture builds Nginx configuration separately from extra_yaml_config; verify the limit reaches it and send a larger body.
There was a problem hiding this comment.
Agreed—the original small requests could not verify that the raised limit took effect. The test fixture already sets a 30 MiB Nginx limit, so I removed the duplicate directive after CI showed it prevented Nginx from starting. In adec29cd, both raised-limit cases send a 31 MiB request, with the plugin set to unlimited or 50 MiB. They expect the upstream gRPC server's ResourceExhausted response rather than an APISIX 413, which would show that the request reached the upstream. CI for this revision is still running.
4 participants
Summary
client-controlbody limit in the APISIX request context after the runtime setter succeeds.grpc_access_phase()after the redirect to@grpc_pass, with the existing setter and 503 error handling.Fixes #14009.
Verification
grpcswith a TLS backend.git diff --checkpassed.action_requiredon this fork PR, so their results are pending.