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

Set keepalive on TCP socket so idleTimeout works #3740

Merged
merged 1 commit into from Aug 8, 2018

Conversation

ajardan
Copy link
Contributor

@ajardan ajardan commented Aug 6, 2018

What does this PR do?

This makes traefik set KeepAlive on TCP connections it accepts, which fixes #3263 and possibly other issues related to socket leak, or file descriptor exhaustion.

Without this change, the configuration option

--respondingtimeouts.idletimeout

has no effect, since it only works for keep-alive connections according to this document:

https://golang.org/src/net/http/server.go?s=93090:93158#L2443

Motivation

This is a relatively trivial change that improves how traefik performs and increases its performance and stability.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

I am not actually the author of this code, it is taken from the net.http.Server package, original source here:

https://golang.org/src/net/http/server.go?s=93090:93158#L3207

@ajardan
Copy link
Contributor Author

ajardan commented Aug 6, 2018

The build failed, but the reason of failure doesn't seem to be related with the introduced changes. What is the procedure in such cases ?

LogRotationSuite.TestTraefikLogRotation: No Traefik logs.
remove traefik.log: no such file or directory
INFO[0608] LogRotationSuite.TestTraefikLogRotation: No Traefik logs. 

----------------------------------------------------------------------
FAIL: log_rotation_test.go:87: LogRotationSuite.TestTraefikLogRotation

log_rotation_test.go:117:
    c.Assert(err, checker.IsNil)
... value *errors.errorString = &errors.errorString{s:"try operation failed: stat traefik.log: no such file or directory"} ("try operation failed: stat traefik.log: no such file or directory")

@ldez
Copy link
Member

ldez commented Aug 6, 2018

The real problem:

NFO[0009] ProxyProtocolSuite.TestProxyProtocolNotTrusted: Traefik logs:  
INFO[0009] time="2018-08-06T18:13:58Z" level=info msg="Using TOML configuration file /home/runner/workspace/src/github.com/containous/traefik/integration/fixtures/proxy-protocol/without.toml828478900"
time="2018-08-06T18:13:58Z" level=info msg="Traefik version 6e47001797f2fe5f7cf5c80d6f55ad3b7c0b348f built on 2018-08-06_06:01:19PM"
time="2018-08-06T18:13:58Z" level=info msg="\nStats collection is disabled.\nHelp us improve Traefik by turning this feature on :)\nMore details on: https://docs.traefik.io/basics/#collected-data\n"
time="2018-08-06T18:13:58Z" level=debug msg="Global configuration loaded "
time="2018-08-06T18:13:58Z" level=info msg="Preparing server traefik &{Address::8080 TLS:<nil> Redirect:<nil> Auth:<nil> WhitelistSourceRange:[] WhiteList:<nil> Compress:false ProxyProtocol:<nil> ForwardedHeaders:0xc4201c7b40} with readTimeout=0s writeTimeout=0s idleTimeout=3m0s"
time="2018-08-06T18:13:58Z" level=info msg="Preparing server http &{Address::8000 TLS:<nil> Redirect:<nil> Auth:<nil> WhitelistSourceRange:[] WhiteList:<nil> Compress:false ProxyProtocol:0xc4201c69c0 ForwardedHeaders:0xc4201c7b20} with readTimeout=0s writeTimeout=0s idleTimeout=3m0s"
time="2018-08-06T18:13:58Z" level=info msg="Enabling ProxyProtocol for trusted IPs [1.2.3.4]"
time="2018-08-06T18:13:58Z" level=info msg="Starting provider configuration.ProviderAggregator {}"
time="2018-08-06T18:13:58Z" level=info msg="Starting server on :8080"
time="2018-08-06T18:13:58Z" level=info msg="Starting server on :8000"
panic: interface conversion: net.Listener is *proxyproto.Listener, not *net.TCPListener

goroutine 51 [running]:
github.com/containous/traefik/h2c.Server.Serve(0xc4209da0d0, 0x26827c0, 0xc4205f5440, 0x1, 0x1)
	/go/src/github.com/containous/traefik/h2c/h2c.go:99 +0x14e
github.com/containous/traefik/server.(*Server).startServer(0xc42096c000, 0xc4201d21c0)
	/go/src/github.com/containous/traefik/server/server.go:503 +0x1c0
created by github.com/containous/traefik/server.(*Server).startHTTPServers
	/go/src/github.com/containous/traefik/server/server.go:325 +0x101
 

----------------------------------------------------------------------
FAIL: proxy_protocol_test.go:41: ProxyProtocolSuite.TestProxyProtocolNotTrusted

proxy_protocol_test.go:58:
    c.Assert(err, checker.IsNil)
... value *errors.errorString = &errors.errorString{s:"try operation failed: Get http://172.17.0.3/whoami: EOF"} ("try operation failed: Get http://172.17.0.3/whoami: EOF")

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏
This is a really good fix! Thank you
But I think it should be better to do it here
https://github.com/containous/traefik/blob/bb331285522b2734fc36b5cde0c9c5170c4ffcfe/server/server.go#L558

@ajardan
Copy link
Contributor Author

ajardan commented Aug 7, 2018

@juliens Moved it to server as you recommended, but not sure in the "best way". It fixed the ProxyProtocol tests though :)

@ldez Thanks a lot for pointing me to the right error, missed that somehow. Will look better next time.

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this PR @ajardan

LGTM 👏 👏 👍

@ldez ldez changed the base branch from v1.7 to v1.6 August 8, 2018 16:53
@ldez ldez removed the bot/no-merge label Aug 8, 2018
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants