test: deflaking unexpected disconnects permanently (envoyproxy#12939)

Allowing unexpected disconnects by default.

Signed-off-by: Alyssa Wilk <[email protected]>

Author: alyssawilk

test/extensions/clusters/aggregate/cluster_integration_test.cc

@@ -132,10 +132,8 @@ class AggregateIntegrationTest : public testing::TestWithParam<Network::Address:
 
     fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_,
                                                   timeSystem(), enable_half_close_));
-    fake_upstreams_[FirstUpstreamIndex]->set_allow_unexpected_disconnects(false);
     fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_,
                                                   timeSystem(), enable_half_close_));
-    fake_upstreams_[SecondUpstreamIndex]->set_allow_unexpected_disconnects(false);
     cluster1_ = ConfigHelper::buildStaticCluster(
         FirstClusterName, fake_upstreams_[FirstUpstreamIndex]->localAddress()->ip()->port(),
         Network::Test::getLoopbackAddressString(GetParam()));
@@ -166,7 +164,6 @@ class AggregateIntegrationTest : public testing::TestWithParam<Network::Address:
     result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_);
     RELEASE_ASSERT(result, result.message());
     xds_stream_->startGrpcStream();
-    fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
   }
 
   envoy::config::cluster::v3::Cluster cluster1_;

test/extensions/clusters/redis/redis_cluster_integration_test.cc

@@ -262,8 +262,6 @@ class RedisClusterIntegrationTest : public testing::TestWithParam<Network::Addre
                              const std::string& auth_password = "") {
     std::string cluster_slot_request = makeBulkStringArray({"CLUSTER", "SLOTS"});
 
-    fake_upstreams_[stream_index]->set_allow_unexpected_disconnects(true);
-
     std::string proxied_cluster_slot_request;
 
     FakeRawConnectionPtr fake_upstream_connection_;

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc

@@ -310,9 +310,6 @@ TEST_P(ProxyFilterIntegrationTest, UpstreamTlsInvalidSAN) {
   upstream_tls_ = true;
   upstream_cert_name_ = "upstream";
   setup();
-  // The upstream connection is going to fail handshake so make sure it can read and we expect
-  // it to disconnect.
-  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
   fake_upstreams_[0]->setReadDisableOnNewConnection(false);
 
   codec_client_ = makeHttpConnection(lookupPort("http"));

test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc

@@ -369,7 +369,6 @@ class ExtAuthzHttpIntegrationTest : public HttpIntegrationTest,
     HttpIntegrationTest::createUpstreams();
     fake_upstreams_.emplace_back(
         new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_, timeSystem()));
-    fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
   }
 
   // By default, HTTP Service uses case sensitive string matcher.
@@ -591,7 +590,6 @@ class ExtAuthzLocalReplyIntegrationTest : public HttpIntegrationTest,
     HttpIntegrationTest::createUpstreams();
     fake_upstreams_.emplace_back(
         new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_, timeSystem()));
-    fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
   }
 
   void cleanup() {

test/extensions/filters/http/router/auto_sni_integration_test.cc

@@ -40,7 +40,6 @@ class AutoSniIntegrationTest : public testing::TestWithParam<Network::Address::I
   void createUpstreams() override {
     fake_upstreams_.emplace_back(new FakeUpstream(
         createUpstreamSslContext(), 0, FakeHttpConnection::Type::HTTP1, version_, timeSystem()));
-    fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
   }
 
   Network::TransportSocketFactoryPtr createUpstreamSslContext() {

test/extensions/filters/http/squash/squash_filter_integration_test.cc

@@ -73,7 +73,6 @@ class SquashFilterIntegrationTest : public testing::TestWithParam<Network::Addre
     HttpIntegrationTest::createUpstreams();
     fake_upstreams_.emplace_back(
         new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, timeSystem()));
-    fake_upstreams_.back()->set_allow_unexpected_disconnects(true);
   }
 
   /**

test/extensions/filters/network/thrift_proxy/integration_test.cc

@@ -259,7 +259,6 @@ TEST_P(ThriftConnManagerIntegrationTest, EarlyClose) {
       request_bytes_.toString().substr(0, request_bytes_.length() - 5);
 
   FakeUpstream* expected_upstream = getExpectedUpstream(false);
-  expected_upstream->set_allow_unexpected_disconnects(true);
 
   IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0"));
   ASSERT_TRUE(tcp_client->write(partial_request));
@@ -377,7 +376,6 @@ TEST_P(ThriftConnManagerIntegrationTest, OnewayEarlyClosePartialRequest) {
       request_bytes_.toString().substr(0, request_bytes_.length() - 1);
 
   FakeUpstream* expected_upstream = getExpectedUpstream(true);
-  expected_upstream->set_allow_unexpected_disconnects(true);
 
   IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0"));
   ASSERT_TRUE(tcp_client->write(partial_request));

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

@@ -325,8 +325,6 @@ TEST_P(QuicHttpIntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) {
 
   initialize();
 
-  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
-
   codec_client_ = makeHttpConnection(lookupPort("http"));
 
   auto encoder_decoder =
@@ -482,7 +480,6 @@ TEST_P(QuicHttpIntegrationTest, ConnectionMigration) {
 
 TEST_P(QuicHttpIntegrationTest, StopAcceptingConnectionsWhenOverloaded) {
   initialize();
-  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
 
   // Put envoy in overloaded state and check that it doesn't accept the new client connection.
   updateResource(file_updater_1_, 0.9);
@@ -522,7 +519,6 @@ TEST_P(QuicHttpIntegrationTest, StopAcceptingConnectionsWhenOverloaded) {
 
 TEST_P(QuicHttpIntegrationTest, NoNewStreamsWhenOverloaded) {
   initialize();
-  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
   updateResource(file_updater_1_, 0.7);
 
   codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));

test/extensions/stats_sinks/metrics_service/metrics_service_integration_test.cc

@@ -28,7 +28,6 @@ class MetricsServiceIntegrationTest : public Grpc::VersionedGrpcClientIntegratio
     HttpIntegrationTest::createUpstreams();
     fake_upstreams_.emplace_back(
         new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, timeSystem()));
-    fake_upstreams_.back()->set_allow_unexpected_disconnects(true);
   }
 
   void initialize() override {

test/integration/README.md

@@ -170,33 +170,4 @@ going on. If your failure mode isn't documented below, ideally some combination
 of cerr << logging and trace logs will help you sort out what is going on (and
 please add to this document as you figure it out!)
 
-## Unexpected disconnects
-
-As commented in `HttpIntegrationTest::cleanupUpstreamAndDownstream()`, the
-tear-down sequence between upstream, Envoy, and client is somewhat sensitive to
-ordering. If a given unit test does not use the provided member variables, for
-example opens multiple client or upstream connections, the test author should be
-aware of test best practices for clean-up which boil down to "Clean up upstream
-first".
-
-Upstream connections run in their own threads, so if the client disconnects with
-open streams, there's a race where Envoy detects the disconnect, and kills the
-corresponding upstream stream, which is indistinguishable from an unexpected
-disconnect and triggers test failure. Because the client is run from the main
-thread, if upstream is closed first, the client will not detect the inverse
-close, so no test failure will occur.
-
-## Unparented upstream connections
-
-The most common failure mode here is if the test adds additional fake
-upstreams for *DS connections (ADS, EDS etc) which are not properly shut down
-(for a very sensitive test framework)
-
-The failure mode here is that during test teardown one closes the DS connection
-and then shuts down Envoy. Unfortunately as Envoy is running in its own thread,
-it will try to re-establish the *DS connection, sometimes creating a connection
-which is then "unparented". The solution here is to explicitly allow Envoy
-reconnects before closing the connection, using
-
-`my_ds_upstream_->set_allow_unexpected_disconnects(true);`