Skip to content

Commit 9be5334

Browse files
oschaafoschaaf
committed
Partial review feedback
Signed-off-by: Otto van der Schaaf <[email protected]>
1 parent ac456f3 commit 9be5334

10 files changed

+102
-75
lines changed

‎source/server/BUILD‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,18 @@ licenses(["notice"]) # Apache 2
99
envoy_package()
1010

1111
envoy_cc_library(
12-
name = "common_lib",
13-
srcs = ["common.cc"],
14-
hdrs = ["common.h"],
12+
name = "well_known_headers_lib",
13+
hdrs = ["well_known_headers.h"],
14+
repository = "@envoy",
15+
deps = [
16+
"@envoy//source/common/singleton:const_singleton_with_external_headers",
17+
],
18+
)
19+
20+
envoy_cc_library(
21+
name = "configuration_lib",
22+
srcs = ["configuration.cc"],
23+
hdrs = ["configuration.h"],
1524
repository = "@envoy",
1625
deps = [
1726
"//api/server:response_options_proto_cc_proto",
@@ -27,7 +36,8 @@ envoy_cc_library(
2736
hdrs = ["http_test_server_filter.h"],
2837
repository = "@envoy",
2938
deps = [
30-
":common_lib",
39+
":configuration_lib",
40+
":well_known_headers_lib",
3141
"//api/server:response_options_proto_cc_proto",
3242
"@envoy//source/exe:envoy_common_lib_with_external_headers",
3343
],
@@ -39,7 +49,8 @@ envoy_cc_library(
3949
hdrs = ["http_dynamic_delay_filter.h"],
4050
repository = "@envoy",
4151
deps = [
42-
":common_lib",
52+
":configuration_lib",
53+
":well_known_headers_lib",
4354
"//api/server:response_options_proto_cc_proto",
4455
"@envoy//source/exe:envoy_common_lib_with_external_headers",
4556
"@envoy//source/extensions/filters/http/fault:fault_filter_lib",

‎source/server/common.h‎

Lines changed: 0 additions & 50 deletions
This file was deleted.

source/server/common.cc renamed to source/server/configuration.cc

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#include "server/common.h"
1+
#include "server/configuration.h"
22

33
#include <string>
44

@@ -11,9 +11,10 @@
1111

1212
namespace Nighthawk {
1313
namespace Server {
14+
namespace Configuration {
1415

15-
bool Utility::mergeJsonConfig(absl::string_view json, nighthawk::server::ResponseOptions& config,
16-
std::string& error_message) {
16+
bool mergeJsonConfig(absl::string_view json, nighthawk::server::ResponseOptions& config,
17+
std::string& error_message) {
1718
error_message = "";
1819
try {
1920
nighthawk::server::ResponseOptions json_config;
@@ -27,8 +28,8 @@ bool Utility::mergeJsonConfig(absl::string_view json, nighthawk::server::Respons
2728
return error_message == "";
2829
}
2930

30-
void Utility::applyConfigToResponseHeaders(Envoy::Http::ResponseHeaderMap& response_headers,
31-
nighthawk::server::ResponseOptions& response_options) {
31+
void applyConfigToResponseHeaders(Envoy::Http::ResponseHeaderMap& response_headers,
32+
nighthawk::server::ResponseOptions& response_options) {
3233
for (const auto& header_value_option : response_options.response_headers()) {
3334
const auto& header = header_value_option.header();
3435
auto lower_case_key = Envoy::Http::LowerCaseString(header.key());
@@ -39,5 +40,6 @@ void Utility::applyConfigToResponseHeaders(Envoy::Http::ResponseHeaderMap& respo
3940
}
4041
}
4142

43+
} // namespace Configuration
4244
} // namespace Server
4345
} // namespace Nighthawk

‎source/server/configuration.h‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
#pragma once
2+
3+
#include <string>
4+
5+
#include "envoy/http/header_map.h"
6+
7+
#include "external/envoy/source/common/singleton/const_singleton.h"
8+
9+
#include "api/server/response_options.pb.h"
10+
11+
namespace Nighthawk {
12+
namespace Server {
13+
namespace Configuration {
14+
15+
/**
16+
* Merges a json string containing configuration into a ResponseOptions instance.
17+
*
18+
* @param json Json-formatted seralization of ResponseOptions to merge into the configuration.
19+
* @param config The target that the json string should be merged into.
20+
* @param error_message Set to an error message if one occurred, else set to an empty string.
21+
* @return bool false if an error occurred.
22+
*/
23+
bool mergeJsonConfig(absl::string_view json, nighthawk::server::ResponseOptions& config,
24+
std::string& error_message);
25+
26+
/**
27+
* Applies ResponseOptions onto a HeaderMap containing response headers.
28+
*
29+
* @param response_headers Response headers to transform to reflect the passed in response
30+
* options.
31+
* @param response_options Configuration specifying how to transform the header map.
32+
*/
33+
void applyConfigToResponseHeaders(Envoy::Http::ResponseHeaderMap& response_headers,
34+
nighthawk::server::ResponseOptions& response_options);
35+
36+
} // namespace Configuration
37+
} // namespace Server
38+
} // namespace Nighthawk

‎source/server/http_dynamic_delay_filter.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44

55
#include "envoy/server/filter_config.h"
66

7-
#include "server/common.h"
7+
#include "server/configuration.h"
8+
#include "server/well_known_headers.h"
89

910
#include "absl/strings/str_cat.h"
1011

@@ -59,8 +60,8 @@ bool HttpDynamicDelayDecoderFilter::computeResponseOptions(
5960
response_options_ = config_->server_config();
6061
const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig);
6162
if (request_config_header) {
62-
if (!Utility::mergeJsonConfig(request_config_header->value().getStringView(), response_options_,
63-
error_message)) {
63+
if (!Configuration::mergeJsonConfig(request_config_header->value().getStringView(),
64+
response_options_, error_message)) {
6465
return false;
6566
}
6667
}

‎source/server/http_test_server_filter.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44

55
#include "envoy/server/filter_config.h"
66

7-
#include "server/common.h"
7+
#include "server/configuration.h"
8+
#include "server/well_known_headers.h"
89

910
#include "absl/strings/numbers.h"
1011

@@ -30,7 +31,7 @@ void HttpTestServerDecoderFilter::sendReply() {
3031
decoder_callbacks_->sendLocalReply(
3132
static_cast<Envoy::Http::Code>(200), response_body,
3233
[this](Envoy::Http::ResponseHeaderMap& direct_response_headers) {
33-
Utility::applyConfigToResponseHeaders(direct_response_headers, base_config_);
34+
Configuration::applyConfigToResponseHeaders(direct_response_headers, base_config_);
3435
},
3536
absl::nullopt, "");
3637
} else {
@@ -48,8 +49,8 @@ HttpTestServerDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& header
4849
base_config_ = config_->server_config();
4950
const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig);
5051
if (request_config_header) {
51-
json_merge_error_ = !Utility::mergeJsonConfig(request_config_header->value().getStringView(),
52-
base_config_, error_message_);
52+
json_merge_error_ = !Configuration::mergeJsonConfig(
53+
request_config_header->value().getStringView(), base_config_, error_message_);
5354
}
5455
if (base_config_.echo_request_headers()) {
5556
std::stringstream headers_dump;

‎source/server/well_known_headers.h‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
#pragma once
2+
3+
#include <string>
4+
5+
#include "envoy/http/header_map.h"
6+
7+
namespace Nighthawk {
8+
namespace Server {
9+
10+
namespace TestServer {
11+
12+
class HeaderNameValues {
13+
public:
14+
const Envoy::Http::LowerCaseString TestServerConfig{"x-nighthawk-test-server-config"};
15+
};
16+
17+
using HeaderNames = Envoy::ConstSingleton<HeaderNameValues>;
18+
19+
} // namespace TestServer
20+
} // namespace Server
21+
} // namespace Nighthawk

‎test/client_test.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ class ClientTest : public testing::Test {};
2626
TEST_F(ClientTest, NormalRun) {
2727
Main program(Nighthawk::Client::TestUtility::createOptionsImpl(
2828
"foo --duration 1 --rps 10 http://localhost:63657/"));
29+
2930
EXPECT_FALSE(program.run());
3031
}
3132

‎test/server/http_dynamic_delay_filter_integration_test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
#include "api/server/response_options.pb.h"
66

7-
#include "server/common.h"
7+
#include "server/configuration.h"
88
#include "server/http_dynamic_delay_filter.h"
99

1010
#include "gtest/gtest.h"

‎test/server/http_test_server_filter_integration_test.cc‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,11 @@
77
#include "api/server/response_options.pb.h"
88
#include "api/server/response_options.pb.validate.h"
99

10-
#include "server/common.h"
10+
#include "server/configuration.h"
1111
#include "server/http_test_server_filter.h"
1212

13+
#include "server/well_known_headers.h"
14+
1315
#include "gtest/gtest.h"
1416

1517
namespace Nighthawk {
@@ -321,11 +323,11 @@ TEST_F(HttpTestServerDecoderFilterTest, HeaderMerge) {
321323
EXPECT_EQ(false, options.response_headers(0).append().value());
322324

323325
Envoy::Http::TestResponseHeaderMapImpl header_map{{":status", "200"}, {"foo", "bar"}};
324-
Server::Utility::applyConfigToResponseHeaders(header_map, options);
326+
Server::Configuration::applyConfigToResponseHeaders(header_map, options);
325327
EXPECT_TRUE(Envoy::TestUtility::headerMapEqualIgnoreOrder(
326328
header_map, Envoy::Http::TestResponseHeaderMapImpl{{":status", "200"}, {"foo", "bar1"}}));
327329

328-
EXPECT_TRUE(Server::Utility::mergeJsonConfig(
330+
EXPECT_TRUE(Server::Configuration::mergeJsonConfig(
329331
R"({response_headers: [ { header: { key: "foo", value: "bar2"}, append: false } ]})", options,
330332
error_message));
331333
EXPECT_EQ("", error_message);
@@ -335,11 +337,11 @@ TEST_F(HttpTestServerDecoderFilterTest, HeaderMerge) {
335337
EXPECT_EQ("bar2", options.response_headers(1).header().value());
336338
EXPECT_EQ(false, options.response_headers(1).append().value());
337339

338-
Server::Utility::applyConfigToResponseHeaders(header_map, options);
340+
Server::Configuration::applyConfigToResponseHeaders(header_map, options);
339341
EXPECT_TRUE(Envoy::TestUtility::headerMapEqualIgnoreOrder(
340342
header_map, Envoy::Http::TestRequestHeaderMapImpl{{":status", "200"}, {"foo", "bar2"}}));
341343

342-
EXPECT_TRUE(Server::Utility::mergeJsonConfig(
344+
EXPECT_TRUE(Server::Configuration::mergeJsonConfig(
343345
R"({response_headers: [ { header: { key: "foo2", value: "bar3"}, append: true } ]})", options,
344346
error_message));
345347
EXPECT_EQ("", error_message);
@@ -349,12 +351,12 @@ TEST_F(HttpTestServerDecoderFilterTest, HeaderMerge) {
349351
EXPECT_EQ("bar3", options.response_headers(2).header().value());
350352
EXPECT_EQ(true, options.response_headers(2).append().value());
351353

352-
Server::Utility::applyConfigToResponseHeaders(header_map, options);
354+
Server::Configuration::applyConfigToResponseHeaders(header_map, options);
353355
EXPECT_TRUE(Envoy::TestUtility::headerMapEqualIgnoreOrder(
354356
header_map, Envoy::Http::TestResponseHeaderMapImpl{
355357
{":status", "200"}, {"foo", "bar2"}, {"foo2", "bar3"}}));
356358

357-
EXPECT_FALSE(Server::Utility::mergeJsonConfig(kBadJson, options, error_message));
359+
EXPECT_FALSE(Server::Configuration::mergeJsonConfig(kBadJson, options, error_message));
358360
EXPECT_EQ("Error merging json config: Unable to parse JSON as proto (INVALID_ARGUMENT:Unexpected "
359361
"token.\nbad_json\n^): bad_json",
360362
error_message);

0 commit comments

Comments
 (0)