From 7ff1f3317fb6841e2e86db26f2cdff1ad67db163 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 18 May 2022 13:50:05 -0400 Subject: [PATCH 1/4] mobile: moving the c++ integration test to use default config Signed-off-by: Alyssa Wilk --- test/common/integration/BUILD | 1 + .../integration/client_integration_test.cc | 92 ++++++++----------- 2 files changed, 39 insertions(+), 54 deletions(-) diff --git a/test/common/integration/BUILD b/test/common/integration/BUILD index ca0ce7e30d..5f50d4f8c1 100644 --- a/test/common/integration/BUILD +++ b/test/common/integration/BUILD @@ -13,6 +13,7 @@ envoy_cc_test( }, repository = "@envoy", deps = [ + "//library/cc:engine_builder_lib", "//library/common/extensions/filters/http/local_error:config", "//library/common/extensions/filters/http/local_error:filter_cc_proto", "//library/common/http:client_lib", diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 090d7ff8b6..7a76226517 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -10,6 +10,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "library/cc/engine_builder.h" +#include "library/common/config/internal.h" #include "library/common/data/utility.h" #include "library/common/http/client.h" #include "library/common/http/header_utility.h" @@ -46,9 +48,8 @@ typedef struct { } callbacks_called; void validateStreamIntel(const envoy_final_stream_intel& final_intel) { - // This test doesn't do DNS lookup - EXPECT_EQ(-1, final_intel.dns_start_ms); - EXPECT_EQ(-1, final_intel.dns_end_ms); + EXPECT_NE(-1, final_intel.dns_start_ms); + EXPECT_NE(-1, final_intel.dns_end_ms); // This test doesn't do TLS. EXPECT_EQ(-1, final_intel.ssl_start_ms); EXPECT_EQ(-1, final_intel.ssl_end_ms); @@ -73,11 +74,26 @@ void validateStreamIntel(const envoy_final_stream_intel& final_intel) { class ClientIntegrationTest : public BaseIntegrationTest, public testing::TestWithParam { public: - ClientIntegrationTest() : BaseIntegrationTest(GetParam(), bootstrap_config()) { + ClientIntegrationTest() : BaseIntegrationTest(GetParam(), defaultConfig()) { use_lds_ = false; autonomous_upstream_ = true; defer_listener_finalization_ = true; HttpTestUtility::addDefaultHeaders(default_request_headers_); + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + // The default stats config has overenthusiastic filters. + bootstrap.clear_stats_config(); + }); + // TODO(alyssawilk) upstream has an issue with logical DNS ipv6 clusters - + // remove this once #21359 lands. + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* static_resources = bootstrap.mutable_static_resources(); + for (int i = 0; i < static_resources->clusters_size(); ++i) { + auto* cluster = static_resources->mutable_clusters(i); + if (cluster->type() == envoy::config::cluster::v3::Cluster::LOGICAL_DNS) { + cluster->clear_type(); + } + } + }); } void initialize() override { @@ -91,18 +107,10 @@ class ClientIntegrationTest : public BaseIntegrationTest, server_started.setReady(); }); server_started.waitReady(); + default_request_headers_.setHost(fake_upstreams_[0]->localAddress()->asStringView()); } void SetUp() override { - config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - // currently ApiListener does not trigger this wait - // https://github.com/envoyproxy/envoy/blob/0b92c58d08d28ba7ef0ed5aaf44f90f0fccc5dce/test/integration/integration.cc#L454 - // Thus, the ApiListener has to be added in addition to the already existing listener in the - // config. - bootstrap.mutable_static_resources()->add_listeners()->MergeFrom( - Server::parseListenerFromV3Yaml(api_listener_config())); - }); - bridge_callbacks_.context = &cc_; bridge_callbacks_.on_headers = [](envoy_headers c_headers, bool, envoy_stream_intel intel, void* context) -> void* { @@ -159,36 +167,15 @@ class ClientIntegrationTest : public BaseIntegrationTest, )EOF"; } - static std::string api_listener_config() { - return R"EOF( -name: api_listener -address: - socket_address: - address: 127.0.0.1 - port_value: 1 -api_listener: - api_listener: - "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.EnvoyMobileHttpConnectionManager - config: - stat_prefix: hcm - route_config: - virtual_hosts: - name: integration - routes: - route: - cluster: cluster_0 - match: - prefix: "/" - domains: "*" - name: route_config_0 - http_filters: - - name: envoy.filters.http.local_error - typed_config: - "@type": type.googleapis.com/envoymobile.extensions.filters.http.local_error.LocalError - - name: envoy.router - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router - )EOF"; + // Use the Envoy mobile default config as much as possible in this test. + // There are some config modifiers below which do result in deltas. + static std::string defaultConfig() { + Platform::EngineBuilder builder; + + ProtobufMessage::StrictValidationVisitorImpl visitor; + envoy::config::bootstrap::v3::Bootstrap bootstrap; + std::string config_str = absl::StrCat(config_header, builder.generateConfigStr()); + return config_str; } Event::ProvisionalDispatcherPtr dispatcher_ = std::make_unique(); @@ -222,8 +209,8 @@ TEST_P(ClientIntegrationTest, Basic) { // Build a set of request headers. Buffer::OwnedImpl request_data = Buffer::OwnedImpl("request body"); - Http::TestRequestHeaderMapImpl headers{ - {AutonomousStream::EXPECT_REQUEST_SIZE_BYTES, std::to_string(request_data.length())}}; + default_request_headers_.addCopy(AutonomousStream::EXPECT_REQUEST_SIZE_BYTES, + std::to_string(request_data.length())); envoy_headers c_headers = Http::Utility::toBridgeHeaders(default_request_headers_); @@ -390,15 +377,14 @@ TEST_P(ClientIntegrationTest, CaseSensitive) { }; // Build a set of request headers. - Http::TestRequestHeaderMapImpl headers{{"FoO", "bar"}}; - headers.header_map_->setFormatter( + default_request_headers_.header_map_->setFormatter( std::make_unique< Extensions::Http::HeaderFormatters::PreserveCase::PreserveCaseHeaderFormatter>( false, envoy::extensions::http::header_formatters::preserve_case::v3:: PreserveCaseFormatterConfig::DEFAULT)); - headers.header_map_->formatter().value().get().processKey("FoO"); - HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Http::Utility::toBridgeHeaders(headers); + default_request_headers_.addCopy("FoO", "bar"); + default_request_headers_.header_map_->formatter().value().get().processKey("FoO"); + envoy_headers c_headers = Http::Utility::toBridgeHeaders(default_request_headers_); // Create a stream. dispatcher_->post([&]() -> void { @@ -432,7 +418,7 @@ TEST_P(ClientIntegrationTest, CaseSensitive) { TEST_P(ClientIntegrationTest, Timeout) { config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(1); + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); auto* em_hcm = listener->mutable_api_listener()->mutable_api_listener(); auto hcm = MessageUtil::anyConvertpost([&]() -> void { From 2bbc4c64d46392a77ae95604207ac7c496a0e444 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 18 May 2022 14:02:03 -0400 Subject: [PATCH 2/4] h/t ali Signed-off-by: Alyssa Wilk --- test/common/integration/client_integration_test.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 7a76226517..7ca6b6af41 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -171,9 +171,6 @@ class ClientIntegrationTest : public BaseIntegrationTest, // There are some config modifiers below which do result in deltas. static std::string defaultConfig() { Platform::EngineBuilder builder; - - ProtobufMessage::StrictValidationVisitorImpl visitor; - envoy::config::bootstrap::v3::Bootstrap bootstrap; std::string config_str = absl::StrCat(config_header, builder.generateConfigStr()); return config_str; } From eeaafdad3cea6860dd0d855f8b249c5ac67caf59 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 18 May 2022 15:15:48 -0400 Subject: [PATCH 3/4] more tests, failed to repro swift crash Signed-off-by: Alyssa Wilk --- library/common/config/config.cc | 1 - .../integration/client_integration_test.cc | 48 ++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/library/common/config/config.cc b/library/common/config/config.cc index 2b8257ae62..1efedcd94c 100644 --- a/library/common/config/config.cc +++ b/library/common/config/config.cc @@ -468,7 +468,6 @@ stats_sinks: *stats_sinks envoy: reloadable_features: allow_multiple_dns_addresses: *dns_multiple_addresses - override_request_timeout_by_gateway_timeout: false http2_delay_keepalive_timeout: *h2_delay_keepalive_timeout )" // Needed due to warning in diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 7ca6b6af41..c25d55753d 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -413,7 +413,7 @@ TEST_P(ClientIntegrationTest, CaseSensitive) { test_server_->waitForCounterEq("http.client.stream_success", 1); } -TEST_P(ClientIntegrationTest, Timeout) { +TEST_P(ClientIntegrationTest, TimeoutOnRequestPath) { config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); auto* em_hcm = listener->mutable_api_listener()->mutable_api_listener(); @@ -448,6 +448,52 @@ TEST_P(ClientIntegrationTest, Timeout) { Envoy::FakeRawConnectionPtr upstream_connection; ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection)); + std::string upstream_request; + EXPECT_TRUE(upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("GET /"), + &upstream_request)); + terminal_callback_.waitReady(); + + ASSERT_EQ(cc_.on_headers_calls, 0); + ASSERT_EQ(cc_.on_data_calls, 0); + ASSERT_EQ(cc_.on_complete_calls, 0); + ASSERT_EQ(cc_.on_error_calls, 1); +} + +TEST_P(ClientIntegrationTest, TimeoutOnResponsePath) { + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto* em_hcm = listener->mutable_api_listener()->mutable_api_listener(); + auto hcm = + MessageUtil::anyConvert(*em_hcm); + hcm.mutable_config()->mutable_stream_idle_timeout()->set_seconds(1); + em_hcm->PackFrom(hcm); + }); + + autonomous_upstream_ = false; + initialize(); + + bridge_callbacks_.on_headers = [](envoy_headers c_headers, bool, envoy_stream_intel, + void* context) -> void* { + Http::ResponseHeaderMapPtr response_headers = toResponseHeaders(c_headers); + callbacks_called* cc_ = static_cast(context); + cc_->on_headers_calls++; + cc_->status = response_headers->Status()->value().getStringView(); + return nullptr; + }; + + // Build a set of request headers. + envoy_headers c_headers = Http::Utility::toBridgeHeaders(default_request_headers_); + + // Create a stream. + dispatcher_->post([&]() -> void { + http_client_->startStream(stream_, bridge_callbacks_, false); + http_client_->sendHeaders(stream_, c_headers, true); + }); + + Envoy::FakeRawConnectionPtr upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection)); + std::string upstream_request; EXPECT_TRUE(upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("GET /"), &upstream_request)); From 0b7007e7503a8875a4be7cf4ad70a27f40a43f33 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 18 May 2022 15:27:41 -0400 Subject: [PATCH 4/4] more checks Signed-off-by: Alyssa Wilk --- test/common/integration/client_integration_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index c25d55753d..3ebe774e32 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -155,6 +155,9 @@ class ClientIntegrationTest : public BaseIntegrationTest, } void TearDown() override { + // Right now each test does one request - if this changes, make the 1 + // configurable. + ASSERT_EQ(cc_.on_complete_calls + cc_.on_cancel_calls + cc_.on_error_calls, 1); test_server_.reset(); fake_upstreams_.clear(); }