tls: select alpn override by upstream protocol

[yxue]

Signed-off-by: crazyxy [email protected]

Description: select alpn override by upstream protocol
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Fixes #Issue: Part of #8197

[mattklein123]

FWIW I'm not thrilled that logic this specific is creeping into the router filter. Can we instead handle this in a similar fashion to:

envoy/include/envoy/http/filter.h

Lines 426 to 438 in 4e858f1

	/**
	* Adds socket options to be applied to any connections used for upstream requests. Note that
	* unique values for the options will likely lead to many connection pools being created. The
	* added options are appended to any previously added.
	*
	* @param options The options to be added.
	*/
	virtual void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr& options) PURE;
	/**
	* @return The socket options to be applied to the upstream request.
	*/
	virtual Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const PURE;

But instead for transport socket options? I think this should be a very similar situation?

cc @lizan can you take a look at ^ and guide in that direction depending on what you think? Thank you!

[lizan]

Yes I think the logic should be in extension (filter) code, then the transport socket options should be modified through filter callbacks.

[yxue]

The filter has limited knowledge to set the transport socket option. The filter only knows the downstream protocol and it can only select the ALPN according to the downstream protocol but I think the ALPN should be selected according to the upstream protocol instead of downstream's. e.g., if the downstream connection uses HTTP/1.1 and upstream connection uses HTTP/2, the ALPN override will be wrong.

[mattklein123]

The filter will know the upstream cluster that the the request is going to be routed to. This should be sufficient to decide at that point which ALPN to use, no? I think this can all be moved into a custom filter other than allowing filters to modify the transport socket options.

[lambdai]

+1 for @yxue
Since router decide if the upstream http protocol is h1 or h2,
ALPN http filter should not set the final alpn, it should provide two pool: h1 and h2

[yxue]

IIUC, we could do that without putting the upstream protocol in the cluster info because the http filter can also access the cluster and route info. Is the reason for the PR that we want to have single source of truth?

[lizan]

Yes, there’s another PR to add Http3 and I expect that logic can be more complicated in the future.

[lizan]

Also, I would still prefer that we add generic facilities for filters to modify transport socket options and then just use that from the router for now.

Unlike socket options its not very generic so I think keep using filter state is fine, so:
#8546

[lizan]

with #8546 and #8544, an http filter can calculate alpn override based on either upstream or downstream protocol and set it to filter state, can we close this? @yxue

I'm inclining to have that filter in istio/proxy because it seems very istio specific. If we see other interest then we can move.

[yxue]

@lizan thanks for PRs. I will close this one and another HTTP filter pr. I already created the PR in istio/proxy istio/proxy#2450