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

[WIP] Implement proxy-protocol v1 support for TCP server #101

Merged
merged 2 commits into from
Sep 28, 2017

Conversation

thetechnick
Copy link
Contributor

@thetechnick thetechnick commented Jul 26, 2017

I have thrown together a small patch to include proxy-protocol v1 support for TCP.
This is not meant as a final implementation, but more a working proof of concept.

TODO

  • Test if IPv6 is relayed correctly
  • Improve error handling and logging
  • Cleanup
  • Add config validation

Testing

To test it, I have deployed the echoheaders application in my kubernetes cluster and setup gobetween to direct traffic to the same ports haproxy does now.
I than checked if the x-forwarded-for header is set to the same client IP as reported for the haproxy setup.

Gobetween config (Note the added proxy_protocol setting)

[servers.sample]
bind = "0.0.0.0:9090"
protocol = "tcp"
balance = "roundrobin"
proxy_protocol = "v1"

max_connections = 10000
client_idle_timeout = "10m"
backend_idle_timeout = "10m"
backend_connection_timeout = "2s"

[servers.sample.discovery]
kind = "static"
static_list = [
    "192.168.100.11:31728",
    "192.168.100.12:31728",
    "192.168.100.13:31728"
]

You can see the output of the haproxy setup here:
http://headers.cluster.thetechnick.ninja/

Why I did not implement v2 yet:

It seems that nginx does not support v2 and HAProxy have no UDP support at all.

The proxy-protocol v1 spec () does not cover UDP:

[...]
a string indicating the proxied INET protocol and family. As of version 1,
only "TCP4" ( \x54 \x43 \x50 \x34 ) for TCP over IPv4, and "TCP6"
( \x54 \x43 \x50 \x36 ) for TCP over IPv6 are allowed
[...]
https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

While the binary v2 protocol header does allow UDP, I do not have a use case for it right now.
The great lib would support the v2 protocol header though.

Edit:
Link to issue: #100

@yyyar yyyar added the feature label Jul 28, 2017
@yyyar yyyar added this to the 0.5.0 milestone Jul 28, 2017
@yyyar
Copy link
Owner

yyyar commented Jul 28, 2017

That's a very nice feature, @thetechnick thanks for the pull request!
Could we make some modifications in config/etc in your branch before we could merge?

@yyyar yyyar requested review from yyyar and illarion July 28, 2017 16:39
@yyyar yyyar self-assigned this Jul 28, 2017
@yyyar yyyar removed their request for review July 28, 2017 16:40
@thetechnick
Copy link
Contributor Author

@yyyar Sure, just write what you want changed.

I also noticed that there is config validation in https://github.com/yyyar/gobetween/blob/master/src/manager/manager.go, so I will add a validation check for the proxy_protocol setting.
Maybe we should also raise a error if the v1 proxy protocol is chosen and the server protocol is set to UDP, because v1 seems not to support UDP.
What do you think?

Should the v2 protocol be also implemented or is this a feature we implement in a second PR?

@shantanugadgil
Copy link
Contributor

I vote for a 0.5.0 "GA" release with proxy v1 support. 👍 😀

@yyyar
Copy link
Owner

yyyar commented Aug 7, 2017

@thetechnick I think we can go with v1 only in next gobetween release (v0.5.0) and then add v2 later. But for further compatibility, I'd better make configuration like this:

[servers.sample]
protocol="tcp"
bind="0.0.0.0:3000"

    [servers.sample.proxy_protocol]   # <- separate section
    version="1"   # string, required

    [servers.sample.discovery]
    # ....

So we could put more options in 'servers.sample.proxyprotocol' object later for version 2 (maybe custom TLVs or some advanced configuration).

If this section is not specified at all, we ignore proxy protocol.

I also noticed that there is config validation in https://github.com/yyyar/gobetween/blob/master/src/manager/manager.go, so I will add a validation check for the proxy_protocol setting.
Maybe we should also raise a error if the v1 proxy protocol is chosen and the server protocol is set to UDP, because v1 seems not to support UDP.

Yes it would be good addiction.

Thank you!

@yyyar
Copy link
Owner

yyyar commented Sep 28, 2017

I've actually gone ahead and done some modifications mentioned before, so it's ready for merge.

@thetechnick thank you for a great feature!

It would be cool to have v2 in future too, but we'll need to think how to merge it with our 'sni' implementation since these two features overlap.

@yyyar yyyar merged commit 42a2e55 into yyyar:master Sep 28, 2017
@yyyar yyyar requested review from yyyar and removed request for illarion and yyyar September 28, 2017 08:36
@thetechnick
Copy link
Contributor Author

@yyyar Great, thanks!
I would have completed this by myself, but I just did not find the time to work on it.

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.

3 participants