-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
util: add PROXY protocol generation functions #10548
util: add PROXY protocol generation functions #10548
Conversation
Signed-off-by: Weston Carlson <wez470@gmail.com>
|
||
// See https://github.com/haproxy/haproxy/blob/master/doc/proxy-protocol.txt for definitions | ||
|
||
constexpr char PROXY_PROTO_V1_SIGNATURE[] = "PROXY "; |
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.
These constants overlap with ones created by the proxy proto listener filter. I opted to not refactor the listener filter to use these common ones for now to keep this PR simple. I think I can do that in a future PR. Willing to do it here if requested though.
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.
How about a TODO so we don't lose track?
/retest |
🐴 hold your horses - no failures detected, yet. |
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.
This is awesome! Thanks for splitting it out :-)
One other thing would be good here, is having a unit test regression test that the header we generate is one the listener filter code can parse. I think it'd be better to land with this PR, but I'm fine with a TODO as long as you land it before landing any config using this utility.
namespace Common { | ||
namespace ProxyProtocol { | ||
|
||
void generate_v1_header(const std::string& src_addr, const std::string& dst_addr, uint32_t src_port, |
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.
per style guide let's swap these from snake case towards generateV1Header etc.
|
||
// See https://github.com/haproxy/haproxy/blob/master/doc/proxy-protocol.txt for definitions | ||
|
||
constexpr char PROXY_PROTO_V1_SIGNATURE[] = "PROXY "; |
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.
How about a TODO so we don't lose track?
Thanks for the feedback! I'll move the regression tests over to this branch 🙂. |
Signed-off-by: Weston Carlson <wez470@gmail.com>
Signed-off-by: Weston Carlson <wez470@gmail.com>
Signed-off-by: Weston Carlson <wez470@gmail.com>
Signed-off-by: Weston Carlson <wez470@gmail.com>
Signed-off-by: Weston Carlson <wez470@gmail.com>
Signed-off-by: Weston Carlson <wez470@gmail.com>
LGTM pending CI cleanup :-) |
@mattklein123 or @zuercher @snowp any of you up for second pass? |
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.
Nice, just one small drive by comment post-submit. cc @alyssawilk
void generateV1Header(const std::string& src_addr, const std::string& dst_addr, uint32_t src_port, | ||
uint32_t dst_port, Network::Address::IpVersion ip_version, | ||
Buffer::Instance& out) { | ||
std::ostringstream stream; |
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.
Sorry late drive by: my (possibly out of date) understanding is that string streams are pretty slow. We probably would want to move this to an envoy buffer or similar in a later perf pass.
Description: Add utility functions for generating v1 and v2 PROXY protocol headers. This is the first step to adding Envoy PROXY proto generation support. These functions can also be used in other places that would also like to generate the header.
Risk Level: Low
Testing: Unit
Docs Changes: None
Release Notes: None
Related to #1031
@alyssawilk