Navigation Menu

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

Support PROXY Protocol #2162

Closed
wants to merge 6 commits into from
Closed

Conversation

elyscape
Copy link

@elyscape elyscape commented Dec 6, 2017

This is a rebase of #1135 onto master. It also adds some additional error checking to that code.

Closes #1135.

@vulpine
Copy link
Contributor

vulpine commented Dec 6, 2017

Apologies for neglecting my branch! Thank you for picking this up. :)

@elyscape elyscape force-pushed the feature/proxy-protocol branch 2 times, most recently from e8acb50 to c6a8ad8 Compare December 6, 2017 22:58
@elyscape
Copy link
Author

elyscape commented Dec 6, 2017

Okay, got some final issues ironed out. This should be good to go now.

@elyscape
Copy link
Author

elyscape commented Dec 6, 2017

It occurred to me just now that this should probably be expanded to work on non-HTTP protocols.

@bagder
Copy link
Member

bagder commented Dec 7, 2017

You'll see that test 1119 and 1139 fail due to:

  1. missing documentation for the new curl_easy_setopt option and
  2. missing the new symbol in docs/libcurl/symbols-in-versions
  3. I don't think there's a test for it (yet) but there's also missing documentation for the new command line option, isn't it?

@elyscape
Copy link
Author

elyscape commented Dec 7, 2017

Good points. I'll look into adding those.

@maglub
Copy link

maglub commented Jan 16, 2018

Hi, I really would like to get this feature merged into curl and would be happy to help. @elyscape, please advice if I I can help you in any way. I have compiled and tested your fork, and it works just as I personally would expect.

vagrant@ldap:~/curl/curl$ ./src/.libs/curl -v --haproxy-protocol --resolve my.vhost.com:443:192.168.1.15 -k https://my.vhost.com:443/ 2>&1 | grep -E "subject:|expire date:"
*  subject: CN=my.vhost.com
*  expire date: Apr 11 18:16:03 2018 GMT

If there is any documentation I can do for you, I will happily help.

@elyscape
Copy link
Author

@maglub Sorry for the delay. I'll try to get the rest of the documentation done this week.

@elyscape
Copy link
Author

elyscape commented Feb 5, 2018

Rebased onto master again with documentation added.

@bagder
Copy link
Member

bagder commented Feb 6, 2018

If you run test 1119 and 1139 locally you'll see that they fail:

1119 - because docs/libcurl/symbols-in-versions isn't updated with your new symbol

1139 - because docs/libcurl/curl_easy_setopt.3 doesn't mention CURLOPT_HAPROXY_PROTOCOL.3 (referring to its "local" man page)

@@ -0,0 +1,11 @@
Long: haproxy-protocol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For convenience to command line users, do you think it would make sense to just call it --haproxy ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would opt for --haproxy-protocol or --proxy-protocol, as the protocol could be used by other implementations too and is more descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my original PR I used --proxy-protocol and was recommended to include haproxy in the argument so as not to cause confusion with other proxy-related options.

@elyscape
Copy link
Author

elyscape commented Feb 6, 2018

@bagder Tests fixed. I had put CURLOPT_HAPROXY_PROTOCOL in docs/libcurl/symbols-in-versions but CURLOPT_HAPROXYPROTOCOL everywhere else.

@bagder
Copy link
Member

bagder commented Feb 13, 2018

The red CI build is because CURLOPT_HAPROXYPROTOCOL.3 is not added to the tarball in the docs/libcurl/opts/Makfile.inc file.

@elyscape
Copy link
Author

@bagder Fixed and rebased onto master. Apologies for the delay; I was on vacation.

@elyscape
Copy link
Author

elyscape commented Mar 2, 2018

Ah, looks like the version I have in there is wrong. Let me know if everything else looks good and I can fix it.

@elyscape
Copy link
Author

Well, 7.59.0 came out before this got merged, so that concern is no longer relevant. Woo!

@bagder When you get a chance, please take another look at this. Thank you!

@bagder bagder closed this in 6baeb6d Mar 17, 2018
@bagder
Copy link
Member

bagder commented Mar 17, 2018

Thanks for all the work on this. Please try it out in the master branch now!

@vulpine
Copy link
Contributor

vulpine commented Mar 19, 2018 via email

@elyscape elyscape deleted the feature/proxy-protocol branch March 19, 2018 20:46
@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants