[Serve] Gate ingress request router body forwarding behind escape hatch#63232
Closed
kouroshHakha wants to merge 4 commits into
Closed
[Serve] Gate ingress request router body forwarding behind escape hatch#63232kouroshHakha wants to merge 4 commits into
kouroshHakha wants to merge 4 commits into
Conversation
Two related fixes for HAProxy + ingress-request-router peer deployments (e.g. LLMRouter): 1. ``_direct_ingress_asgi`` was calling ``route.startswith(self._route_prefix)`` with ``_route_prefix=None`` for ingress-request-router peer deployments (their route prefix is None), raising TypeError and surfacing as uvicorn 500. Treat a missing prefix as ``/``. 2. When the Lua dispatch failed (router unreachable / non-200 / unparseable reply / unknown replica), HAProxy was silently falling through to the primary backend, invisibly bypassing the operator's router policy. Arm ``txn.ingress_request_router_failed = "<reason>"`` in those paths and add a frontend ``http-request return 503`` (with ``X-Serve-Reason``) ordered before any ``use_backend`` so a failed dispatch cannot reach the primary backend. Tests: - ``test_router_failure_503_rule_appears_before_use_backend`` pins the template-ordering invariant. - ``test_router_failure_fails_loud_with_reason`` runs HAProxy against a stub broken router; asserts 503 + ``X-Serve-Reason``, and primary backend cumulative ``stot == 0``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing prefix
Address Gemini's review comment: ``route.startswith("")`` is always true,
which correctly handles the empty-path ASGI edge case where ``"".startswith("/")``
would 404 the request. No behavior change for any non-empty path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues caught in code review:
1. The empty-body early return in the Lua action was leaving
``txn.ingress_request_router_failed`` unset, so a POST with an empty
body to a router-bearing app silently fell through to the primary
backend — the exact silent bypass this change is meant to close. Arm
the sentinel with ``empty_body`` so it surfaces as 503 +
``X-Serve-Reason``. Promote the failure-semantics comment to a
top-level note describing the contract for the whole action; the
"past this point" framing no longer applies because every router
decision now arms the sentinel on failure.
2. ``test_router_failure_503_rule_appears_before_use_backend`` searched
for the substring ``use_backend``, but the rendered template includes
that string inside an explanatory comment block ("static use_backend
selection below") that renders before the 503 rule. The assertion
would fail deterministically. Switch to line-based parsing that
matches actual ``use_backend <name>`` directive lines.
Extends the integration test to also cover the empty-body path:
verified manually that an empty-body POST returns 503 with
``X-Serve-Reason: empty_body`` and primary backend stot stays 0.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bootstrap of ray-project#63183: round-robin /internal/route ignores the request body, so make body forwarding opt-in via ``RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY`` (off by default). When off, the data plane saves on every routed request: - HAProxy ``wait-for-body`` round-trip and per-connection buffer cost (~2 * tune.bufsize * maxconn). - Lua ``txn.sf:req_body()`` plus body re-emit on the socket. Rendered config gates ``tune.bufsize`` and ``wait-for-body`` on the flag; the Lua template gates the body read on a ``FORWARD_BODY`` substitution. This also resolves the empty-body invariant gap from the previous PR. The empty-body Lua return is removed entirely: when ``FORWARD_BODY`` is off, the action calls the router with ``body=""``; when it's on, empty bodies are still treated as legitimate input passed through to the router. The router decides whether an empty body is a failure; the data plane never silently bypasses it. The transient ``empty_body`` sentinel from the previous PR (added as a stop-gap until this change landed) is therefore dropped. ## Test plan - New parametrized template-render test pins that ``tune.bufsize``, ``wait-for-body``, and the Lua ``FORWARD_BODY`` constant render consistently with the env var on/off. - Existing ``test_ingress_request_router_end_to_end`` flips the flag on via monkeypatch; body-forwarded path stays covered end-to-end. - ``test_router_failure_fails_loud_with_reason`` now exercises both empty and non-empty POSTs through the broken router and asserts both surface as 503 ``router_non_200`` (proves empty bodies still go through routing rather than silently bypassing it). - Manually verified end-to-end with real HAProxy + working router: FORWARD_BODY=false sends body="" to router for any POST shape; FORWARD_BODY=true forwards the request body through unchanged. Co-Authored-By: Seiji Eicher <seiji@anyscale.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY configuration flag, allowing users to opt-in to request body forwarding for body-aware routing policies in HAProxy. This optimization reduces memory usage and processing overhead for default round-robin routing. The PR also implements a 'fail-loud' mechanism that returns a 503 status code with a specific reason header if the ingress router fails, preventing silent fallbacks to the primary backend. Feedback suggests updating a comment in constants.py to more accurately reflect how the boolean environment variable is evaluated.
| # active. Bodies longer than this are truncated; the Lua forwards what it has | ||
| # with an `X-Body-Truncated: <bytes>/<content-length>` header so the router can | ||
| # do best-effort prefix matching. Memory cost is ~2 * bufsize * maxconn. | ||
| # Only consulted when RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY=1. |
Contributor
There was a problem hiding this comment.
The comment states that this constant is only consulted when RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY=1. Since get_env_bool is used for that flag, it can also be enabled using other truthy values like true, yes, or on. It would be more accurate to say 'when enabled'.
Suggested change
| # Only consulted when RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY=1. | |
| # Only consulted when RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY is enabled. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 48402f5. Configure here.
| # body-aware routing policies (prefix cache, etc.). | ||
| RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY = get_env_bool( | ||
| "RAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY", False | ||
| ) |
There was a problem hiding this comment.
Wrong default type passed to get_env_bool
Medium Severity
get_env_bool expects a string default ("0" or "1") but receives the boolean False. It works by coincidence: str(False) produces "False", which is not "1", so the function returns False. However, if the default were ever changed to True, it would silently fail—str(True) yields "True" which also isn't "1", so it would return False instead of the intended True. Every other caller in the file correctly passes "0" or "1".
Reviewed by Cursor Bugbot for commit 48402f5. Configure here.
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
1 participant
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.
Stacked on #63215. Review #63215 first; this PR's diff will collapse to just the FORWARD_BODY changes once that merges.
Rebases #63183 onto master with #63215 (fail-loud) landed.
Summary
Round-robin
/internal/routeignores the request body, so make body forwarding opt-in viaRAY_SERVE_INGRESS_REQUEST_ROUTER_FORWARD_BODY(off by default).When off, the data plane saves on every routed request:
wait-for-bodyround-trip and per-connection buffer cost (~2 * tune.bufsize * maxconn).txn.sf:req_body()plus body re-emit on the socket.Rendered HAProxy config gates
tune.bufsizeandwait-for-bodyon the flag; the Lua template gates the body read on aFORWARD_BODYsubstitution.Also closes the empty-body invariant gap from #63215. The empty-body Lua return is removed entirely: in either FORWARD_BODY mode, an empty body now flows through to the router with
body=""rather than silently bypassing it. The router decides whether an empty body is a failure. The transientempty_bodysentinel from #63215 (a stop-gap until this PR landed) is therefore dropped.Test plan
tune.bufsize,wait-for-body, and the LuaFORWARD_BODYconstant render consistently with the env var on/off.test_ingress_request_router_end_to_endflips the flag on via monkeypatch; body-forwarded path stays covered end-to-end.test_router_failure_fails_loud_with_reasonexercises both empty and non-empty POSTs through the broken router and asserts both surface as 503router_non_200— pins that empty bodies still go through routing rather than silently bypassing it.FORWARD_BODY=falsesendsbody=""to the router for any POST shape;FORWARD_BODY=trueforwards the request body through unchanged.🤖 Generated with Claude Code