diff --git a/.bazelrc b/.bazelrc index ac352e308..b22616bce 100644 --- a/.bazelrc +++ b/.bazelrc @@ -11,10 +11,10 @@ build:macos --copt -UDEBUG # Bazel doesn't need more than 200MB of memory for local build based on memory profiling: # https://docs.bazel.build/versions/master/skylark/performance.html#memory-profiling # The default JVM max heapsize is 1/4 of physical memory up to 32GB which could be large -# enough to consume all memory constrained by cgroup in large host, which is the case in CircleCI. +# enough to consume all memory constrained by cgroup in large host. # Limiting JVM heapsize here to let it do GC more when approaching the limit to # leave room for compiler/linker. -# The number 2G is choosed heuristically to both support in CircleCI and large enough for RBE. +# The number 2G is chosen heuristically to both support large VM and small VM with RBE. # Startup options cannot be selected via config. startup --host_jvm_args=-Xmx2g @@ -201,8 +201,6 @@ build:remote --spawn_strategy=remote,sandboxed,local build:remote --strategy=Javac=remote,sandboxed,local build:remote --strategy=Closure=remote,sandboxed,local build:remote --strategy=Genrule=remote,sandboxed,local -# rules_rust is not remote runnable (yet) -build:remote --strategy=Rustc=sandboxed,local build:remote --remote_timeout=7200 build:remote --auth_enabled=true build:remote --remote_download_toplevel diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 583aa2a84..7b38a6368 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -1,7 +1,7 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") -ENVOY_COMMIT = "2097fe908f2abb718757dbd4087d793c861d7c5a" # October 12th, 2020 -ENVOY_SHA = "323360544ee355f0eddea742b31a80a94899090db1d64029cd22880083b311c0" +ENVOY_COMMIT = "f95f5391b0b8683081ec786ea946026594955fc6" # October 21st, 2020 +ENVOY_SHA = "1129dcb0e18ec79ab56f59cbe5150f564e32d9221edd7ba1bd84b9b5377cbe35" HDR_HISTOGRAM_C_VERSION = "0.11.2" # October 12th, 2020 HDR_HISTOGRAM_C_SHA = "637f28b5f64de2e268131e4e34e6eef0b91cf5ff99167db447d9b2825eae6bad" @@ -11,7 +11,9 @@ def nighthawk_dependencies(): name = "envoy", sha256 = ENVOY_SHA, strip_prefix = "envoy-%s" % ENVOY_COMMIT, + # // clang-format off: Envoy's format check: Only repository_locations.bzl may contains URL references url = "https://github.com/envoyproxy/envoy/archive/%s.tar.gz" % ENVOY_COMMIT, + # // clang-format on ) http_archive( name = "dep_hdrhistogram_c", @@ -50,5 +52,7 @@ cc_library( """, sha256 = HDR_HISTOGRAM_C_SHA, strip_prefix = "HdrHistogram_c-%s" % HDR_HISTOGRAM_C_VERSION, + # // clang-format off url = "https://github.com/HdrHistogram/HdrHistogram_c/archive/%s.tar.gz" % HDR_HISTOGRAM_C_VERSION, + # // clang-format on ) diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index 21bf877f9..86f4c681d 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -21,9 +21,11 @@ void StreamDecoder::decodeHeaders(Envoy::Http::ResponseHeaderMapPtr&& headers, b stream_info_.response_code_ = static_cast(response_code); if (!latency_response_header_name_.empty()) { const auto timing_header_name = Envoy::Http::LowerCaseString(latency_response_header_name_); - const Envoy::Http::HeaderEntry* timing_header = response_headers_->get(timing_header_name); - if (timing_header != nullptr) { - absl::string_view timing_value = timing_header->value().getStringView(); + const Envoy::Http::HeaderMap::GetResult& timing_header = + response_headers_->get(timing_header_name); + if (!timing_header.empty()) { + absl::string_view timing_value = + timing_header.size() == 1 ? timing_header[0]->value().getStringView() : "multiple values"; int64_t origin_delta; if (absl::SimpleAtoi(timing_value, &origin_delta) && origin_delta >= 0) { origin_latency_statistic_.addValue(origin_delta); @@ -142,6 +144,7 @@ StreamDecoder::streamResetReasonToResponseFlag(Envoy::Http::StreamResetReason re return Envoy::StreamInfo::ResponseFlag::LocalReset; case Envoy::Http::StreamResetReason::Overflow: return Envoy::StreamInfo::ResponseFlag::UpstreamOverflow; + case Envoy::Http::StreamResetReason::ConnectError: case Envoy::Http::StreamResetReason::RemoteReset: case Envoy::Http::StreamResetReason::RemoteRefusedStreamReset: return Envoy::StreamInfo::ResponseFlag::UpstreamRemoteReset; diff --git a/source/server/http_filter_config_base.cc b/source/server/http_filter_config_base.cc index a8dc933e9..b76505023 100644 --- a/source/server/http_filter_config_base.cc +++ b/source/server/http_filter_config_base.cc @@ -13,17 +13,23 @@ FilterConfigurationBase::FilterConfigurationBase( void FilterConfigurationBase::computeEffectiveConfiguration( const Envoy::Http::RequestHeaderMap& headers) { - const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig); - if (request_config_header) { + const auto& request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig); + if (request_config_header.size() == 1) { + // We could be more flexible and look for the first request header that has a value, + // but without a proper understanding of a real use case for that, we are assuming that any + // existence of duplicate headers here is an error. nighthawk::server::ResponseOptions response_options = *server_config_; std::string error_message; - if (Configuration::mergeJsonConfig(request_config_header->value().getStringView(), + if (Configuration::mergeJsonConfig(request_config_header[0]->value().getStringView(), response_options, error_message)) { effective_config_ = std::make_shared(std::move(response_options)); } else { effective_config_ = absl::InvalidArgumentError(error_message); } + } else if (request_config_header.size() > 1) { + effective_config_ = absl::InvalidArgumentError( + "Received multiple configuration headers in the request, expected only one."); } } diff --git a/test/request_source/request_source_plugin_test.cc b/test/request_source/request_source_plugin_test.cc index a7fa62cec..fc20b64ea 100644 --- a/test/request_source/request_source_plugin_test.cc +++ b/test/request_source/request_source_plugin_test.cc @@ -90,7 +90,8 @@ TEST_F(StubRequestSourcePluginTest, CreateRequestSourcePluginCreatesWorkingPlugi Nighthawk::RequestGenerator generator = plugin->get(); Nighthawk::RequestPtr request = generator(); Nighthawk::HeaderMapPtr header = request->header(); - EXPECT_EQ(header->get(Envoy::Http::LowerCaseString("test_value"))->value().getStringView(), + ASSERT_EQ(header->get(Envoy::Http::LowerCaseString("test_value")).size(), 1); + EXPECT_EQ(header->get(Envoy::Http::LowerCaseString("test_value"))[0]->value().getStringView(), absl::string_view(std::to_string(test_value))); } TEST_F(FileBasedRequestSourcePluginTest, CreateEmptyConfigProtoCreatesCorrectType) { diff --git a/test/server/http_dynamic_delay_filter_integration_test.cc b/test/server/http_dynamic_delay_filter_integration_test.cc index 3840a11ec..0dfdb797b 100644 --- a/test/server/http_dynamic_delay_filter_integration_test.cc +++ b/test/server/http_dynamic_delay_filter_integration_test.cc @@ -48,19 +48,21 @@ name: dynamic-delay // 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); + EXPECT_TRUE(upstream_request_->headers().get(kDelayHeaderString).empty()); // 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); + EXPECT_TRUE(upstream_request_->headers().get(kDelayHeaderString).empty()); // 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"); + ASSERT_EQ(upstream_request_->headers().get(kDelayHeaderString).size(), 1); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)[0]->value().getStringView(), + "1600"); } // Verify expectations with static/file-based static_delay configuration. @@ -75,13 +77,17 @@ name: dynamic-delay // 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"); + ASSERT_EQ(upstream_request_->headers().get(kDelayHeaderString).size(), 1); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)[0]->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"); + ASSERT_EQ(upstream_request_->headers().get(kDelayHeaderString).size(), 1); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)[0]->value().getStringView(), + "1330"); // Overriding the statically configured static delay via request-level configuration should be // reflected in the output. @@ -92,7 +98,7 @@ name: dynamic-delay // 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(kDelayHeaderString)->value().getStringView(), + // EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)[0]->value().getStringView(), // "200"); // Overriding the statically configured static delay via request-level configuration should be @@ -100,7 +106,9 @@ name: dynamic-delay setRequestLevelConfiguration("{static_delay: \"2.2s\"}"); getResponse(ResponseOrigin::UPSTREAM); // 2.2 seconds -> 2200 ms. - EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)->value().getStringView(), "2200"); + ASSERT_EQ(upstream_request_->headers().get(kDelayHeaderString).size(), 1); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)[0]->value().getStringView(), + "2200"); } // Verify expectations with static/file-based concurrency_based_linear_delay configuration. @@ -116,7 +124,8 @@ name: dynamic-delay 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"); + ASSERT_EQ(upstream_request_->headers().get(kDelayHeaderString).size(), 1); + EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString)[0]->value().getStringView(), "60"); } class ComputeTest : public testing::Test { diff --git a/test/server/http_filter_base_test.cc b/test/server/http_filter_base_test.cc index 212bd59b5..adf90896f 100644 --- a/test/server/http_filter_base_test.cc +++ b/test/server/http_filter_base_test.cc @@ -90,5 +90,25 @@ TEST_P(HttpFilterBaseIntegrationTest, EmptyRequestLevelConfigurationShouldFail) EXPECT_THAT(response->body(), HasSubstr(kBadConfigErrorSentinel)); } +TEST_P(HttpFilterBaseIntegrationTest, MultipleValidConfigurationHeadersFails) { + // Make sure we fail when two valid configuration headers are send. + setRequestLevelConfiguration("{}"); + appendRequestLevelConfiguration("{}"); + Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::EXTENSION); + ASSERT_TRUE(response->complete()); + EXPECT_THAT(response->body(), + HasSubstr("Received multiple configuration headers in the request")); +} + +TEST_P(HttpFilterBaseIntegrationTest, SingleValidPlusEmptyConfigurationHeadersFails) { + // Make sure we fail when both a valid configuration plus an empty configuration header is send. + setRequestLevelConfiguration("{}"); + appendRequestLevelConfiguration(""); + Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::EXTENSION); + ASSERT_TRUE(response->complete()); + EXPECT_THAT(response->body(), + HasSubstr("Received multiple configuration headers in the request")); +} + } // namespace } // namespace Nighthawk \ No newline at end of file diff --git a/test/server/http_filter_integration_test_base.cc b/test/server/http_filter_integration_test_base.cc index d204133ad..009ae2928 100644 --- a/test/server/http_filter_integration_test_base.cc +++ b/test/server/http_filter_integration_test_base.cc @@ -21,6 +21,12 @@ void HttpFilterIntegrationTestBase::setRequestLevelConfiguration( setRequestHeader(Server::TestServer::HeaderNames::get().TestServerConfig, request_level_config); } +void HttpFilterIntegrationTestBase::appendRequestLevelConfiguration( + absl::string_view request_level_config) { + appendRequestHeader(Server::TestServer::HeaderNames::get().TestServerConfig, + request_level_config); +} + void HttpFilterIntegrationTestBase::switchToPostWithEntityBody() { setRequestHeader(Envoy::Http::Headers::get().Method, Envoy::Http::Headers::get().MethodValues.Post); @@ -31,6 +37,11 @@ void HttpFilterIntegrationTestBase::setRequestHeader( request_headers_.setCopy(header_name, header_value); } +void HttpFilterIntegrationTestBase::appendRequestHeader( + const Envoy::Http::LowerCaseString& header_name, absl::string_view header_value) { + request_headers_.addCopy(header_name, header_value); +} + Envoy::IntegrationStreamDecoderPtr HttpFilterIntegrationTestBase::getResponse(ResponseOrigin expected_origin) { cleanupUpstreamAndDownstream(); diff --git a/test/server/http_filter_integration_test_base.h b/test/server/http_filter_integration_test_base.h index 8027753ec..53b5cc79e 100644 --- a/test/server/http_filter_integration_test_base.h +++ b/test/server/http_filter_integration_test_base.h @@ -52,6 +52,16 @@ class HttpFilterIntegrationTestBase : public Envoy::HttpIntegrationTest { */ void setRequestLevelConfiguration(absl::string_view request_level_config); + /** + * Make getResponse add request-level configuration. Test server extensions read that + * configuration and merge it with their static configuration to determine a final effective + * configuration. See TestServerConfig in well_known_headers.h for the up to date header name. + * + * @param request_level_config Configuration to be delivered by request-header in future calls to + * getResponse(). For example: "{response_body_size:1024}". + */ + void appendRequestLevelConfiguration(absl::string_view request_level_config); + /** * Switch getResponse() to use the POST request method with an entity body. * Doing so will make tests hit a different code paths in extensions. @@ -67,6 +77,15 @@ class HttpFilterIntegrationTestBase : public Envoy::HttpIntegrationTest { void setRequestHeader(const Envoy::Http::LowerCaseString& header_name, absl::string_view header_value); + /** + * Appends a request header value. + * + * @param header_name Name of the request header to set. + * @param header_value Value to set for the request header. + */ + void appendRequestHeader(const Envoy::Http::LowerCaseString& header_name, + absl::string_view header_value); + /** * Fetch a response, according to the options specified by the class methods. By default, * simulates a GET request with minimal headers. diff --git a/test/server/http_test_server_filter_integration_test.cc b/test/server/http_test_server_filter_integration_test.cc index 8b355101f..5850dab7e 100644 --- a/test/server/http_test_server_filter_integration_test.cc +++ b/test/server/http_test_server_filter_integration_test.cc @@ -38,8 +38,8 @@ class HttpTestServerIntegrationTest : public HttpFilterIntegrationTestBase, EXPECT_EQ("200", response->headers().Status()->value().getStringView()); if (expect_header) { auto inserted_header = response->headers().get(Envoy::Http::LowerCaseString("x-supplied-by")); - ASSERT_NE(nullptr, inserted_header); - EXPECT_EQ("nighthawk-test-server", inserted_header->value().getStringView()); + ASSERT_EQ(1, inserted_header.size()); + EXPECT_EQ("nighthawk-test-server", inserted_header[0]->value().getStringView()); } if (response_body_size == 0) { EXPECT_EQ(nullptr, response->headers().ContentType()); @@ -107,8 +107,10 @@ TEST_P(HttpTestServerIntegrationTest, TestHeaderConfig) { Envoy::IntegrationStreamDecoderPtr response = getResponse(ResponseOrigin::EXTENSION); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); - EXPECT_EQ("bar2", - response->headers().get(Envoy::Http::LowerCaseString("foo"))->value().getStringView()); + ASSERT_EQ(1, response->headers().get(Envoy::Http::LowerCaseString("foo")).size()); + EXPECT_EQ( + "bar2", + response->headers().get(Envoy::Http::LowerCaseString("foo"))[0]->value().getStringView()); EXPECT_EQ(std::string(10, 'a'), response->body()); } @@ -180,8 +182,10 @@ TEST_P(HttpTestServerIntegrationTest, TestNoStaticConfigHeaderConfig) { ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); - EXPECT_EQ("bar2", - response->headers().get(Envoy::Http::LowerCaseString("foo"))->value().getStringView()); + ASSERT_EQ(1, response->headers().get(Envoy::Http::LowerCaseString("foo")).size()); + EXPECT_EQ( + "bar2", + response->headers().get(Envoy::Http::LowerCaseString("foo"))[0]->value().getStringView()); EXPECT_EQ("", response->body()); } diff --git a/test/server/http_time_tracking_filter_integration_test.cc b/test/server/http_time_tracking_filter_integration_test.cc index 5f1348c56..c61124392 100644 --- a/test/server/http_time_tracking_filter_integration_test.cc +++ b/test/server/http_time_tracking_filter_integration_test.cc @@ -44,16 +44,15 @@ TEST_P(HttpTimeTrackingIntegrationTest, ReturnsPositiveLatencyForStaticConfigura // 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); + EXPECT_EQ( + response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)).size(), 0); // On the second request we should observe a delta. response = getResponse(ResponseOrigin::UPSTREAM); - const Envoy::Http::HeaderEntry* latency_header_2 = + const Envoy::Http::HeaderMap::GetResult& latency_header = response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)); - ASSERT_NE(latency_header_2, nullptr); - EXPECT_TRUE(absl::SimpleAtoi(latency_header_2->value().getStringView(), &latency)); + ASSERT_EQ(latency_header.size(), 1); + EXPECT_TRUE(absl::SimpleAtoi(latency_header[0]->value().getStringView(), &latency)); EXPECT_GT(latency, 0); } @@ -63,18 +62,18 @@ TEST_P(HttpTimeTrackingIntegrationTest, ReturnsPositiveLatencyForPerRequestConfi // 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); + EXPECT_TRUE( + response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)).empty()); // 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 = + const Envoy::Http::HeaderMap::GetResult& latency_header = response->headers().get(Envoy::Http::LowerCaseString(kLatencyResponseHeaderName)); - ASSERT_NE(latency_header, nullptr); + ASSERT_EQ(latency_header.size(), 1); int64_t latency; - EXPECT_TRUE(absl::SimpleAtoi(latency_header->value().getStringView(), &latency)); + EXPECT_TRUE(absl::SimpleAtoi(latency_header[0]->value().getStringView(), &latency)); // TODO(oschaaf): figure out if we can use simtime here, and verify actual timing matches // what we'd expect using that. EXPECT_GT(latency, 0); diff --git a/test/stream_decoder_test.cc b/test/stream_decoder_test.cc index 8c426bbd8..4603c0e65 100644 --- a/test/stream_decoder_test.cc +++ b/test/stream_decoder_test.cc @@ -211,6 +211,9 @@ TEST_F(StreamDecoderTest, StreamResetReasonToResponseFlag) { ASSERT_EQ(StreamDecoder::streamResetReasonToResponseFlag( Envoy::Http::StreamResetReason::RemoteRefusedStreamReset), Envoy::StreamInfo::ResponseFlag::UpstreamRemoteReset); + ASSERT_EQ( + StreamDecoder::streamResetReasonToResponseFlag(Envoy::Http::StreamResetReason::ConnectError), + Envoy::StreamInfo::ResponseFlag::UpstreamRemoteReset); } // This test parameterization structure carries the response header name that ought to be treated @@ -245,5 +248,22 @@ TEST_P(LatencyTrackingViaResponseHeaderTest, LatencyTrackingViaResponseHeader) { EXPECT_EQ(origin_latency_statistic_.count(), expected_count); } +// Test that a single response carrying multiple valid latency response headers does not +// get tracked. This will also yield a burst of warnings, which we unfortunately cannot +// easily verify here. +TEST_F(StreamDecoderTest, LatencyTrackingWithMultipleResponseHeadersFails) { + const std::string kLatencyTrackingResponseHeader = "latency-in-response-header"; + auto decoder = new StreamDecoder( + *dispatcher_, time_system_, *this, [](bool, bool) {}, connect_statistic_, latency_statistic_, + response_header_size_statistic_, response_body_size_statistic_, origin_latency_statistic_, + request_headers_, false, 0, random_generator_, http_tracer_, kLatencyTrackingResponseHeader); + Envoy::Http::ResponseHeaderMapPtr headers{ + new Envoy::Http::TestResponseHeaderMapImpl{{":status", "200"}, + {kLatencyTrackingResponseHeader, "1"}, + {kLatencyTrackingResponseHeader, "2"}}}; + decoder->decodeHeaders(std::move(headers), true); + EXPECT_EQ(origin_latency_statistic_.count(), 0); +} + } // namespace Client } // namespace Nighthawk