Envoy: Client side ALPN support (upstream cluster)

[mandarjog]

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 tohttp/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.

[mandarjog]

@PiotrSikora

[mattklein123]

Optimally, this would be done as part of also solving https://github.com/envoyproxy/envoy/issues/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]

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

1. Add a new connection pool impl, maybe HttpAutoConnectionPool. This impl is supposed to maintain list of the both http1 and http2 codec client, and auto select based on alpn value.
2. We use include::envoy::http::protocol
   to dispatch the conn pool. We can add another one, Http1And2 to indicate the auto selection under h2c upgrade and ALPN.
3. Plumb the ClientConnectionImpl::nextProtocol information with ClusterManagerImpl when 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 new HttpAutoConnectionPool.

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]

@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]

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]

@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]

Right now the code is organized as

1. We create codeclient, deciding it's http1 or http2
2. Within that codeclient constructor, we establish networking level connection.
3. Later on when network connection and transport socket is ready, only by then we know the negotiated ALPN value, but it's too late. Codec is already chosen in step 1.

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.cc test seem a good qua-lifer to see if anything break.

[incfly]

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 /http1 routes to a cluster without any alpn configured, for /http2 routes 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]

@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 https://github.com/envoyproxy/envoy/pull/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]

+1 - Could we have an update on this?

[eddiewang]

+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:

i can confirm that with a downgrade to istio v1.3.5 everything works as expected again. I was trying an upgrade to v1.4 so it was using whatever Envoy that version of Istio’s mesh uses. Thanks for tracing down this issue, very interesting bug because around half of my services were still working (probably because they supported HTTP2).

Drove me crazy because HTTP still worked, and I thought it was a mismanaged cert or an issue with istio’s SDS service.

[mooperd]

@eddiewang I don't think this is your specific problem.

[snowp]

@incfly Is this something you're still planning on working on?

Are we still thinking that a new connection pool is the right way to handle this? Not sure if there is an easier way of achieving this with other changes to the connection pool logic. The alternative that comes to mind would be including the desired ALPN in the connection pool hash (used today for socket settings), though that might not provide a good story for downgrades.

I think there's both a desire to support downgrading a cluster configured to do HTTP/1.1 when HTTP/2 ALPN fails, but also to be able to adjust the ALPN protocols depending on which protocol was selected (when using USE_DOWNSTREAM_PROTOCOL) to make sure that the selected HTTP protocol aligns with the selected ALPN protocol.

@lizan @ggreenway @mattklein123 for thoughts

[lizan]

Yes I think a new connection pool is still needed for this.

[incfly]

@snowp Sorry I was get distracted to something else on our team project. I got to a point where ALPN plumbing to connection pool creation time. Feel free re-assign if anyone feel the urgency.

To provide some outcome of my research/investigation: by the time I looked at this, the HTTP1 and HTTP2 connection has quite some duplication, like ActiveClient, etc.

This new connection, per requirement, need to handle upgrade and able to switch between 1 and 2, without further code duplication. That's the part I feel we will have to spend quite some time to refactor to unblock this new connection pool.

[snowp]

@incfly Does the work done here https://github.com/envoyproxy/envoy/pull/9668 make things easier?

No immediate rush here, just trying to get a sense for where we're at and if someone's working on this

[mattklein123]

@snowp I think there are 2 ways of thinking about this feature:
1) Setting ALPN to match what you want to happen (to match downstream for example). This I think could be done with transport socket options and a different connection pool hash. So I don't think we would need a new CP in that case.
2) What this issue is mainly tracking which is to allow h2, http/1.1 and then actually properly negotiate which one. This is more closely related to how we would do http/1.1 -> h2 upgrade. For that per @lizan I think we do need a new CP.

cc @htuch ^ is related to the h1 -> h2 upgrade intern project I listed.

[snowp]

I think 1) is easier to do but 2) is much more valuable as you are able to do HTTP/2 whenever its supported - it definitely solves the problem that prompted me bringing this up.

I'm not sure if 1) is useful enough to warrant investing in it vs the more useful 2), unless anyone can think of some specific use cases?

[snowp]

I was playing with the code to get this working, I think something like this would work:

The new conn pool keeps a std::unique_ptr<ConnectionPool::Instance> delegated_conn_pool_ that manages the actual connection pooling. In order to create this, the first attempt to create a stream will be be managed by the outer connection pool:
1) The outer connection pool will create a raw connection to the upstream if newStream is called when no connection has been established or are in progress.
2) Subsequent calls to newStream during the connection setup will enqueue the streams (subject to max_pending_reqs).
3) If the connection is successful, we allocate delegated_conn_pool_ to the appropriate connection pool based on the negotiated ALPN, falling back to one of the original protocol selection mechanisms. At this point we drain the queue of pending stream setups, and will delegate all further newStream calls to the inner conn pool. We also assign the raw connection we set up to the newly created conn pool: this would allow us to immediately make use of this connection within the inner conn pool.
4) If connection setup fails, we fail all pending streams similar to how we fail pending streams for the existing conn pools. We'll be back with no pending connection setup and no inner conn pool, so we'd start the whole again if we got another newStream call.

This seems like it would be a relatively easy way to implement this and reuse most of the existing code base. I can think of two drawbacks to this implementation:
1) Should HTTP/1.1 be negotiated, we'll potentially suffer increased HOL blocking compared to the existing HTTP/1.1 conn pool as all incoming requests will be blocked on the first connection setup, but we might need to immediately create more connections once we know to use HTTP/1.1 to support the desired parallelism.
2) No real support for mixed upstream protocols: one could imagine the upstream being an intermediate TCP proxy which fronts multiple upstreams which attempt to ALPN negotiate differently, or even the case of ip/port reuse. If HTTP/2 was negotiated, we'd pin ourselves to HTTP/2 for the lifetime of the conn pool, so if another connection was to an upstream that only supports HTTP/1.1 we'd be in trouble.

While 1) would be a potential perf regression, 2) is no worse than what we have today although it might surprise people if they expect each connection to be ALPN negotiated independently.

[mattklein123]

@snowp this makes sense to me. Note that I would like to implement this using the new connection pool extension mechanism being implemented by @alyssawilk in https://github.com/envoyproxy/envoy/pull/11327.

[snowp]

@mattklein123 Looking at the extension API it seems like it might be a bit tricky to use it for this purpose: it abstracts away how to provide a GenericConnPool from a cluster name, which means that it won't have access to the individual host until after we request a conn pool for it or access to the host -> conn pool map that is maintained in the TLS ClusterEntry.

In my mind we'd want this new conn pool implementation to implement the same interface as the other HTTP conn pools so that it could be maintained in the same conn pool map, WDYT?

[mattklein123]

In my mind we'd want this new conn pool implementation to implement the same interface as the other HTTP conn pools so that it could be maintained in the same conn pool map, WDYT?

Yes, agreed. Can you work with @alyssawilk to evolve the extension API so it can work that way?

[alyssawilk]

I'm really not sure what the plan was w.r.t delegated connection pool. Even if we had an outer pool wrapping an inner HTTP/1 and inner HTTP/2 pool we'd need to create a connection somewhere, and hope ALPN matched? Or we need to create raw TCP connections and once they're established, stick that connection into a codec?

I think we want to go go the latter route, and if we do I don't think there's much of a need to have a delegated connection pool. Given almost all the logic between TCP, HTTP/1 and HTTP/2 is shared in the connection pool, and almost all of the application-specific logic is actually in the ActiveClient class, I think we could have a MixedConnectionPool. If alpn is cached, just creates a HTTP/1 or HTTP/2 type based on alpn (with graceful failure if alpn changes). If there's no cached ALPN, it creates raw TCP ActiveClients, and once they're connected, pulls the network::connection from the TCP active client and sticks it in the appropriate HTTP/1 or HTTP/2 ActiveClient.

I think it'd take a little bit of fiddling to move the last bit of pool-specific logic into the active client classes, and a bit of work to be able to pull the connection out of ActiveTcpClient and into anything else, so wanted to check in and make sure this doesn't sound too sketchy before I tackle it.

[alyssawilk]

Basically something like mixed_connection_pool.* here
https://github.com/envoyproxy/envoy/compare/master...alyssawilk:h1_h2?expand=1
but without all the FIXMEs :-P

[mattklein123]

@alyssawilk yes +1 I agree with the implementation you have outlined and this is roughly how I would have implemented it also. Do you want to just open a PR with your draft to take a look there? Exciting!

[alyssawilk]

I'll go ahead and fix the FIXMES and get some tests working before I send it out. Just wanted to check in early in since it sounded to me like it was a different plan than folks had already agreed on :-)

[snowp]

Yeah the idea was to make a raw connection to perform ALPN negotiation before deciding which delegated pool to use, then handing that connection over to the pool once ALPN was decided. My motivation for doing a delegated pool was to be able to reuse the protocol specific handling in each pool (e.g. number of allowed concurrent streams), but if the consolidation of the connection pool has come far enough that might no longer be necessary.