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

Add a timeout to RemoteAddr() to allow http.Serve in go < 1.6 work #4

Merged
merged 2 commits into from
Jul 18, 2016

Conversation

keymon
Copy link
Contributor

@keymon keymon commented Jul 12, 2016

What

This PR solves the issue described in #1, also when running go < 1.6.

As a summary: go < 1.6, http.Serve() will call from the main go routine RemoteAddr() which blocks the caller until the client sends some data. Because that, http.Serve() will stop accepting new connections.

Motivation

Cloud Foundry gorouter, which is a critical component of this PaaS, has recently merged a PR to add Proxy Protocol support using this library.

But gorouter uses golang 1.5 and it is affected by the issue described in #1 as described in this comment.

I will take a while to get the CF release start using golang 1.6 for the gorouter, and this is a unexpected behaviour using this library, so we consider convenient solve the problem in this library.

Proposed solution

When using Proxy Procotol, the Proxy Protocol will be sent immediately by the proxy once the connection is stablish. Because that, we add an optional timeout to get the Proxy header when calling RemoteAddr(). If we don't get the header in the given time, consider it as a connection that does not use Proxy Protocol.

Consequences and backward compatibility

  • This PR maintains the same behaviour than before if Listener.ProxyHeaderTimeout is not defined (zero).
  • The Listener struct now has a new field ProxyHeaderTimeout, so you must explicitly name the Listener field when initialising. README has been updated.

Additional changes

We found out that in the case of getting an invalid header, the connection is not closed, despite the comment and the tests expects so. We added the conn.Close().

Additionally, the test was randomly failing as the error returned by Read() might be err.EOF or `syscall.ECONNRESET .

@keymon keymon force-pushed the non_blocking_remote_addr branch 6 times, most recently from c264d77 to 269f66d Compare July 13, 2016 14:56
The library user can define a maximum time to wait
for the PROXY protocol header, before failing out to
normal connection.

We can assume that a proxy in front of the service will
send the PROXY header immediatelly.

This solves the issue of clients getting block when
getting the RemoteAddr() for an incoming connection that
does not send any data. That is the case of http.Serve on
go < 1.6 as described in armon#1
RemoteAddr() must close the connection and clear the buffer if we
receive a invalid PROXY protocol header.

Change the client test as it can get an EOF or a ECONNRESET.
@keymon
Copy link
Contributor Author

keymon commented Jul 13, 2016

Note about using SetReadDeadLine:

From https://groups.google.com/forum/#!topic/golang-nuts/afgEYsoV8j0

But brief summary: yes, Deadline is a fixed point in time. Set it before a time-sensitive operation (whether that's some composite operation like reading an HTTP header or some low-level Read) and keep calling SetDeadline in the future when you need to extend the deadline. Or reset it with SetDeadline(time.Time{}), which was just fixed.

@keymon keymon changed the title WIP: Add a timeout to RemoteAddr() to allow http.Serve in go < 1.6 work Add a timeout to RemoteAddr() to allow http.Serve in go < 1.6 work Jul 13, 2016
@crhino
Copy link

crhino commented Jul 18, 2016

Any updates on this PR? We are thinking about how to go about patching GoRouter for this and would like to just pull in a new SHA for this library.

@armon armon merged commit 3daa90a into armon:master Jul 18, 2016
@armon
Copy link
Owner

armon commented Jul 18, 2016

LGTM! Thanks!

shashwathi pushed a commit to cloudfoundry/gorouter that referenced this pull request Jul 21, 2016
Configure go-proxyprotocol library to wait up to 100ms
for a Proxy Protocol header, before considering the incomming
connection as normal.

This requires a patched go-proxyproto library [1] and fixes #141 [2]

[1] armon/go-proxyproto#4
[2] #141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants