Adding end-to-end Timeout Functionality#2268
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds resilience-based route and listener timeout configuration, reusable upstream references, validation, transformation, Envoy translation, operator schemas, and integration coverage across REST, LLM provider/proxy, and MCP proxy paths. ChangesGateway timeout resilience and upstream references
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant APIConfig
participant Validator
participant Transformer
participant Translator
participant Envoy
APIConfig->>Validator: resilience and upstreamDefinitions
Validator->>Transformer: validated configuration
Transformer->>Translator: route timeout data
Translator->>Envoy: Timeout and IdleTimeout
sequenceDiagram
participant Feature
participant TimeoutSteps
participant Gateway
participant Backend
Feature->>TimeoutSteps: record time or send request
TimeoutSteps->>Gateway: HTTP request or incomplete headers
Gateway->>Backend: upstream request
Gateway->>TimeoutSteps: timeout response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
O-sura
marked this pull request as ready for review
June 26, 2026 03:53
O-sura
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
June 26, 2026 03:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
gateway/gateway-controller/api/management-openapi.yaml (1)
5821-5827: 📐 Maintainability & Code Quality | 🔵 TrivialPreserve the LLM-specific
resiliencescope constraint in generated models.The
resiliencefield forLLMProviderConfigDataandLLMProxyConfigDatacurrently lists a generic description in the generated Go code that implies operation-level overrides are supported ("Can be set at the API level ... and/or the operation level"). However, the OpenAPI spec explicitly restricts this to the API level only ("Supported at the API level only").Because the description is written as a sibling to the
$ref, the code generator is likely ignoring it or defaulting to the referencedResilienceschema's generic description. To ensure the specific "API-level only" constraint is preserved in the generated documentation:Wrap the reference in an
allOfblock or create a dedicated schema for these fields:resilience: allOf: - $ref: '`#/components/schemas/Resilience`' description: > API-level backend/route timeout configuration. Applies to all routes generated for this LLM Provider (the routes that forward traffic upstream). Supported at the API level only - LLM routes are synthesized by the gateway, so there is no operation-level override.Apply this pattern to both
LLMProviderConfigData(lines 5821-5827) andLLMProxyConfigData(lines 6118-6124).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/api/management-openapi.yaml` around lines 5821 - 5827, The generated models are dropping the LLM-specific API-level-only constraint for the resilience field because the description is attached beside a $ref and may be overridden by the shared Resilience schema. Update the resilience definitions in LLMProviderConfigData and LLMProxyConfigData in the OpenAPI spec to wrap the reference in an allOf block or use a dedicated schema so the field-level description is preserved. Make sure the generated Go docs retain the “Supported at the API level only” wording and do not imply operation-level overrides.kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yaml (1)
101-115: 🗄️ Data Integrity & Integration | 🔵 TrivialDuration pattern is stricter than runtime parser
The CRD regex
^\d+(\.\d+)?(ms|s|m|h)$enforces single-unit durations (e.g., "30s"), while the controller logic ingateway/gateway-controller/pkg/config/api_validator.go(line 471) uses Go'stime.ParseDuration, which natively supports compound forms (e.g., "1h30m"). This mismatch causes the API server to reject valid durations that the runtime would accept.If the goal is parity with the runtime, update the CRD pattern to match Go's accepted formats. If single units are intentional, document the restriction in the field description.
Also applies to lines 150-164.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yaml` around lines 101 - 115, The duration validation for the resilience fields is stricter in the CRD than in the controller’s runtime parsing, so valid values accepted by `time.ParseDuration` in `api_validator.go` can be rejected by the API server. Update the `resilience.idleTimeout` and `resilience.timeout` schema patterns in the CRD to match Go duration syntax if parity is intended, or otherwise revise the field descriptions to explicitly state that only single-unit durations are allowed. Apply the same change to the duplicate resilience schema block referenced in the other section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/configs/config-template.toml`:
- Around line 215-225: Clarify the HCM timeout contract by aligning the template
and validation behavior around idle_timeout: update the comments in
config-template.toml to match what validateTimeoutConfig() and
TestConfig_ValidateHCMTimeouts actually accept, or adjust those validation paths
if idle_timeout should not allow 0s. Use the router.http_listener.timeouts
section and the validateTimeoutConfig/TestConfig_ValidateHCMTimeouts symbols to
keep the template text and runtime rules consistent.
In `@gateway/gateway-controller/pkg/config/api_validator.go`:
- Around line 457-490: The timeout validation in validateResilienceTimeouts is
more permissive than the CRD pattern, so align it with the admission rule or
document the intentional mismatch. Update validateResilienceTimeouts in
api_validator.go to enforce the same duration format as the CRD regex used for
Resilience.Timeout and Resilience.IdleTimeout, and make sure the downstream
ResolveResilience parsing behavior in translator.go matches that contract so
values like bare 0, compound durations, and negatives are handled consistently.
In `@gateway/it/features/upstream-connect-timeout.feature`:
- Line 50: The upstream connect timeout scenario uses an unreliable target and
the surrounding comment is stale. Update the comment in the feature scenario to
match the actual target used by the test, and adjust the test input in the
connect_timeout case to use a documented blackhole address such as 192.0.2.1 in
the scenario or otherwise guarantee that 10.255.255.1 is silently dropped in the
test environment. Use the scenario text and the connect_timeout setup in
upstream-connect-timeout.feature to keep the timeout assertion stable and avoid
immediate unreachable failures.
---
Nitpick comments:
In `@gateway/gateway-controller/api/management-openapi.yaml`:
- Around line 5821-5827: The generated models are dropping the LLM-specific
API-level-only constraint for the resilience field because the description is
attached beside a $ref and may be overridden by the shared Resilience schema.
Update the resilience definitions in LLMProviderConfigData and
LLMProxyConfigData in the OpenAPI spec to wrap the reference in an allOf block
or use a dedicated schema so the field-level description is preserved. Make sure
the generated Go docs retain the “Supported at the API level only” wording and
do not imply operation-level overrides.
In
`@kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yaml`:
- Around line 101-115: The duration validation for the resilience fields is
stricter in the CRD than in the controller’s runtime parsing, so valid values
accepted by `time.ParseDuration` in `api_validator.go` can be rejected by the
API server. Update the `resilience.idleTimeout` and `resilience.timeout` schema
patterns in the CRD to match Go duration syntax if parity is intended, or
otherwise revise the field descriptions to explicitly state that only
single-unit durations are allowed. Apply the same change to the duplicate
resilience schema block referenced in the other section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7996541b-429f-42d2-8d91-7c080025f8ab
📒 Files selected for processing (36)
gateway/build-manifest.yamlgateway/configs/config-template.tomlgateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/api_validator_test.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/config_test.gogateway/gateway-controller/pkg/config/llm_validator.gogateway/gateway-controller/pkg/config/llm_validator_resilience_test.gogateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/models/runtime_deploy_config.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/llm_resilience_test.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/it/features/backend-timeout.featuregateway/it/features/llm-backend-timeout.featuregateway/it/features/upstream-connect-timeout.featuregateway/it/steps_backend_timeout.gogateway/it/steps_timeouts.gogateway/it/suite_test.gogateway/it/test-config.tomlkubernetes/gateway-operator/api/v1alpha1/llmprovider_types.gokubernetes/gateway-operator/api/v1alpha1/llmproxy_types.gokubernetes/gateway-operator/api/v1alpha1/restapi_types.gokubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.gokubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yamlkubernetes/gateway-operator/config/samples/api_v1_restapi.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_restapis.yaml
💤 Files with no reviewable changes (1)
- gateway/it/steps_backend_timeout.go
O-sura
force-pushed
the
apip-resiliency
branch
2 times, most recently
from
July 7, 2026 10:45
f8b3ebb to
bfbf2df
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
gateway/it/features/upstream-connect-timeout.feature (1)
70-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment describes the wrong timeout mechanism.
This scenario omits
timeout.connectand points at an unreachable host, so the 503 is triggered by the global default connect timeout fallback — not the "route timeout" mentioned in the comment (an overall route/request timeout typically surfaces as 504, not 503). Consider rewording to avoid confusing future maintainers about what's being exercised.✏️ Proposed comment fix
- # Global-default scenario: route timeout comes from it config (6s); elapsed-time assertion verifies configured global timeout. + # Global-default scenario: no per-upstream connect timeout is set, so the global default + # connect_timeout (6s, from it config) applies; elapsed-time assertion verifies the fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/it/features/upstream-connect-timeout.feature` around lines 70 - 71, The scenario comment in the upstream-connect-timeout feature is describing the wrong timeout mechanism, which can mislead readers about what is being tested. Update the comment near RestApi without upstream timeout uses global defaults to say it is exercising the global default connect timeout fallback for an unreachable host, and avoid mentioning route/request timeout or 504 semantics; keep the wording aligned with the scenario’s actual behavior and the 503 result.gateway/gateway-controller/pkg/utils/mcp_transformer.go (1)
117-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRef/URL upstream mapping duplicated with
llm_transformer.go.This branch (
RefvsUrlmapping plusUpstreamDefinitionsthreading) is identical to the logic added intransformProviderinllm_transformer.go(Lines 350-359). Consider extracting a shared helper (e.g.,buildUpstreamFromRefOrUrl(ref, url *string) api.Upstream) to avoid future divergence between the two transformers.♻️ Proposed shared helper
func buildMainUpstream(ref, url *string) api.Upstream { if ref != nil && strings.TrimSpace(*ref) != "" { return api.Upstream{Ref: ref} } return api.Upstream{Url: url} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/utils/mcp_transformer.go` around lines 117 - 128, The Ref/Url upstream mapping and UpstreamDefinitions threading in the MCP transformer are duplicated with the same logic in transformProvider, so extract the shared branch into a helper (for example buildMainUpstream or similar) and reuse it from mcp_transformer.go and llm_transformer.go. Keep the helper responsible for choosing between Ref and Url based on the trimmed ref value, then assign UpstreamDefinitions separately where needed so both transformers stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/gateway-controller/pkg/config/llm_validator_additional_test.go`:
- Line 278: The current `validateUpstreamWithAuth` test coverage only uses nil
`definitions`, so the ref-resolution path is still untested. Add an additional
test case in `llm_validator_additional_test.go` that calls
`validator.validateUpstreamWithAuth` with a populated `upstreamDefinitions`
value, and assert the expected behavior for the ref-resolution branch using the
existing `validator` test setup.
In `@gateway/gateway-controller/pkg/config/mcp_validator.go`:
- Around line 219-223: The validation in mcp_validator.go is using a misleading
URL-only message when both url and ref are missing; update the case in the
validator logic so the error is attached to fieldPrefix rather than
fieldPrefix.url, and make the ValidationError.Message field-agnostic to reflect
that either url or ref can satisfy the requirement. Use the existing
hasURL/hasRef check in the same validation branch to keep the fix localized.
- Around line 211-212: In mcp_validator.go, the URL validation around
hasURL/hasRef needs two fixes: trim the upstream.Url value before passing it to
url.Parse so whitespace-only formatting doesn’t cause false failures, and update
the !hasURL && !hasRef error path to use a generic message that matches the XOR
rule (either url or ref is required). Keep the logic aligned with the existing
hasURL, hasRef, and validation branch in the same validation function.
In `@gateway/gateway-controller/pkg/config/upstream_validation.go`:
- Around line 142-150: The connect-timeout validation only checks the regex and
can still accept duration strings that overflow when parsed later. In the
validation path that appends the ValidationError for def.Timeout.Connect, add a
final time.ParseDuration check after the regex match, mirroring the overflow
guard used in validateResilienceTimeouts in api_validator.go. Keep the existing
error message for invalid format, and make the new check reject pathological
values like very long digit strings that would otherwise pass the regex.
- Around line 47-53: The UpstreamDefinition name validation only rejects empty
values, so add the CRD’s name constraints in the upstream validation path.
Update the logic in the UpstreamDefinition validation loop to enforce the 1-100
length limit and the ^[a-zA-Z0-9\-_]+$ pattern before accepting REST input,
alongside the existing empty-name check. Use the existing ValidationError
handling in upstream_validation.go so invalid names are reported consistently
for ref resolution and cluster naming.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/mcp_transformer.go`:
- Around line 117-128: The Ref/Url upstream mapping and UpstreamDefinitions
threading in the MCP transformer are duplicated with the same logic in
transformProvider, so extract the shared branch into a helper (for example
buildMainUpstream or similar) and reuse it from mcp_transformer.go and
llm_transformer.go. Keep the helper responsible for choosing between Ref and Url
based on the trimmed ref value, then assign UpstreamDefinitions separately where
needed so both transformers stay consistent.
In `@gateway/it/features/upstream-connect-timeout.feature`:
- Around line 70-71: The scenario comment in the upstream-connect-timeout
feature is describing the wrong timeout mechanism, which can mislead readers
about what is being tested. Update the comment near RestApi without upstream
timeout uses global defaults to say it is exercising the global default connect
timeout fallback for an unreachable host, and avoid mentioning route/request
timeout or 504 semantics; keep the wording aligned with the scenario’s actual
behavior and the 503 result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1561ccd0-041b-4c3d-a80b-bdf2c2a8eb03
📒 Files selected for processing (47)
gateway/configs/config-template.tomlgateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/api_validator_test.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/config_test.gogateway/gateway-controller/pkg/config/llm_mcp_upstream_ref_validation_test.gogateway/gateway-controller/pkg/config/llm_validator.gogateway/gateway-controller/pkg/config/llm_validator_additional_test.gogateway/gateway-controller/pkg/config/llm_validator_resilience_test.gogateway/gateway-controller/pkg/config/mcp_validator.gogateway/gateway-controller/pkg/config/upstream_validation.gogateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/models/runtime_deploy_config.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/llm_mcp_upstream_ref_test.gogateway/gateway-controller/pkg/utils/llm_resilience_test.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/gateway-controller/pkg/utils/mcp_transformer.gogateway/gateway-controller/pkg/xds/resilience_duration_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/it/features/backend-timeout.featuregateway/it/features/backend_timeout.featuregateway/it/features/llm-backend-timeout.featuregateway/it/features/upstream-connect-timeout.featuregateway/it/steps_backend_timeout.gogateway/it/steps_timeouts.gogateway/it/suite_test.gogateway/it/test-config.tomlkubernetes/gateway-operator/api/v1alpha1/llmprovider_types.gokubernetes/gateway-operator/api/v1alpha1/llmproxy_types.gokubernetes/gateway-operator/api/v1alpha1/mcp_types.gokubernetes/gateway-operator/api/v1alpha1/restapi_types.gokubernetes/gateway-operator/api/v1alpha1/upstream_definition_types.gokubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.gokubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_mcps.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yamlkubernetes/gateway-operator/config/samples/api_v1_restapi.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_mcps.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_restapis.yaml
💤 Files with no reviewable changes (2)
- gateway/it/features/backend_timeout.feature
- gateway/it/steps_backend_timeout.go
✅ Files skipped from review due to trivial changes (3)
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yaml
- gateway/gateway-controller/pkg/api/management/generated.go
- kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (25)
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yaml
- kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go
- gateway/it/test-config.toml
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yaml
- gateway/gateway-controller/pkg/xds/resilience_duration_test.go
- gateway/it/features/backend-timeout.feature
- gateway/it/steps_timeouts.go
- gateway/it/features/llm-backend-timeout.feature
- gateway/gateway-controller/pkg/models/runtime_deploy_config.go
- gateway/it/suite_test.go
- kubernetes/gateway-operator/config/samples/api_v1_restapi.yaml
- kubernetes/gateway-operator/api/v1alpha1/restapi_types.go
- gateway/gateway-controller/pkg/config/api_validator_test.go
- gateway/gateway-controller/pkg/config/llm_validator_resilience_test.go
- gateway/gateway-controller/pkg/config/config.go
- gateway/gateway-controller/pkg/constants/constants.go
- gateway/gateway-controller/pkg/transform/restapi.go
- gateway/gateway-controller/pkg/utils/llm_resilience_test.go
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_restapis.yaml
- gateway/configs/config-template.toml
- gateway/gateway-controller/pkg/transform/restapi_test.go
- gateway/gateway-controller/pkg/xds/translator_test.go
- gateway/gateway-controller/api/management-openapi.yaml
- gateway/gateway-controller/pkg/xds/translator.go
- gateway/gateway-controller/pkg/config/config_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
gateway/gateway-controller/pkg/config/mcp_validator_test.go (1)
660-777: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMCP upstream-ref and resilience tests have slightly less coverage than LLM counterparts.
The MCP tests cover the core paths (valid ref, not found, both url+ref, malformed/valid connect timeout, valid/nil/0s/malformed/compound/negative resilience). However, compared to the LLM tests, the following cases are missing:
- Malformed
idleTimeout(only negative is tested)- Invalid name characters in upstream definitions
- Name length > 100 chars
- Fractional connect timeout acceptance
- Overflow connect timeout rejection
Since both validators delegate to the same shared
validateUpstreamDefinitionsList, the core logic is exercised by the LLM tests. Adding parity cases here would improve confidence that MCP-specific wiring doesn't diverge.🧪 Suggested additional test cases
func TestValidateMCP_UpstreamRef(t *testing.T) { validator := NewMCPValidator() // ... existing cases ... + t.Run("valid fractional connect timeout accepted", func(t *testing.T) { + defs := &[]api.UpstreamDefinition{upstreamDef("mcp-backend", "500ms")} + errs := validator.Validate(mcpWithUpstream(defs, api.MCPProxyConfigData_Upstream{Ref: stringPtr("mcp-backend")})) + assert.Empty(t, errs) + }) + + t.Run("connect timeout that overflows time.Duration rejected", func(t *testing.T) { + defs := &[]api.UpstreamDefinition{upstreamDef("mcp-backend", "99999999999999999999s")} + errs := validator.Validate(mcpWithUpstream(defs, api.MCPProxyConfigData_Upstream{Ref: stringPtr("mcp-backend")})) + assertHasFieldError(t, errs, "spec.upstreamDefinitions[0].timeout.connect") + }) + + t.Run("definition name with invalid characters rejected (CRD pattern)", func(t *testing.T) { + defs := &[]api.UpstreamDefinition{upstreamDef("bad name!", "6s")} + errs := validator.Validate(mcpWithUpstream(defs, api.MCPProxyConfigData_Upstream{Ref: stringPtr("bad name!")})) + assertHasFieldError(t, errs, "spec.upstreamDefinitions[0].name") + }) + + t.Run("definition name over 100 chars rejected", func(t *testing.T) { + long := strings.Repeat("a", 101) + defs := &[]api.UpstreamDefinition{upstreamDef(long, "6s")} + errs := validator.Validate(mcpWithUpstream(defs, api.MCPProxyConfigData_Upstream{Ref: stringPtr(long)})) + assertHasFieldError(t, errs, "spec.upstreamDefinitions[0].name") + }) } func TestValidateMCP_Resilience(t *testing.T) { validator := NewMCPValidator() // ... existing cases ... + t.Run("malformed idleTimeout is rejected", func(t *testing.T) { + errs := validator.Validate(mcpWithResilience(&api.Resilience{IdleTimeout: stringPtr("abc")})) + assertHasFieldError(t, errs, "spec.resilience.idleTimeout") + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/config/mcp_validator_test.go` around lines 660 - 777, The MCP validator tests in TestValidateMCP_UpstreamRef and TestValidateMCP_Resilience are missing parity coverage with the LLM suite. Add focused cases for malformed resilience idleTimeout, invalid upstream definition names (illegal characters and >100 length), and connect timeout edge cases (fractional acceptance and overflow rejection) using the existing helpers mcpWithUpstream, mcpWithResilience, and upstreamDef so the MCP wiring is validated against the shared validateUpstreamDefinitionsList behavior.gateway/gateway-controller/pkg/api/management/generated.go (1)
1079-1084: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMCPProxyConfigData.Resilience doc doesn't match MCP's actual default behavior.
The doc comment states resilience "Can be set at the API level... and/or the operation level" and that unset timeout falls back to "the gateway's global route timeout defaults". But
kubernetes/gateway-operator/api/v1alpha1/mcp_types.godocuments MCP resilience as "Resilience configures API-level backend/route timeouts applied to the traffic-forwarding routes generated for this MCP proxy. Supported at the API level only. Because MCP transports are long-lived streams, the route timeout defaults to disabled ("0s") for MCP when unset." The graph evidence testTestMCPTransform_Resilience_DefaultsRouteTimeoutDisabledconfirms Timeout defaults to"0s"(disabled) for MCP, not the generic gateway default. There is no operation-level override surface for MCP proxies at all (noOperationsfield onMCPProxyConfigData).This generic doc text appears copy-pasted from
APIConfigData.Resilience/Operation.Resilienceand should be corrected to reflect MCP-specific semantics in the sourcemanagement-openapi.yaml, then regenerated.📝 Suggested doc fix (apply in management-openapi.yaml, then regenerate)
- // Resilience Backend/route timeout configuration. Maps to Envoy RouteAction timeouts. Can be set at the API level (applies to all routes) and/or the operation level (applies to that operation's route). When set at both levels, the operation-level value takes precedence. When unset, the gateway's global route timeout defaults apply. + // Resilience API-level backend/route timeout configuration applied to the traffic-forwarding routes generated for this MCP proxy. Supported at the API level only (MCP has no per-operation override surface). Because MCP transports are long-lived streams, the route timeout defaults to disabled ("0s") when unset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/api/management/generated.go` around lines 1079 - 1084, The Resilience doc on MCPProxyConfigData is still using the generic API/operation timeout wording, but MCP only supports API-level resilience and defaults to disabled timeout when unset. Update the source documentation for Resilience in the management OpenAPI definitions (then regenerate generated.go) so it matches MCP semantics: remove any operation-level language, describe API-level-only support, and state that the unset default is "0s" rather than the gateway global route timeout. Use the MCPProxyConfigData.Resilience field and the related MCP types as the reference points when making the doc change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kubernetes/gateway-operator/api/v1alpha1/restapi_types.go`:
- Around line 227-229: Add validation for UpstreamDefinition.BasePath so only
well-formed path prefixes are accepted. Update the BasePath field in
restapi_types.go with a pattern that mirrors the existing Context validation
approach, enforcing a leading slash and rejecting trailing slashes or other
malformed values. Make sure the marker is placed on the BasePath field so the
generated CRD and admission validation both enforce it.
- Around line 257-261: The Weight field in RestAPI-related types is documented
as defaulting to 100, but it currently only has min/max validation and leaves
omitted values nil. Update the Weight definition in the struct containing the
Weight *int field to either add the kubebuilder default annotation for 100 or,
if nil is intended, remove the default wording from the comment so the API
contract matches the generated CRD.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/api/management/generated.go`:
- Around line 1079-1084: The Resilience doc on MCPProxyConfigData is still using
the generic API/operation timeout wording, but MCP only supports API-level
resilience and defaults to disabled timeout when unset. Update the source
documentation for Resilience in the management OpenAPI definitions (then
regenerate generated.go) so it matches MCP semantics: remove any operation-level
language, describe API-level-only support, and state that the unset default is
"0s" rather than the gateway global route timeout. Use the
MCPProxyConfigData.Resilience field and the related MCP types as the reference
points when making the doc change.
In `@gateway/gateway-controller/pkg/config/mcp_validator_test.go`:
- Around line 660-777: The MCP validator tests in TestValidateMCP_UpstreamRef
and TestValidateMCP_Resilience are missing parity coverage with the LLM suite.
Add focused cases for malformed resilience idleTimeout, invalid upstream
definition names (illegal characters and >100 length), and connect timeout edge
cases (fractional acceptance and overflow rejection) using the existing helpers
mcpWithUpstream, mcpWithResilience, and upstreamDef so the MCP wiring is
validated against the shared validateUpstreamDefinitionsList behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea2b69c8-ace9-4922-8519-b1224d599b2d
📒 Files selected for processing (24)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/llm_validator.gogateway/gateway-controller/pkg/config/llm_validator_test.gogateway/gateway-controller/pkg/config/mcp_validator.gogateway/gateway-controller/pkg/config/mcp_validator_test.gogateway/gateway-controller/pkg/utils/llm_provider_transformer_test.gogateway/gateway-controller/pkg/utils/mcp_transformer.gogateway/gateway-controller/pkg/utils/mcp_transformer_test.gogateway/gateway-controller/pkg/xds/translator_test.gokubernetes/gateway-operator/api/v1alpha1/mcp_types.gokubernetes/gateway-operator/api/v1alpha1/restapi_types.gokubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.gokubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_mcps.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yamlkubernetes/gateway-operator/config/samples/api_v1_restapi.yamlkubernetes/gateway-operator/internal/controller/httproute_mapper.gokubernetes/gateway-operator/internal/controller/httproute_mapper_test.gokubernetes/gateway-operator/internal/gatewayclient/yaml_payload_test.gokubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_mcps.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_restapis.yaml
✅ Files skipped from review due to trivial changes (2)
- kubernetes/gateway-operator/internal/controller/httproute_mapper_test.go
- kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (7)
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproviders.yaml
- kubernetes/gateway-operator/config/samples/api_v1_restapi.yaml
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproviders.yaml
- gateway/gateway-controller/pkg/xds/translator_test.go
- gateway/gateway-controller/pkg/config/llm_validator.go
- gateway/gateway-controller/pkg/config/mcp_validator.go
- gateway/gateway-controller/api/management-openapi.yaml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/xds/translator.go (1)
3287-3309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify route-timeout precedence logic.
combineRouteResilienceandgateway/gateway-controller/pkg/transform/restapi.go’sbuildRouteTimeoutboth implement the same op-over-API, per-field precedence and nil-elision rules. Extract one shared helper and have both paths use it to avoid the two code paths drifting apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/xds/translator.go` around lines 3287 - 3309, Unify the duplicated operation-over-API timeout precedence and nil-elision logic currently in combineRouteResilience and buildRouteTimeout. Extract a shared helper for resolving route and idle timeout values, then update both functions to use it while preserving connect-timeout handling and existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 3287-3309: Unify the duplicated operation-over-API timeout
precedence and nil-elision logic currently in combineRouteResilience and
buildRouteTimeout. Extract a shared helper for resolving route and idle timeout
values, then update both functions to use it while preserving connect-timeout
handling and existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f26aa3d7-e996-444a-aaaf-dbc0cc1e1781
📒 Files selected for processing (51)
gateway/build-manifest.yamlgateway/configs/config-template.tomlgateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/api_validator_test.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/config_test.gogateway/gateway-controller/pkg/config/llm_validator.gogateway/gateway-controller/pkg/config/llm_validator_additional_test.gogateway/gateway-controller/pkg/config/llm_validator_test.gogateway/gateway-controller/pkg/config/mcp_validator.gogateway/gateway-controller/pkg/config/mcp_validator_test.gogateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/models/runtime_deploy_config.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/llm_provider_transformer_test.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/gateway-controller/pkg/utils/mcp_transformer.gogateway/gateway-controller/pkg/utils/mcp_transformer_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/it/features/backend-timeout.featuregateway/it/features/backend_timeout.featuregateway/it/features/llm-backend-timeout.featuregateway/it/features/upstream-connect-timeout.featuregateway/it/steps_backend_timeout.gogateway/it/steps_timeouts.gogateway/it/suite_test.gogateway/it/test-config.tomlkubernetes/gateway-operator/api/v1alpha1/llmprovider_types.gokubernetes/gateway-operator/api/v1alpha1/llmproxy_types.gokubernetes/gateway-operator/api/v1alpha1/mcp_types.gokubernetes/gateway-operator/api/v1alpha1/restapi_types.gokubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.gokubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_mcps.yamlkubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yamlkubernetes/gateway-operator/config/samples/api_v1_restapi.yamlkubernetes/gateway-operator/internal/controller/httproute_mapper.gokubernetes/gateway-operator/internal/controller/httproute_mapper_test.gokubernetes/gateway-operator/internal/gatewayclient/yaml_payload_test.gokubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yamlkubernetes/helm/gateway-helm-chart/values.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproviders.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_mcps.yamlkubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_restapis.yamlkubernetes/helm/operator-helm-chart/values.yaml
💤 Files with no reviewable changes (2)
- gateway/it/steps_backend_timeout.go
- gateway/it/features/backend_timeout.feature
✅ Files skipped from review due to trivial changes (1)
- kubernetes/gateway-operator/internal/controller/httproute_mapper_test.go
🚧 Files skipped from review as they are similar to previous changes (45)
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yaml
- gateway/build-manifest.yaml
- kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml
- kubernetes/helm/gateway-helm-chart/values.yaml
- kubernetes/gateway-operator/internal/controller/httproute_mapper.go
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_mcps.yaml
- kubernetes/gateway-operator/internal/gatewayclient/yaml_payload_test.go
- gateway/it/test-config.toml
- kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go
- gateway/gateway-controller/pkg/config/llm_validator_additional_test.go
- gateway/configs/config-template.toml
- gateway/gateway-controller/pkg/models/runtime_deploy_config.go
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproviders.yaml
- kubernetes/gateway-operator/api/v1alpha1/mcp_types.go
- gateway/it/features/backend-timeout.feature
- gateway/gateway-controller/pkg/constants/constants.go
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yaml
- gateway/gateway-controller/pkg/config/api_validator_test.go
- gateway/gateway-controller/pkg/transform/restapi_test.go
- kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_restapis.yaml
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproviders.yaml
- kubernetes/gateway-operator/config/samples/api_v1_restapi.yaml
- kubernetes/helm/operator-helm-chart/values.yaml
- gateway/it/features/llm-backend-timeout.feature
- gateway/gateway-controller/pkg/transform/restapi.go
- gateway/it/suite_test.go
- gateway/gateway-controller/pkg/utils/mcp_transformer.go
- gateway/gateway-controller/pkg/utils/mcp_transformer_test.go
- gateway/gateway-controller/pkg/config/config_test.go
- gateway/it/steps_timeouts.go
- gateway/gateway-controller/pkg/utils/llm_transformer.go
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_mcps.yaml
- gateway/gateway-controller/pkg/config/mcp_validator.go
- gateway/gateway-controller/pkg/config/llm_validator.go
- kubernetes/gateway-operator/api/v1alpha1/restapi_types.go
- kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_restapis.yaml
- kubernetes/gateway-operator/api/v1alpha1/llmprovider_types.go
- gateway/gateway-controller/pkg/xds/translator_test.go
- gateway/gateway-controller/pkg/config/mcp_validator_test.go
- gateway/it/features/upstream-connect-timeout.feature
- gateway/gateway-controller/pkg/utils/llm_provider_transformer_test.go
- kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go
- gateway/gateway-controller/api/management-openapi.yaml
- gateway/gateway-controller/pkg/config/llm_validator_test.go
- gateway/gateway-controller/pkg/config/api_validator.go
Krishanx92
approved these changes
Jul 11, 2026
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
2 participants
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.
Purpose
This PR introduces end-to-end timeout configuration across the gateway data plane. It adds a new resilience block to RestApi resources, allowing API- and operation-level configuration of backend request and idle timeouts, which are translated into Envoy route timeouts and can be disabled with "0s". It also enhances support for upstream connect timeouts and exposes Envoy HTTP Connection Manager (HCM) downstream timeouts through config.toml, enabling protection against slow or stalled clients and backends. The implementation includes CRD and schema updates, validation, deployment transforms, xDS translation, documentation updates, and comprehensive integration and unit test coverage to verify timeout behavior across the stack.