transport_socket(http_11_proxy): Add config for default proxy address #42914
Conversation
This patch adds an optional config parameter for specifying a default proxy address to be used by the transport socket if a proxy address is not found in the filter state metadata. If the parameter is not set, behavior of the transport socket remains identical to before this patch. Signed-off-by: Tony Allen <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto
Show resolved
Hide resolved
envoy/network/transport_socket.h
Outdated
| /** | ||
| * @return the default Http11ProxyInfo if configured, or nullopt. | ||
| */ | ||
| virtual absl::optional<TransportSocketOptions::Http11ProxyInfo> http11ProxyInfo() const PURE; |
There was a problem hiding this comment.
OptRef like the above options would probably be better (avoids copy of shared_ptr)
|
|
||
| // Proxy address was not found in the metadata. If a default proxy address is set, return that. | ||
| const auto* proxy_config = | ||
| dynamic_cast<const Network::Http11ProxyConfiguration*>(&socket_factory); |
There was a problem hiding this comment.
Hmmm, wouldn't this break with nested factories (e.g. TLS over HTTP CONNECT)?
There was a problem hiding this comment.
Something like TLS over HTTP CONNECT would be configured as an http_11_proxy transport socket with a TLS transport socket configured as the inner/wrapped transport, right? The nested factories would have the http_11_proxy TS factory on the outside, so this should still work.
I just checked and the integration test actually uses TLS for the inner transport socket by default, so the inner socket is TLS for the integration test in this patch.
There was a problem hiding this comment.
I think it can be the other way: CONNECT inside TLS factory (e.g. HBONE thing from Istio). I think it's OK to accept this limitation, but we should make it clear in the docs.
There was a problem hiding this comment.
There should be an easy way to solve it? The config should be loaded into the factory and pass the default that way.
There was a problem hiding this comment.
In the xDS representation, the http_11_proxy transport socket is configured with a child, which would be the TLS transport socket. The TLS transport socket does not have a way to configure a child transport socket, so it can't be configured the other way.
I'm not sure how that config maps to the underlying Envoy implementation, though.
Signed-off-by: Tony Allen <[email protected]>
Signed-off-by: Tony Allen <[email protected]>
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
Just a couple of quick drive-by comments from an xDS perspective...
api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto
Outdated
Show resolved
Hide resolved
|
|
||
| // Proxy address was not found in the metadata. If a default proxy address is set, return that. | ||
| const auto* proxy_config = | ||
| dynamic_cast<const Network::Http11ProxyConfiguration*>(&socket_factory); |
There was a problem hiding this comment.
In the xDS representation, the http_11_proxy transport socket is configured with a child, which would be the TLS transport socket. The TLS transport socket does not have a way to configure a child transport socket, so it can't be configured the other way.
I'm not sure how that config maps to the underlying Envoy implementation, though.
Signed-off-by: Tony Allen <[email protected]>
envoy/network/transport_socket.h
Outdated
| /** | ||
| * @return the default Http11ProxyInfo if configured, or nullopt. | ||
| */ | ||
| virtual OptRef<const TransportSocketOptions::Http11ProxyInfo> http11ProxyInfo() const { |
There was a problem hiding this comment.
call it defaultHttp11ProxyInfo similar to defaultSNI down below?
Signed-off-by: Tony Allen <[email protected]>
4 participants
This patch adds an optional config parameter for specifying a default
proxy address to be used by the transport socket if a proxy address is
not found in the filter state metadata. If the parameter is not set,
behavior of the transport socket remains identical to before this patch.
Risk Level: Low. New parameter.
Testing: unit/integration
Docs Changes: config documented
Release Notes: done
Platform Specific Features: n/a