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 go-proxyproto 100ms timeout for Proxy Protocol Header #143

Closed
wants to merge 2 commits into from
Closed

Add go-proxyproto 100ms timeout for Proxy Protocol Header #143

wants to merge 2 commits into from

Conversation

keymon
Copy link
Contributor

@keymon keymon commented Jul 19, 2016

What

As described in #141, currently the Proxy Protocol implementation has a bug that might cause go router stop accepting connections when using Proxy Protocol and a client connects without sending data.

Solutions are upgrade to Golang 1.6 used a patched version of upstream go-proxyproto which allows define a timeout to receive the Proxy Header.

This PR configure a 100ms timeout to receive the Proxy Header Protocol.

Consequences

This PR:

  • hardcodes the timeout 100ms
  • assumes that 100ms is enough time to receive the Proxy Header from the proxy.
    • That is very likely the case, as the proxy will send the header immediately after stablish the connection, and hopefully there is not such latency between the proxy and the gorouter!!!
    • In the worse case scenario, if it timeouts after 100ms, the connection will be drop as invalid HTTP request
  • If a client connects and does not send data, gorouter will still be block for 100ms and not accepting connections. This might degrade the performance if there is are clients constantly connecting and not sending data.

Dependencies and additional work

IMPORTANT:

This code depends on armon/go-proxyproto@3daa90a and it is not compatible with previous versions of the library

Once this PR is merged, https://github.com/cloudfoundry-incubator/routing-release must be updated with the submodule references for github.com/cloudfoundry/gorouter and https://github.com/armon/go-proxyproto

I will not do a PR on routing-release as it needs to be updated by whoever merges this.

How to test

We included the test added by @shashwathi to check that multiple connections can be established.

Thx

(our project story https://www.pivotaltracker.com/story/show/126483997)

flawedmatrix and others added 2 commits July 19, 2016 10:08
[#116601821]

Signed-off-by: Shash Reddy <[email protected]>
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
@cfdreddbot
Copy link

Hey keymon!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/126578835

The labels on this github issue will be updated when the story is started.

@keymon keymon changed the title Goproxy timeout Add go-proxyproto 100ms timeout for Proxy Protocol Header Jul 19, 2016
@keymon
Copy link
Contributor Author

keymon commented Jul 19, 2016

/cc @crhino

@keymon
Copy link
Contributor Author

keymon commented Jul 19, 2016

Build is failing because the dependencies must be updated in https://github.com/cloudfoundry-incubator/routing-release.

@shashwathi
Copy link
Contributor

We've merged in the PR as part of this commit: 45c41d9

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.

5 participants