feat Route Timeouts

[arkodg]

Fixes: #877

[codecov]

Codecov Report

Merging #1899 (dc2ec96) into main (a785be8) will decrease coverage by 0.07%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##             main    #1899      +/-   ##
==========================================
- Coverage   65.51%   65.44%   -0.07%     
==========================================
  Files          88       88              
  Lines       13097    13120      +23     
==========================================
+ Hits         8580     8586       +6     
- Misses       3989     4003      +14     
- Partials      528      531       +3     

Files	Coverage Δ
internal/gatewayapi/route.go	88.12% <100.00%> (+0.21%)	⬆️
internal/ir/xds.go	75.51% <ø> (ø)
internal/xds/translator/route.go	90.90% <100.00%> (+0.10%)	⬆️
internal/ir/zz_generated.deepcopy.go	14.19% <0.00%> (-0.09%)	⬇️

... and 2 files with indirect coverage changes

[arkodg]

the timeout API was accidentally made part of gateway-api v0.8.1 so I took a quick stab at implementing it.
However prefer waiting until this API is officially out in v1.0 with conformance tests in the next few weeks

[arkodg]

proactively raising some questions

• should we always set a default route timeout ? or default to Envoy's default of 15s which imo is too high
• should we always explicitly set the default retry policy ? imo we should
  • retry count - 1 (this is Envoy default) or 2
  • retry on - reset,connect-failure,refused-stream,unavailable,cancelled,retriable-status-codes
  • per try timeout - route timeout / retry count
  • retriable_status_codes - 503
  • retry_host_predicate - envoy.retry_host_predicates.previous_hosts
  • host_selection_retry_max_attempts: 3

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-retrypolicy

ptal @envoyproxy/gateway-maintainers

[zhaohuabing]

proactively raising some questions

• should we always set a default route timeout ? or default to Envoy's default of 15s which imo is too high

Maybe stick to Envoy's default until we get a strong reason to change it - like many feedbacks from end users.

• should we always explicitly set the default retry policy ? imo we should
  • retry count - 1 (this is Envoy default) or 2
  • retry on - reset,connect-failure,refused-stream,unavailable,cancelled,retriable-status-codes
  • per try timeout - route timeout / retry count
  • retriable_status_codes - 503
  • retry_host_predicate - envoy.retry_host_predicates.previous_hosts
  • host_selection_retry_max_attempts: 3

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-retrypolicy

ptal @envoyproxy/gateway-maintainers

We cant assume the backkend services are always idempotent. I have encountered issues on retries - the service has been called multiple times on the same requests and caused problems on the server side.

[arkodg]

proactively raising some questions

• should we always set a default route timeout ? or default to Envoy's default of 15s which imo is too high

Maybe stick to Envoy's default until we get a strong reason to change it - like many feedbacks from end users.

• should we always explicitly set the default retry policy ? imo we should

  • retry count - 1 (this is Envoy default) or 2
  • retry on - reset,connect-failure,refused-stream,unavailable,cancelled,retriable-status-codes
  • per try timeout - route timeout / retry count
  • retriable_status_codes - 503
  • retry_host_predicate - envoy.retry_host_predicates.previous_hosts
  • host_selection_retry_max_attempts: 3

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-retrypolicy
ptal @envoyproxy/gateway-maintainers

We cant assume the backkend services are always idempotent. I have encountered issues on retries - the service has been called multiple times on the same requests and caused problems on the server side.

hey @zhaohuabing not adding any retries in this PR
@kflynn and I chatted about this in the community meeting last week and decided to make retries opt in using a seperate API.

Also sticking to envoy timeout default as the default

[zirain]

any conformance/e2e test for this?

[arkodg]

kubernetes-sigs/gateway-api#2254 should be get merged soon

[zirain]

let's wait for that?

[arkodg]

let's wait for that?

there are already translation tests in gateway-api & xds layer

[zhaohuabing]

LGTM

I'm okay with merging it now and updating the conformance test later when kubernetes-sigs/gateway-api#2254 is merged.

[kflynn]

Looks OK from here! 🙂