Limit ingress HTTP request body size#4751
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d190507bde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| ) | ||
| .layer(NormalizePathLayer::trim_trailing_slash()) | ||
| .layer(RequestBodyLimitLayer::new(request_size_limit)) |
There was a problem hiding this comment.
When browser clients send an oversized request with a Content-Length header, RequestBodyLimitLayer returns the 413 immediately without calling its inner service. Because CorsLayer::very_permissive() is added after this line (server.rs:180), it is inside the limit layer and never gets a chance to add CORS headers to those early 413 responses, so browser callers cannot observe the intended Payload Too Large response. Place the limit layer inside the CORS layer (or otherwise apply CORS to the early response).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This unfortunately requires this change to be accepted and merged to mainstream tower-http tower-rs/tower-http#679
We can also switch to using my fork for now. But since the chances of sending such a large payload from the browser are minimal, we can just keep using this mainstream tower-http until the PR is approved and merged.
I will leave a todo in code so we keep track of this
Test Results 8 files ±0 8 suites ±0 5m 11s ⏱️ +22s For more details on these errors, see this check. Results for commit 83484da. ± Comparison against base commit dcdd8ac. ♻️ This comment has been updated with latest results. |
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for enforcing request limits at the ingress @muhamadazmy. The changes look really nice :-) +1 for merging after checking why the e2e tests are failing with this PR.
| use restate_types::journal_v2::Signal; | ||
| use restate_types::net::address::SocketAddress; | ||
|
|
||
| pub type BoxError = Box<dyn std::error::Error + Send + Sync + 'static>; |
There was a problem hiding this comment.
Could you use GenericError, instead?
| req.extensions_mut() | ||
| .insert(ConnectInfo::new(SocketAddress::Anonymous)); | ||
| req.extensions_mut().insert(opentelemetry::Context::new()); |
There was a problem hiding this comment.
Out of curiosity: Why are those extensions needed?
There was a problem hiding this comment.
I assume the RequestBodyLimitLayer needs those?
There was a problem hiding this comment.
This function is a based on the original handle_with_schemas_and_dispatcher. The extensions are accessed by the service handler function, and are injected automatically by the connection handler. So we need to inject those extensions here for the tests to pass.
There was a problem hiding this comment.
I thought the handler should never be called in these tests? Removing those extensions lets the tests still to pass.
There was a problem hiding this comment.
There are 2 test scenarios. One of them sends a full body with content-length header. This one short circuits and fails immediately.
The other test doesn't have content-length header which is normally the case with streaming. In this scenario the handler is called, and fails only if the read body hit the size limit. This scenario of course still calls the handler and fails during body collection.
Removing the extension injection here, causes the second scenario to panic of course.
Adds a `RequestBodyLimitLayer` to the ingress HTTP server so oversized request bodies are rejected with 413 Payload Too Large rather than being streamed in full. Introduces a new `ingress.request-size-limit` config option. It defaults to (and is clamped at) `networking.message-size-limit`, since requests larger than that cap cannot be transmitted over the cluster-internal network. Fixes #4153
2 participants
Limit ingress HTTP request body size
Adds a
RequestBodyLimitLayerto the ingress HTTP server so oversizedrequest bodies are rejected with 413 Payload Too Large rather than being
streamed in full.
Introduces a new
ingress.request-size-limitconfig option. It defaultsto (and is clamped at)
networking.message-size-limit, since requestslarger than that cap cannot be transmitted over the cluster-internal
network.
Fixes #4153