http: forwarding transport failure details for CONNECT and ALPN conne…

…ctions (#16975) Previously, L7 connections forwarded over TCP didn't get connection failure details (auto_http before codec assignment, CONNECT requests during the entire lifetime) but now they should. Risk Level: low Testing: unit test. Docs Changes: n/a Release Notes: n/a Fixes #/16881 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Jun 21, 2021 · 62ca8bd · 62ca8bd
1 parent ed81f0c
commit 62ca8bd
Show file tree

Hide file tree

Showing 18 changed files with 25 additions and 15 deletions.
diff --git a/envoy/tcp/conn_pool.h b/envoy/tcp/conn_pool.h
@@ -103,10 +103,11 @@ class Callbacks {
   /**
    * Called when a pool error occurred and no connection could be acquired for making the request.
    * @param reason supplies the failure reason.
+   * @param transport_failure_reason supplies the details of the transport failure reason.
    * @param host supplies the description of the host that caused the failure. This may be nullptr
    *             if no host was involved in the failure (for example overflow).
    */
-  virtual void onPoolFailure(PoolFailureReason reason,
+  virtual void onPoolFailure(PoolFailureReason reason, absl::string_view transport_failure_reason,
                              Upstream::HostDescriptionConstSharedPtr host) PURE;
 
   /**

diff --git a/source/common/tcp/conn_pool.h b/source/common/tcp/conn_pool.h
@@ -204,10 +204,10 @@ class ConnPoolImpl : public Envoy::ConnectionPool::ConnPoolImplBase,
   }
 
   void onPoolFailure(const Upstream::HostDescriptionConstSharedPtr& host_description,
-                     absl::string_view, ConnectionPool::PoolFailureReason reason,
+                     absl::string_view failure_reason, ConnectionPool::PoolFailureReason reason,
                      Envoy::ConnectionPool::AttachContext& context) override {
     auto* callbacks = typedContext<TcpAttachContext>(context).callbacks_;
-    callbacks->onPoolFailure(reason, host_description);
+    callbacks->onPoolFailure(reason, failure_reason, host_description);
   }
 
   // These two functions exist for testing parity between old and new Tcp Connection Pools.

diff --git a/source/common/tcp/original_conn_pool.cc b/source/common/tcp/original_conn_pool.cc
@@ -128,7 +128,7 @@ OriginalConnPoolImpl::newConnection(ConnectionPool::Callbacks& callbacks) {
     return pending_requests_.front().get();
   } else {
     ENVOY_LOG(debug, "max pending requests overflow");
-    callbacks.onPoolFailure(ConnectionPool::PoolFailureReason::Overflow, nullptr);
+    callbacks.onPoolFailure(ConnectionPool::PoolFailureReason::Overflow, "", nullptr);
     host_->cluster().stats().upstream_rq_pending_overflow_.inc();
     return nullptr;
   }
@@ -184,7 +184,7 @@ void OriginalConnPoolImpl::onConnectionEvent(ActiveConn& conn, Network::Connecti
         PendingRequestPtr request =
             pending_requests_to_purge.front()->removeFromList(pending_requests_to_purge);
         host_->cluster().stats().upstream_rq_pending_failure_eject_.inc();
-        request->callbacks_.onPoolFailure(reason, conn.real_host_description_);
+        request->callbacks_.onPoolFailure(reason, "", conn.real_host_description_);
       }
     }
 

diff --git a/source/common/tcp_proxy/upstream.cc b/source/common/tcp_proxy/upstream.cc
@@ -172,7 +172,7 @@ void TcpConnPool::newStream(GenericConnectionPoolCallbacks& callbacks) {
   }
 }
 
-void TcpConnPool::onPoolFailure(ConnectionPool::PoolFailureReason reason,
+void TcpConnPool::onPoolFailure(ConnectionPool::PoolFailureReason reason, absl::string_view,
                                 Upstream::HostDescriptionConstSharedPtr host) {
   upstream_handle_ = nullptr;
   callbacks_->onGenericPoolFailure(reason, host);

diff --git a/source/common/tcp_proxy/upstream.h b/source/common/tcp_proxy/upstream.h
@@ -29,6 +29,7 @@ class TcpConnPool : public GenericConnPool, public Tcp::ConnectionPool::Callback
 
   // Tcp::ConnectionPool::Callbacks
   void onPoolFailure(ConnectionPool::PoolFailureReason reason,
+                     absl::string_view transport_failure_reason,
                      Upstream::HostDescriptionConstSharedPtr host) override;
   void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data,
                    Upstream::HostDescriptionConstSharedPtr host) override;

diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc b/source/extensions/filters/network/dubbo_proxy/router/router_impl.cc
@@ -281,6 +281,7 @@ void Router::UpstreamRequest::encodeData(Buffer::Instance& data) {
 }
 
 void Router::UpstreamRequest::onPoolFailure(ConnectionPool::PoolFailureReason reason,
+                                            absl::string_view,
                                             Upstream::HostDescriptionConstSharedPtr host) {
   conn_pool_handle_ = nullptr;
 

diff --git a/source/extensions/filters/network/dubbo_proxy/router/router_impl.h b/source/extensions/filters/network/dubbo_proxy/router/router_impl.h
@@ -61,6 +61,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
 
     // Tcp::ConnectionPool::Callbacks
     void onPoolFailure(ConnectionPool::PoolFailureReason reason,
+                       absl::string_view transport_failure_reason,
                        Upstream::HostDescriptionConstSharedPtr host) override;
     void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn,
                      Upstream::HostDescriptionConstSharedPtr host) override;

diff --git a/source/extensions/filters/network/rocketmq_proxy/router/router_impl.cc b/source/extensions/filters/network/rocketmq_proxy/router/router_impl.cc
@@ -166,6 +166,7 @@ void RouterImpl::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDat
 }
 
 void RouterImpl::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason,
+                                                absl::string_view,
                                                 Upstream::HostDescriptionConstSharedPtr host) {
   if (router_.handle_) {
     ENVOY_LOG(trace, "#onPoolFailure, reset cancellable handle to nullptr");

diff --git a/source/extensions/filters/network/rocketmq_proxy/router/router_impl.h b/source/extensions/filters/network/rocketmq_proxy/router/router_impl.h
@@ -41,6 +41,7 @@ class RouterImpl : public Router, public Logger::Loggable<Logger::Id::rocketmq>
     UpstreamRequest(RouterImpl& router);
 
     void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason,
+                       absl::string_view transport_failure_reason,
                        Upstream::HostDescriptionConstSharedPtr host) override;
 
     void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn,

diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc
@@ -483,6 +483,7 @@ void Router::UpstreamRequest::releaseConnection(const bool close) {
 void Router::UpstreamRequest::resetStream() { releaseConnection(true); }
 
 void Router::UpstreamRequest::onPoolFailure(ConnectionPool::PoolFailureReason reason,
+                                            absl::string_view,
                                             Upstream::HostDescriptionConstSharedPtr host) {
   conn_pool_handle_ = nullptr;
 

diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h
@@ -234,6 +234,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
 
     // Tcp::ConnectionPool::Callbacks
     void onPoolFailure(ConnectionPool::PoolFailureReason reason,
+                       absl::string_view transport_failure_reason,
                        Upstream::HostDescriptionConstSharedPtr host) override;
     void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn,
                      Upstream::HostDescriptionConstSharedPtr host) override;

diff --git a/source/extensions/upstreams/http/tcp/upstream_request.h b/source/extensions/upstreams/http/tcp/upstream_request.h
@@ -49,9 +49,10 @@ class TcpConnPool : public Router::GenericConnPool, public Envoy::Tcp::Connectio
 
   // Tcp::ConnectionPool::Callbacks
   void onPoolFailure(ConnectionPool::PoolFailureReason reason,
+                     absl::string_view transport_failure_reason,
                      Upstream::HostDescriptionConstSharedPtr host) override {
     upstream_handle_ = nullptr;
-    callbacks_->onPoolFailure(reason, "", host);
+    callbacks_->onPoolFailure(reason, transport_failure_reason, host);
   }
 
   void onPoolReady(Envoy::Tcp::ConnectionPool::ConnectionDataPtr&& conn_data,

diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc
@@ -59,7 +59,7 @@ struct ConnPoolCallbacks : public Tcp::ConnectionPool::Callbacks {
     pool_ready_.ready();
   }
 
-  void onPoolFailure(ConnectionPool::PoolFailureReason reason,
+  void onPoolFailure(ConnectionPool::PoolFailureReason reason, absl::string_view,
                      Upstream::HostDescriptionConstSharedPtr host) override {
     reason_ = reason;
     host_ = host;

diff --git a/test/common/tcp_proxy/tcp_proxy_test_base.h b/test/common/tcp_proxy/tcp_proxy_test_base.h
@@ -150,7 +150,7 @@ class TcpProxyTestBase : public testing::Test {
 
   void raiseEventUpstreamConnectFailed(uint32_t conn_index,
                                        ConnectionPool::PoolFailureReason reason) {
-    conn_pool_callbacks_.at(conn_index)->onPoolFailure(reason, upstream_hosts_.at(conn_index));
+    conn_pool_callbacks_.at(conn_index)->onPoolFailure(reason, "", upstream_hosts_.at(conn_index));
   }
 
   Tcp::ConnectionPool::Cancellable* onNewConnection(Tcp::ConnectionPool::Cancellable* connection) {

diff --git a/test/extensions/upstreams/http/tcp/upstream_request_test.cc b/test/extensions/upstreams/http/tcp/upstream_request_test.cc
@@ -68,9 +68,9 @@ TEST_F(TcpConnPoolTest, OnPoolFailure) {
   EXPECT_CALL(mock_pool_, newConnection(_)).WillOnce(Return(&cancellable_));
   conn_pool_->newStream(&mock_generic_callbacks_);
 
-  EXPECT_CALL(mock_generic_callbacks_, onPoolFailure(_, _, _));
+  EXPECT_CALL(mock_generic_callbacks_, onPoolFailure(_, "foo", _));
   conn_pool_->onPoolFailure(Envoy::Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure,
-                            host_);
+                            "foo", host_);
 
   // Make sure that the pool failure nulled out the pending request.
   EXPECT_FALSE(conn_pool_->cancelAnyPendingStream());

diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc
@@ -47,7 +47,7 @@ class TestFilter : public Network::ReadFilter {
     Request(TestFilter& parent, Buffer::Instance& data) : parent_(parent) { data_.move(data); }
 
     // Tcp::ConnectionPool::Callbacks
-    void onPoolFailure(ConnectionPool::PoolFailureReason,
+    void onPoolFailure(ConnectionPool::PoolFailureReason, absl::string_view,
                        Upstream::HostDescriptionConstSharedPtr) override {
       ASSERT(false);
     }

diff --git a/test/mocks/tcp/mocks.cc b/test/mocks/tcp/mocks.cc
@@ -41,9 +41,9 @@ void MockInstance::poolFailure(PoolFailureReason reason, bool host_null) {
   callbacks_.pop_front();
   handles_.pop_front();
   if (host_null) {
-    cb->onPoolFailure(reason, nullptr);
+    cb->onPoolFailure(reason, "", nullptr);
   } else {
-    cb->onPoolFailure(reason, host_);
+    cb->onPoolFailure(reason, "", host_);
   }
 }
 

diff --git a/test/mocks/tcp/mocks.h b/test/mocks/tcp/mocks.h
@@ -17,7 +17,8 @@ namespace ConnectionPool {
 
 class MockCallbacks : public Callbacks {
   MOCK_METHOD(void, onPoolFailure,
-              (PoolFailureReason reason, Upstream::HostDescriptionConstSharedPtr host));
+              (PoolFailureReason reason, absl::string_view details,
+               Upstream::HostDescriptionConstSharedPtr host));
   MOCK_METHOD(void, onPoolReady,
               (ConnectionDataPtr && conn, Upstream::HostDescriptionConstSharedPtr host));
 };