router: bound inference request body size and HTTP listener connections - #1477
router: bound inference request body size and HTTP listener connections#1477vivek-gite wants to merge 4 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This update introduces configurable request limits for the kthenaRouter, including maximum request body size, read header timeout, idle timeout, and maximum header bytes. These limits help prevent excessive resource usage by clients and ensure more stable server performance. The new parameters are documented in the Helm chart values and integrated into the router's deployment configuration. - Added requestLimits section in values.yaml - Updated README.md to include new parameters - Implemented limits in the router's HTTP server configuration - Added tests to validate request body size enforcement and header limits Signed-off-by: [Gite Vivek Kumar] [vivekkumargite@outlook.com] Signed-off-by: Gite Vivek Kumar <vivekkumargite@outlook.com>
2773619 to
68db7d5
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the Kthena Router’s public HTTP listeners against request/connection exhaustion by adding configurable limits for inference request body size and HTTP server connection/header behavior, with Helm + docs updates and targeted unit tests.
Changes:
- Enforce a configurable max inference request body size in
ParseModelRequestusinghttp.MaxBytesReader, returning HTTP 413 on overflow. - Centralize listener construction via
newHTTPServerto applyReadHeaderTimeout,IdleTimeout, andMaxHeaderBytesto default + Gateway listeners. - Add Helm values/env wiring and documentation for the new knobs, plus tests covering parsing and server behaviors.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/kthena-router/router/router.go | Adds MaxRequestBodyBytes env parsing and enforces body size via http.MaxBytesReader with 413 handling. |
| pkg/kthena-router/router/router_test.go | Adds unit tests for request body limit enforcement and env parsing behavior. |
| cmd/kthena-router/app/server.go | Introduces serverLimits, parses new timeout/header env vars, and stores them on Server. |
| cmd/kthena-router/app/server_test.go | Adds tests ensuring server limit env parsing defaults/fallbacks work as intended. |
| cmd/kthena-router/app/router.go | Adds newHTTPServer helper and ensures limits are applied to created listeners. |
| cmd/kthena-router/app/router_listener_test.go | Adds integration-style tests for header timeout/size enforcement and streaming compatibility. |
| docs/kthena/docs/reference/helm-chart-values.md | Documents the new Helm chart values for request/connection limits. |
| charts/kthena/values.yaml | Adds top-level chart values for networking.kthenaRouter.requestLimits.*. |
| charts/kthena/charts/networking/values.yaml | Adds subchart defaults for kthenaRouter.requestLimits.*. |
| charts/kthena/charts/networking/templates/kthena-router/component/deployment.yaml | Wires Helm values into router container env vars for the new limits. |
| charts/kthena/charts/networking/README.md | Adds documentation section describing the request/connection limits and env var mapping. |
Suppressed comments (1)
cmd/kthena-router/app/router_listener_test.go:339
- This test uses http.Get with no timeout, which can hang indefinitely on regressions or unexpected network behavior. Use an http.Client with a Timeout so the test fails quickly instead of stalling CI.
resp, err := http.Get("http://" + addr + "/v1/chat/completions")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // The server may reply 431 or close the connection while the client is | ||
| // still writing; either way the request must not reach the handler. | ||
| resp, err := http.DefaultClient.Do(req) |
| if elapsed := time.Since(start); elapsed < readHeaderTimeout { | ||
| t.Errorf("connection closed after %v, want at least %v", elapsed, readHeaderTimeout) | ||
| } |
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (5)
pkg/kthena-router/router/router.go:626
- When the request body exceeds MaxRequestBodyBytes, ParseModelRequest discards the original *http.MaxBytesError by returning errors.New(msg). Keeping the wrapped original error improves internal observability/debugging while still sending the sanitized client message.
return nil, errors.New(msg)
docs/kthena/docs/reference/helm-chart-values.md:36
- The table says oversized headers are rejected with HTTP 431, but net/http may also close the connection without a 431 (the tests already allow either behavior). The docs should reflect that so operators don't assume a guaranteed status code.
| networking.kthenaRouter.requestLimits.maxHeaderBytes | int | `1048576` | Largest request header block accepted, in bytes.<br/> A larger header block is rejected with HTTP 431. |
charts/kthena/charts/networking/README.md:173
- This description implies oversized headers always get HTTP 431, but net/http may also close the connection without returning 431 (your tests already accept either outcome). Update wording to avoid promising a specific response code.
| `kthenaRouter.requestLimits.maxHeaderBytes` | int | `1048576` | Largest request header block accepted, in bytes (1Mi). A larger header block is rejected with HTTP 431. `net/http` allows a few KiB of slack above this value. |
charts/kthena/values.yaml:136
- These values.yaml comments state oversized headers are rejected with HTTP 431, but net/http may also close the connection without returning 431 (as reflected in the tests). Adjust the comment to avoid guaranteeing a specific status code.
# -- Largest request header block accepted, in bytes.<br/>
# A larger header block is rejected with HTTP 431.
maxHeaderBytes: 1048576
charts/kthena/charts/networking/values.yaml:58
- The comment here guarantees HTTP 431 for oversized headers, but net/http can also close the connection without a 431 (the listener test already allows either). Update wording so the chart values docs match actual server behavior.
# maxHeaderBytes is the largest request header block accepted, in bytes
# (default: 1048576, i.e. 1Mi). Larger headers get HTTP 431.
maxHeaderBytes: 1048576
Signed-off-by: Gite Vivek Kumar <71180467+vivek-gite@users.noreply.github.com>
|
@vivek-gite please read contibuting guide first to respect the coding convention and not make the pr contains merge commit |
| Handler: handler, | ||
| ReadHeaderTimeout: limits.readHeaderTimeout, | ||
| IdleTimeout: limits.idleTimeout, | ||
| MaxHeaderBytes: limits.maxHeaderBytes, |
There was a problem hiding this comment.
I think we can define constant timeout unless there is a must to configure
| # -- Largest inference request body accepted, in bytes.<br/> | ||
| # A larger request is rejected with HTTP 413 before it is buffered.<br/> | ||
| # Set to `0` to disable the limit. | ||
| maxRequestBodyBytes: 33554432 |
There was a problem hiding this comment.
a comment, how many MB
and same apply to below header.
| idleTimeout: 120s | ||
| # -- Largest request header block accepted, in bytes.<br/> | ||
| # A larger header block is rejected with HTTP 431. | ||
| maxHeaderBytes: 1048576 |
There was a problem hiding this comment.
There is a default 1MB in golang http package
What this does
Adds configurable request-size and connection-lifetime bounds to the Kthena Router's HTTP listeners, so a single client cannot exhaust router memory or hold connections open indefinitely.
Fixes #1475
Request body limit ([pkg/kthena-router/router/router.go](pkg/kthena-router/router/router.go)) —
ParseModelRequestnow wraps the body inhttp.MaxBytesReaderbeforeio.ReadAll, so an oversized payload is rejected with HTTP 413 rather than buffered in full. The response carries only the byte limit, no internal detail.Listener bounds ([cmd/kthena-router/app/server.go](cmd/kthena-router/app/server.go), [cmd/kthena-router/app/router.go](cmd/kthena-router/app/router.go)) — every listener (default server and each Gateway port) is now built by a shared
newHTTPServerthat setsReadHeaderTimeout,IdleTimeout, andMaxHeaderBytes.Configuration
networking.kthenaRouter.requestLimits.*)MAX_REQUEST_BODY_BYTESmaxRequestBodyBytes33554432(32Mi)0or negative disablesREAD_HEADER_TIMEOUTreadHeaderTimeout10sIDLE_TIMEOUTidleTimeout120sMAX_HEADER_BYTESmaxHeaderBytes1048576(1Mi)Parsing follows the existing
DRAIN_TIMEOUT/SESSION_BOOST_TIMEOUTpattern: env var read at startup, invalid values log a warning and fall back to the default, so a bad value can never leave a listener unbounded.Compatibility
WriteTimeoutandReadTimeoutare deliberately left unset. A global write deadline would truncate long-running streaming inference responses; a read deadline would cap large body uploads by time rather than size.TestNewHTTPServerAppliesLimitsasserts both stay zero so a future change can't silently regress this.MAX_HEADER_BYTESdefaults tohttp.DefaultMaxHeaderBytes, i.e. the value net/http already enforced — this knob only makes it tunable, it doesn't tighten anything by default.Design notes
ParseModelRequestrather than a middleware because that is the single point where every proxied request body is read (AuthMiddlewareandAccessLogMiddlewarenever touch the body, andBuildDecodeRequestre-serializes from the already-parsed map). Putting it there guarantees the cap applies before the first byte is buffered, with no second read path to keep in sync.parseDrainTimeoutwas collapsed onto a newparsePositiveDurationEnvhelper rather than copy-pasting its body twice for the new timeouts. Behavior and the log message are unchanged.MaxRequestBodyBytesis an exported package var mirroring the existingEnableFairnessScheduling/EnableSessionBoostconvention, which keepsParseModelRequest's exported signature intact.Tests
pkg/kthena-router/router:TestParseModelRequestEnforcesMaxRequestBodyBytes— below limit, exactly at limit, one byte above, far above, and limit-disabled. A counting reader asserts an oversized body is never read pastlimit+1bytes, proving it isn't fully buffered.TestParseMaxRequestBodyBytes— default, explicit, zero/negative (disable), unparsable.cmd/kthena-router/app:TestParseServerLimits— defaults, overrides, unparsable, and non-positive values.TestNewHTTPServerAppliesLimits— fields wired correctly;WriteTimeout/ReadTimeoutremain zero.TestNewHTTPServerClosesSlowHeaderConnections— raw socket sends a partial header block; the server closes the connection afterReadHeaderTimeoutand the handler is never reached.TestNewHTTPServerRejectsOversizedHeaders— oversized header block never reaches the handler (accepts either a 431 or a server-side close, since net/http may hang up while the client is still writing).TestNewHTTPServerAllowsLongStreamingResponses— a 500ms chunked SSE response completes intact under a 100msReadHeaderTimeout/IdleTimeout.Docs
charts/kthena/charts/networking/README.mdgains a "Request and Connection Limits" section (values, env-var mapping, fallback behavior).helm-chart-values.mdrows were added by hand in helm-docs' alphabetical order — please confirmmake gen-docsproduces no diff.Reviewer notes
localhost:15000was intentionally left unchanged; the issue scopes this to the public listeners.MaxHeaderBytesin net/http permits a few KiB of slack above the configured value (initialReadLimitSizeadds 4096). The chart README says so, and the test overshoots by 16× to avoid depending on that slack.