test: fixing a teardown ordering bug (envoyproxy#11542)

As (now) commented, when codecs access runtime, it creates a destruction order invariant that the fake upstreams are torn down before the server (and its runtime). Fixing this in the base integration test and cleaning up test sub-classes. Risk Level: n/a (test only) Testing: http2 test passes cleanly with 200 runs (where before it flaked out) Docs Changes: n/a Release Notes: n/a Fixes envoyproxy#11538 Signed-off-by: Alyssa Wilk <[email protected]>
htuch · Jun 11, 2020 · 4a91d7f · 4a91d7f
1 parent e720cda
commit 4a91d7f
Show file tree

Hide file tree

Showing 38 changed files with 24 additions and 215 deletions.
diff --git a/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc b/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc
@@ -32,11 +32,6 @@ class TcpGrpcAccessLogIntegrationTest : public Grpc::GrpcClientIntegrationParamT
     enable_half_close_ = true;
   }
 
-  ~TcpGrpcAccessLogIntegrationTest() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   void createUpstreams() override {
     BaseIntegrationTest::createUpstreams();
     fake_upstreams_.emplace_back(

diff --git a/test/extensions/clusters/aggregate/cluster_integration_test.cc b/test/extensions/clusters/aggregate/cluster_integration_test.cc
@@ -119,11 +119,7 @@ class AggregateIntegrationTest : public testing::TestWithParam<Network::Address:
     use_lds_ = false;
   }
 
-  void TearDown() override {
-    cleanUpXdsConnection();
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
+  void TearDown() override { cleanUpXdsConnection(); }
 
   void initialize() override {
     use_lds_ = false;

diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc
@@ -143,11 +143,6 @@ class RedisClusterIntegrationTest : public testing::TestWithParam<Network::Addre
       : BaseIntegrationTest(GetParam(), config), num_upstreams_(num_upstreams),
         version_(GetParam()) {}
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   void initialize() override {
     setUpstreamCount(num_upstreams_);
     setDeterministic();

diff --git a/test/extensions/common/aws/aws_metadata_fetcher_integration_test.cc b/test/extensions/common/aws/aws_metadata_fetcher_integration_test.cc
@@ -71,11 +71,6 @@ class AwsMetadataIntegrationTestBase : public ::testing::Test, public BaseIntegr
   }
 
   void SetUp() override { BaseIntegrationTest::initialize(); }
-
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
 };
 
 class AwsMetadataIntegrationTestSuccess : public AwsMetadataIntegrationTestBase {

diff --git a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc
@@ -27,11 +27,7 @@ class AwsLambdaFilterIntegrationTest : public testing::TestWithParam<Network::Ad
     setUpstreamProtocol(FakeHttpConnection::Type::HTTP1);
   }
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstream_connection_.reset();
-    fake_upstreams_.clear();
-  }
+  void TearDown() override { fake_upstream_connection_.reset(); }
 
   void setupLambdaFilter(bool passthrough) {
     const std::string filter =

diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc
@@ -52,11 +52,7 @@ name: grpc_http1_reverse_bridge
     HttpIntegrationTest::initialize();
   }
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstream_connection_.reset();
-    fake_upstreams_.clear();
-  }
+  void TearDown() override { fake_upstream_connection_.reset(); }
 
 protected:
   FakeHttpConnection::Type upstream_protocol_;

diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc
@@ -28,9 +28,7 @@ class GrpcJsonTranscoderIntegrationTest
 public:
   GrpcJsonTranscoderIntegrationTest()
       : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {}
-  /**
-   * Global initializer for all integration tests.
-   */
+
   void SetUp() override {
     setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
     const std::string filter =
@@ -45,15 +43,6 @@ class GrpcJsonTranscoderIntegrationTest
         fmt::format(filter, TestEnvironment::runfilesPath("test/proto/bookstore.descriptor")));
   }
 
-  /**
-   * Global destructor for all integration tests.
-   */
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstream_connection_.reset();
-    fake_upstreams_.clear();
-  }
-
 protected:
   template <class RequestType, class ResponseType>
   void testTranscoding(Http::RequestHeaderMap&& request_headers, const std::string& request_body,

diff --git a/test/extensions/filters/network/direct_response/direct_response_integration_test.cc b/test/extensions/filters/network/direct_response/direct_response_integration_test.cc
@@ -21,21 +21,10 @@ class DirectResponseIntegrationTest : public testing::TestWithParam<Network::Add
       )EOF");
   }
 
-  /**
-   * Initializer for an individual test.
-   */
   void SetUp() override {
     useListenerAccessLog("%RESPONSE_CODE_DETAILS%");
     BaseIntegrationTest::initialize();
   }
-
-  /**
-   *  Destructor for an individual test.
-   */
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
 };
 
 INSTANTIATE_TEST_SUITE_P(IpVersions, DirectResponseIntegrationTest,

diff --git a/test/extensions/filters/network/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/network/local_ratelimit/local_ratelimit_integration_test.cc
@@ -10,11 +10,6 @@ class LocalRateLimitIntegrationTest : public Event::TestUsingSimulatedTime,
   LocalRateLimitIntegrationTest()
       : BaseIntegrationTest(GetParam(), ConfigHelper::tcpProxyConfig()) {}
 
-  ~LocalRateLimitIntegrationTest() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   void setup(const std::string& filter_yaml) {
     config_helper_.addNetworkFilter(filter_yaml);
     BaseIntegrationTest::initialize();

diff --git a/test/extensions/filters/network/mysql_proxy/mysql_integration_test.cc b/test/extensions/filters/network/mysql_proxy/mysql_integration_test.cc
@@ -35,14 +35,7 @@ class MySQLIntegrationTest : public testing::TestWithParam<Network::Address::IpV
 public:
   MySQLIntegrationTest() : BaseIntegrationTest(GetParam(), mysqlConfig()){};
 
-  // Initializer for an individual integration test.
   void SetUp() override { BaseIntegrationTest::initialize(); }
-
-  // Destructor for an individual integration test.
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
 };
 
 INSTANTIATE_TEST_SUITE_P(IpVersions, MySQLIntegrationTest,

diff --git a/test/extensions/filters/network/postgres_proxy/postgres_integration_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_integration_test.cc
@@ -28,11 +28,6 @@ class PostgresIntegrationTest : public testing::TestWithParam<Network::Address::
   PostgresIntegrationTest() : BaseIntegrationTest(GetParam(), postgresConfig()){};
 
   void SetUp() override { BaseIntegrationTest::initialize(); }
-
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
 };
 INSTANTIATE_TEST_SUITE_P(IpVersions, PostgresIntegrationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()));

diff --git a/test/extensions/filters/network/rbac/integration_test.cc b/test/extensions/filters/network/rbac/integration_test.cc
@@ -60,11 +60,6 @@ class RoleBasedAccessControlNetworkFilterIntegrationTest
 
     BaseIntegrationTest::initialize();
   }
-
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
 };
 
 INSTANTIATE_TEST_SUITE_P(IpVersions, RoleBasedAccessControlNetworkFilterIntegrationTest,

diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc
@@ -294,11 +294,6 @@ class RedisProxyIntegrationTest : public testing::TestWithParam<Network::Address
       : BaseIntegrationTest(GetParam(), config), num_upstreams_(num_upstreams),
         version_(GetParam()) {}
 
-  ~RedisProxyIntegrationTest() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   // This method encodes a fake upstream's IP address and TCP port in the
   // same format as one would expect from a Redis server in
   // an ask/moved redirection error.

diff --git a/test/extensions/filters/network/thrift_proxy/integration_test.cc b/test/extensions/filters/network/thrift_proxy/integration_test.cc
@@ -122,11 +122,6 @@ class ThriftConnManagerIntegrationTest
     BaseThriftIntegrationTest::initialize();
   }
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
 protected:
   // Multiplexed requests are handled by the service name route match,
   // while oneway's are handled by the "poke" method. All other requests

diff --git a/test/extensions/filters/network/thrift_proxy/translation_integration_test.cc b/test/extensions/filters/network/thrift_proxy/translation_integration_test.cc
@@ -82,11 +82,6 @@ class ThriftTranslationIntegrationTest
     BaseThriftIntegrationTest::initialize();
   }
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   Buffer::OwnedImpl downstream_request_bytes_;
   Buffer::OwnedImpl downstream_response_bytes_;
   Buffer::OwnedImpl upstream_request_bytes_;

diff --git a/test/extensions/filters/udp/dns_filter/dns_filter_integration_test.cc b/test/extensions/filters/udp/dns_filter/dns_filter_integration_test.cc
@@ -75,11 +75,6 @@ class DnsFilterIntegrationTest : public testing::TestWithParam<Network::Address:
     BaseIntegrationTest::initialize();
   }
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   void requestResponseWithListenerAddress(const Network::Address::Instance& listener_address,
                                           const std::string& data_to_send,
                                           Network::UdpRecvData& response_datagram) {

diff --git a/test/extensions/filters/udp/udp_proxy/udp_proxy_integration_test.cc b/test/extensions/filters/udp/udp_proxy/udp_proxy_integration_test.cc
@@ -46,14 +46,6 @@ class UdpProxyIntegrationTest : public testing::TestWithParam<Network::Address::
     BaseIntegrationTest::initialize();
   }
 
-  /**
-   *  Destructor for an individual test.
-   */
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
   void requestResponseWithListenerAddress(const Network::Address::Instance& listener_address) {
     // Send datagram to be proxied.
     Network::Test::UdpSyncPeer client(version_);

diff --git a/test/integration/ads_integration.cc b/test/integration/ads_integration.cc
@@ -30,11 +30,7 @@ AdsIntegrationTest::AdsIntegrationTest()
   sotw_or_delta_ = sotwOrDelta();
 }
 
-void AdsIntegrationTest::TearDown() {
-  cleanUpXdsConnection();
-  test_server_.reset();
-  fake_upstreams_.clear();
-}
+void AdsIntegrationTest::TearDown() { cleanUpXdsConnection(); }
 
 envoy::config::cluster::v3::Cluster AdsIntegrationTest::buildCluster(const std::string& name) {
   return TestUtility::parseYaml<envoy::config::cluster::v3::Cluster>(fmt::format(R"EOF(

diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc
@@ -601,11 +601,7 @@ class AdsFailIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest,
     sotw_or_delta_ = sotwOrDelta();
   }
 
-  void TearDown() override {
-    cleanUpXdsConnection();
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
+  void TearDown() override { cleanUpXdsConnection(); }
 
   void initialize() override {
     config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
@@ -645,11 +641,7 @@ class AdsConfigIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest,
     sotw_or_delta_ = sotwOrDelta();
   }
 
-  void TearDown() override {
-    cleanUpXdsConnection();
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
+  void TearDown() override { cleanUpXdsConnection(); }
 
   void initialize() override {
     config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
@@ -810,11 +802,7 @@ class AdsClusterFromFileIntegrationTest : public Grpc::DeltaSotwIntegrationParam
     sotw_or_delta_ = sotwOrDelta();
   }
 
-  void TearDown() override {
-    cleanUpXdsConnection();
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
+  void TearDown() override { cleanUpXdsConnection(); }
 
   void initialize() override {
     config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {

diff --git a/test/integration/api_version_integration_test.cc b/test/integration/api_version_integration_test.cc
@@ -218,8 +218,6 @@ class ApiVersionIntegrationTest : public testing::TestWithParam<Params>,
     if (xds_stream_ != nullptr) {
       cleanUpXdsConnection();
     }
-    test_server_.reset();
-    fake_upstreams_.clear();
   }
 
   std::string endpoint_;

diff --git a/test/integration/cds_integration_test.cc b/test/integration/cds_integration_test.cc
@@ -42,8 +42,6 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht
   void TearDown() override {
     if (!test_skipped_) {
       cleanUpXdsConnection();
-      test_server_.reset();
-      fake_upstreams_.clear();
     }
   }
 

diff --git a/test/integration/echo_integration_test.cc b/test/integration/echo_integration_test.cc
@@ -28,18 +28,7 @@ class EchoIntegrationTest : public testing::TestWithParam<Network::Address::IpVe
       )EOF");
   }
 
-  /**
-   * Initializer for an individual test.
-   */
   void SetUp() override { BaseIntegrationTest::initialize(); }
-
-  /**
-   *  Destructor for an individual test.
-   */
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
 };
 
 INSTANTIATE_TEST_SUITE_P(IpVersions, EchoIntegrationTest,

diff --git a/test/integration/filter_manager_integration_test.cc b/test/integration/filter_manager_integration_test.cc
@@ -410,11 +410,6 @@ class InjectDataToFilterChainIntegrationTest
 
   void SetUp() override { addAuxiliaryFilter(config_helper_); }
 
-  void TearDown() override {
-    test_server_.reset();
-    fake_upstreams_.clear();
-  }
-
 protected:
   // Returns configuration for a given auxiliary filter
   std::string filterConfig(const std::string& auxiliary_filter_name) override {

diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc
@@ -196,9 +196,6 @@ class HeaderIntegrationTest
       RELEASE_ASSERT(result, result.message());
       eds_connection_.reset();
     }
-    cleanupUpstreamAndDownstream();
-    test_server_.reset();
-    fake_upstreams_.clear();
   }
 
   void addHeader(Protobuf::RepeatedPtrField<envoy::config::core::v3::HeaderValueOption>* field,

diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc
@@ -253,11 +253,7 @@ void HttpIntegrationTest::useAccessLog(absl::string_view format) {
   ASSERT_TRUE(config_helper_.setAccessLog(access_log_name_, format));
 }
 
-HttpIntegrationTest::~HttpIntegrationTest() {
-  cleanupUpstreamAndDownstream();
-  test_server_.reset();
-  fake_upstreams_.clear();
-}
+HttpIntegrationTest::~HttpIntegrationTest() { cleanupUpstreamAndDownstream(); }
 
 void HttpIntegrationTest::setDownstreamProtocol(Http::CodecClient::Type downstream_protocol) {
   downstream_protocol_ = downstream_protocol;

diff --git a/test/integration/integration.cc b/test/integration/integration.cc
@@ -282,6 +282,14 @@ BaseIntegrationTest::BaseIntegrationTest(Network::Address::IpVersion version,
           },
           version, config) {}
 
+BaseIntegrationTest::~BaseIntegrationTest() {
+  // Tear down the fake upstream before the test server.
+  // When the HTTP codecs do runtime checks, it is important to finish all
+  // runtime access before the server, and the runtime singleton, go away.
+  fake_upstreams_.clear();
+  test_server_.reset();
+}
+
 Network::ClientConnectionPtr BaseIntegrationTest::makeClientConnection(uint32_t port) {
   return makeClientConnectionWithOptions(port, nullptr);
 }

diff --git a/test/integration/integration.h b/test/integration/integration.h
@@ -162,7 +162,7 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
                       Network::Address::IpVersion version,
                       const std::string& config = ConfigHelper::httpProxyConfig());
 
-  virtual ~BaseIntegrationTest() = default;
+  virtual ~BaseIntegrationTest();
 
   // TODO(jmarantz): Remove this once
   // https://github.com/envoyproxy/envoy-filter-example/pull/69 is reverted.