add support for the PROXY protocol (v1 only) #2654
Conversation
|
I don't think any of the CI failures are related to my commit... |
I started a re-run of CI (edit: started them again, tests be flaky as this branch passed in my fork) |
|
I found a bug in this in my own testing: on about 0.0025% of requests, parsing fails with It turns out that this happens when the initial read on the socket returned the PROXY protocol header and only the PROXY protocol header (which depends on TCP_CORK and a bunch of other stuff on the client side, but is easily reproducible by hand). This is because if you call I'm going to push an update to this branch that ensures we don't do that. |
…rt reads The HTTP parser requires that the buffer be non-empty when it's invoked, but if the entire buffer was the PROXY protocol, we may sometimes invoke it with an empty buffer. This causes the following error: Puma::HttpParserError: Requested start is after data buffer end. The fix? Any time we consume the entire buffer for the PROXY protocol, simply return `false` to indicate that we require more data.
|
This is good to be reviewed, for what it's worth |
|
Just commenting to say I haven't forgotten about this. This is up for review by anyone other than the original author (we allow code review from anyone as we discuss in CONTRIBUTING.md) |
|
Putting out to the puma-contrib channel that I want one more review on this but I think this would be a great addition for the next release |
There was a problem hiding this comment.
Looks good to me! I tried it out with this HAProxy config:
defaults
timeout connect 10s
timeout client 30s
timeout server 30s
# send everything to stdout
log stdout format raw daemon
frontend www
bind *:4444
use_backend proxy
frontend www2
bind *:5555
use_backend noproxy
backend proxy
mode http
server server1 127.0.0.1:9292 send-proxy
backend noproxy
mode tcp
server server1 127.0.0.1:9292
|
Thanks for the review @dentarg! |
* add support for the PROXY protocol (v1 only) * slightly simplify test * address review feedback * move PROXY protocol parsing into its own method; avoid issue with short reads The HTTP parser requires that the buffer be non-empty when it's invoked, but if the entire buffer was the PROXY protocol, we may sometimes invoke it with an empty buffer. This causes the following error: Puma::HttpParserError: Requested start is after data buffer end. The fix? Any time we consume the entire buffer for the PROXY protocol, simply return `false` to indicate that we require more data.
Description
This adds support for parsing the PROXY Protocol (version 1 only). It's behind the
set_remote_addressfeature when invoked likeset_remote_address proxy_protocol: :v1. I put thev1in there in case some bold soul wants to add v2 support at some point.This gracefully handles the case where the client doesn't send the PROXY protocol and falls back to normal logic.
Closes #2651
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.