-
-
Notifications
You must be signed in to change notification settings - Fork 117
WIP: Add TLV support and general refactorings #2
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
Conversation
@@ -0,0 +1,152 @@ | |||
package proxyproto |
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.
Just realized I forgot to add any copyright notice, will at least add that it's derived from https://github.com/armon/go-proxyproto/blob/master/protocol.go
I like the idea of the connection wrapping even though it seems out-of-scope for this library. However, don't like the fact the API is broken with this change. It's been some time since I last picked on this so I'll need to take a few hours to review properly. |
@pires are you planning on merging these changes? I am also interested in support for v1 and v2 with the Listener interface. |
I'd like to merge it, yes but, unfortunately, I haven't had the time to properly review it. The fact it breaks the existing API is also doesn't help. |
Sorry for closing, it was accidental. |
Would be cool if you could resume the TLV support based on current master branch 🙇 |
Good job on #8 , was planning of reviewing it later, but you move fast :) Right now I've got different priorities so can't tell when I'll revisit this (same goes for golang/go#20956 ). Having said that, I do expect to be working on some HTTP projects later this year when I'll surely need something similar. Though, if that takes too long, I by no way would take offence if someone else would continue this work. |
Sorry, I should've waited indeed. I may be back to this PR and try and adapt your TLV work. Thank you very much! |
Apologies for the drive-by I was looking into the package for a Vault issue I'm curious about. But how might this compare against the PR on elastic's fork here? It seems to have a similar goal of supporting TLVs in go-proxyproto. |
@dekimsey I'd ask the author of that PR. Chances are he's seen this PR as well, and he can explain why he thought one way to go about it would be better than another. |
e4eae17
to
b6563d9
Compare
Hey @Freeaqingme! Thank you for your involvement so far in helping w/ this library. Is there anything from this PR that you think is missing from the current tip? |
Thank you very much for your contribution even if it didn't get merged. |
Hi,
I was looking for proxyprotocol support for Golang where I could encapsulate a listener like what is possible with https://github.com/armon/go-proxyproto , except I needed to use version 2 with TLV.
Therefore I was happy to find your repo, and took the liberty to extend it a bit here and there. It's still a WIP, though I'm curious what you think about the (albeit also still-in-progress) API.
Dolf