Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 0 additions & 2 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
@@ -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 = "9dce187eca9aa10e98c0ae81368c078384528801" # October 17th, 2020
ENVOY_SHA = "fa5a452d52475cfc64c0557c4258e1a91fdceb725131aaa02956e83523b23269"

HDR_HISTOGRAM_C_VERSION = "0.11.2" # October 12th, 2020
HDR_HISTOGRAM_C_SHA = "637f28b5f64de2e268131e4e34e6eef0b91cf5ff99167db447d9b2825eae6bad"
Expand All @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be the first instance where us just using envoy's .bazelrc isn't great for us. No action required here on this PR, but we should keep an eye on it.

url = "https://github.com/envoyproxy/envoy/archive/%s.tar.gz" % ENVOY_COMMIT,
# // clang-format on
)
http_archive(
name = "dep_hdrhistogram_c",
Expand Down Expand Up @@ -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
)
8 changes: 5 additions & 3 deletions source/client/stream_decoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ void StreamDecoder::decodeHeaders(Envoy::Http::ResponseHeaderMapPtr&& headers, b
stream_info_.response_code_ = static_cast<uint32_t>(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[0]->value().getStringView();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could possibly use a comment or maybe even an assertion of sorts. It looks like the envoy function was changed to now allow multiple HeaderEntries to be returned rather than one?

I gather that our use case would always expect only one - would it be reasonable to add a failure here if we received more than one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a sparse warning log line in dc0b618 and also testing for that case to make sure we don't track timings when there's ambiguity because of multiple header/value pairs.

int64_t origin_delta;
if (absl::SimpleAtoi(timing_value, &origin_delta) && origin_delta >= 0) {
origin_latency_statistic_.addValue(origin_delta);
Expand Down Expand Up @@ -142,6 +143,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;
Expand Down
6 changes: 3 additions & 3 deletions source/server/http_filter_config_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we have an else condition here now? again, taking into consideration receiving more than 1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. Done in dc0b618 + explicit tests for the case.

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<const nighthawk::server::ResponseOptions>(std::move(response_options));
Expand Down
3 changes: 2 additions & 1 deletion test/request_source/request_source_plugin_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
25 changes: 17 additions & 8 deletions test/server/http_dynamic_delay_filter_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -92,15 +98,17 @@ 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
// 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");
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.
Expand All @@ -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 {
Expand Down
16 changes: 10 additions & 6 deletions test/server/http_test_server_filter_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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());
}

Expand Down
21 changes: 10 additions & 11 deletions test/server/http_time_tracking_filter_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions test/stream_decoder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down