ProxyProtocolUpstream Transport socket should support connection pool setting

[lambdai]

Title: ProxyProtocolUpstream Transport socket should support connection pool setting
Description:
Currently ProxyProtocolUpstreamTransport hard coded connection pool key as "src_address" + "dst_address" and require both address not null.

However, per Proxy protocol v2, UNSPEC address is supported.
This limitation blocks from using proxy protocol carrying arbitrary per connection metadata via proxy protocol TLV

Problem 1:
Although we can populate src address and dst address by authentic address, the fact that addresses participating connection pool hashkey would create unnecessary connection pool.
The best approach is to allow UNSPEC address that does not take address into connection pool.

Problem 2:
TLV is not involved in connection pool.
That means a connection created with metadata "foo" share the same connection pool with connection annotated metadata "bar".
That's a hard failure in some use cases.

Proposal

+++ b/api/envoy/extensions/transport_sockets/proxy_protocol/v3/upstream_proxy_protocol.proto
@@ -24,4 +24,14 @@ message ProxyProtocolUpstreamTransport {
 
   // The underlying transport socket being wrapped.
   config.core.v3.TransportSocket transport_socket = 2 [(validate.rules).message = {required: true}];
+
+  // Control the connection pool settings for PROXY protocol socket.
+  ConnectionPoolSetting connection_pool_settings = 3;
+}
+
+// ConnectionPool settings for PROXY protocol socket.
+
+message ConnectionPoolSetting {
+  bool allow_unspecified_address = 1;
+  bool tlv_as_pool_key = 2;
 }

A zero initialized ConnectionPoolSetting should maintain the current behavior.

Let me know if the API make sense.

I have the implementation running well at my side. If anyone is feeling useful, l can create the PR for the API and the follow up implementation.

Thank you!

Ref
proxy protocol: https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

[yanavlasov]

Which TLV will be used as key? There can be multiple TLVs in the header.

[lambdai]

I think the short answer is all the TLVs.
I see ProxyProtocol transport sockets could select paritial downstream TLVs,
we can always extend ConnectionPoolSetting with initial hash all the TLVs

[lambdai]

Let me know if you have cases that selecting subset of TLVs
I am +1 with that configuration.
At my side, I am using INCLUDE_ALL to relay all the downstream TLVs because I had the control of the downstream TLVs.
I didn't propose much more than I requested but I am happy to contribute and maintain more use cases

[lambdai]

Actually, the best way to decribe the feature 1 is to allow LOCAL address along with TLVs.

Currently TLVs is not attached to the PP header if the address is local.

envoy/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

Lines 91 to 103 in 3bf801c

	if (!options_ || !options_->proxyProtocolOptions().has_value()) {
	Common::ProxyProtocol::generateV2LocalHeader(header_buffer_);
	} else {
	const auto options = options_->proxyProtocolOptions().value();
	if (!Common::ProxyProtocol::generateV2Header(options, header_buffer_, pass_all_tlvs_,
	pass_through_tlvs_)) {
	// There is a warn log in generateV2Header method.
	stats_.v2_tlvs_exceed_max_length_.inc();
	}
	ENVOY_LOG(trace, "generated proxy protocol v2 header, length: {}, buffer: {}",
	header_buffer_.length(), toHex(header_buffer_));
	}

[yanavlasov]

I do not have specific use cases, just asking clarifying questions. The feature looks ok to me.

[lambdai]

Thank you @yanavlasov
I am creating the PR