Add configurable timeouts and curate default timeout settings #1873

m3co-code · 2017-07-18T06:37:15Z

This PR adds some configurable timeouts to Traefik. Namely it introduces:

ForwardingTimeouts -> Timeouts for forwarding requests to the Backend Server's. In specific
- DialTimeout
- ResponseHeaderTimeout
RespondingTimeouts -> Timeouts for the http.Server instances that Traefik opens.
- ReadTimeout
- WriteTimeout
- Move of IdleTimeout into this configuration section with proper deprecation for uniformity.

The motivation for this PR are the suboptimal default settings of go itself and it is important to make the load balancer robust against bad behaving clients / backends in a microservice environment.

In order to introduce this configuration options I had to first clean up the global modification of the http.DefaultTransport. I think a global modification of it is in general considered a bad practice and leads to unintended side-effects. To do this I added a createHTTPTransport method in the server that will produce a http.Transport with a very similar configuration as the 'http.DefaultTransport'. Apart of the removal form global modification this has another big benefit, given TLS is configured we create a new http.Transport for that and before this PR this Transport did not have any timeouts configured at all, not even those from the http.DefaultTransport.

I know that this PR is largish and timeouts in general are really difficult to test. Let me know when I can support reviewers in any way, usually I am also hanging around in the Traefik slack channel. I tried to split the commits logically so that at least those should give some guideline of the introduced changes and hopefully make reviewing easier.

For background information: There are very good articles regarding timeouts in golang. Here some links:

emilevauge · 2017-07-24T15:21:52Z

@marco-jantke Thanks a lot for this one, awesome job !
Articles linked are really good.
Design LGTM

ldez

SGTM, few comments

ldez · 2017-08-02T14:03:49Z

server/configuration.go

@@ -54,11 +61,13 @@ type GlobalConfiguration struct {
 	DefaultEntryPoints        DefaultEntryPoints      `description:"Entrypoints to be used by frontends that do not specify any entrypoint"`
 	ProvidersThrottleDuration flaeg.Duration          `description:"Backends throttle duration: minimum duration between 2 events from providers before applying a new configuration. It avoids unnecessary reloads if multiples events are sent in a short amount of time."`
 	MaxIdleConnsPerHost       int                     `description:"If non-zero, controls the maximum idle (keep-alive) to keep per-host.  If zero, DefaultMaxIdleConnsPerHost is used"`
-	IdleTimeout               flaeg.Duration          `description:"maximum amount of time an idle (keep-alive) connection will remain idle before closing itself."`
+	IdleTimeout               flaeg.Duration          `description:"(Deprecated) see RespondingTimeouts.IdleTimeout"`


Could you add // Deprecated ?

Sure, done.

timoreimann · 2017-08-14T11:55:04Z

docs/toml.md

-#
+# IdleTimeout
+# 
+# Deprecated - see [respondingTimeouts] section


Shouldn't we keep the description of what the parameter means until we truly remove it?

Yes, makes sense. Not sure why I removed it in the first place.

timoreimann · 2017-08-14T11:55:18Z

docs/toml.md

@@ -328,6 +326,65 @@ To write JSON format logs, specify `json` as the format:
 # interval = "30s"
 ```

+## Responding timeouts
+```
+# Timeout settings for the http servers Traefik starts


s/http/HTTP/

timoreimann · 2017-08-14T14:46:22Z

docs/toml.md

+# 
+[respondingTimeouts]
+
+# ReadTimeout is the maximum duration for reading the entire request, including the body. 


I think we should say readTimeout instead of ReadTimeout. I know it's the beginning of a sentence, but we're referring to an option name (i.e., a proper name) here.

Similar for the other added options.

Thanks, done.

timoreimann · 2017-08-14T14:55:14Z

docs/toml.md

+# 
+# readTimeout = "5s"
+
+# WriteTimeout is the maximum duration before timing out writes of the response.


Not sure if "writes of the response" is clear enough. Does that refer to the time until the response is returned? If so, is it the complete response or the first byte only?

I added another explanatory sentence. Please check again.

timoreimann · 2017-08-14T14:55:52Z

docs/toml.md

+
+## Forwarding timeouts
+```
+# Timeout settings for requests forwarded to the Backend Servers


s/Backend Servers/backend servers/

timoreimann · 2017-08-14T20:04:03Z

server/server.go

+	// When RespondingTimeouts.IdleTimout is configured and its not the default value, always use that setting
+	if server.globalConfiguration.RespondingTimeouts != nil && time.Duration(server.globalConfiguration.RespondingTimeouts.IdleTimeout) != time.Duration(DefaultIdleTimeout) {
+		idleTimeout = time.Duration(server.globalConfiguration.RespondingTimeouts.IdleTimeout)
+	}


Could we outsource the timeout setup into a dedicated function? WDYT?

Good suggestion. Outsourced it.

timoreimann · 2017-08-14T20:05:03Z

server/server.go

-	}
-	return &http.Transport{
-		TLSClientConfig: config,
+// getRoundTripper will either use server.defaultForwardingRoundTripperor create a new one


Space missing at defaultForwardingRoundTripperor.

Thanks, done.

timoreimann · 2017-08-14T20:07:04Z

server/server_test.go

+				RespondingTimeouts: &RespondingTimeouts{
+					IdleTimeout:  flaeg.Duration(10 * time.Second),
+					ReadTimeout:  flaeg.Duration(10 * time.Second),
+					WriteTimeout: flaeg.Duration(10 * time.Second),


I'd probably choose slightly different numbers for the three timeouts just to rule out the possibility that timeout values are inadvertently reused.

Good point, adapted the test.

timoreimann · 2017-08-14T20:07:44Z

server/server_test.go

+			globalConfig: GlobalConfiguration{
+				IdleTimeout: flaeg.Duration(45 * time.Second),
+				RespondingTimeouts: &RespondingTimeouts{
+					IdleTimeout: flaeg.Duration(80 * time.Second),


Did we document somewhere that the new option will override the old one?

Nope we did not so far. I added a note in toml.md at the deprecated option.

timoreimann · 2017-08-14T20:10:42Z

integration/timeout_test.go

+	err = try.GetRequest("http://127.0.0.1:8080/api/providers", 60*time.Second, try.BodyContains("Path:/dialTimeout"))
+	c.Assert(err, checker.IsNil)
+
+	// This simulates a DialTimeout to the BackendServer.


s/BackendServer/backend server/?

Thanks, done.

m3co-code · 2017-08-16T10:39:15Z

@timoreimann thanks for review iteration nr1 :) I implemented your feedback. PTAL again.

timoreimann

LGTM, thanks Marco. 👏

ldez

LGTM

nmengin

Many thanks for this PR.
Only one suggestion.

nmengin · 2017-08-16T17:20:13Z

server/server.go

+	if globalConfig.RespondingTimeouts != nil && time.Duration(globalConfig.RespondingTimeouts.IdleTimeout) != time.Duration(DefaultIdleTimeout) {
+		idleTimeout = time.Duration(globalConfig.RespondingTimeouts.IdleTimeout)
+	}
+


@marco-jantke

Is there a specific reason to check parameters in this order or can we simplify the initialization like described below?

var idleTimeout int64 // When RespondingTimeouts.IdleTimout is configured, always use that setting if globalConfig.RespondingTimeouts != nil { idleTimeout = time.Duration(globalConfig.RespondingTimeouts.IdleTimeout) } else if globalConfig.IdleTimeout != nil { // Backwards compatibility for deprecated IdleTimeout idleTimeout = time.Duration(globalConfig.IdleTimeout) } else { // Default value if neither the deprecated IdleTimeout nor the new RespondingTimeouts.IdleTimout are configured idleTimeout = time.Duration(DefaultIdleTimeout) }

The initialization priority stays as now :

New parameter

Deprecated parameter

Default value

When the deprecated parameter will be able to be deleted, we'll just have to delete the else if block.

What is your mind about this suggestion?

Thanks for the suggestion, I think its much clearer than before. I just had to tweak the implementation a bit because globalConfig.IdleTimeout is a flaeg.Duration and can not be compared to nil.

I think we need to update flaeg to the last version.

nmengin

LGTM 👏

m3co-code · 2017-08-17T13:30:01Z

I rebased the PR to one commit and made an update to the latest master version. Should be good to merge :)

The following are the commit messages / description of the original commits before squashing. remove modifications of global http.DefaultTransport and create an http.Transport that is used for forwarding to the backends. We get a couple benefits from this: - it is in general bad practice to modify global state as before and could lead to unintended side-effects - it is possible to set settings on the Transport that only for the communication to the backends. - the same Transport settings (like timeouts) are used when using TLS to a backend add ForwardingTimeouts and add docs add server Timeouts and add docs move IdleTimeout to Timeouts section and deprecated former option rename Timeouts config to RespondingTimeouts add unit test for RespondingTimeouts add integration test for ForwardingTimeouts add Deprecated comment improve documentation of timeout settings use different timeouts in tests to make it more expressive extract building of server timeouts to own function simplify idleTimeout loading

ldez added kind/enhancement a new or improved feature. status/1-needs-design-review labels Jul 18, 2017

emilevauge added status/2-needs-review and removed status/1-needs-design-review labels Jul 24, 2017

ldez modified the milestone: 1.4 Jul 27, 2017

ldez suggested changes Aug 2, 2017

View reviewed changes

ldez added contributor/waiting-for-corrections size/M and removed size/M contributor/waiting-for-corrections labels Aug 2, 2017

ldez added the area/middleware label Aug 10, 2017

timoreimann suggested changes Aug 14, 2017

View reviewed changes

ldez added the size/L label Aug 16, 2017

timoreimann approved these changes Aug 16, 2017

View reviewed changes

ldez approved these changes Aug 16, 2017

View reviewed changes

nmengin reviewed Aug 16, 2017

View reviewed changes

nmengin added the contributor/waiting-for-feedback label Aug 16, 2017

ldez removed the contributor/waiting-for-feedback label Aug 17, 2017

nmengin approved these changes Aug 17, 2017

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Aug 17, 2017

m3co-code force-pushed the connection-timeouts branch from bd14483 to 691b041 Compare August 17, 2017 13:27

traefiker added the bot/need-human-merge label Aug 17, 2017

m3co-code force-pushed the connection-timeouts branch from 691b041 to c0a447d Compare August 18, 2017 05:40

nmengin approved these changes Aug 18, 2017

View reviewed changes

timoreimann removed the bot/need-human-merge label Aug 18, 2017

traefiker added the bot/need-human-merge label Aug 18, 2017

ldez removed the bot/need-human-merge label Aug 18, 2017

traefiker added the status/4-merge-in-progress label Aug 18, 2017

traefiker force-pushed the connection-timeouts branch from c0a447d to 53bbcce Compare August 18, 2017 13:14

traefiker removed the status/4-merge-in-progress label Aug 18, 2017

traefiker merged commit 14a0d66 into traefik:master Aug 18, 2017

traefiker removed the status/3-needs-merge label Aug 18, 2017

ozkatz mentioned this pull request Aug 21, 2017

Set gateway timeout #1187

Closed

m3co-code mentioned this pull request Aug 25, 2017

Refactor globalConfiguration / WebProvider #1938

Merged

trauts2 mentioned this pull request Feb 2, 2018

Adjusting the request timeout values rawmind0/alpine-traefik#78

Closed

trauts2 mentioned this pull request Feb 21, 2018

Adjusting the request timeout values rawmind0/rancher-traefik#64

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable timeouts and curate default timeout settings #1873

Add configurable timeouts and curate default timeout settings #1873

m3co-code commented Jul 18, 2017

emilevauge commented Jul 24, 2017

ldez left a comment

ldez Aug 2, 2017

m3co-code Aug 9, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

timoreimann Aug 14, 2017

m3co-code Aug 16, 2017

m3co-code commented Aug 16, 2017 •

edited

Loading

timoreimann left a comment

ldez left a comment

nmengin left a comment

nmengin Aug 16, 2017

m3co-code Aug 17, 2017

ldez Aug 17, 2017

nmengin left a comment

m3co-code commented Aug 17, 2017

Add configurable timeouts and curate default timeout settings #1873

Add configurable timeouts and curate default timeout settings #1873

Conversation

m3co-code commented Jul 18, 2017

emilevauge commented Jul 24, 2017

ldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code commented Aug 16, 2017 • edited Loading

timoreimann left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

m3co-code commented Aug 17, 2017

m3co-code commented Aug 16, 2017 •

edited

Loading