grid: Propagate transport failure reason of both connection pools (envoyproxy#39253)

Previously, if the TCP pool failed before the HTTP/3 pool failed, the
`ConnectivityGrid` would propagate the transport failure reason for both
the TCP and QUIC pools. However, if the QUIC pool failed first, the
`ConnectivityGrid` would not propagate the transport failure reason for
the QUIC pool (only for the TCP pool).

This change ensures that the `ConnectivityGrid` propagates the transport
failure reason for *both* TCP and QUIC pools if both pools fail in the
`ConnectivityGrid`, regardless of the order in which the pools fail.

---------

Signed-off-by: Ali Beyad <[email protected]>

Author: abeyad

source/common/http/conn_pool_grid.cc

@@ -140,11 +140,9 @@ void ConnectivityGrid::WrapperCallbacks::onConnectionAttemptFailed(
 
   // If there is another connection attempt in flight then let that proceed.
   if (!connection_attempts_.empty()) {
-    if (!grid_.isPoolHttp3(attempt->pool())) {
-      // TCP pool failed before HTTP/3 pool.
-      prev_tcp_pool_failure_reason_ = reason;
-      prev_tcp_pool_transport_failure_reason_ = transport_failure_reason;
-    }
+    prev_pool_failure_reason_ = reason;
+    prev_pool_transport_failure_reason_ = fmt::format(
+        "{}: {}", grid_.isPoolHttp3(attempt->pool()) ? "QUIC" : "TCP", transport_failure_reason);
     return;
   }
 
@@ -167,12 +165,12 @@ void ConnectivityGrid::WrapperCallbacks::signalFailureAndDeleteSelf(
   if (callbacks != nullptr) {
     ENVOY_LOG(trace, "Passing pool failure up to caller.");
     std::string failure_str;
-    if (prev_tcp_pool_failure_reason_.has_value()) {
-      // TCP pool failed early on, log its error details as well.
-      failure_str = fmt::format("{} (with earlier TCP attempt failure reason {}, {})",
-                                transport_failure_reason,
-                                static_cast<int>(prev_tcp_pool_failure_reason_.value()),
-                                prev_tcp_pool_transport_failure_reason_);
+    if (prev_pool_failure_reason_.has_value()) {
+      // The other pool (either TCP or QUIC depending on which failed first) also failed, log its
+      // error details as well.
+      failure_str = fmt::format(
+          "{} (with earlier attempt failure reason {}, {})", transport_failure_reason,
+          static_cast<int>(prev_pool_failure_reason_.value()), prev_pool_transport_failure_reason_);
       transport_failure_reason = failure_str;
     }
     callbacks->onPoolFailure(reason, transport_failure_reason, host);

source/common/http/conn_pool_grid.h

@@ -172,8 +172,8 @@ class ConnectivityGrid : public ConnectionPool::Instance,
     bool tcp_attempt_succeeded_{};
     // Latch the passed-in stream options.
     const Instance::StreamOptions stream_options_{};
-    absl::optional<ConnectionPool::PoolFailureReason> prev_tcp_pool_failure_reason_;
-    std::string prev_tcp_pool_transport_failure_reason_;
+    absl::optional<ConnectionPool::PoolFailureReason> prev_pool_failure_reason_;
+    std::string prev_pool_transport_failure_reason_;
   };
   using WrapperCallbacksPtr = std::unique_ptr<WrapperCallbacks>;
 

test/common/http/conn_pool_grid_test.cc

@@ -783,7 +783,7 @@ TEST_F(ConnectivityGridTest, ParallelConnectionsTcpFailsFirst) {
   EXPECT_FALSE(grid_->isHttp3Broken());
   // Only the previous TCP failure details will be appended.
   EXPECT_EQ(callbacks_.transport_failure_reason_,
-            "handshake timeout2 (with earlier TCP attempt failure reason 1, TCP failure details)");
+            "handshake timeout2 (with earlier attempt failure reason 1, TCP: TCP failure details)");
 }
 
 // Test the first pool failing inline but http/3 happy eyeballs succeeding inline
@@ -1011,7 +1011,47 @@ TEST_F(ConnectivityGridTest, TcpFailsFollowedByH3Failure) {
   EXPECT_FALSE(grid_->isHttp3Broken());
   EXPECT_TRUE(alternate_protocols_->findAlternatives(origin_).has_value());
   EXPECT_EQ(callbacks_.transport_failure_reason_,
-            "handshake time out (with earlier TCP attempt failure reason 1, network unreachable)");
+            "handshake time out (with earlier attempt failure reason 1, TCP: network unreachable)");
+}
+
+// Tests one HTTP/2 pool and one HTTP/3 pool connecting and both fail with HTTP/3 failing first.
+TEST_F(ConnectivityGridTest, H3FailsFirstFollowedByTcpFailure) {
+  initialize();
+  addHttp3AlternateProtocol();
+  EXPECT_EQ(grid_->http3Pool(), nullptr);
+
+  // This timer will be returned and armed as the grid creates the wrapper's
+  // failover timer.
+  Event::MockTimer* failover_timer = new NiceMock<MockTimer>(&dispatcher_);
+
+  grid_->newStream(decoder_, callbacks_, {/*can_send_early_data_=*/false, /*can_use_http3_=*/true});
+  EXPECT_NE(grid_->http3Pool(), nullptr);
+  EXPECT_TRUE(failover_timer->enabled_);
+
+  // Kick off the second connection.
+  failover_timer->invokeCallback();
+  EXPECT_NE(grid_->http2Pool(), nullptr);
+
+  // HTTP/3 pool fails first. Failure shouldn't be propagated to the original caller, but instead
+  // wait for the HTTP/2 pool to finish.
+  EXPECT_NE(grid_->callbacks(0), nullptr);
+  EXPECT_CALL(callbacks_.pool_failure_, ready()).Times(0);
+  grid_->callbacks(0)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure,
+                                     "network unreachable", host_);
+  EXPECT_FALSE(grid_->isHttp3Broken());
+
+  // HTTP/2 pool fails.
+  EXPECT_NE(grid_->callbacks(1), nullptr);
+  EXPECT_CALL(callbacks_.pool_failure_, ready());
+  grid_->callbacks(1)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure,
+                                     "handshake time out", host_);
+
+  // HTTP/3 shouldn't be marked broken as TCP also failed.
+  EXPECT_FALSE(grid_->isHttp3Broken());
+  EXPECT_TRUE(alternate_protocols_->findAlternatives(origin_).has_value());
+  EXPECT_EQ(
+      callbacks_.transport_failure_reason_,
+      "handshake time out (with earlier attempt failure reason 1, QUIC: network unreachable)");
 }
 
 // Test both connections happening in parallel and the second connecting before