Add a timeout to RemoteAddr() to allow http.Serve in go < 1.6 work #4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keymon
force-pushed
the
non_blocking_remote_addr
branch
6 times, most recently
from
c264d77 to
269f66d
Compare
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
force-pushed
the
non_blocking_remote_addr
branch
from
269f66d to
efc48d4
Compare
|
Note about using From https://groups.google.com/forum/#!topic/golang-nuts/afgEYsoV8j0
|
keymon
changed the title
|
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. |
|
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 routineRemoteAddr()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
Listener.ProxyHeaderTimeoutis not defined (zero).Listenerstruct now has a new fieldProxyHeaderTimeout, so you must explicitly name theListenerfield 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 beerr.EOFor `syscall.ECONNRESET .