From b64f67c089d2b2d9c97731d28d149221a7353b4e Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 31 Aug 2018 16:24:00 -0700 Subject: [PATCH 1/8] Fix crash in tcp_proxy. Closing the upstream connection is not safe from the Filter destructor, because it triggers events back into the downstream connection, which is partially destructed. Ensure that the upstream connection is closed before the destructor is called. Fixes #4310 Signed-off-by: Greg Greenway --- source/common/tcp_proxy/tcp_proxy.cc | 31 ++-- test/config/utility.cc | 8 +- test/config/utility.h | 7 +- test/integration/fake_upstream.cc | 2 +- test/integration/ssl_integration_test.cc | 2 +- .../integration/tcp_proxy_integration_test.cc | 164 +++++++++++++++++- 6 files changed, 194 insertions(+), 20 deletions(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index c491fa94c04e..073c769a11db 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -135,13 +135,8 @@ Filter::~Filter() { access_log->log(nullptr, nullptr, nullptr, getRequestInfo()); } - if (upstream_handle_) { - upstream_handle_->cancel(); - } - - if (upstream_conn_data_) { - upstream_conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); - } + ASSERT(upstream_handle_ == nullptr); + ASSERT(upstream_conn_data_ == nullptr); } TcpProxyStats Config::SharedConfig::generateStats(Stats::Scope& scope) { @@ -406,17 +401,29 @@ void Filter::onDownstreamEvent(Network::ConnectionEvent event) { if (event == Network::ConnectionEvent::RemoteClose) { upstream_conn_data_->connection().close(Network::ConnectionCloseType::FlushWrite); - if (upstream_conn_data_ != nullptr && - upstream_conn_data_->connection().state() != Network::Connection::State::Closed) { - config_->drainManager().add(config_->sharedConfig(), std::move(upstream_conn_data_), - std::move(upstream_callbacks_), std::move(idle_timer_), - read_callbacks_->upstreamHost()); + // Events raised from the previous line may cause upstream_conn_data_ to be NULL if + // it was able to immediately flush all data. + + if (upstream_conn_data_ != nullptr) { + if (upstream_conn_data_->connection().state() != Network::Connection::State::Closed) { + config_->drainManager().add(config_->sharedConfig(), std::move(upstream_conn_data_), + std::move(upstream_callbacks_), std::move(idle_timer_), + read_callbacks_->upstreamHost()); + } else { + upstream_conn_data_.reset(); + } } } else if (event == Network::ConnectionEvent::LocalClose) { upstream_conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); upstream_conn_data_.reset(); disableIdleTimer(); } + } else if (upstream_handle_) { + if (event == Network::ConnectionEvent::LocalClose || + event == Network::ConnectionEvent::RemoteClose) { + upstream_handle_->cancel(); + upstream_handle_ = nullptr; + } } } diff --git a/test/config/utility.cc b/test/config/utility.cc index 2cfdca3098e1..16cbc610f91d 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -356,7 +356,7 @@ void ConfigHelper::setClientCodec( } } -void ConfigHelper::addSslConfig() { +void ConfigHelper::addTlsListenerConfig() { RELEASE_ASSERT(!finalized_, ""); auto* filter_chain = @@ -381,6 +381,12 @@ void ConfigHelper::addSslConfig() { TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); } +void ConfigHelper::addTlsUpstreamConfig() { + RELEASE_ASSERT(!finalized_, ""); + auto* cluster = bootstrap_.mutable_static_resources()->mutable_clusters(0); + cluster->mutable_tls_context()->set_sni("example.com"); +} + void ConfigHelper::renameListener(const std::string& name) { auto* static_resources = bootstrap_.mutable_static_resources(); if (static_resources->listeners_size() > 0) { diff --git a/test/config/utility.h b/test/config/utility.h index a5c2982f1679..886b5b5c8b1a 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -88,8 +88,11 @@ class ConfigHelper { envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::CodecType type); - // Add the default SSL configuration. - void addSslConfig(); + // Add the default TLS listener configuration. + void addTlsListenerConfig(); + + // Add the default TLS upstream configuration. + void addTlsUpstreamConfig(); // Renames the first listener to the name specified. void renameListener(const std::string& name); diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index ef3ee25532fd..bd940c9f39c9 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -536,7 +536,7 @@ AssertionResult FakeRawConnection::write(const std::string& data, bool end_strea Network::FilterStatus FakeRawConnection::ReadFilter::onData(Buffer::Instance& data, bool end_stream) { Thread::LockGuard lock(parent_.lock_); - ENVOY_LOG(debug, "got {} bytes", data.length()); + ENVOY_LOG(debug, "got {} bytes, end_stream {}", data.length(), end_stream); parent_.data_.append(data.toString()); parent_.half_closed_ = end_stream; data.drain(data.length()); diff --git a/test/integration/ssl_integration_test.cc b/test/integration/ssl_integration_test.cc index f72f0e7c518c..38f6243466a2 100644 --- a/test/integration/ssl_integration_test.cc +++ b/test/integration/ssl_integration_test.cc @@ -28,7 +28,7 @@ namespace Envoy { namespace Ssl { void SslIntegrationTest::initialize() { - config_helper_.addSslConfig(); + config_helper_.addTlsListenerConfig(); HttpIntegrationTest::initialize(); runtime_.reset(new NiceMock()); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index c1d2305eb65c..dc11a64c0d0f 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -2,6 +2,7 @@ #include "envoy/config/accesslog/v2/file.pb.h" #include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.validate.h" +#include "envoy/server/transport_socket_config.h" #include "common/filesystem/filesystem_impl.h" #include "common/network/utility.h" @@ -9,7 +10,7 @@ #include "test/integration/ssl_utility.h" #include "test/integration/utility.h" - +#include "test/test_common/logging.h" #include "gtest/gtest.h" using testing::_; @@ -20,6 +21,74 @@ using testing::NiceMock; namespace Envoy { namespace { +// Transport socket that does not raise a Connected event until it has +// received a message from the other side indicating that it should be connected. +class DelayConnectSocket : public Network::RawBufferSocket { + // Network::TransportSocket + void onConnected() override { + // Delay raising the event. + } + Network::IoResult doRead(Buffer::Instance& buffer) override { + Network::IoResult result = Network::RawBufferSocket::doRead(buffer); + if (buffer.length() > 0 && !raised_connected_) { + raised_connected_ = true; + Network::RawBufferSocket::onConnected(); + } + + if (result.end_stream_read_ && !raised_connected_) { + result.end_stream_read_ = false; + result.action_ = Network::PostIoAction::Close; + } + + return result; + } + +private: + bool raised_connected_{false}; +}; + +class DelayConnectSocketFactory : public Network::TransportSocketFactory { +public: + Network::TransportSocketPtr createTransportSocket() const override { + return std::make_unique(); + } + bool implementsSecureTransport() const override { return false; } +}; + +class DelayConnectSocketConfigFactory + : public Server::Configuration::UpstreamTransportSocketConfigFactory, + public Server::Configuration::DownstreamTransportSocketConfigFactory { +public: + static constexpr const char* NAME = "DELAY_CONNECT"; + std::string name() const override { return NAME; } + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return std::make_unique(); + } + + // Server::Configuration::UpstreamTransportSocketConfigFactory + Network::TransportSocketFactoryPtr + createTransportSocketFactory(const Protobuf::Message&, + Server::Configuration::TransportSocketFactoryContext&) override { + return std::make_unique(); + } + + // Server::Configuration::DownstreamTransportSocketConfigFactory + Network::TransportSocketFactoryPtr + createTransportSocketFactory(const Protobuf::Message&, + Server::Configuration::TransportSocketFactoryContext&, + const std::vector&) override { + return std::make_unique(); + } +}; + +Registry::RegisterFactory + upstream_registered_; + +Registry::RegisterFactory + downstream_registered_; + INSTANTIATE_TEST_CASE_P(IpVersions, TcpProxyIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -355,12 +424,62 @@ TEST_P(TcpProxyIntegrationTest, TestIdletimeoutWithLargeOutstandingData) { ASSERT_TRUE(fake_upstream_connection->waitForDisconnect(true)); } +// Test that a downstream disconnect while the upstream connection is not yet connected +// successfully cleans up the upstream connection. +TEST_P(TcpProxyIntegrationTest, TestDownstreamDisconnectWithUpstreamConnectionInProgress) { + LogLevelSetter setter(spdlog::level::trace); + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void { + bootstrap.mutable_static_resources() + ->mutable_listeners(0) + ->mutable_filter_chains(0) + ->mutable_transport_socket() + ->set_name(DelayConnectSocketConfigFactory::NAME); + bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_transport_socket()->set_name( + DelayConnectSocketConfigFactory::NAME); + }); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + + FakeRawConnectionPtr fake_upstream_connection; + AssertionResult result = fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection); + RELEASE_ASSERT(result, result.message()); + + tcp_client->close(); + std::string cx_active_stat_name; + for (auto& gauge : test_server_->gauges()) { + if (std::regex_match(gauge->name(), + std::regex(R"EOF(^listener\..*_0\.downstream_cx_active$)EOF"))) { + cx_active_stat_name = gauge->name(); + } + } + RELEASE_ASSERT(!cx_active_stat_name.empty(), "Must find the stat we're waiting for"); + ENVOY_LOG_MISC(debug, "Starting wait for downstream conn closed"); + test_server_->waitForGaugeEq(cx_active_stat_name, 0); + ENVOY_LOG_MISC(debug, "Finished wait for downstream conn closed"); + // test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_pending_active", 0); + + // This establishes the DELAY_CONNECT connection. + fake_upstream_connection->write("a", false); + + // The upstream connection was established after the downstream connection was closed, + // so it stayed in the connpool and should be used for this 2nd downstream connection. + IntegrationTcpClientPtr tcp_client2 = makeTcpConnection(lookupPort("tcp_proxy")); + tcp_client2->write("foo", true); + ASSERT_TRUE(fake_upstream_connection->waitForData(3)); + ASSERT_TRUE(fake_upstream_connection->waitForHalfClose()); + fake_upstream_connection->write("", true); + tcp_client2->waitForData("ab"); + tcp_client2->waitForDisconnect(); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); +} + INSTANTIATE_TEST_CASE_P(IpVersions, TcpProxySslIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); void TcpProxySslIntegrationTest::initialize() { - config_helper_.addSslConfig(); + config_helper_.addTlsListenerConfig(); TcpProxyIntegrationTest::initialize(); context_manager_.reset(new Ssl::ContextManagerImpl(runtime_)); @@ -455,7 +574,7 @@ TEST_P(TcpProxySslIntegrationTest, DownstreamHalfClose) { Buffer::OwnedImpl empty_buffer; ssl_client_->write(empty_buffer, true); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); - ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); + ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose(true)); const std::string data("data"); ASSERT_TRUE(fake_upstream_connection_->write(data, false)); @@ -493,5 +612,44 @@ TEST_P(TcpProxySslIntegrationTest, UpstreamHalfClose) { ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); } +// Test that upstream connection is properly closed if the downstream +// connection aborts the handshake. +TEST_P(TcpProxySslIntegrationTest, AbortTlsHandshake) { + initialize(); + + // Connect to a TLS listener with a raw client to ensure that the handshake doesn't + // succeed. + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + + AssertionResult result = fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_); + RELEASE_ASSERT(result, result.message()); + + tcp_client->close(); + ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); +} + +// Test that upstream connection is properly closed if the downstream +// TLS handshake fails. +TEST_P(TcpProxySslIntegrationTest, FailTlsHandshake) { + initialize(); + + // Connect to a TLS listener with a raw client to ensure that the handshake doesn't + // succeed. + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + + AssertionResult result = fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_); + RELEASE_ASSERT(result, result.message()); + + // Write garbage so that the remote TLS server will fail the handshake. + tcp_client->write(std::string(10000, 'a'), false); + + ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); + fake_upstream_connection_->close(); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + tcp_client->close(); +} + } // namespace } // namespace Envoy From a02bc7e29468eec880c728b87ac1ea578bd7c9f7 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 14:56:51 -0700 Subject: [PATCH 2/8] fix tests Signed-off-by: Greg Greenway --- test/common/http/conn_manager_impl_test.cc | 15 +++++++++++++++ .../network/filter_manager_impl_test.cc | 2 ++ test/common/tcp_proxy/tcp_proxy_test.cc | 15 ++++++++++++++- .../integration/tcp_proxy_integration_test.cc | 19 ++++--------------- test/integration/xfcc_integration_test.cc | 2 +- 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 90e6e21f45a3..95aad5f19c2a 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1705,6 +1705,10 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); + Tcp::ConnectionPool::UpstreamCallbacks* upstream_callbacks = nullptr; + EXPECT_CALL(*conn_pool_.connection_data_, addUpstreamCallbacks(_)) + .WillOnce( + Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) { upstream_callbacks = &cb; })); conn_pool_.host_->hostname_ = "newhost"; conn_pool_.poolReady(upstream_conn_); @@ -1715,6 +1719,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { EXPECT_EQ(0U, stats_.named_.downstream_cx_http1_active_.value()); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); + upstream_callbacks->onEvent(Network::ConnectionEvent::RemoteClose); conn_manager_.reset(); EXPECT_EQ(0U, stats_.named_.downstream_cx_websocket_active_.value()); } @@ -1753,9 +1758,14 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyData) { EXPECT_CALL(upstream_conn_, write(_, false)); EXPECT_CALL(upstream_conn_, write(BufferEqual(&early_data), false)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); + Tcp::ConnectionPool::UpstreamCallbacks* upstream_callbacks = nullptr; + EXPECT_CALL(*conn_pool_.connection_data_, addUpstreamCallbacks(_)) + .WillOnce( + Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) { upstream_callbacks = &cb; })); conn_pool_.poolReady(upstream_conn_); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); + upstream_callbacks->onEvent(Network::ConnectionEvent::RemoteClose); conn_manager_.reset(); } @@ -1828,8 +1838,13 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyEndStream) { EXPECT_CALL(upstream_conn_, write(_, false)); EXPECT_CALL(upstream_conn_, write(_, true)).Times(0); + Tcp::ConnectionPool::UpstreamCallbacks* upstream_callbacks = nullptr; + EXPECT_CALL(*conn_pool_.connection_data_, addUpstreamCallbacks(_)) + .WillOnce( + Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) { upstream_callbacks = &cb; })); conn_pool_.poolReady(upstream_conn_); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); + upstream_callbacks->onEvent(Network::ConnectionEvent::RemoteClose); conn_manager_.reset(); } diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index 06837dfbd450..8aeaf11a978b 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -214,6 +214,8 @@ TEST_F(NetworkFilterManagerTest, RateLimitAndTcpProxy) { EXPECT_CALL(upstream_connection, write(BufferEqual(&buffer), _)); read_buffer_.add("hello"); manager.onRead(); + + connection.raiseEvent(ConnectionEvent::RemoteClose); } } // namespace Network diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index f447f62b08e3..218c72f308b6 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -348,6 +348,12 @@ class TcpProxyTest : public testing::Test { .WillByDefault(SaveArg<0>(&access_log_data_)); } + ~TcpProxyTest() { + if (filter_ != nullptr) { + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + } + } + void configure(const envoy::config::filter::network::tcp_proxy::v2::TcpProxy& config) { config_.reset(new Config(config, factory_context_)); } @@ -873,6 +879,7 @@ TEST_F(TcpProxyTest, IdleTimeoutWithOutstandingDataFlushed) { TEST_F(TcpProxyTest, AccessLogUpstreamHost) { setup(1, accessLogConfig("%UPSTREAM_HOST% %UPSTREAM_CLUSTER%")); raiseEventUpstreamConnected(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); filter_.reset(); EXPECT_EQ(access_log_data_, "127.0.0.1:80 fake_cluster"); } @@ -881,6 +888,7 @@ TEST_F(TcpProxyTest, AccessLogUpstreamHost) { TEST_F(TcpProxyTest, AccessLogUpstreamLocalAddress) { setup(1, accessLogConfig("%UPSTREAM_LOCAL_ADDRESS%")); raiseEventUpstreamConnected(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); filter_.reset(); EXPECT_EQ(access_log_data_, "2.2.2.2:50000"); } @@ -893,6 +901,7 @@ TEST_F(TcpProxyTest, AccessLogDownstreamAddress) { filter_callbacks_.connection_.remote_address_ = Network::Utility::resolveUrl("tcp://1.1.1.1:40000"); setup(1, accessLogConfig("%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT% %DOWNSTREAM_LOCAL_ADDRESS%")); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); filter_.reset(); EXPECT_EQ(access_log_data_, "1.1.1.1 1.1.1.2:20000"); } @@ -1075,6 +1084,9 @@ TEST_F(TcpProxyRoutingTest, NonRoutableConnection) { EXPECT_EQ(total_cx + 1, config_->stats().downstream_cx_total_.value()); EXPECT_EQ(non_routable_cx + 1, config_->stats().downstream_cx_no_route_.value()); + + // Cleanup + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); } TEST_F(TcpProxyRoutingTest, RoutableConnection) { @@ -1087,7 +1099,8 @@ TEST_F(TcpProxyRoutingTest, RoutableConnection) { connection_.local_address_ = std::make_shared("1.2.3.4", 9999); // Expect filter to try to open a connection to specified cluster. - EXPECT_CALL(factory_context_.cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)); + EXPECT_CALL(factory_context_.cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) + .WillOnce(Return(nullptr)); filter_->onNewConnection(); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index dc11a64c0d0f..97a96d0341d1 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -10,7 +10,6 @@ #include "test/integration/ssl_utility.h" #include "test/integration/utility.h" -#include "test/test_common/logging.h" #include "gtest/gtest.h" using testing::_; @@ -426,8 +425,9 @@ TEST_P(TcpProxyIntegrationTest, TestIdletimeoutWithLargeOutstandingData) { // Test that a downstream disconnect while the upstream connection is not yet connected // successfully cleans up the upstream connection. -TEST_P(TcpProxyIntegrationTest, TestDownstreamDisconnectWithUpstreamConnectionInProgress) { - LogLevelSetter setter(spdlog::level::trace); +// TODO(ggreenway): re-enable this test after tcp_proxy does not readDisable() the downstream +// during upstream connect +TEST_P(TcpProxyIntegrationTest, DISABLED_TestDownstreamDisconnectWithUpstreamConnectionInProgress) { config_helper_.addConfigModifier([&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void { bootstrap.mutable_static_resources() ->mutable_listeners(0) @@ -446,18 +446,7 @@ TEST_P(TcpProxyIntegrationTest, TestDownstreamDisconnectWithUpstreamConnectionIn RELEASE_ASSERT(result, result.message()); tcp_client->close(); - std::string cx_active_stat_name; - for (auto& gauge : test_server_->gauges()) { - if (std::regex_match(gauge->name(), - std::regex(R"EOF(^listener\..*_0\.downstream_cx_active$)EOF"))) { - cx_active_stat_name = gauge->name(); - } - } - RELEASE_ASSERT(!cx_active_stat_name.empty(), "Must find the stat we're waiting for"); - ENVOY_LOG_MISC(debug, "Starting wait for downstream conn closed"); - test_server_->waitForGaugeEq(cx_active_stat_name, 0); - ENVOY_LOG_MISC(debug, "Finished wait for downstream conn closed"); - // test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_pending_active", 0); + test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_pending_active", 0); // This establishes the DELAY_CONNECT connection. fake_upstream_connection->write("a", false); diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index 0d81e51bfa8d..78e97b9fb5b6 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -121,7 +121,7 @@ void XfccIntegrationTest::initialize() { }); if (tls_) { - config_helper_.addSslConfig(); + config_helper_.addTlsListenerConfig(); } runtime_.reset(new NiceMock()); From ddf1476b72cfbde277f7c1fef3eaa50635f0767b Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 15:13:42 -0700 Subject: [PATCH 3/8] fix_format Signed-off-by: Greg Greenway --- test/integration/tcp_proxy_integration_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 97a96d0341d1..c9eff3b00a92 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -10,6 +10,7 @@ #include "test/integration/ssl_utility.h" #include "test/integration/utility.h" + #include "gtest/gtest.h" using testing::_; From 5aad0569e7b7ab5130c8db841434016cee82b8ed Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 15:21:19 -0700 Subject: [PATCH 4/8] revert no longer needed change Signed-off-by: Greg Greenway --- test/config/utility.cc | 8 +------- test/config/utility.h | 7 ++----- test/integration/ssl_integration_test.cc | 2 +- test/integration/tcp_proxy_integration_test.cc | 2 +- test/integration/xfcc_integration_test.cc | 2 +- 5 files changed, 6 insertions(+), 15 deletions(-) diff --git a/test/config/utility.cc b/test/config/utility.cc index 16cbc610f91d..2cfdca3098e1 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -356,7 +356,7 @@ void ConfigHelper::setClientCodec( } } -void ConfigHelper::addTlsListenerConfig() { +void ConfigHelper::addSslConfig() { RELEASE_ASSERT(!finalized_, ""); auto* filter_chain = @@ -381,12 +381,6 @@ void ConfigHelper::addTlsListenerConfig() { TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); } -void ConfigHelper::addTlsUpstreamConfig() { - RELEASE_ASSERT(!finalized_, ""); - auto* cluster = bootstrap_.mutable_static_resources()->mutable_clusters(0); - cluster->mutable_tls_context()->set_sni("example.com"); -} - void ConfigHelper::renameListener(const std::string& name) { auto* static_resources = bootstrap_.mutable_static_resources(); if (static_resources->listeners_size() > 0) { diff --git a/test/config/utility.h b/test/config/utility.h index 886b5b5c8b1a..a5c2982f1679 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -88,11 +88,8 @@ class ConfigHelper { envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::CodecType type); - // Add the default TLS listener configuration. - void addTlsListenerConfig(); - - // Add the default TLS upstream configuration. - void addTlsUpstreamConfig(); + // Add the default SSL configuration. + void addSslConfig(); // Renames the first listener to the name specified. void renameListener(const std::string& name); diff --git a/test/integration/ssl_integration_test.cc b/test/integration/ssl_integration_test.cc index 38f6243466a2..f72f0e7c518c 100644 --- a/test/integration/ssl_integration_test.cc +++ b/test/integration/ssl_integration_test.cc @@ -28,7 +28,7 @@ namespace Envoy { namespace Ssl { void SslIntegrationTest::initialize() { - config_helper_.addTlsListenerConfig(); + config_helper_.addSslConfig(); HttpIntegrationTest::initialize(); runtime_.reset(new NiceMock()); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index c9eff3b00a92..4353b94b370b 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -469,7 +469,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, TcpProxySslIntegrationTest, TestUtility::ipTestParamsToString); void TcpProxySslIntegrationTest::initialize() { - config_helper_.addTlsListenerConfig(); + config_helper_.addSslConfig(); TcpProxyIntegrationTest::initialize(); context_manager_.reset(new Ssl::ContextManagerImpl(runtime_)); diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index 78e97b9fb5b6..0d81e51bfa8d 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -121,7 +121,7 @@ void XfccIntegrationTest::initialize() { }); if (tls_) { - config_helper_.addTlsListenerConfig(); + config_helper_.addSslConfig(); } runtime_.reset(new NiceMock()); From 7e2361cdbdee972dc60828a503779f64bcd76dbb Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 15:42:27 -0700 Subject: [PATCH 5/8] Add test that covers new code Signed-off-by: Greg Greenway --- test/common/tcp_proxy/tcp_proxy_test.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 218c72f308b6..1df01629ffa8 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -740,6 +740,22 @@ TEST_F(TcpProxyTest, DisconnectBeforeData) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); } +// Test that if the downstream connection is closed before the upstream connection +// is established, the upstream connection is cancelled. +TEST_F(TcpProxyTest, RemoteClosetBeforeUpstreamConnected) { + setup(1); + EXPECT_CALL(*conn_pool_handles_.at(0), cancel()); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); +} + +// Test that if the downstream connection is closed before the upstream connection +// is established, the upstream connection is cancelled. +TEST_F(TcpProxyTest, LocalClosetBeforeUpstreamConnected) { + setup(1); + EXPECT_CALL(*conn_pool_handles_.at(0), cancel()); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); +} + TEST_F(TcpProxyTest, UpstreamConnectFailure) { setup(1, accessLogConfig("%RESPONSE_FLAGS%")); From 756d2accff37c46a5b1845b6cdb3c581817fa47c Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 16:38:33 -0700 Subject: [PATCH 6/8] fix build Signed-off-by: Greg Greenway --- test/integration/tcp_proxy_integration_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 4353b94b370b..623af9ee1af9 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -450,7 +450,7 @@ TEST_P(TcpProxyIntegrationTest, DISABLED_TestDownstreamDisconnectWithUpstreamCon test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_pending_active", 0); // This establishes the DELAY_CONNECT connection. - fake_upstream_connection->write("a", false); + ASSERT_TRUE(fake_upstream_connection->write("a", false)); // The upstream connection was established after the downstream connection was closed, // so it stayed in the connpool and should be used for this 2nd downstream connection. @@ -458,7 +458,7 @@ TEST_P(TcpProxyIntegrationTest, DISABLED_TestDownstreamDisconnectWithUpstreamCon tcp_client2->write("foo", true); ASSERT_TRUE(fake_upstream_connection->waitForData(3)); ASSERT_TRUE(fake_upstream_connection->waitForHalfClose()); - fake_upstream_connection->write("", true); + ASSERT_TRUE(fake_upstream_connection->write("", true)); tcp_client2->waitForData("ab"); tcp_client2->waitForDisconnect(); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); @@ -636,7 +636,7 @@ TEST_P(TcpProxySslIntegrationTest, FailTlsHandshake) { tcp_client->write(std::string(10000, 'a'), false); ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); - fake_upstream_connection_->close(); + ASSERT_TRUE(fake_upstream_connection_->close()); ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); tcp_client->close(); } From f2b3fbf9c3e7bb4905716447a2d9d5a90bfb8e79 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 17:05:23 -0700 Subject: [PATCH 7/8] fix tsan error in test (use after free) Signed-off-by: Greg Greenway --- test/common/http/conn_manager_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 95aad5f19c2a..266a705a5fb9 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1718,8 +1718,8 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_total_.value()); EXPECT_EQ(0U, stats_.named_.downstream_cx_http1_active_.value()); - filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); upstream_callbacks->onEvent(Network::ConnectionEvent::RemoteClose); + filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); EXPECT_EQ(0U, stats_.named_.downstream_cx_websocket_active_.value()); } @@ -1764,8 +1764,8 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyData) { Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) { upstream_callbacks = &cb; })); conn_pool_.poolReady(upstream_conn_); - filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); upstream_callbacks->onEvent(Network::ConnectionEvent::RemoteClose); + filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } @@ -1843,8 +1843,8 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyEndStream) { .WillOnce( Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) { upstream_callbacks = &cb; })); conn_pool_.poolReady(upstream_conn_); - filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); upstream_callbacks->onEvent(Network::ConnectionEvent::RemoteClose); + filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } From 5fab62887a093cbd2a0e79876bb1f95b0843fa23 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 4 Sep 2018 17:22:36 -0700 Subject: [PATCH 8/8] Revert tcp_proxy_integration_test fixes. These changes can't be made until behavior changes to tcp_proxy are made. Signed-off-by: Greg Greenway --- .../integration/tcp_proxy_integration_test.cc | 150 +----------------- 1 file changed, 1 insertion(+), 149 deletions(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 623af9ee1af9..c1d2305eb65c 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -2,7 +2,6 @@ #include "envoy/config/accesslog/v2/file.pb.h" #include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.validate.h" -#include "envoy/server/transport_socket_config.h" #include "common/filesystem/filesystem_impl.h" #include "common/network/utility.h" @@ -21,74 +20,6 @@ using testing::NiceMock; namespace Envoy { namespace { -// Transport socket that does not raise a Connected event until it has -// received a message from the other side indicating that it should be connected. -class DelayConnectSocket : public Network::RawBufferSocket { - // Network::TransportSocket - void onConnected() override { - // Delay raising the event. - } - Network::IoResult doRead(Buffer::Instance& buffer) override { - Network::IoResult result = Network::RawBufferSocket::doRead(buffer); - if (buffer.length() > 0 && !raised_connected_) { - raised_connected_ = true; - Network::RawBufferSocket::onConnected(); - } - - if (result.end_stream_read_ && !raised_connected_) { - result.end_stream_read_ = false; - result.action_ = Network::PostIoAction::Close; - } - - return result; - } - -private: - bool raised_connected_{false}; -}; - -class DelayConnectSocketFactory : public Network::TransportSocketFactory { -public: - Network::TransportSocketPtr createTransportSocket() const override { - return std::make_unique(); - } - bool implementsSecureTransport() const override { return false; } -}; - -class DelayConnectSocketConfigFactory - : public Server::Configuration::UpstreamTransportSocketConfigFactory, - public Server::Configuration::DownstreamTransportSocketConfigFactory { -public: - static constexpr const char* NAME = "DELAY_CONNECT"; - std::string name() const override { return NAME; } - ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return std::make_unique(); - } - - // Server::Configuration::UpstreamTransportSocketConfigFactory - Network::TransportSocketFactoryPtr - createTransportSocketFactory(const Protobuf::Message&, - Server::Configuration::TransportSocketFactoryContext&) override { - return std::make_unique(); - } - - // Server::Configuration::DownstreamTransportSocketConfigFactory - Network::TransportSocketFactoryPtr - createTransportSocketFactory(const Protobuf::Message&, - Server::Configuration::TransportSocketFactoryContext&, - const std::vector&) override { - return std::make_unique(); - } -}; - -Registry::RegisterFactory - upstream_registered_; - -Registry::RegisterFactory - downstream_registered_; - INSTANTIATE_TEST_CASE_P(IpVersions, TcpProxyIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -424,46 +355,6 @@ TEST_P(TcpProxyIntegrationTest, TestIdletimeoutWithLargeOutstandingData) { ASSERT_TRUE(fake_upstream_connection->waitForDisconnect(true)); } -// Test that a downstream disconnect while the upstream connection is not yet connected -// successfully cleans up the upstream connection. -// TODO(ggreenway): re-enable this test after tcp_proxy does not readDisable() the downstream -// during upstream connect -TEST_P(TcpProxyIntegrationTest, DISABLED_TestDownstreamDisconnectWithUpstreamConnectionInProgress) { - config_helper_.addConfigModifier([&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void { - bootstrap.mutable_static_resources() - ->mutable_listeners(0) - ->mutable_filter_chains(0) - ->mutable_transport_socket() - ->set_name(DelayConnectSocketConfigFactory::NAME); - bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_transport_socket()->set_name( - DelayConnectSocketConfigFactory::NAME); - }); - initialize(); - - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); - - FakeRawConnectionPtr fake_upstream_connection; - AssertionResult result = fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection); - RELEASE_ASSERT(result, result.message()); - - tcp_client->close(); - test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_pending_active", 0); - - // This establishes the DELAY_CONNECT connection. - ASSERT_TRUE(fake_upstream_connection->write("a", false)); - - // The upstream connection was established after the downstream connection was closed, - // so it stayed in the connpool and should be used for this 2nd downstream connection. - IntegrationTcpClientPtr tcp_client2 = makeTcpConnection(lookupPort("tcp_proxy")); - tcp_client2->write("foo", true); - ASSERT_TRUE(fake_upstream_connection->waitForData(3)); - ASSERT_TRUE(fake_upstream_connection->waitForHalfClose()); - ASSERT_TRUE(fake_upstream_connection->write("", true)); - tcp_client2->waitForData("ab"); - tcp_client2->waitForDisconnect(); - ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); -} - INSTANTIATE_TEST_CASE_P(IpVersions, TcpProxySslIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -564,7 +455,7 @@ TEST_P(TcpProxySslIntegrationTest, DownstreamHalfClose) { Buffer::OwnedImpl empty_buffer; ssl_client_->write(empty_buffer, true); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); - ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose(true)); + ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); const std::string data("data"); ASSERT_TRUE(fake_upstream_connection_->write(data, false)); @@ -602,44 +493,5 @@ TEST_P(TcpProxySslIntegrationTest, UpstreamHalfClose) { ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); } -// Test that upstream connection is properly closed if the downstream -// connection aborts the handshake. -TEST_P(TcpProxySslIntegrationTest, AbortTlsHandshake) { - initialize(); - - // Connect to a TLS listener with a raw client to ensure that the handshake doesn't - // succeed. - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); - - AssertionResult result = fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_); - RELEASE_ASSERT(result, result.message()); - - tcp_client->close(); - ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); -} - -// Test that upstream connection is properly closed if the downstream -// TLS handshake fails. -TEST_P(TcpProxySslIntegrationTest, FailTlsHandshake) { - initialize(); - - // Connect to a TLS listener with a raw client to ensure that the handshake doesn't - // succeed. - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); - - AssertionResult result = fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_); - RELEASE_ASSERT(result, result.message()); - - // Write garbage so that the remote TLS server will fail the handshake. - tcp_client->write(std::string(10000, 'a'), false); - - ASSERT_TRUE(fake_upstream_connection_->waitForHalfClose()); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - tcp_client->close(); -} - } // namespace } // namespace Envoy