Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 <alyssar@chromium.org>
  • Loading branch information
alyssawilk committed Jun 21, 2021
1 parent ed81f0c commit 62ca8bd
Show file tree
Hide file tree
Showing 18 changed files with 25 additions and 15 deletions.
3 changes: 2 additions & 1 deletion envoy/tcp/conn_pool.h
Expand Up @@ -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;

/**
Expand Down
4 changes: 2 additions & 2 deletions source/common/tcp/conn_pool.h
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions source/common/tcp/original_conn_pool.cc
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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_);
}
}

Expand Down
2 changes: 1 addition & 1 deletion source/common/tcp_proxy/upstream.cc
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions source/common/tcp_proxy/upstream.h
Expand Up @@ -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;
Expand Down
Expand Up @@ -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;

Expand Down
Expand Up @@ -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;
Expand Down
Expand Up @@ -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");
Expand Down
Expand Up @@ -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,
Expand Down
Expand Up @@ -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;

Expand Down
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion source/extensions/upstreams/http/tcp/upstream_request.h
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion test/common/tcp/conn_pool_test.cc
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion test/common/tcp_proxy/tcp_proxy_test_base.h
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions test/extensions/upstreams/http/tcp/upstream_request_test.cc
Expand Up @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion test/integration/tcp_conn_pool_integration_test.cc
Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions test/mocks/tcp/mocks.cc
Expand Up @@ -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_);
}
}

Expand Down
3 changes: 2 additions & 1 deletion test/mocks/tcp/mocks.h
Expand Up @@ -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));
};
Expand Down

0 comments on commit 62ca8bd

Please sign in to comment.