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     

... and 2 files with indirect coverage changes

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

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

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.

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

any conformance/e2e test for this?

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

let's wait for that?

let's wait for that?

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