http3: fixing an upstream threading issue and bumping http3 upstream code back to alpha

changelogs/current.yaml

@@ -50,6 +50,10 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: http3_upstream
+  change: |
+    Fixing a bug with HTTP/3 upstream using a non-threadsafe cache cross-thread. Bumping HTTP/3 support down
+    to alpha as the severity of this bug indicates it is both not in use and not GA quality code.
 - area: tracers
   change: |
     use unary RPC calls for OpenTelemetry trace exports, rather than client-side streaming connections.

docs/root/intro/arch_overview/http/http3.rst

@@ -8,8 +8,7 @@ HTTP/3 overview
   While HTTP/3 **downstream support is deemed ready for production use**, improvements are ongoing,
   tracked in the `area-quic <https://github.com/envoyproxy/envoy/labels/area%2Fquic>`_ tag.
 
-  HTTP/3 **upstream support is fine for locally controlled networks**, but is alpha for
-  general internet use - key features are implemented but have not been tested at scale.
+  HTTP/3 upstream support is alpha - key features are implemented but have not been tested at scale.
 
 .. _arch_overview_http3_downstream:
 

source/common/quic/quic_client_transport_socket_factory.cc

@@ -33,7 +33,10 @@ QuicClientTransportSocketFactory::QuicClientTransportSocketFactory(
     Server::Configuration::TransportSocketFactoryContext& factory_context)
     : QuicTransportSocketFactoryBase(factory_context.statsScope(), "client"),
       fallback_factory_(std::make_unique<Extensions::TransportSockets::Tls::ClientSslSocketFactory>(
-          std::move(config), factory_context.sslContextManager(), factory_context.statsScope())) {}
+          std::move(config), factory_context.sslContextManager(), factory_context.statsScope())),
+      tls_slot_(factory_context.serverFactoryContext().threadLocal()) {
+  tls_slot_.set([](Event::Dispatcher&) { return std::make_shared<ThreadLocalQuicConfig>(); });
+}
 
 void QuicClientTransportSocketFactory::initialize() {
   if (!fallback_factory_->clientContextConfig()->alpnProtocols().empty()) {
@@ -55,15 +58,18 @@ std::shared_ptr<quic::QuicCryptoClientConfig> QuicClientTransportSocketFactory::
     return nullptr;
   }
 
-  if (client_context_ != context) {
+  ASSERT(tls_slot_.currentThreadRegistered());
+  ThreadLocalQuicConfig& tls_config = *tls_slot_;
+
+  if (tls_config.client_context_ != context) {
     // If the context has been updated, update the crypto config.
-    client_context_ = context;
-    crypto_config_ = std::make_shared<quic::QuicCryptoClientConfig>(
+    tls_config.client_context_ = context;
+    tls_config.crypto_config_ = std::make_shared<quic::QuicCryptoClientConfig>(
         std::make_unique<Quic::EnvoyQuicProofVerifier>(std::move(context)),
         std::make_unique<quic::QuicClientSessionCache>());
   }
   // Return the latest crypto config.
-  return crypto_config_;
+  return tls_config.crypto_config_;
 }
 
 REGISTER_FACTORY(QuicClientTransportSocketConfigFactory,

source/common/quic/quic_client_transport_socket_factory.h

@@ -45,14 +45,22 @@ class QuicClientTransportSocketFactory : public Network::CommonUpstreamTransport
   // fallback_factory_ will update the context.
   void onSecretUpdated() override {}
 
+  // The cache in the QuicCryptoClientConfig is not thread-safe, so crypto_config_ needs to
+  // be a thread local object. client_context lets the thread local object determine if the crypto
+  // config needs to be updated.
+  struct ThreadLocalQuicConfig : public ThreadLocal::ThreadLocalObject {
+    // Latch the latest client context, to determine if it has updated since last
+    // checked.
+    Envoy::Ssl::ClientContextSharedPtr client_context_;
+    // If client_context_ changes, client config will be updated as well.
+    std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config_;
+  };
+
 private:
   // The QUIC client transport socket can create TLS sockets for fallback to TCP.
   std::unique_ptr<Extensions::TransportSockets::Tls::ClientSslSocketFactory> fallback_factory_;
-  // Latch the latest client context, to determine if it has updated since last
-  // checked.
-  Envoy::Ssl::ClientContextSharedPtr client_context_;
-  // If client_context_ changes, client config will be updated as well.
-  std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config_;
+  // The storage for thread local quic config.
+  ThreadLocal::TypedSlot<ThreadLocalQuicConfig> tls_slot_;
 };
 
 class QuicClientTransportSocketConfigFactory

test/common/http/conn_pool_grid_test.cc

@@ -13,6 +13,7 @@
 #include "test/mocks/http/stream_decoder.h"
 #include "test/mocks/http/stream_encoder.h"
 #include "test/mocks/network/connection.h"
+#include "test/mocks/server/transport_socket_factory_context.h"
 #include "test/mocks/ssl/mocks.h"
 #include "test/mocks/upstream/cluster_info.h"
 #include "test/test_common/simulated_time_system.h"
@@ -131,7 +132,10 @@ class ConnectivityGridTest : public Event::TestUsingSimulatedTime, public testin
         options_({Http::Protocol::Http11, Http::Protocol::Http2, Http::Protocol::Http3}),
         alternate_protocols_(std::make_shared<HttpServerPropertiesCacheImpl>(
             dispatcher_, std::vector<std::string>(), nullptr, 10)),
-        quic_stat_names_(store_.symbolTable()) {}
+        quic_stat_names_(store_.symbolTable()) {
+    ON_CALL(factory_context_.server_context_, threadLocal())
+        .WillByDefault(ReturnRef(thread_local_));
+  }
 
   void initialize() {
     quic_connection_persistent_info_ =
@@ -194,6 +198,9 @@ class ConnectivityGridTest : public Event::TestUsingSimulatedTime, public testin
 
   StreamInfo::MockStreamInfo info_;
   NiceMock<MockRequestEncoder> encoder_;
+
+  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context_;
+  testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
 };
 
 // Test the first pool successfully connecting.
@@ -945,7 +952,6 @@ TEST_F(ConnectivityGridTest, Http3FailedH2SuceedsInline) {
 } // namespace Http
 } // namespace Envoy
 
-#include "test/mocks/server/transport_socket_factory_context.h"
 #include "source/common/quic/quic_client_transport_socket_factory.h"
 namespace Envoy {
 namespace Http {
@@ -957,9 +963,8 @@ TEST_F(ConnectivityGridTest, RealGrid) {
   dispatcher_.allow_null_callback_ = true;
   // Set the cluster up to have a quic transport socket.
   Envoy::Ssl::ClientContextConfigPtr config(new NiceMock<Ssl::MockClientContextConfig>());
-  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context;
   auto factory =
-      std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config), factory_context);
+      std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config), factory_context_);
   factory->initialize();
   auto& matcher =
       static_cast<Upstream::MockTransportSocketMatcher&>(*cluster_->transport_socket_matcher_);
@@ -999,12 +1004,11 @@ TEST_F(ConnectivityGridTest, ConnectionCloseDuringAysnConnect) {
   dispatcher_.allow_null_callback_ = true;
   // Set the cluster up to have a quic transport socket.
   Envoy::Ssl::ClientContextConfigPtr config(new NiceMock<Ssl::MockClientContextConfig>());
-  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context;
   Ssl::ClientContextSharedPtr ssl_context(new Ssl::MockClientContext());
-  EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _))
+  EXPECT_CALL(factory_context_.context_manager_, createSslClientContext(_, _))
       .WillOnce(Return(ssl_context));
   auto factory =
-      std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config), factory_context);
+      std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config), factory_context_);
   factory->initialize();
   auto& matcher =
       static_cast<Upstream::MockTransportSocketMatcher&>(*cluster_->transport_socket_matcher_);

test/common/http/http3/conn_pool_test.cc

@@ -39,6 +39,7 @@ class MockPoolConnectResultCallback : public PoolConnectResultCallback {
 class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testing::Test {
 public:
   Http3ConnPoolImplTest() {
+    ON_CALL(context_.server_context_, threadLocal()).WillByDefault(ReturnRef(thread_local_));
     EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _))
         .WillRepeatedly(Return(ssl_context_));
     factory_.emplace(std::unique_ptr<Envoy::Ssl::ClientContextConfig>(
@@ -86,6 +87,7 @@ class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testi
   std::unique_ptr<Http3ConnPoolImpl> pool_;
   MockPoolConnectResultCallback connect_result_callback_;
   std::shared_ptr<Network::MockSocketOption> socket_option_{new Network::MockSocketOption()};
+  testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
 };
 
 class MockQuicClientTransportSocketFactory : public Quic::QuicClientTransportSocketFactory {

test/common/quic/client_connection_factory_impl_test.cc

@@ -27,6 +27,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime,
                                   public testing::TestWithParam<Network::Address::IpVersion> {
 protected:
   void initialize() {
+    ON_CALL(context_.server_context_, threadLocal()).WillByDefault(ReturnRef(thread_local_));
     EXPECT_CALL(*cluster_, perConnectionBufferLimitBytes()).WillOnce(Return(45));
     EXPECT_CALL(*cluster_, connectTimeout).WillOnce(Return(std::chrono::seconds(10)));
     auto* protocol_options = cluster_->http3_options_.mutable_quic_protocol_options();
@@ -88,6 +89,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime,
   QuicStatNames quic_stat_names_{store_.symbolTable()};
   quic::DeterministicConnectionIdGenerator connection_id_generator_{
       quic::kQuicDefaultConnectionIdLength};
+  testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
 };
 
 TEST_P(QuicNetworkConnectionTest, BufferLimits) {

test/common/quic/quic_transport_socket_factory_test.cc

@@ -19,6 +19,7 @@ class QuicServerTransportSocketFactoryConfigTest : public Event::TestUsingSimula
   QuicServerTransportSocketFactoryConfigTest()
       : server_api_(Api::createApiForTest(server_stats_store_, simTime())) {
     ON_CALL(context_.server_context_, api()).WillByDefault(ReturnRef(*server_api_));
+    ON_CALL(context_.server_context_, threadLocal()).WillByDefault(ReturnRef(thread_local_));
   }
 
   void verifyQuicServerTransportSocketFactory(std::string yaml, bool expect_early_data) {
@@ -35,6 +36,7 @@ class QuicServerTransportSocketFactoryConfigTest : public Event::TestUsingSimula
   Stats::TestUtil::TestStore server_stats_store_;
   Api::ApiPtr server_api_;
   NiceMock<Server::Configuration::MockTransportSocketFactoryContext> context_;
+  testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
 };
 
 TEST_F(QuicServerTransportSocketFactoryConfigTest, EarlyDataEnabledByDefault) {
@@ -113,6 +115,7 @@ TEST_F(QuicServerTransportSocketFactoryConfigTest, ClientAuthUnsupported) {
 class QuicClientTransportSocketFactoryTest : public testing::Test {
 public:
   QuicClientTransportSocketFactoryTest() {
+    ON_CALL(context_.server_context_, threadLocal()).WillByDefault(ReturnRef(thread_local_));
     EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _)).WillOnce(Return(nullptr));
     EXPECT_CALL(*context_config_, setSecretUpdateCallback(_))
         .WillOnce(testing::SaveArg<0>(&update_callback_));
@@ -125,6 +128,7 @@ class QuicClientTransportSocketFactoryTest : public testing::Test {
   NiceMock<Ssl::MockClientContextConfig>* context_config_{
       new NiceMock<Ssl::MockClientContextConfig>};
   std::function<void()> update_callback_;
+  testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
 };
 
 TEST_F(QuicClientTransportSocketFactoryTest, SupportedAlpns) {

test/integration/base_integration_test.cc

@@ -74,6 +74,7 @@ BaseIntegrationTest::BaseIntegrationTest(const InstanceConstSharedPtrFn& upstrea
       }));
   ON_CALL(factory_context_.server_context_, api()).WillByDefault(ReturnRef(*api_));
   ON_CALL(factory_context_, statsScope()).WillByDefault(ReturnRef(*stats_store_.rootScope()));
+  ON_CALL(factory_context_.server_context_, threadLocal()).WillByDefault(ReturnRef(thread_local_));
 
 #ifndef ENVOY_ADMIN_FUNCTIONALITY
   config_helper_.addConfigModifier(

test/integration/base_integration_test.h

@@ -523,6 +523,7 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
 
   Network::DownstreamTransportSocketFactoryPtr
   createUpstreamTlsContext(const FakeUpstreamConfig& upstream_config);
+  testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context_;
   Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{timeSystem()};
 

test/integration/http_integration.cc

@@ -368,7 +368,7 @@ void HttpIntegrationTest::initialize() {
   // Needs to be instantiated before base class calls initialize() which starts a QUIC listener
   // according to the config.
   quic_transport_socket_factory_ = IntegrationUtil::createQuicUpstreamTransportSocketFactory(
-      *api_, stats_store_, context_manager_, san_to_match_);
+      *api_, stats_store_, context_manager_, thread_local_, san_to_match_);
 
   // Needed to config QUIC transport socket factory, and needs to be added before base class calls
   // initialize().

test/integration/quic_http_integration_test.cc

@@ -300,6 +300,8 @@ class QuicHttpIntegrationTestBase : public HttpIntegrationTest {
     ON_CALL(context.server_context_, api()).WillByDefault(testing::ReturnRef(*api_));
     ON_CALL(context, statsScope()).WillByDefault(testing::ReturnRef(stats_scope_));
     ON_CALL(context, sslContextManager()).WillByDefault(testing::ReturnRef(context_manager_));
+    ON_CALL(context.server_context_, threadLocal())
+        .WillByDefault(testing::ReturnRef(thread_local_));
     envoy::extensions::transport_sockets::quic::v3::QuicUpstreamTransport
         quic_transport_socket_config;
     auto* tls_context = quic_transport_socket_config.mutable_upstream_tls_context();
@@ -330,6 +332,60 @@ class QuicHttpIntegrationTestBase : public HttpIntegrationTest {
     }
   }
 
+  void testMultipleUpstreamQuicConnections() {
+    // As with testMultipleQuicConnections this function may be flaky when run with
+    // --runs_per_test=N where N > 1 but without --jobs=1.
+    setConcurrency(8);
+
+    // Avoid having to figure out which requests land on which connections by
+    // having one request per upstream connection.
+    setUpstreamProtocol(Http::CodecType::HTTP3);
+    envoy::config::listener::v3::QuicProtocolOptions options;
+    options.mutable_quic_protocol_options()->mutable_max_concurrent_streams()->set_value(1);
+    mergeOptions(options);
+
+    initialize();
+    std::vector<IntegrationCodecClientPtr> codec_clients;
+    std::vector<IntegrationStreamDecoderPtr> responses;
+    std::vector<FakeStreamPtr> upstream_requests;
+    std::vector<FakeHttpConnectionPtr> upstream_connections;
+
+    // Create |concurrency| clients
+    size_t num_requests = concurrency_ * 2;
+    for (size_t i = 1; i <= concurrency_; ++i) {
+      // See testMultipleQuicConnections for why this should result in connection spread.
+      designated_connection_ids_.push_back(quic::test::TestConnectionId(i << 32));
+      codec_clients.push_back(makeHttpConnection(lookupPort("http")));
+    }
+
+    // Create |num_requests| requests and wait for them to be received upstream
+    for (size_t i = 0; i < num_requests; ++i) {
+      responses.push_back(
+          codec_clients[i % concurrency_]->makeHeaderOnlyRequest(default_request_headers_));
+      waitForNextUpstreamRequest();
+      ASSERT(upstream_request_ != nullptr);
+      upstream_connections.push_back(std::move(fake_upstream_connection_));
+      upstream_requests.push_back(std::move(upstream_request_));
+    }
+
+    // Send |num_requests| responses as fast as possible to regression test
+    // against a prior credentials insert race.
+    for (size_t i = 0; i < num_requests; ++i) {
+      upstream_requests[i]->encodeHeaders(default_response_headers_, true);
+    }
+
+    // Wait for |num_requests| responses to complete.
+    for (size_t i = 0; i < num_requests; ++i) {
+      ASSERT_TRUE(responses[i]->waitForEndStream());
+      EXPECT_TRUE(responses[i]->complete());
+    }
+
+    // Close |concurrency| clients
+    for (size_t i = 0; i < concurrency_; ++i) {
+      codec_clients[i]->close();
+    }
+  }
+
   void testMultipleQuicConnections() {
     // Enabling SO_REUSEPORT with 8 workers. Unfortunately this setting makes the test rarely flaky
     // if it is configured to run with --runs_per_test=N where N > 1 but without --jobs=1.
@@ -373,11 +429,8 @@ class QuicHttpIntegrationTestBase : public HttpIntegrationTest {
     for (size_t i = 0; i < concurrency_; ++i) {
       fake_upstream_connection_ = nullptr;
       upstream_request_ = nullptr;
-      auto encoder_decoder =
-          codec_clients[i]->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
-                                                                        {":path", "/test/long/url"},
-                                                                        {":scheme", "http"},
-                                                                        {":authority", "host"}});
+      auto encoder_decoder = codec_clients[i]->startRequest(default_request_headers_);
+
       auto& request_encoder = encoder_decoder.first;
       auto response = std::move(encoder_decoder.second);
       codec_clients[i]->sendData(request_encoder, 1000, true);
@@ -693,7 +746,13 @@ TEST_P(QuicHttpIntegrationTest, EarlyDataDisabled) {
   codec_client_->close();
 }
 
-// Ensure multiple quic connections work, regardless of platform BPF support
+// Not only test multiple quic connections, but disconnect and reconnect to
+// trigger resumption.
+TEST_P(QuicHttpIntegrationTest, MultipleUpstreamQuicConnections) {
+  setUpstreamProtocol(Http::CodecType::HTTP3);
+  testMultipleUpstreamQuicConnections();
+}
+
 TEST_P(QuicHttpIntegrationTest, MultipleQuicConnectionsDefaultMode) {
   testMultipleQuicConnections();
 }

test/integration/utility.cc

@@ -142,11 +142,13 @@ class TestConnectionCallbacks : public Network::ConnectionCallbacks {
 Network::UpstreamTransportSocketFactoryPtr
 IntegrationUtil::createQuicUpstreamTransportSocketFactory(Api::Api& api, Stats::Store& store,
                                                           Ssl::ContextManager& context_manager,
+                                                          ThreadLocal::Instance& threadlocal,
                                                           const std::string& san_to_match) {
   NiceMock<Server::Configuration::MockTransportSocketFactoryContext> context;
   ON_CALL(context.server_context_, api()).WillByDefault(testing::ReturnRef(api));
   ON_CALL(context, statsScope()).WillByDefault(testing::ReturnRef(*store.rootScope()));
   ON_CALL(context, sslContextManager()).WillByDefault(testing::ReturnRef(context_manager));
+  ON_CALL(context.server_context_, threadLocal()).WillByDefault(testing::ReturnRef(threadlocal));
   envoy::extensions::transport_sockets::quic::v3::QuicUpstreamTransport
       quic_transport_socket_config;
   auto* tls_context = quic_transport_socket_config.mutable_upstream_tls_context();
@@ -236,9 +238,10 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt
   }
 
 #ifdef ENVOY_ENABLE_QUIC
+  testing::NiceMock<ThreadLocal::MockInstance> threadlocal;
   Extensions::TransportSockets::Tls::ContextManagerImpl manager(time_system);
   Network::UpstreamTransportSocketFactoryPtr transport_socket_factory =
-      createQuicUpstreamTransportSocketFactory(api, mock_stats_store, manager,
+      createQuicUpstreamTransportSocketFactory(api, mock_stats_store, manager, threadlocal,
                                                "spiffe://lyft.com/backend-team");
   auto& quic_transport_socket_factory =
       dynamic_cast<Quic::QuicClientTransportSocketFactory&>(*transport_socket_factory);