From 62ca8bd2b5960ed1c6ce2be97d3120cee719ecab Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Mon, 21 Jun 2021 13:31:56 -0400 Subject: [PATCH] http: forwarding transport failure details for CONNECT and ALPN connections (#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 --- envoy/tcp/conn_pool.h | 3 ++- source/common/tcp/conn_pool.h | 4 ++-- source/common/tcp/original_conn_pool.cc | 4 ++-- source/common/tcp_proxy/upstream.cc | 2 +- source/common/tcp_proxy/upstream.h | 1 + .../filters/network/dubbo_proxy/router/router_impl.cc | 1 + .../filters/network/dubbo_proxy/router/router_impl.h | 1 + .../filters/network/rocketmq_proxy/router/router_impl.cc | 1 + .../filters/network/rocketmq_proxy/router/router_impl.h | 1 + .../filters/network/thrift_proxy/router/router_impl.cc | 1 + .../filters/network/thrift_proxy/router/router_impl.h | 1 + source/extensions/upstreams/http/tcp/upstream_request.h | 3 ++- test/common/tcp/conn_pool_test.cc | 2 +- test/common/tcp_proxy/tcp_proxy_test_base.h | 2 +- test/extensions/upstreams/http/tcp/upstream_request_test.cc | 4 ++-- test/integration/tcp_conn_pool_integration_test.cc | 2 +- test/mocks/tcp/mocks.cc | 4 ++-- test/mocks/tcp/mocks.h | 3 ++- 18 files changed, 25 insertions(+), 15 deletions(-) diff --git a/envoy/tcp/conn_pool.h b/envoy/tcp/conn_pool.h index 14dd5677907f..6b31a7e6bc12 100644 --- 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 index e1208f720535..c8dea852f572 100644 --- 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(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 index 33796607c448..3820feeebf30 100644 --- 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 index 3e866bf94f83..c20d818b3632 100644 --- 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 index 4da9830209d9..3f2bfd9c6778 100644 --- 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 index 561c9eeda61c..4c64decc03bd 100644 --- 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 index 7c62e60fdab6..c67b7ab77871 100644 --- 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 index 5086f685d7f6..7c24db78482f 100644 --- 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 index 30e0cdbe07d6..38a61dcdfef2 100644 --- 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 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 index 7d8580b714e6..34a8d4486ad4 100644 --- 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 index 69d2e3a03d47..6fd9039dde43 100644 --- 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 index 7cdd070f041a..ce947b05f943 100644 --- 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 index 1399e589cc96..fab429254d5f 100644 --- 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 index cb1c62a9a59d..37676f5bbced 100644 --- 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 index 1c18cb0c0770..4ceaa9f3ddda 100644 --- 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 index a04f1f8f4d0e..faea32638e77 100644 --- 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 index 75b07cf8cd5f..d6828f046a14 100644 --- 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 index 04f0426eb303..6b486918cb99 100644 --- 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)); };