From 164d98b7cc8f64c23766693ffd3d3499848ce15b Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 23 Sep 2020 22:00:21 +0200 Subject: [PATCH 1/2] Add generic c++ integration test for uniform extension behavior. (#533) Test basic behavioural properties of all test-server extensions. Any new extensions may piggy-back on this by enlisting themselves. In a follow up we'll purge code from the pre-existing tests that can now be deprecated. Split out from #512 Signed-off-by: Otto van der Schaaf --- test/server/BUILD | 16 ++++- test/server/http_filter_base_test.cc | 94 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 test/server/http_filter_base_test.cc diff --git a/test/server/BUILD b/test/server/BUILD index 71ae178ab..68d5bab08 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -20,15 +20,26 @@ envoy_cc_test_library( ], ) +envoy_cc_test( + name = "http_filter_base_test", + srcs = ["http_filter_base_test.cc"], + repository = "@envoy", + deps = [ + ":http_filter_integration_test_base_lib", + "//source/server:http_dynamic_delay_filter_config", + "//source/server:http_test_server_filter_config", + "//source/server:http_time_tracking_filter_config", + ], +) + envoy_cc_test( name = "http_test_server_filter_integration_test", srcs = ["http_test_server_filter_integration_test.cc"], repository = "@envoy", deps = [ + ":http_filter_integration_test_base_lib", "//source/server:http_test_server_filter_config", - "@envoy//include/envoy/upstream:cluster_manager_interface_with_external_headers", "@envoy//source/common/api:api_lib_with_external_headers", - "@envoy//test/integration:http_integration_lib", ], ) @@ -50,7 +61,6 @@ envoy_cc_test( deps = [ ":http_filter_integration_test_base_lib", "//source/server:http_time_tracking_filter_config", - "@envoy//include/envoy/upstream:cluster_manager_interface_with_external_headers", "@envoy//source/common/api:api_lib_with_external_headers", "@envoy//test/test_common:simulated_time_system_lib", ], diff --git a/test/server/http_filter_base_test.cc b/test/server/http_filter_base_test.cc new file mode 100644 index 000000000..212bd59b5 --- /dev/null +++ b/test/server/http_filter_base_test.cc @@ -0,0 +1,94 @@ +#include "server/http_dynamic_delay_filter.h" +#include "server/http_test_server_filter.h" +#include "server/http_time_tracking_filter.h" + +#include "test/server/http_filter_integration_test_base.h" + +#include "gtest/gtest.h" + +namespace Nighthawk { +namespace { + +using ::testing::HasSubstr; + +enum TestRequestMethod { GET, POST }; + +const std::string kBadConfigErrorSentinel = + "didn't understand the request: Error merging json config: Unable to parse " + "JSON as proto (INVALID_ARGUMENT:Unexpected"; + +class HttpFilterBaseIntegrationTest + : public HttpFilterIntegrationTestBase, + public testing::TestWithParam< + std::tuple> { +public: + HttpFilterBaseIntegrationTest() + : HttpFilterIntegrationTestBase(std::get<0>(GetParam())), config_(std::get<1>(GetParam())) { + initializeFilterConfiguration(config_); + if (std::get<2>(GetParam()) == TestRequestMethod::POST) { + switchToPostWithEntityBody(); + } + }; + + ResponseOrigin getHappyFlowResponseOrigin() { + // Modulo the test-server, extensions are expected to need an upstream to synthesize a reply + // when the effective configuration is valid. + return config_.find_first_of("name: test-server") == 0 ? ResponseOrigin::EXTENSION + : ResponseOrigin::UPSTREAM; + } + +protected: + const std::string config_; +}; + +INSTANTIATE_TEST_SUITE_P( + IpVersions, HttpFilterBaseIntegrationTest, + ::testing::Combine(testing::ValuesIn(Envoy::TestEnvironment::getIpVersionsForTest()), + testing::ValuesIn({absl::string_view(R"EOF( +name: time-tracking +typed_config: + "@type": type.googleapis.com/nighthawk.server.ResponseOptions + emit_previous_request_delta_in_response_header: "foo" +)EOF"), + absl::string_view(R"EOF( +name: dynamic-delay +typed_config: + "@type": type.googleapis.com/nighthawk.server.ResponseOptions + static_delay: 0.1s +)EOF"), + absl::string_view("name: test-server")}), + testing::ValuesIn({TestRequestMethod::GET, TestRequestMethod::POST}))); + +TEST_P(HttpFilterBaseIntegrationTest, NoRequestLevelConfigurationShouldSucceed) { + Envoy::IntegrationStreamDecoderPtr response = getResponse(getHappyFlowResponseOrigin()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_TRUE(response->body().empty()); +} + +TEST_P(HttpFilterBaseIntegrationTest, EmptyJsonRequestLevelConfigurationShouldSucceed) { + setRequestLevelConfiguration("{}"); + Envoy::IntegrationStreamDecoderPtr response = getResponse(getHappyFlowResponseOrigin()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_TRUE(response->body().empty()); +} + +TEST_P(HttpFilterBaseIntegrationTest, BadJsonAsRequestLevelConfigurationShouldFail) { + // When sending bad request-level configuration, the extension ought to reply directly. + setRequestLevelConfiguration("bad_json"); + Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::EXTENSION); + EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500); + EXPECT_THAT(response->body(), HasSubstr(kBadConfigErrorSentinel)); +} + +TEST_P(HttpFilterBaseIntegrationTest, EmptyRequestLevelConfigurationShouldFail) { + // When sending empty request-level configuration, the extension ought to reply directly. + setRequestLevelConfiguration(""); + Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::EXTENSION); + EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500); + EXPECT_THAT(response->body(), HasSubstr(kBadConfigErrorSentinel)); +} + +} // namespace +} // namespace Nighthawk \ No newline at end of file From b189069e940e3f67e52c98ed62996bb9ec2a9213 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 24 Sep 2020 08:58:07 +0200 Subject: [PATCH 2/2] Use shared test facilities for time-tracking and dynamic-delay (#541) Switch tests for these extensions to use the recent shared test facilities. Eliminate tests we generalize in #533. Split out from #512 Signed-off-by: Otto van der Schaaf --- ...p_dynamic_delay_filter_integration_test.cc | 151 +++++++----------- ...p_time_tracking_filter_integration_test.cc | 105 +++--------- 2 files changed, 82 insertions(+), 174 deletions(-) diff --git a/test/server/http_dynamic_delay_filter_integration_test.cc b/test/server/http_dynamic_delay_filter_integration_test.cc index c4a38e429..3840a11ec 100644 --- a/test/server/http_dynamic_delay_filter_integration_test.cc +++ b/test/server/http_dynamic_delay_filter_integration_test.cc @@ -1,16 +1,18 @@ #include -#include "external/envoy/test/integration/http_integration.h" - #include "api/server/response_options.pb.h" #include "server/configuration.h" #include "server/http_dynamic_delay_filter.h" +#include "test/server/http_filter_integration_test_base.h" + #include "gtest/gtest.h" namespace Nighthawk { +const Envoy::Http::LowerCaseString kDelayHeaderString("x-envoy-fault-delay-request"); + /** * Support class for testing the dynamic delay filter. We rely on the fault filter for * inducing the actual delay, so this aims to prove that: @@ -20,57 +22,17 @@ namespace Nighthawk { * - Failure modes work. * - TODO(#393): An end to end test which proves that the interaction between this filter * and the fault filter work as expected. + * + * The Dynamic Delay filter communicates with the fault filter by adding kDelayHeaderString + * to the request headers. We use that in tests below to verify expectations. The fault filter + * accepts input values via request headers specified in milliseconds, so our expectations are + * also using milliseconds. */ class HttpDynamicDelayIntegrationTest - : public Envoy::HttpIntegrationTest, + : public HttpFilterIntegrationTestBase, public testing::TestWithParam { -protected: - HttpDynamicDelayIntegrationTest() - : HttpIntegrationTest(Envoy::Http::CodecClient::Type::HTTP1, GetParam()), - request_headers_({{":method", "GET"}, {":path", "/"}, {":authority", "host"}}), - delay_header_string_(Envoy::Http::LowerCaseString("x-envoy-fault-delay-request")) {} - - // We don't override SetUp(): tests in this file will call setup() instead to avoid having to - // create a fixture per filter configuration. - void setup(const std::string& config) { - config_helper_.addFilter(config); - HttpIntegrationTest::initialize(); - } - - // Fetches a response with request-level configuration set in the request header. - Envoy::IntegrationStreamDecoderPtr getResponse(absl::string_view request_level_config, - bool setup_for_upstream_request = true) { - const Envoy::Http::LowerCaseString key("x-nighthawk-test-server-config"); - Envoy::Http::TestRequestHeaderMapImpl request_headers = request_headers_; - request_headers.setCopy(key, request_level_config); - return getResponse(request_headers, setup_for_upstream_request); - } - - // Fetches a response with the default request headers, expecting the fake upstream to supply - // the response. - Envoy::IntegrationStreamDecoderPtr getResponse() { return getResponse(request_headers_); } - - // Fetches a response using the provided request headers. When setup_for_upstream_request - // is true, the expectation will be that an upstream request will be needed to provide a - // response. If it is set to false, the extension is expected to supply the response, and - // no upstream request ought to occur. - Envoy::IntegrationStreamDecoderPtr - getResponse(const Envoy::Http::TestRequestHeaderMapImpl& request_headers, - bool setup_for_upstream_request = true) { - cleanupUpstreamAndDownstream(); - codec_client_ = makeHttpConnection(lookupPort("http")); - Envoy::IntegrationStreamDecoderPtr response; - if (setup_for_upstream_request) { - response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); - } else { - response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); - } - return response; - } - - const Envoy::Http::TestRequestHeaderMapImpl request_headers_; - const Envoy::Http::LowerCaseString delay_header_string_; +public: + HttpDynamicDelayIntegrationTest() : HttpFilterIntegrationTestBase(GetParam()){}; }; INSTANTIATE_TEST_SUITE_P(IpVersions, HttpDynamicDelayIntegrationTest, @@ -78,69 +40,72 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, HttpDynamicDelayIntegrationTest, // Verify expectations with an empty dynamic-delay configuration. TEST_P(HttpDynamicDelayIntegrationTest, NoStaticConfiguration) { - setup(R"( + initializeFilterConfiguration(R"( name: dynamic-delay typed_config: "@type": type.googleapis.com/nighthawk.server.ResponseOptions )"); - // Don't send any config request header - getResponse(); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_), nullptr); - // Send a config request header with an empty / default config. Should be a no-op. - getResponse("{}"); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_), nullptr); - // Send a config request header, this should become effective. - getResponse("{static_delay: \"1.6s\"}"); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_)->value().getStringView(), - "1600"); - - // Send a malformed config request header. This ought to shortcut and directly reply, - // hence we don't expect an upstream request. - auto response = getResponse("bad_json", false); - EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500); - EXPECT_EQ( - response->body(), - "dynamic-delay didn't understand the request: Error merging json config: Unable to parse " - "JSON as proto (INVALID_ARGUMENT:Unexpected token.\nbad_json\n^): bad_json"); - // Send an empty config header, which ought to trigger failure mode as well. - response = getResponse("", false); - EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500); - EXPECT_EQ( - response->body(), - "dynamic-delay didn't understand the request: Error merging json config: Unable to " - "parse JSON as proto (INVALID_ARGUMENT:Unexpected end of string. Expected a value.\n\n^): "); + // Don't send any config request header ... + getResponse(ResponseOrigin::UPSTREAM); + // ... we shouldn't observe any delay being requested via the upstream request headers. + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString), nullptr); + + // Send a config request header with an empty / default configuration .... + setRequestLevelConfiguration("{}"); + getResponse(ResponseOrigin::UPSTREAM); + // ... we shouldn't observe any delay being requested via the upstream request headers. + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString), nullptr); + + // Send a config request header requesting a 1.6s delay... + setRequestLevelConfiguration("{static_delay: \"1.6s\"}"); + getResponse(ResponseOrigin::UPSTREAM); + // ...we should observe a delay of 1.6s in the upstream request. + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), "1600"); } // Verify expectations with static/file-based static_delay configuration. TEST_P(HttpDynamicDelayIntegrationTest, StaticConfigurationStaticDelay) { - setup(R"EOF( + initializeFilterConfiguration(R"EOF( name: dynamic-delay typed_config: "@type": type.googleapis.com/nighthawk.server.ResponseOptions static_delay: 1.33s )EOF"); - getResponse(); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_)->value().getStringView(), - "1330"); - getResponse("{}"); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_)->value().getStringView(), - "1330"); - getResponse("{static_delay: \"0.2s\"}"); + + // Without any request-level configuration, we expect the statically configured static delay to + // apply. + getResponse(ResponseOrigin::UPSTREAM); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), "1330"); + + // With an empty request-level configuration, we expect the statically configured static delay to + // apply. + setRequestLevelConfiguration("{}"); + getResponse(ResponseOrigin::UPSTREAM); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), "1330"); + + // Overriding the statically configured static delay via request-level configuration should be + // reflected in the output. + setRequestLevelConfiguration("{static_delay: \"0.2s\"}"); + getResponse(ResponseOrigin::UPSTREAM); // TODO(#392): This fails, because the duration is a two-field message: it would make here to see // both the number of seconds and nanoseconds to be overridden. // However, the seconds part is set to '0', which equates to the default of the underlying int // type, and the fact that we are using proto3, which doesn't merge default values. // Hence the following expectation will fail, as it yields 1200 instead of the expected 200. - // EXPECT_EQ(upstream_request_->headers().get(delay_header_string_)->value().getStringView(), + // EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), // "200"); - getResponse("{static_delay: \"2.2s\"}"); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_)->value().getStringView(), - "2200"); + + // Overriding the statically configured static delay via request-level configuration should be + // reflected in the output. + setRequestLevelConfiguration("{static_delay: \"2.2s\"}"); + getResponse(ResponseOrigin::UPSTREAM); + // 2.2 seconds -> 2200 ms. + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), "2200"); } // Verify expectations with static/file-based concurrency_based_linear_delay configuration. TEST_P(HttpDynamicDelayIntegrationTest, StaticConfigurationConcurrentDelay) { - setup(R"EOF( + initializeFilterConfiguration(R"EOF( name: dynamic-delay typed_config: "@type": type.googleapis.com/nighthawk.server.ResponseOptions @@ -148,8 +113,10 @@ name: dynamic-delay minimal_delay: 0.05s concurrency_delay_factor: 0.01s )EOF"); - getResponse(); - EXPECT_EQ(upstream_request_->headers().get(delay_header_string_)->value().getStringView(), "60"); + getResponse(ResponseOrigin::UPSTREAM); + // Based on the algorithm of concurrency_based_linear_delay, for the first request we expect to + // observe the configured minimal_delay + concurrency_delay_factor = 0.06s -> 60ms. + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), "60"); } class ComputeTest : public testing::Test { diff --git a/test/server/http_time_tracking_filter_integration_test.cc b/test/server/http_time_tracking_filter_integration_test.cc index 55d08e9c9..5f1348c56 100644 --- a/test/server/http_time_tracking_filter_integration_test.cc +++ b/test/server/http_time_tracking_filter_integration_test.cc @@ -1,10 +1,5 @@ #include -#include "envoy/upstream/cluster_manager.h" -#include "envoy/upstream/upstream.h" - -#include "external/envoy/test/common/upstream/utility.h" -#include "external/envoy/test/integration/http_integration.h" #include "external/envoy/test/test_common/simulated_time_system.h" #include "api/server/response_options.pb.h" @@ -12,7 +7,8 @@ #include "server/configuration.h" #include "server/http_time_tracking_filter.h" -#include "server/well_known_headers.h" + +#include "test/server/http_filter_integration_test_base.h" #include "gtest/gtest.h" @@ -32,53 +28,10 @@ name: time-tracking )EOF"; class HttpTimeTrackingIntegrationTest - : public Envoy::HttpIntegrationTest, + : public HttpFilterIntegrationTestBase, public testing::TestWithParam { -protected: - HttpTimeTrackingIntegrationTest() - : HttpIntegrationTest(Envoy::Http::CodecClient::Type::HTTP1, GetParam()), - request_headers_({{":method", "GET"}, {":path", "/"}, {":authority", "host"}}) {} - - // We don't override SetUp(): tests in this file will call setup() instead to avoid having to - // create a fixture per filter configuration. - void setup(const std::string& config) { - config_helper_.addFilter(config); - HttpIntegrationTest::initialize(); - } - - // Fetches a response with request-level configuration set in the request header. - Envoy::IntegrationStreamDecoderPtr getResponse(absl::string_view request_level_config, - bool setup_for_upstream_request = true) { - Envoy::Http::TestRequestHeaderMapImpl request_headers = request_headers_; - request_headers.setCopy(Nighthawk::Server::TestServer::HeaderNames::get().TestServerConfig, - request_level_config); - return getResponse(request_headers, setup_for_upstream_request); - } - - // Fetches a response with the default request headers, expecting the fake upstream to supply - // the response. - Envoy::IntegrationStreamDecoderPtr getResponse() { return getResponse(request_headers_); } - - // Fetches a response using the provided request headers. When setup_for_upstream_request - // is true, the expectation will be that an upstream request will be needed to provide a - // response. If it is set to false, the extension is expected to supply the response, and - // no upstream request ought to occur. - Envoy::IntegrationStreamDecoderPtr - getResponse(const Envoy::Http::TestRequestHeaderMapImpl& request_headers, - bool setup_for_upstream_request = true) { - cleanupUpstreamAndDownstream(); - codec_client_ = makeHttpConnection(lookupPort("http")); - Envoy::IntegrationStreamDecoderPtr response; - if (setup_for_upstream_request) { - response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); - } else { - response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); - } - return response; - } - - const Envoy::Http::TestRequestHeaderMapImpl request_headers_; +public: + HttpTimeTrackingIntegrationTest() : HttpFilterIntegrationTestBase(GetParam()){}; }; INSTANTIATE_TEST_SUITE_P(IpVersions, HttpTimeTrackingIntegrationTest, @@ -86,13 +39,17 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, HttpTimeTrackingIntegrationTest, // Verify expectations with static/file-based time-tracking configuration. TEST_P(HttpTimeTrackingIntegrationTest, ReturnsPositiveLatencyForStaticConfiguration) { - setup(fmt::format(kProtoConfigTemplate, kDefaultProtoFragment)); - Envoy::IntegrationStreamDecoderPtr response = getResponse(); + initializeFilterConfiguration(fmt::format(kProtoConfigTemplate, kDefaultProtoFragment)); + + // As the first request doesn't have a prior one, we should not observe a delta. + Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::UPSTREAM); int64_t latency; const Envoy::Http::HeaderEntry* latency_header_1 = response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)); EXPECT_EQ(latency_header_1, nullptr); - response = getResponse(); + + // On the second request we should observe a delta. + response = getResponse(ResponseOrigin::UPSTREAM); const Envoy::Http::HeaderEntry* latency_header_2 = response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)); ASSERT_NE(latency_header_2, nullptr); @@ -102,14 +59,17 @@ TEST_P(HttpTimeTrackingIntegrationTest, ReturnsPositiveLatencyForStaticConfigura // Verify expectations with an empty time-tracking configuration. TEST_P(HttpTimeTrackingIntegrationTest, ReturnsPositiveLatencyForPerRequestConfiguration) { - setup(fmt::format(kProtoConfigTemplate, "")); - // Don't send any config request header - getResponse(); - // Send a config request header with an empty / default config. Should be a no-op. - getResponse("{}"); - // Send a config request header, this should become effective. - Envoy::IntegrationStreamDecoderPtr response = - getResponse(fmt::format("{{{}}}", kDefaultProtoFragment)); + initializeFilterConfiguration(fmt::format(kProtoConfigTemplate, "")); + // As the first request doesn't have a prior one, we should not observe a delta. + setRequestLevelConfiguration("{}"); + Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::UPSTREAM); + EXPECT_EQ(response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)), + nullptr); + + // With request level configuration indicating that the timing header should be emitted, + // we should be able to observe it. + setRequestLevelConfiguration(fmt::format("{{{}}}", kDefaultProtoFragment)); + response = getResponse(ResponseOrigin::UPSTREAM); const Envoy::Http::HeaderEntry* latency_header = response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)); ASSERT_NE(latency_header, nullptr); @@ -120,25 +80,6 @@ TEST_P(HttpTimeTrackingIntegrationTest, ReturnsPositiveLatencyForPerRequestConfi EXPECT_GT(latency, 0); } -TEST_P(HttpTimeTrackingIntegrationTest, BehavesWellWithBadPerRequestConfiguration) { - setup(fmt::format(kProtoConfigTemplate, "")); - // Send a malformed config request header. This ought to shortcut and directly reply, - // hence we don't expect an upstream request. - Envoy::IntegrationStreamDecoderPtr response = getResponse("bad_json", false); - EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500); - EXPECT_EQ( - response->body(), - "time-tracking didn't understand the request: Error merging json config: Unable to parse " - "JSON as proto (INVALID_ARGUMENT:Unexpected token.\nbad_json\n^): bad_json"); - // Send an empty config header, which ought to trigger failure mode as well. - response = getResponse("", false); - EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500); - EXPECT_EQ( - response->body(), - "time-tracking didn't understand the request: Error merging json config: Unable to " - "parse JSON as proto (INVALID_ARGUMENT:Unexpected end of string. Expected a value.\n\n^): "); -} - class HttpTimeTrackingFilterConfigTest : public testing::Test, public Envoy::Event::TestUsingSimulatedTime {};