Repository navigation
Client side ALPN support (upstream cluster) #3431
Description
Title: Envoy should support client side protocol negotiation via alpn
Description:
At present if the an upstream cluster has http2_protocolOptions, it will always send h2 traffic upstream. When used with ALPN = {"h2", "http/1.1"} the expectation is that it will be negotiated down to http/1.1if the server only supporthttp/1.1` I don't think this is happening.
We need this in Istio so that we can set "ALPN" options on all outbound tls connections and upgrade.
We also need to have http/1.1 --> h2c, there is a separate issue #1502 filed for this.
Reacted by Bastian Kuberek, Frank He, Marcin Falkowski, Eddie Wang, ehildebrandt, nikhil-kanotra, Antarus, Teju Nareddy, Henrik Feldt and Davide Setti
Activity
mandarjog commentedon May 17, 2018
@PiotrSikora
mattklein123 commentedon May 17, 2018
Optimally, this would be done as part of also solving #1502. The right solution here is to have a new connection pool implementation that can do both HTTP/1.1, HTTP/2, ALPN negotiation, upgrade, etc. I would prefer to not complicate the existing connection pool implementations with this logic.
incfly commentedon Jul 22, 2019
I did some study on this bug.
Using new connection pool for http1/http2 protocol selection with ALPN information seems feasible to me. High level things to do
ALPN upgrade
HttpAutoConnectionPool. This impl is supposed to maintain list of the both http1 and http2 codec client, and auto select based on alpn value.include::envoy::http::protocolto dispatch the conn pool. We can add another one,
Http1And2to indicate the auto selection under h2c upgrade and ALPN.ClientConnectionImpl::nextProtocolinformation withClusterManagerImplwhen selecting http conn pool.H2C upgrade
I don't find any Envoy own implementation on HTTP its own auto-upgrade,
h2c. I assume the h2 auto upgrade over plaintext has to be implemented, and then ingtegrated with this newHttpAutoConnectionPool.H2C upgrade part I'm not that familiar. I hope we can focus on implementing ALPN part first, and the proposed changes structurally fit the enhancement later.
@PiotrSikora @mattklein123
Let me know what you think, thanks!
mattklein123 commentedon Jul 22, 2019
@incfly this is on the right track.
For H2C upgrade, nghttp2 supports this internally to the library. See https://nghttp2.org/documentation/nghttp2_session_upgrade2.html#c.nghttp2_session_upgrade2. This can then be coupled with an HTTP1 codec to start the process, which then switches over to an HTTP2 codec after the upgrade. There will need to be some work/thought put into how this is done, most likely by having specific h2 codec constructors that are meant for upgrade scenarios.
incfly commentedon Jul 30, 2019
I've started spending some time on this FR and @mattklein123 maybe we can update assignees to avoid potential duplicate work?
One question, what will be a feature guarding mechanism recommended to control this pool selection behavior change? A flag or a field under api/cluster or something else?
mattklein123 commentedon Jul 31, 2019
@incfly I assigned the issue to you. In terms of configuration, yeah, I think we need to look at some config on the cluster, similar to how we configure http/1 vs. http/2 today. TBH I'm not thrilled with how this is all configured today, but we can clean this up in v3 cc @htuch. I would take a stab at something and we can talk about it in CR. Thank you!
incfly commentedon Aug 9, 2019
Right now the code is organized as
Appears to me we can defer the codec creation in
onConnectionEvent, by then we'll have enough information about ALPN.I guess, this won't be an issue, HTTP req/resp can't make any progress untill transport socket is ready.
This might require some refactoring, and
http_integration.cctest seem a good qua-lifer to see if anything break.incfly commentedon Aug 22, 2019
A very very rough POC can be found at https://github.com/envoyproxy/envoy/compare/master...incfly:httpx-pool?expand=1
Mostly this httpx connection pool go through
upgrade()in case of connected && ALPN == H2.It'll be great to get some quick feedback so that I tell it's right direct to continue polishing it.
This POC duplicate lots of code from both http1 and http2 connection pool, which can cause some maintainece overhead for future changes on http1 and http2 implementation. But to certain extend, this is expected, since we don't want to mess up and modify on existing http1 connection pool. And overtime, we can deprecate HTTP1 connection pool.
The
upgrade()is invoked in the case of connection event and ALPN is negotiated as "h2".With this change, I set up a client and server envoy, for
/http1routes to a cluster without any alpn configured, for/http2routes to alpn h2 configured. Then I can verify from server side stats, the downstream_rq_http2 and http1 metrics increment correspondingly. (client side still has some to be done work to get stats working)mattklein123 commentedon Aug 23, 2019
@incfly at a very high level this looks like it's on the right track, but I feel that we need to reconcile the code duplication, especially as we already have this issue in the context of #7852 cc @conqerAtapple. I would like to see a plan to come up with a base class that can be used across all of the implementations with potentially just the callouts needed to do the upgrade?
mooperd commentedon Nov 20, 2019
+1 - Could we have an update on this?
eddiewang commentedon Nov 21, 2019
+1 spent hours tracing this down, but on the new Istio 1.4 around half of my external services stopped being accessible, here's the comment I wrote in the istio forum:
mooperd commentedon Nov 24, 2019
@eddiewang I don't think this is your specific problem.
16 remaining items
network: adding some accessors for ALPN work. (#13785)
http2: moving functionality from http2 pool to client (#13869)
http: moving functions and member variables out of http1 pool (#13867)
tcp: make it possible for TCP connections to be creatd by non-TCP poo…
conn_pool: allowing multiple protocols in ALPN (#13901)