quic: HTTP/3 upstream initial checkin by alyssawilk · Pull Request #15027 · envoyproxy/envoy

alyssawilk · 2021-02-11T19:42:04Z

This is working end to end for a number of the protocol integration tests, but still needs a bunch of work before it's production ready (I triaged some of the excludes but not all, watermarks not working etc).
I think given this works for the happy path it's worth checking in, and I can iterate on getting individual tests working and doing the TODOs.

Risk Level: low (minor core refactors, major changes are all behind hidden config)
Testing: integration tests, some unit tests
Docs Changes: n/a
Release Notes: n/a
#14829

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-02-11T19:46:55Z

cc @DavidSchinazi

danzh2010

Thanks for working out this framework!

source/common/http/http3/quic_codec_factory.h

test/extensions/quic_listeners/quiche/integration/quic_protocol_integration_test.cc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-02-18T21:01:48Z

comments should be addressed, PTAL

danzh2010

Sorry for the late review. I have some thoughts on the EnvoyQuicClientSessionWithExtra, happy to discuss. Looks good over all, but I would prefer someone who is familiar with allocateConnPool() implementation and cluster_manager_impl to review that part.

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

danzh2010 · 2021-02-22T16:43:32Z

source/extensions/quic_listeners/quiche/client_connection_factory_impl.cc

+    Event::Dispatcher& dispatcher, TimeSource& time_source) {
+  // TODO(#14829): reject the config if a raw buffer socket is configured.
+  auto* ssl_socket_factory =
+      dynamic_cast<Extensions::TransportSockets::Tls::ClientSslSocketFactory*>(


Instead of adding getter for config_ in ClientSslSocketFactory(), can we use QuicClientTransportSocketFactory instead? Maybe worth to reject the config if it is configured to use anything else.

yeah, have a TODO to reject the config already. Using the quic transport factory involves a bunch of extra workarounds due to the release assert on creating transport sockets. It means I have to fix the TODO for not creating the connection (which used the TLS config) and tossing it, but isn't too much extra cruft to get that TODO done :-)

source/extensions/quic_listeners/quiche/client_connection_factory_impl.h

source/extensions/transport_sockets/tls/ssl_socket.h

source/common/http/http2/conn_pool.h

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

danzh2010

LGTM, alone aside a few nits

danzh2010 · 2021-02-24T16:47:47Z

source/common/http/http3/conn_pool.h

+namespace Http3 {
+
+// TODO(#14829) the constructor of Http2::ActiveClient sets max requests per
+// connection based on HTTP/2 config. Sort out the HTTP/3 config story.


quic listener configures this field in envoy.config.listener.v3.QuicProtocolOptions. I guess it's a question about where to place this config and how to plumb thru.

Not related to this PR, but given some recent discussions around whether QUIC should be an extension vs built-in (potentially with the option to compile it out) may influence our decisions here. I would suggest a short document on QUIC build and configuration so we can discuss the options?

danzh2010 · 2021-02-24T16:55:44Z

test/integration/base_integration_test.cc

+    std::vector<std::string> server_names;
+    auto& config_factory = Config::Utility::getAndCheckFactoryByName<
+        Server::Configuration::DownstreamTransportSocketConfigFactory>(
+        "envoy.transport_sockets.quic");


nits: use Extensions::TransportSockets::TransportSocketNames::get().Quic instead

I think we're moving away from wkn files per #7238
I used hard-coded strings to avoid further use of wkn. will let Matt comment on his preference on these ones since I do lean towards using predefined values myself.

I think it's fine either way for now, but again related to why I think we need to undo all the extension stuff.

danzh2010 · 2021-02-24T16:58:14Z

test/integration/fake_upstream.h

+        envoy::config::listener::v3::QuicProtocolOptions config;
+        auto& config_factory =
+            Config::Utility::getAndCheckFactoryByName<Server::ActiveUdpListenerConfigFactory>(
+                "quiche_quic_listener");


nits: use Envoy.Quic.QuicListenerName instead

danzh2010 · 2021-02-24T17:17:22Z

test/integration/base_integration_test.cc

+    config_helper_.addConfigModifier(
+        [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
+          // Docker doesn't allow writing to the v6 address returned by
+          // Network::Utility::getLocalAddress.


Is this a docker bug that leads us to hardcode address here?

Yeah, this test passes locally for me without that change, but failed on CI. I as far as I could tell the docker container was resolving a local v6 address but was cranky at sending to its non-loopback v6 address, where it was fine doing so with v4.
AFIK we have similar issues in-house with v6 docker behaving differently so I'm not terribly surprised.

source/extensions/quic_listeners/quiche/client_connection_factory_impl.h

danzh2010 · 2021-02-24T17:31:16Z

source/common/http/http3/quic_client_connection_factory.h

+namespace Envoy {
+namespace Http {
+
+// A factory to create EnvoyQuicClientConnection instance for QUIC


nits: s/EnvoyQuicClientConnection/EnvoyQuicClientSession and EnvoyQuicClientConnection

danzh2010 · 2021-02-24T17:33:53Z

source/common/http/http3/quic_client_connection_factory.h

+  virtual std::unique_ptr<Network::ClientConnection>
+  createQuicNetworkConnection(Network::Address::InstanceConstSharedPtr server_addr,
+                              Network::Address::InstanceConstSharedPtr local_addr,
+                              Network::TransportSocketFactory& transport_socket_factory,


nits: pass in QuicClientTransportSocketFactory instead.

this is a core http3 API and that's a QUIC class in the extension directory so I think leaving as-is makes sense?

Ah, I see. That makes sense! Maybe I should move Quic TransportSocketFactory into core code instead. It doesn't depends on QUICHE.

I think we should move it all at this point. See my other comments.

source/common/http/http2/conn_pool.h

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk

@mattklein123 you up for a pass on the conn pool side of things?

alyssawilk · 2021-02-24T18:07:52Z

source/common/http/http3/quic_client_connection_factory.h

+  virtual std::unique_ptr<Network::ClientConnection>
+  createQuicNetworkConnection(Network::Address::InstanceConstSharedPtr server_addr,
+                              Network::Address::InstanceConstSharedPtr local_addr,
+                              Network::TransportSocketFactory& transport_socket_factory,


this is a core http3 API and that's a QUIC class in the extension directory so I think leaving as-is makes sense?

alyssawilk · 2021-02-24T18:10:00Z

test/integration/base_integration_test.cc

+    std::vector<std::string> server_names;
+    auto& config_factory = Config::Utility::getAndCheckFactoryByName<
+        Server::Configuration::DownstreamTransportSocketConfigFactory>(
+        "envoy.transport_sockets.quic");


I think we're moving away from wkn files per #7238
I used hard-coded strings to avoid further use of wkn. will let Matt comment on his preference on these ones since I do lean towards using predefined values myself.

alyssawilk · 2021-02-24T18:11:48Z

test/integration/base_integration_test.cc

+    config_helper_.addConfigModifier(
+        [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
+          // Docker doesn't allow writing to the v6 address returned by
+          // Network::Utility::getLocalAddress.


Yeah, this test passes locally for me without that change, but failed on CI. I as far as I could tell the docker container was resolving a local v6 address but was cranky at sending to its non-loopback v6 address, where it was fine doing so with v4.
AFIK we have similar issues in-house with v6 docker behaving differently so I'm not terribly surprised.

alyssawilk · 2021-03-02T14:22:05Z

ping!

alyssawilk · 2021-03-02T14:28:34Z

(mac failure is the known timeout issue)

mattklein123

Very cool. LGTM with some comments and one line that I think was meant to be deleted.

/wait

mattklein123 · 2021-03-02T17:41:45Z

source/common/http/http3/conn_pool.h

+namespace Http3 {
+
+// TODO(#14829) the constructor of Http2::ActiveClient sets max requests per
+// connection based on HTTP/2 config. Sort out the HTTP/3 config story.


Not related to this PR, but given some recent discussions around whether QUIC should be an extension vs built-in (potentially with the option to compile it out) may influence our decisions here. I would suggest a short document on QUIC build and configuration so we can discuss the options?

mattklein123 · 2021-03-02T17:43:23Z

source/common/http/http3/conn_pool.cc

+            data.host_description_->transportSocketFactory();
+        data.connection_ =
+            Config::Utility::getAndCheckFactoryByName<Http::QuicClientConnectionFactory>(
+                Http::QuicCodecNames::get().Quiche)


Just another example of an extension leaking into core code which I don't think makes sense.

mattklein123 · 2021-03-02T17:44:39Z

source/common/http/http3/quic_client_connection_factory.h

+  virtual std::unique_ptr<Network::ClientConnection>
+  createQuicNetworkConnection(Network::Address::InstanceConstSharedPtr server_addr,
+                              Network::Address::InstanceConstSharedPtr local_addr,
+                              Network::TransportSocketFactory& transport_socket_factory,


I think we should move it all at this point. See my other comments.

mattklein123 · 2021-03-02T17:49:18Z

test/integration/fake_upstream.cc

 }

 void FakeUpstream::onRecvDatagram(Network::UdpRecvData& data) {
+  std::cerr << "Got datagrams. Need to dispatch\n";


mattklein123 · 2021-03-02T17:50:55Z

test/integration/base_integration_test.cc

+    std::vector<std::string> server_names;
+    auto& config_factory = Config::Utility::getAndCheckFactoryByName<
+        Server::Configuration::DownstreamTransportSocketConfigFactory>(
+        "envoy.transport_sockets.quic");


I think it's fine either way for now, but again related to why I think we need to undo all the extension stuff.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

LGTM (let's discuss/track how to deal with the extension status in #12829)

quic: HTTP/3 upstream prototype

bb5b9b4

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned danzh2010 Feb 11, 2021

alyssawilk requested review from PiotrSikora, asraa, ggreenway, lizan and mattklein123 as code owners February 11, 2021 19:42

alyssawilk unassigned danzh2010 Feb 11, 2021

alyssawilk force-pushed the upstream_quic branch from c33cfa1 to bb5b9b4 Compare February 11, 2021 20:41

danzh2010 reviewed Feb 11, 2021

View reviewed changes

source/common/http/http3/quic_codec_factory.h Outdated Show resolved Hide resolved

source/common/http/http3/quic_codec_factory.h Outdated Show resolved Hide resolved

test/extensions/quic_listeners/quiche/integration/quic_protocol_integration_test.cc Show resolved Hide resolved

antoniovicente assigned danzh2010 Feb 12, 2021

alyssawilk added 4 commits February 16, 2021 14:40

fixing factories, adding cerr logging for v6 Ci failures

546ec73

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

missing files

22bc900

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

format

4f35951

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

more factory cleanup, hopeful v6 fix

d6fccfa

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

danzh2010 reviewed Feb 22, 2021

View reviewed changes

alyssawilk assigned mattklein123 Feb 22, 2021

comments

37dc945

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

danzh2010 reviewed Feb 24, 2021

View reviewed changes

comments

d95ca6f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk commented Feb 24, 2021

View reviewed changes

danzh2010 previously approved these changes Mar 2, 2021

View reviewed changes

mattklein123 requested changes Mar 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 2, 2021

oops

9811ae2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed danzh2010’s stale review via 9811ae2 March 2, 2021 20:52

repokitteh-read-only bot removed the waiting label Mar 2, 2021

mattklein123 approved these changes Mar 2, 2021

View reviewed changes

alyssawilk merged commit 0a58f84 into envoyproxy:main Mar 3, 2021

alyssawilk mentioned this pull request Mar 3, 2021

test: fixing CI #15282

Merged

alyssawilk mentioned this pull request Mar 11, 2021

quiche: enable downstream HTTP3 in quic_protocol_integration_test #15424

Merged

alyssawilk deleted the upstream_quic branch August 18, 2021 13:59

Conversation

alyssawilk commented Feb 11, 2021

Uh oh!

alyssawilk commented Feb 11, 2021

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk commented Feb 18, 2021

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Mar 2, 2021

Uh oh!

alyssawilk commented Mar 2, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!