Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mobile: moving the c++ integration test to use default config #2293

Merged
merged 2 commits into from
May 20, 2022
Merged

mobile: moving the c++ integration test to use default config #2293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ff1f33
mobile: moving the c++ integration test to use default config
alyssawilk May 18, 2022
2bbc4c6
h/t ali
alyssawilk May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions test/common/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
89 changes: 35 additions & 54 deletions test/common/integration/client_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand All @@ -73,11 +74,26 @@ void validateStreamIntel(const envoy_final_stream_intel& final_intel) {
class ClientIntegrationTest : public BaseIntegrationTest,
public testing::TestWithParam<Network::Address::IpVersion> {
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What filters cause issues exactly? Should this be adjusted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I added this as agenda to today's meeting as I wanted to get context on your stats filters. the stats config here:
https://github.com/envoyproxy/envoy-mobile/blob/main/library/common/config/config.cc#L421
filters out http.client.stream_success and friends which is used in the integration test.

I'm inclined to land this one as-is, and then work out with you folks if we should remove the stats matcher entirely (and move it to Lyft internal config), make it configurable, or what.

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 {
Expand All @@ -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* {
Expand Down Expand Up @@ -159,36 +167,12 @@ 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;
std::string config_str = absl::StrCat(config_header, builder.generateConfigStr());
return config_str;
}

Event::ProvisionalDispatcherPtr dispatcher_ = std::make_unique<Event::ProvisionalDispatcher>();
Expand Down Expand Up @@ -222,8 +206,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_);

Expand Down Expand Up @@ -390,15 +374,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 {
Expand Down Expand Up @@ -432,7 +415,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::anyConvert<envoy::extensions::filters::network::http_connection_manager::v3::
Expand All @@ -454,9 +437,7 @@ TEST_P(ClientIntegrationTest, Timeout) {
};

// Build a set of request headers.
Http::TestRequestHeaderMapImpl headers{{"FoO", "bar"}};
HttpTestUtility::addDefaultHeaders(headers);
envoy_headers c_headers = Http::Utility::toBridgeHeaders(headers);
envoy_headers c_headers = Http::Utility::toBridgeHeaders(default_request_headers_);

// Create a stream.
dispatcher_->post([&]() -> void {
Expand Down