Add PROXY protocol support #3098
Conversation
…t cluster to allow modifying listeners before starting, and fix tests
command/server/config.go
Outdated
| "tls_disable": "1", | ||
| "address": "127.0.0.1:8200", | ||
| "tls_disable": true, | ||
| "proxy_protocol": "use_if_authorized", |
There was a problem hiding this comment.
I wonder if a nested proxy_protocol block would be nicer here, with keys of behavior and allowed_addrs.
There was a problem hiding this comment.
I think the answer is "yes" although we don't do that with tls, so I worry it would be sort of a "okay, why are they changing this up now" issue UX wise. We could support that later on and move tls and proxy both.
There was a problem hiding this comment.
See 5dea9eb for a quick change to address this at the moment.
helper/proxyutil/proxyutil.go
Outdated
| Listener: listener, | ||
| } | ||
|
|
||
| if config.Behavior == "use_if_authorized" || config.Behavior == "deny_if_unauthorized" { |
There was a problem hiding this comment.
Could you move this up into your switch statement above?
| "net" | ||
| "sync" | ||
|
|
||
| proxyproto "github.com/armon/go-proxyproto" |
There was a problem hiding this comment.
This appears to be missing from the vendor directory.
There was a problem hiding this comment.
Yeah, stupid me, I got sockaddr and forgot proxyproto. Added.
helper/proxyutil/proxyutil.go
Outdated
| AllowedAddrs []*sockaddr.SockAddrMarshaler `json:"allowed_addrs"` | ||
| } | ||
|
|
||
| func (p *ProxyProtoConfig) SetAllowedAddrs(addrs interface{}) error { |
There was a problem hiding this comment.
This method could really use some unit tests to ensure the allowed addresses are being set correctly.
|
Also, can you add some docs for the config options. The behaviors especially took me a bit to get. |
… change to a struct later
|
Yeah, I'll make sure to add docs before merging this. |
helper/proxyutil/proxyutil.go
Outdated
| } | ||
| } | ||
|
|
||
| if config.Behavior == "use_if_authorized" { |
There was a problem hiding this comment.
Not sure if this is a standard or not, but this name is pretty confusing to me. Should it be use_if_unauthorized?
There was a problem hiding this comment.
maybe this should properly be "use_if_allowed" but I thought that might be even more confusing. Alternately I could change the name to "AuthorizedAddrs" instead of AllowedAddrs? (or maybe AuthorizedHosts?)
|
@jefferai I can barely explain how happy I am to see this! Thank you sooo much! |
|
:-D |
|
🎉 Thanks @jefferai! This is awesome! 😀 |
|
@jefferai any documentation on how to use this awesomeness? Am I reading wrong but it doesn't look like this part of the 0.8.0 release ? |
Fixes #799
Ping #815