http: allowing an optional proxy proto header when terminating CONNECT requests #10975
Conversation
…T requests Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@wez470 would you be up for doing a review pass of this one? |
|
Yes, I'll take a look. |
| .value() | ||
| .proxy_protocol_config() | ||
| .version(); | ||
| // FIXME versions. |
There was a problem hiding this comment.
What's this FIXME in regards to?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| ->connectConfig() | ||
| .value() | ||
| .has_proxy_protocol_config()) { | ||
| auto version = upstream_request_->parent() |
There was a problem hiding this comment.
I suspect we are going to need this elsewhere so it might be worth it to do a utility that takes the config, the connection, and the buffer to fill, but up to you if you want to do this now or not.
| EXPECT_CALL(connection_, write(BufferEqual(&expected_data), false)); | ||
| tcp_upstream_->encodeHeaders(request_, false); | ||
|
|
||
| // Data is proxied as usuak. |
There was a problem hiding this comment.
typo usuak (maybe why format is failing)
|
/lgtm api |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| const Network::Connection& connection, Buffer::Instance& out) { | ||
| const Network::Address::Ip& dest_address = *connection.localAddress()->ip(); | ||
| const Network::Address::Ip& source_address = *connection.remoteAddress()->ip(); | ||
| if (config.version() == envoy::config::core::v3::ProxyProtocolConfig::V1) { |
There was a problem hiding this comment.
Added the utility, which I think is 100% worthwhile especially if folks eventually make use of extensions.
one concern I noticed that I've got source/dest local/remote inverted from what Weston did in
https://github.com/envoyproxy/envoy/pull/10682/files
I think this is right - we're trying to communicate about the downstream connection. the client's destination address is the server's local address, and the client's source address is the remote address as Envoy sees it, but I'd appreciate a sanity check in case I have it backwards.
There was a problem hiding this comment.
Yeah this looks right to me. We should fix in other places if needed.
There was a problem hiding this comment.
Yup this is correct for downstream connections.
As you noticed, mine defaults to the reversed way for V1 (https://github.com/envoyproxy/envoy/pull/10682/files#diff-a25674ee929a59f2ed07b39eedfff4ccR84). This is because if there's no downstream address info (in health checks, for example), the proxy proto info is just the standard upstream connection info. If there is downstream address info available, it's sets up the src/dst the same as you have here (https://github.com/envoyproxy/envoy/pull/10682/files#diff-a25674ee929a59f2ed07b39eedfff4ccR93)
| Extensions::Common::ProxyProtocol::generateProxyProtoHeader( | ||
| connect_config.proxy_protocol_config(), connection, data); |
There was a problem hiding this comment.
We don't need to action on this in this PR, but I think this means that the proxy proto code is no longer an extension. Can we move it into the core somewhere?
| const Network::Connection& connection, Buffer::Instance& out) { | ||
| const Network::Address::Ip& dest_address = *connection.localAddress()->ip(); | ||
| const Network::Address::Ip& source_address = *connection.remoteAddress()->ip(); | ||
| if (config.version() == envoy::config::core::v3::ProxyProtocolConfig::V1) { |
There was a problem hiding this comment.
Yeah this looks right to me. We should fix in other places if needed.
Risk Level: low (only affects CONNECT requests)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a
Part of #1630 #1451