envoyproxy / envoyproxy/envoy

Global timeouts on route not working when max_stream_duration is set

Open
#19,073 13 comments 0 reactions 0 assignees View on GitHub
area/http area/router bug help wanted
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.