-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Support PROXY Protocol #2162
Conversation
Apologies for neglecting my branch! Thank you for picking this up. :) |
e8acb50
to
c6a8ad8
Compare
Okay, got some final issues ironed out. This should be good to go now. |
c6a8ad8
to
d29ae12
Compare
It occurred to me just now that this should probably be expanded to work on non-HTTP protocols. |
You'll see that test 1119 and 1139 fail due to:
|
Good points. I'll look into adding those. |
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.
If there is any documentation I can do for you, I will happily help. |
@maglub Sorry for the delay. I'll try to get the rest of the documentation done this week. |
d29ae12
to
7ea372c
Compare
Rebased onto master again with documentation added. |
If you run test 1119 and 1139 locally you'll see that they fail: 1119 - because 1139 - because |
@@ -0,0 +1,11 @@ | |||
Long: haproxy-protocol |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
7ea372c
to
0716721
Compare
@bagder Tests fixed. I had put |
The red CI build is because |
This argument is unnecessary here.
0716721
to
1e4edfd
Compare
@bagder Fixed and rebased onto master. Apologies for the delay; I was on vacation. |
Ah, looks like the version I have in there is wrong. Let me know if everything else looks good and I can fix it. |
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! |
Thanks for all the work on this. Please try it out in the master branch now! |
Thank you all for helping get this into master! <3
…On 17 March 2018 at 10:51, Daniel Stenberg ***@***.***> wrote:
Thanks for all the work on this. Please try it out in the master branch
now!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2162 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABUhDgc18ayFAcLzKu-zFHV2Jvt2UEY6ks5tfOqhgaJpZM4Q4kUU>
.
--
Sent from the Outside Broadcast Unit
|
This is a rebase of #1135 onto master. It also adds some additional error checking to that code.
Closes #1135.