diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 4ffb4644177d..429c90284172 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -13,6 +13,7 @@ Bug Fixes * data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. * jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header. +* tcp_proxy: fix a crash that occurs when configured for :ref:`upstream tunneling ` and the downstream connection disconnects while the the upstream connection or http/2 stream is still being established. Removed Config or Runtime ------------------------- diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index fb76f6a942c7..bdf197e36584 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -589,6 +589,11 @@ Network::FilterStatus Filter::onNewConnection() { } void Filter::onDownstreamEvent(Network::ConnectionEvent event) { + if (event == Network::ConnectionEvent::LocalClose || + event == Network::ConnectionEvent::RemoteClose) { + downstream_closed_ = true; + } + if (upstream_) { Tcp::ConnectionPool::ConnectionDataPtr conn_data(upstream_->onDownstreamEvent(event)); if (conn_data != nullptr && @@ -637,7 +642,9 @@ void Filter::onUpstreamEvent(Network::ConnectionEvent event) { Upstream::Outlier::Result::LocalOriginConnectFailed); } - initializeUpstreamConnection(); + if (!downstream_closed_) { + initializeUpstreamConnection(); + } } else { if (read_callbacks_->connection().state() == Network::Connection::State::Open) { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index 6579995d1340..4d9b9f9e7da2 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -384,6 +384,7 @@ class Filter : public Network::ReadFilter, Network::Socket::OptionsSharedPtr upstream_options_; uint32_t connect_attempts_{}; bool connecting_{}; + bool downstream_closed_{}; }; // This class deals with an upstream connection that needs to finish flushing, when the downstream diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index 93ace7f7f52f..5ac9c60630da 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -561,6 +561,69 @@ TEST_P(TcpTunnelingIntegrationTest, ResetStreamTest) { tcp_client->waitForDisconnect(); } +TEST_P(TcpTunnelingIntegrationTest, UpstreamConnectingDownstreamDisconnect) { + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + return; + } + +#if defined(WIN32) + // TODO(ggreenway): figure out why this test fails on Windows and remove this disable. + // Failing tests: + // IpAndHttpVersions/TcpTunnelingIntegrationTest.UpstreamConnectingDownstreamDisconnect/IPv4_HttpDownstream_Http3UpstreamBareHttp2, + // IpAndHttpVersions/TcpTunnelingIntegrationTest.UpstreamConnectingDownstreamDisconnect/IPv6_HttpDownstream_Http2UpstreamWrappedHttp2, + // Times out at the end of the test on `ASSERT_TRUE(upstream_request_->waitForReset());`. + return; +#endif + + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy proxy_config; + proxy_config.set_stat_prefix("tcp_stats"); + proxy_config.set_cluster("cluster_0"); + proxy_config.mutable_tunneling_config()->set_hostname("host.com:80"); + + // Enable retries. The crash is due to retrying after the downstream connection is closed, which + // can't occur if retries are not enabled. + proxy_config.mutable_max_connect_attempts()->set_value(2); + + auto* listeners = bootstrap.mutable_static_resources()->mutable_listeners(); + for (auto& listener : *listeners) { + if (listener.name() != "tcp_proxy") { + continue; + } + auto* filter_chain = listener.mutable_filter_chains(0); + auto* filter = filter_chain->mutable_filters(0); + filter->mutable_typed_config()->PackFrom(proxy_config); + + // Use TLS because it will respond to a TCP half-close during handshake by closing the + // connection. + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + ConfigHelper::initializeTls({}, *tls_context.mutable_common_tls_context()); + filter_chain->mutable_transport_socket()->set_name("envoy.transport_sockets.tls"); + filter_chain->mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context); + + break; + } + }); + + enableHalfClose(false); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + + // Wait for the request for a connection, but don't send a response back yet. This ensures that + // tcp_proxy is stuck in `connecting_`. + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + + // Close the client connection. The TLS transport socket will detect this even while + // `readDisable(true)` on the connection, and will raise a `RemoteClose` event. + tcp_client->close(); + + ASSERT_TRUE(upstream_request_->waitForReset()); + ASSERT_TRUE(fake_upstream_connection_->close()); +} + TEST_P(TcpTunnelingIntegrationTest, TestIdletimeoutWithLargeOutstandingData) { enableHalfClose(false); config_helper_.setBufferLimits(1024, 1024);