feat(routing): make AI gateway route request timeout configurable by Adam-D-Lewis · Pull Request #125 · nebari-dev/llm-serving-pack

Adam-D-Lewis · 2026-07-01T22:17:26Z

Problem

The operator generates an AIGatewayRoute for each model's external
(llm.<baseDomain>) and internal (llm-internal.<baseDomain>) endpoint, but
it never set a request timeout. The Envoy AI Gateway therefore applies its
built-in 60s default (defaultRequestTimeout) to the HTTPRoute it
renders from each route. That is too short for many real LLM requests — long
or reasoning-heavy generations, large prompts, and cold model loads routinely
run past 60s and get cut off with a 504 / stream reset.

The route's own timeout is the only effective place to raise this:

In Envoy Gateway an explicit HTTPRoute request timeout takes precedence
over a BackendTrafficPolicy (the policy timeout is applied setIfNil), so
a policy attachment cannot override the 60s.
The route is controller-generated, so it cannot be patched from GitOps
without fighting the operator's reconcile.

Change

Add a chart-wide Helm value defaults.routing.requestTimeout, plumbed
through:

values.yaml → LLM_ROUTE_REQUEST_TIMEOUT env (operator Deployment) →
config.OperatorConfig.RouteRequestTimeout → spec.rules[].timeouts.request
on both the external and internal AIGatewayRoutes.

It defaults to empty, which leaves the field unset and preserves the
gateway's existing 60s behavior on upgrade. Set it to a Gateway API duration
string to raise the timeout for all of a cluster's model routes:

defaults:
  routing:
    requestTimeout: "600s"

Testing

go test ./internal/config/... ./internal/controller/reconcilers/... — new
cases assert both routes carry timeouts.request when set and omit the
block when empty; the config loader parses the new env var.
go build ./..., gofmt, go vet clean.
helm lint + helm template verified: no timeout rendered by default, the
configured duration when set.

Follow-up

A per-model override on LLMModel.spec (so individual models can set their
own timeout, overriding this chart-wide default) is left for a separate PR.

The operator generates an AIGatewayRoute per model endpoint but set no request timeout, so the Envoy AI Gateway applied its built-in 60s default to the rendered HTTPRoute. That is too short for many LLM generations (long or reasoning outputs, large prompts, cold model loads) and cannot be raised by a BackendTrafficPolicy, since an explicit route timeout always takes precedence - the route itself is the only effective lever. Add a chart-wide defaults.routing.requestTimeout value, plumbed through the LLM_ROUTE_REQUEST_TIMEOUT env var and OperatorConfig into both the external and internal AIGatewayRoutes as spec.rules[].timeouts.request. It defaults to empty, which leaves the field unset and preserves the gateway's 60s default on upgrade; set it to a duration such as "600s" to raise the timeout. The value is validated against the Gateway API duration format at operator startup. PassthroughModel routes keep their own fixed 120s timeout and are unaffected. A per-model LLMModel.spec override is left for a follow-up.

dcmcand

Thanks for this - the motivation is solid and the technical homework behind it is genuinely good. I verified all three of the load-bearing upstream claims in your description against source rather than taking them on faith, and every one of them holds:

AIGatewayRouteRule.Timeouts *gwapiv1.HTTPRouteTimeouts really is a field at the envoyproxy/ai-gateway v0.5.0 tag (the version this pack's runbook pins), in both the Go types and the generated CRD YAML. This does not generate objects the API server would reject.
The duration regex at config.go:121 is byte-for-byte sigs.k8s.io/gateway-api@v1.4.1's own kubebuilder Duration pattern. No divergence in either direction, so the operator and the API server agree on what is valid.
"An explicit route timeout beats a BackendTrafficPolicy" is correct: envoyproxy/gateway's internal/xds/translator/route.go, getEffectiveRequestTimeout, returns the route's own timeouts.request when set and only falls back to the policy timeout otherwise. Ruling out BackendTrafficPolicy was the right call.

I also went looking for the streaming failure mode I expected to flag and did not find one: idleTimeout() in that same file defaults a route's stream idle timeout to max(1h, effective request timeout) when no ClientTrafficPolicy or BackendTrafficPolicy is present, and this pack deploys neither. So 600s will not be quietly undercut by a shorter idle cutoff today.

Requesting changes on two things, one of which is the real problem here.

Blocker 1: the branch is on a stale base and the refactor reverts the fix for #116

This is the significant one. The branch is 101 commits behind main, and main has since landed dac9bc4 ("make per-model API-key auth work end-to-end on AI Gateway v0.5", #116/#117), which removed the hostname parameter and the Host header matcher from buildAIGatewayRoute. The reason is in that commit: the AI Gateway v0.5 controller will not register a model whose match rule carries any header beyond x-ai-eg-model, and the extra Host matcher made every request 404 with "model not configured in the Gateway".

Your rule-hoisting refactor carries that Host matcher along with it (plus the now-obsolete "defence in depth" comment), so merging as-is reintroduces a real routing outage. gh pr view confirms mergeable: CONFLICTING, and the single conflict is exactly this file. The green checks are against the stale base and do not tell us anything about the merged result.

The rebase is not mechanical: keep requestTimeout, drop hostname and the Host matcher, drop the stale comment. SharedExternalHostname/SharedInternalHostname are still used at clustertls_singleton.go:163, llmmodel_controller.go:573, and passthrough.go:88, so they stay - they just are not passed to buildAIGatewayRoute anymore. Given 101 commits of drift, a quick sanity pass over the touched files after rebasing is probably worth it beyond just this signature.

Blocker 2: the knob is under `defaults:` but has no per-CR override

defaults.* is documented as cluster-wide fallbacks that an LLMModel spec can override, and every existing entry has a counterpart (defaults.serving.image -> spec.serving.image; defaults.storage.storageClassName -> spec.model.storage.storageClassName, configuration.mdx:144; defaults.monitoring.enabled -> spec.serving.monitoring.enabled, configuration.mdx:181). Knobs that are deliberately cluster-wide-only live under platform:/auth: and are labelled "not per-model". Since the override does not exist yet, this entry as shipped promises something that is not there. Three ways out, noted inline.

Blocker 3 (small): the values reference now says something untrue

docs/src/content/docs/configuration.mdx:9-13 opens with "This page is the authoritative reference for every knob the pack exposes" and "All tables are derived directly from ... values.yaml". This PR adds a knob to values.yaml and no row to the Defaults table at configuration.mdx:109-115, so both claims are now false. README.md:207 needs the same row.

That matters more than a normal docs nit here, because the whole point of this knob is to be found by an admin debugging 504s at 60s. If it only exists in values.yaml comments, the person who needs it will not find it. configuration.md is not in scripts/check-content-parity.mjs's MAP, so editing it will not trip the parity check.

Worth considering alongside it: docs/src/content/docs/troubleshooting.md has no timeout or 504 section at all, and "requests cut off at 60s with a 504" is precisely the symptom this addresses. That file is in the parity MAP, so adding a section needs an exclusion entry next to the existing #115/#117/#120 comments.

Everything else is inline and non-blocking.

dcmcand · 2026-07-30T08:10:49Z

+				"headers": []interface{}{
+					map[string]interface{}{
+						"type":  "Exact",
+						"name":  "Host",


Blocker: this Host matcher no longer exists on main. dac9bc4 (#116/#117) removed it, and the hostname parameter with it, because the AI Gateway v0.5 controller does not register a model whose match rule carries any header beyond x-ai-eg-model - the extra matcher 404'd every request with "model not configured in the Gateway".

Hoisting the rule into a variable is a nice change on its own, but it carries this matcher forward, so the merge would revert that fix. The conflict gh pr view reports (mergeable: CONFLICTING) is in exactly this file.

After rebasing, the rule should keep only the x-ai-eg-model match, and the comment at lines 91-95 justifying Host as defence in depth should go with it - sectionName scoping on parentRefs is what does that job now.

dcmcand · 2026-07-30T08:10:50Z

 	poolName string,
 	modelName string,
 	hostname string,
+	requestTimeout string,


Related to the rebase: hostname on line 87 is gone from this signature on main, so the resolution is requestTimeout in and hostname out, not both. SharedExternalHostname/SharedInternalHostname themselves are still live (clustertls_singleton.go:163, llmmodel_controller.go:573, passthrough.go:88) - they just are not passed here anymore.

Stylistic, while you are in here: even after dropping hostname this is a long positional parameter list with several consecutive bare strings, which the compiler cannot help you order correctly. buildJWTSecurityPolicy in this package already takes cfg *config.OperatorConfig directly; doing the same (or an options struct) would absorb the next knob for free.

dcmcand · 2026-07-30T08:10:50Z

+  # LLMModel's external (llm.<baseDomain>) and internal
+  # (llm-internal.<baseDomain>) endpoints. (PassthroughModel routes use a
+  # fixed timeout and are not affected by these settings.)
+  routing:


Blocker: defaults.* is documented as cluster-wide fallbacks that an LLMModel can override, and every existing entry has a spec counterpart - defaults.serving.image -> spec.serving.image, defaults.storage.storageClassName -> spec.model.storage.storageClassName (configuration.mdx:144), defaults.monitoring.enabled -> spec.serving.monitoring.enabled (configuration.mdx:181). Knobs that are intentionally cluster-wide-only (platform.gateway.manageSharedListeners, platform.tls.secretName, auth.oidc.*) live outside this block and are labelled "not per-model".

As shipped this entry promises an override that does not exist, and if the follow-up slips, released chart versions carry the broken contract. Three ways out, in my order of preference:

Add the LLMModel.spec field in this PR (spec.routing.requestTimeout or spec.endpoints.*.requestTimeout), with cfg.RouteRequestTimeout as the fallback in BuildRoutingResources. Makes the placement correct on arrival and avoids a values rename later.

Ship it as platform.gateway.requestTimeout now, matching manageSharedListeners, and relocate when the CR field lands. Note that relocating is a breaking values rename for anyone who adopts it in between.

Keep the placement and amend configuration.mdx to say this one is chart-wide-only pending the per-model field, so the chart and the docs agree.

Whichever you pick also settles the naming question I left on the Deployment template.

dcmcand · 2026-07-30T08:10:50Z

+    # reasoning outputs, cold model loads) by setting a Gateway API duration
+    # string such as "600s" or "10m". The route is the only effective place
+    # to set this: an explicit route timeout takes precedence over any
+    # BackendTrafficPolicy. A future LLMModel.spec field will be able to


Stylistic: the shipped chart promises behavior that does not exist yet. If the follow-up slips, this misleads anyone reading the released values file. Prefer stating the present limitation - "this applies to every model on the cluster; there is no per-model override" - and tracking the future field in an issue.

dcmcand · 2026-07-30T08:10:50Z

+    # to set this: an explicit route timeout takes precedence over any
+    # BackendTrafficPolicy. A future LLMModel.spec field will be able to
+    # override this per model.
+    requestTimeout: ""


Stylistic: worth one more line here, because "0s" is a trap. Per GEP-2257 and gateway-api's own HTTPRouteTimeouts.Request docs, a zero-valued timeout disables the timeout rather than meaning "as short as possible". "0s" passes the regex at config.go:121 cleanly, so an admin could set it reaching for the strictest possible timeout and get an unbounded one instead. Same note would help on the OperatorConfig field comment.

Also worth saying explicitly that an invalid value fails operator startup (see my note on config.go:160), so operators know the blast radius before they typo it.

dcmcand · 2026-07-30T08:10:50Z

+			},
+			wantErr:     true,
+			errContains: []string{"LLM_ROUTE_REQUEST_TIMEOUT"},
+		},


Stylistic: these two cases cover a non-duration string and a unitless number, but not the case the regex's own justification at config.go:117-120 cites as the reason for not using time.ParseDuration - that it "allows decimals". Nothing asserts "1.5s" is rejected.

Two more branches the pattern implies and nothing pins down: a valid compound duration like "1h30m" exercising {1,4}, and a digit-cap boundary like "123456s" that should fail {1,5}. Cheap to add to the existing table, and they lock in the behavior the comment claims.

dcmcand · 2026-07-30T08:10:50Z

 // sectionName that rule (and the SecurityPolicies bound to this route)
 // would catch traffic for unrelated listeners on the shared Gateway.
+//
+// The 120s request timeout on the rules below is fixed and intentionally


Good instinct documenting the deliberate non-application rather than leaving the reader to wonder, and I confirmed the claim is accurate - the fixed 120s at lines 311 and 335 is untouched by this PR.

Question on the resulting defaults, though: the pack now has two unrelated mechanisms for the same route field, and the ordering reads backwards against this PR's own argument. Remote-provider passthrough traffic gets 120s, while locally generated completions - the case the description argues routinely exceeds 60s - stay at 60s until an admin opts in. The latency-profile rationale explains why they might differ, but not why the local one is the shorter of the two.

Either let defaults.routing.requestTimeout apply to passthrough routes with 120s as the unset fallback, or file the convergence follow-up and cite its issue number here, the way other deliberate LLMModel/Passthrough divergences in this repo do.

dcmcand · 2026-07-30T08:10:50Z

+		},
+	}
+
+	// requestTimeout, when set, becomes spec.rules[].timeouts.request on the


This comment is doing real work - putting the "why not BackendTrafficPolicy" reasoning where the next person to touch this will actually see it. I verified the precedence claim against envoyproxy/gateway's internal/xds/translator/route.go (getEffectiveRequestTimeout) and it is correct.

Two things that would make it more durable:

Cite the version. Sibling code does - routing.go:33 says "verified against envoyproxy/ai-gateway main", and modelservice.go:1 / inferencepool.go:1 cite chart versions. This pack does not install the AI Gateway (envoyAIGateway.install: false), so a cluster on a different version could prune the field with no status signal. "Verified against ai-gateway v0.5.0" would pin what was actually checked.

One caveat the current phrasing hides. RouteAction.MaxStreamDuration is populated unconditionally from a BackendTrafficPolicy's spec.timeout.http.maxStreamDuration, and is not subject to the same "route wins" precedence - it is a different Envoy field. Moot today (there is no BackendTrafficPolicy anywhere in this repo), but "a policy attachment cannot raise it" reads as "no policy can ever affect this route's timeout behavior", which is not quite complete. If someone later adds a policy for an unrelated reason and sets maxStreamDuration, streaming completions get re-capped regardless of this value, and that is a rough debugging session to walk into.

dcmcand · 2026-07-30T08:10:50Z

              value: {{ .Values.platform.tls.secretName | default "" | quote }}
            - name: LLM_MANAGE_SHARED_LISTENERS
              value: {{ .Values.platform.gateway.manageSharedListeners | quote }}
+            - name: LLM_ROUTE_REQUEST_TIMEOUT


Stylistic (naming): every other defaults.* value carries the tier marker - LLM_DEFAULT_SERVING_IMAGE, LLM_DEFAULT_EPP_IMAGE, LLM_DEFAULT_STORAGE_CLASS_NAME, with matching Default* fields on OperatorConfig. LLM_ROUTE_REQUEST_TIMEOUT reads like a platform.*-tier knob. This resolves itself once you pick an option on the values.yaml placement: staying under defaults: wants LLM_DEFAULT_ROUTE_REQUEST_TIMEOUT/DefaultRouteRequestTimeout; moving to platform.gateway.* makes the current names correct as-is.

Separately, and explicitly not something I am asking you to fix: --set defaults.routing=null panics with nil pointer evaluating interface {}.requestTimeout. I checked, and it is not a regression - the identical crash reproduces today with --set defaults.storage=null, so this field just inherits a chart-wide sharp edge. The realistic upgrade path (an existing values file that simply omits routing) renders cleanly through Helm's map merge, which is the case that actually matters here. Hardening it chart-wide via (.Values.defaults.routing).requestTimeout or a values.schema.json is a separate task.

dcmcand · 2026-07-30T08:10:50Z


+// ruleRequestTimeout returns the spec.rules[0].timeouts.request value of the
+// given AIGatewayRoute and whether a timeouts block is present on the rule.
+func ruleRequestTimeout(t *testing.T, route *unstructured.Unstructured) (string, bool) {


This helper is the right shape - returning (value, present) lets the empty-config case assert the timeouts block is genuinely absent rather than just comparing against "", which is the assertion that actually protects the upgrade-safety property. Both new cases check the external and internal route rather than only the one that prompted the change.

dcmcand · 2026-07-30T08:11:27Z

Verification evidence

Recording what I actually ran, so the "verified" claims in my review are checkable rather than asserted.

Upstream source checks (against the pinned version, examples/envoy-ai-gateway.yaml:42,77 and docs/src/content/docs/installation.md:496,623 both pin v0.5.0):

Claim	Where I checked it	Result
`spec.rules[].timeouts.request` exists on `AIGatewayRoute`	`envoyproxy/ai-gateway@v0.5.0`, `api/v1alpha1/ai_gateway_route.go` + generated `aigateway.envoyproxy.io_aigatewayroutes.yaml`	Confirmed: `AIGatewayRouteRule.Timeouts *gwapiv1.HTTPRouteTimeouts`
Duration regex matches upstream	`sigs.k8s.io/gateway-api@v1.4.1` `Duration` kubebuilder pattern	Byte-for-byte identical to `config.go:121`
Route timeout beats `BackendTrafficPolicy`	`envoyproxy/gateway`, `internal/xds/translator/route.go`, `getEffectiveRequestTimeout`	Confirmed: route's own `timeouts.request` wins, policy is the fallback
Streaming not undercut by idle timeout	same file, `idleTimeout()`	Defaults to `max(1h, effective request timeout)` when no `ClientTrafficPolicy`/`BackendTrafficPolicy` exists; repo-wide grep confirms this pack deploys neither
`"0s"` semantics	GEP-2257 / `gateway-api` `HTTPRouteTimeouts.Request` docs	Zero disables the timeout entirely, and passes the regex

Build and test, in a worktree at 8b16b33:

go build ./... && go vet ./... && gofmt -l .     -> all exit 0, gofmt silent
go test ./internal/config/...                    -> ok (10 subtests, both new cases pass)
go test ./internal/controller/reconcilers/...     -> ok (both new routing cases pass)
go test ./internal/controller/...                 -> FAIL: missing kubebuilder envtest etcd binary

The envtest failure is a local sandbox limitation, not attributable to this PR: it reproduces identically on the base commit and touches nothing this diff modifies.

Chart:

helm lint charts/nebari-llm-serving                             -> 0 failed (pre-existing "icon is recommended" INFO)
helm template ... --set defaults.routing.requestTimeout=600s     -> value: "600s"
helm template ...  (no override)                                 -> value: ""
helm template ... -f <overrides file with no "routing" key>       -> value: ""      <- realistic upgrade path, clean
helm template ... --set defaults.routing.requestTimeout=600      -> value: "600"    <- renders, then rejected by the operator's regex at startup
helm template ... --set defaults.routing=null                    -> nil pointer panic (also reproduces with defaults.storage=null, so pre-existing)

Empty-by-default does preserve upgrade behavior, and I confirmed the mechanism that makes it work in both directions: because createOrUpdateUnstructured does a full-object replace rather than a merge-patch, reverting the value to "" actually removes timeouts from live routes instead of leaving a stale field behind. Worth knowing that this correctness depends on the apply strategy.

One caveat on lint: my local golangci-lint is 2.12.2 against CI's pinned v2.4.0 and produced 50+ goconst findings CI does not see, so I deferred to the CI result (all 5 checks pass) rather than reporting noise from a version mismatch. Note those checks ran against the stale base.

General notes

The overall shape is right, and I want to be clear that the two blockers are not about the engineering: chart value -> env var -> validated-once-at-startup config field -> stamped onto both routes by a pure, testable builder is exactly the path this operator already uses, with no shortcut where a reconciler reads the environment itself. Validation failing loudly at startup via setupLog.Error + os.Exit(1) beats a bad value reaching the API server. The empty-string sentinel for "unset" mirrors LLM_TLS_SECRET_NAME and LLM_DEFAULT_STORAGE_CLASS_NAME in the same file, so it is idiomatic here rather than a one-off. And applying it to both endpoints with both asserted in tests, rather than fixing only the endpoint that surfaced the problem, is the right call.

The only structural problem is that it was written against a base that has moved 101 commits, and one of those commits restructured the exact function this touches for a reason that matters. That is a rebase, not a redesign.

Things I noticed that are not this PR's problem

Filing these here so they do not get lost, but they should not gate this:

defaults.monitoring.enabled (values.yaml:144-145) reaches no env var on the operator Deployment. Only LLM_DEFAULT_SERVING_IMAGE, LLM_DEFAULT_EPP_IMAGE, and LLM_DEFAULT_STORAGE_CLASS_NAME are wired, yet configuration.mdx:181 documents spec.serving.monitoring.enabled as defaulting to it. Looks like it configures nothing today.
The --set defaults.<group>=null nil-pointer panic is chart-wide, not specific to the new field. A values.schema.json or (.Values.defaults.x).y guards would fix the class.
dev/manifests/operator.yaml:126-144 does not set LLM_ROUTE_REQUEST_TIMEOUT, so the new behavior cannot be exercised in local dev. Harmless (optional, empty default) and that list is already non-exhaustive, but worth a line if you want to test this locally.

Adam-D-Lewis requested a review from tylerpotts July 1, 2026 22:25

Adam-D-Lewis force-pushed the feat/configurable-route-request-timeout branch from b2d8e2b to 8b16b33 Compare July 1, 2026 22:31

dcmcand requested changes Jul 30, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(routing): make AI gateway route request timeout configurable - #125

feat(routing): make AI gateway route request timeout configurable#125
Adam-D-Lewis wants to merge 1 commit into
mainfrom
feat/configurable-route-request-timeout

Adam-D-Lewis commented Jul 1, 2026

Uh oh!

dcmcand left a comment

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand Jul 30, 2026

Uh oh!

dcmcand commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Adam-D-Lewis commented Jul 1, 2026

Problem

Change

Testing

Follow-up

Uh oh!

dcmcand left a comment

Choose a reason for hiding this comment

Blocker 1: the branch is on a stale base and the refactor reverts the fix for #116

Blocker 2: the knob is under defaults: but has no per-CR override

Blocker 3 (small): the values reference now says something untrue

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcmcand commented Jul 30, 2026

Verification evidence

General notes

Things I noticed that are not this PR's problem

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Blocker 2: the knob is under `defaults:` but has no per-CR override