Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contour should configure a client request timeout #1073

Closed
davecheney opened this issue May 9, 2019 · 7 comments · Fixed by #1601
Closed

Contour should configure a client request timeout #1073

davecheney opened this issue May 9, 2019 · 7 comments · Fixed by #1601
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@davecheney
Copy link
Contributor

In #1070 the timeout for idle connections was set to 60 seconds, but we punted on setting a request timeout because no reasonable value could be found.

This issue is to track the research on a reasonable value which should be applied for all requests. This is complicated by the following factor:

  • request timeout is a property of the http connection manager attached to a listener. For https connections there is one manager per connection, but for http all vhosts share the same manager, this makes it complicated to provide a per ingress/ingressroute configuration.
  • ideally we don't want to make this value configurable -- the best result will be choosing a value which is small enough to prevent sloloris style resource exhaustion, and large enough to permit gigabyte style uploads.
  • possibly http_connection_manager.request_timeout is not the right parameter. We may be better served elsewhere.
@davecheney davecheney added kind/feature Categorizes issue or PR as related to a new feature. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels May 9, 2019
@davecheney davecheney added this to the 0.13.0 milestone May 9, 2019
@davecheney davecheney removed good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jun 13, 2019
@youngnick youngnick self-assigned this Jun 13, 2019
@youngnick
Copy link
Member

I wasn't sure which timeout did what, so I spent some time reading Envoy documentation. Here's what I found:

1073

Envoy timeouts

There are five timeouts you can set on connections through Envoy, three at the the connection manager, and two at the route.

Connection manager timeouts

idle_timeout

Applies to the idle period when no streams are active, and will terminate the connection.

There is no default, but we set to 60s in Contour (see the previous issue)

stream_idle_timeout

this applies to each individual stream that is part of a connection. It may be configured at both the connection manager and per-route granularity. Header/data/trailer events on the stream reset the idle timeout. Note that I believe that 'events' in this context refers to the edge of one of those three things - header, data or trailer, not to the reception of packets. Will need to check Envoy code to confirm.

Defaults to 5 minutes. We currently don't set a value.

request_timeout

This applies to requests from the connection manager to upstream clusters. This timer starts when the request is started, and is disabled when the last byte of the request is sent upstream. That is, this is an Envoy timeout for the sending of data to the upstream backend, not a client timeout. However, since we are proxying requests, it is a proxy (heh) for a client timeout.

This defaults to disabled, we currently do not set it.

drain_timeout

This is about how long Envoy will wait between sending the HTTP/2 start of shutdown message and shutting down the connection.

Defaults to 5s, we do not set it.

Route timeouts

These timeouts apply on a per-route basis, and cover the communication with the upstream cluster.

timeout

Specifies the upstream timeout for the route. If not specified, the default is 15s. This spans between the point at which the entire downstream request (i.e. end-of-stream) has been processed and when the upstream response has been completely processed.

We do not set this one, but it establishes an upper bound on the upstream response time.

idle_timeout

Similarly to the connection manager stream_idle_timeout, this sets how long that streams matching this route can be idle. A new header or data encoding or decoding event will reset this timer.

It is unset by default, and the connection manager stream_idle_default value is used instead. If this is set, it overrides the connection manager value, even if idle_timeout is set to zero (which disables the timeout).

Thoughts

Setting a http_connection_manager.request_timeout may be the right thing, but I'd like to have a play with it and come up with some testing before I commit to that.

@davecheney
Copy link
Contributor Author

@youngnick thanks for the update. Do you think we need to do anything for 0.13? If this isn't urgent that we ship it next week for 0.13 would you mind moving the milestone to 0.14?

@youngnick
Copy link
Member

I don't think there's any urgency, so I'll move it.

@youngnick youngnick modified the milestones: 0.13.0, 0.14.0 Jun 13, 2019
@youngnick youngnick modified the milestones: 0.14.0, 0.15.0 Jul 2, 2019
@davecheney davecheney modified the milestones: 0.15.0, 1.0.0-rc.1 Aug 19, 2019
@davecheney
Copy link
Contributor Author

Moving to rc1 to decide if we want to address this for Contour 1.0

@youngnick
Copy link
Member

The way Envoy talks about these timeouts is very oriented towards Envoy itself, so I made a diagram to help.

Envoy request and response timeouts

We already allow setting the route-level timeout in HTTPProxy, which is, from the client's point of view, a response timeout. But we don't have anything that approximates a request timeout.

The connection manager request timeout is the correct place for this. In Contour, this should be a global setting, that defaults to disabled (which is the Envoy default).
This is because this request timeout includes the whole request time, so is governed by factors like how big the average request is, how much bandwidth the client has, and so on.
Setting a default timeout in this case is almost certain to inadvertently break some users.

So, in summary, we should:

  • allow the setting of this as a global timeout in the configuration file.
  • default this timeout to disabled.

@davecheney
Copy link
Contributor Author

@youngnick thanks for following this thought. Do you want to look at

  • allow the setting of this as a global timeout in the configuration file.
  • default this timeout to disabled.

These two (the latter might come for free) for rc.1?

@youngnick
Copy link
Member

Yep, I think so.

youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes projectcontour#1073.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit that referenced this issue Oct 1, 2019
Uses the Envoy Connection Manager request_timeout setting.

Fixes #1073.

Signed-off-by: Nick Young <ynick@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants