Global timeouts on route not working when max_stream_duration is set
- Dominant language
- C++
- Stars
- 28.9k
- Forks
- 5.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 437
Description
## Description
I don't know if this is intended or not but it seems that the global timeouts set on the route via `timeout` [[Link]](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#:~:text=auto_host_rewrite%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22host_rewrite_header%22%3A%20%22...%22%2C%0A%20%20%22host_rewrite_path_regex%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22-,timeout,-%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22idle_timeout%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22retry_policy%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22request_mirror_policies) doesn't work when `max_stream_duration` [[Link]](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#:~:text=internal_redirect_action%22%3A%20%22...%22%2C%0A%20%20%22max_internal_redirects%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22hedge_policy%22%3A%20%22%7B...%7D%22%2C%0A%20%20%22-,max_stream_duration,-%22%3A%20%22%7B...%7D%22%0A%7D) is also set.
Documentation on either of these configs doesn't mention that they can't be used together but, looking at the code it seems to be the case.
https://github.com/envoyproxy/envoy/blob/main/source/common/router/router.cc#L137
```
if (!route.usingNewTimeouts()) {
...
}
```
What's a little confusing/surprising to us is the fact that setting the global timeouts using the header `x-envoy-upstream-rq-timeout-ms` still works even when `max_stream_duration` is supplied.
https://github.com/envoyproxy/envoy/blob/main/source/common/router/router.cc#L167
```
if (respect_expected_rq_timeout) {
...
}
```
Is this behavior intended? If this is indeed the intention then we should update the docs and clarify the behavior of `x-envoy-upstream-rq-timeout-ms`.
## Use Case
We have a mix of streaming and non-streaming routes where we are setting both the `timeout` as well as `max_stream_duration` **Current Configuration:** (max_stream_duration = timeout + 240s).
We were expecting that the non-streaming routes won't have any impact from adding `max_stream_duration` and these would still continue to return **504** status codes once the global timeout get exhausted. Streaming route on the other hand would benefit from setting `max_stream_duration` as global timeout isn't very useful on these routes.
But after the change of adding a higher `max_stream_duration` on all the routes, the non-streaming routes started returning a **408** status code, not respecting the global timeouts being set via `timeout` config. Since it changed the final status code being returned from **504 --> 408**, it started breaking some of our existing clients who have a specific post-processing logic for handling/retrying on the **504** status codes.
For now we have mitigated the issue by setting the `x-envoy-upstream-rq-timeout-ms` header on these routes as this header is still respected irrespective of whether or not the `max_stream_duration` is set on the routes. We would like to understand the correct/long term fix for this.
## Repro Steps
Try to set both the `timeout` as well as `max_stream_duration` on any of your route definitions. Even you you set a lower global timeout (say 60s) and a higher stream timeout (say 300s), the global timeout timer will never fire and you'll end up getting a **408** status code after **300s** when the stream timeout timer fires.
```
{
"cluster": "authN",
"timeout": "60s",
"idle_timeout": "3600s",
"max_stream_duration": "{\"max_stream_duration\": \"300s\"}",
...
}
```
Contributor guide
Assessment
This issue has not been assessed yet.