Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
503e8c6
test-server: dynamic delay filter extension [DRAFT]
oschaaf Jun 29, 2020
b44cf2c
Merge remote-tracking branch 'upstream/master' into test-server-dynam…
oschaaf Jun 29, 2020
159599f
Fix tests, improve naming
oschaaf Jun 29, 2020
3ec87c0
s/guage/gauge/
oschaaf Jun 29, 2020
277c786
Review feedback
oschaaf Jun 29, 2020
a34b8cd
Merge remote-tracking branch 'upstream/master' into test-server-dynam…
oschaaf Jun 29, 2020
5d6fb7e
Review feedback + tests + some polish
oschaaf Jul 1, 2020
d7bfb51
Add TODO, cover bad header config in test.
oschaaf Jul 1, 2020
caf8c78
Enhance testing, (doc) comments, and assert on assumptions
oschaaf Jul 2, 2020
aeddcdd
Add override keyword to destructor
oschaaf Jul 2, 2020
b1069da
Review feedback: fix atomic initialization & remove "using namespace …
oschaaf Jul 3, 2020
e0f88a0
clang-tidy: add NOLINT for MUTABLE_CONSTRUCT_ON_FIRST_USE
oschaaf Jul 3, 2020
c40e9ce
Partial review feedback
oschaaf Jul 6, 2020
6113345
save state
oschaaf Jul 7, 2020
1cd6cd7
Merge remote-tracking branch 'upstream/master' into test-server-dynam…
oschaaf Jul 8, 2020
8ee78db
review feedback
oschaaf Jul 8, 2020
3f006f6
Move comment out of method body to fix coverage
oschaaf Jul 8, 2020
3a875cd
Merge commit '611334586db1c81d7e2c97875b769c867e8c6a95' into test-ser…
oschaaf Jul 9, 2020
3e3c6ef
save state: Review feedback + fault filter base
oschaaf Jul 9, 2020
ac456f3
Update documentation & fix destructor
oschaaf Jul 9, 2020
9be5334
Partial review feedback
oschaaf Jul 13, 2020
ad5e34c
Review feedback
oschaaf Jul 13, 2020
3c80a0b
Fix missing dependency
oschaaf Jul 13, 2020
8f74535
clang-tidy: fix include
oschaaf Jul 13, 2020
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 BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ envoy_cc_binary(
name = "nighthawk_test_server",
repository = "@envoy",
deps = [
"//source/server:http_dynamic_delay_filter_config",
"//source/server:http_test_server_filter_config",
"@envoy//source/exe:envoy_main_entry_lib",
],
Expand Down
17 changes: 17 additions & 0 deletions api/server/response_options.proto
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ package nighthawk.server;
import "google/protobuf/wrappers.proto";
import "validate/validate.proto";
import "envoy/api/v2/core/base.proto";
import "google/protobuf/duration.proto";

message ConcurrencyBasedDelay {
// Minimal delay to add to replies.
google.protobuf.Duration minimal_delay = 1 [(validate.rules).duration.gte.nanos = 0];
// Factor to use when adding latency as concurrency increases.
google.protobuf.Duration concurrency_delay_factor = 2 [(validate.rules).duration.gte.nanos = 0];
Comment thread
oschaaf marked this conversation as resolved.
// Multiplier.
uint64 concurrency_delay_multiplier = 3;
Comment thread
oschaaf marked this conversation as resolved.
Outdated
}

// Options that control the test server response.
message ResponseOptions {
Expand All @@ -14,4 +24,11 @@ message ResponseOptions {
uint32 response_body_size = 2 [(validate.rules).uint32 = {lte: 4194304}];
// If true, then echo request headers in the response body.
bool echo_request_headers = 3;

oneof oneof_delay_options {
Comment thread
oschaaf marked this conversation as resolved.
// Static delay duration.
google.protobuf.Duration static_delay = 4 [(validate.rules).duration.gte.nanos = 0];
// Concurrency based delay configuration.
ConcurrencyBasedDelay concurrency_based_delay = 5;
}
}
38 changes: 36 additions & 2 deletions source/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,39 @@ licenses(["notice"]) # Apache 2

envoy_package()

envoy_cc_library(
name = "common_lib",
srcs = ["common.cc"],
hdrs = ["common.h"],
repository = "@envoy",
deps = [
"//api/server:response_options_proto_cc_proto",
"@envoy//source/common/protobuf:message_validator_lib_with_external_headers",
"@envoy//source/common/protobuf:utility_lib_with_external_headers",
"@envoy//source/common/singleton:const_singleton_with_external_headers",
],
)

envoy_cc_library(
name = "http_test_server_filter_lib",
srcs = ["http_test_server_filter.cc"],
hdrs = ["http_test_server_filter.h"],
repository = "@envoy",
deps = [
":common_lib",
"//api/server:response_options_proto_cc_proto",
"@envoy//source/exe:envoy_common_lib_with_external_headers",
],
)

envoy_cc_library(
name = "http_dynamic_delay_filter_lib",
srcs = ["http_dynamic_delay_filter.cc"],
hdrs = ["http_dynamic_delay_filter.h"],
repository = "@envoy",
deps = [
":common_lib",
"//api/server:response_options_proto_cc_proto",
"@envoy//source/common/protobuf:message_validator_lib_with_external_headers",
"@envoy//source/common/protobuf:utility_lib_with_external_headers",
"@envoy//source/exe:envoy_common_lib_with_external_headers",
],
)
Expand All @@ -30,3 +54,13 @@ envoy_cc_library(
"@envoy//include/envoy/server:filter_config_interface_with_external_headers",
],
)

envoy_cc_library(
name = "http_dynamic_delay_filter_config",
srcs = ["http_dynamic_delay_filter_config.cc"],
repository = "@envoy",
deps = [
":http_dynamic_delay_filter_lib",
"@envoy//include/envoy/server:filter_config_interface_with_external_headers",
],
)
43 changes: 43 additions & 0 deletions source/server/common.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#include "server/common.h"

#include <string>

#include "external/envoy/source/common/protobuf/message_validator_impl.h"
#include "external/envoy/source/common/protobuf/utility.h"

#include "api/server/response_options.pb.validate.h"

#include "absl/strings/numbers.h"

namespace Nighthawk {
namespace Server {

bool Utility::mergeJsonConfig(absl::string_view json, nighthawk::server::ResponseOptions& config,
absl::optional<std::string>& error_message) {
error_message = absl::nullopt;
try {
nighthawk::server::ResponseOptions json_config;
auto& validation_visitor = Envoy::ProtobufMessage::getStrictValidationVisitor();
Envoy::MessageUtil::loadFromJson(std::string(json), json_config, validation_visitor);
config.MergeFrom(json_config);
Envoy::MessageUtil::validate(config, validation_visitor);
} catch (const Envoy::EnvoyException& exception) {
error_message.emplace(fmt::format("Error merging json config: {}", exception.what()));
}
return error_message == absl::nullopt;
}

void Utility::applyConfigToResponseHeaders(Envoy::Http::ResponseHeaderMap& response_headers,
nighthawk::server::ResponseOptions& response_options) {
for (const auto& header_value_option : response_options.response_headers()) {
const auto& header = header_value_option.header();
auto lower_case_key = Envoy::Http::LowerCaseString(header.key());
if (!header_value_option.append().value()) {
response_headers.remove(lower_case_key);
}
response_headers.addCopy(lower_case_key, header.value());
}
}

} // namespace Server
} // namespace Nighthawk
50 changes: 50 additions & 0 deletions source/server/common.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#pragma once

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we add unit tests for this library?

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.

There are pre-existing unit tests for this. For example, see

TEST_F(HttpTestServerDecoderFilterTest, HeaderMerge) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(for the future) Looks like the changes in this library may have been just indirectly related to the work in this PR. If reasonable, can we try to split such changes into their own PR in the future to minimize the size of PRs and the review complexity?

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.

Yeah, you're right, sorry about that. Will extract these mechanical refactors out into a separate PR next time.


#include <string>

#include "envoy/http/header_map.h"

#include "external/envoy/source/common/singleton/const_singleton.h"

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

namespace Nighthawk {
namespace Server {

namespace TestServer {

class HeaderNameValues {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As per the comment below (ideally, please read that one first) - are we moving this definition here just because we don't have a better place for it and this file happens to have a generic enough name? And a similar question - does this have to be a class?

Alternative approach - this currently only seems to be used in a single location - how about we just inline the literal there, is it worth having a this defined as a constant?

If yes, can we define a new header specifically designed for this? And if we are doing that - is attaching these onto a class an idiom used in Envoy, or can we can try to figure out an approach that doesn't require a class? Even an ordinary function returning static constexpr might be preferred.

inline absl::string_view MyString() {
  static constexpr char kHello[] = "Hello";
  return kHello;
}

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.

Like for above, see 9be5334. However, I kept the Envoy construct, because this is shared/reused across the two extensions, and having the LowerCaseString is convenient -- that is the form that consumers are probably going to end up needing when they want access to this.

public:
const Envoy::Http::LowerCaseString TestServerConfig{"x-nighthawk-test-server-config"};
};

using HeaderNames = Envoy::ConstSingleton<HeaderNameValues>;

} // namespace TestServer

class Utility {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am wondering if we can improve the naming and structure of this new library. Both "common" and "Utility" are very generic names that a) don't tell the reader much and b) attract pretty much any helper / utility function. After all everything is an utility.

Secondly it doesn't seem necessary for this to be a class at all, since the two methods on it don't really need any state. How would you feel about changing these into regular functions and renaming the library into something more specific.

E.g. response_options.h

Feel free to choose any other name that is specific enough not to attract unrelated functions.

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.

Done in 9be5334, let me know if that looks better.

public:
/**
* Merges a json string containing configuration into a ResponseOptions instance.
*
* @param json Json-formatted seralization of ResponseOptions to merge into the configuration.
* @param config The target that the json string should be merged into.
* @param error_message Will contain an error message iff an error occurred.
Comment thread
oschaaf marked this conversation as resolved.
Outdated
* @return bool false iff an error occurred.
*/
static bool mergeJsonConfig(absl::string_view json, nighthawk::server::ResponseOptions& config,
absl::optional<std::string>& error_message);

/**
* Applies ResponseOptions onto a HeaderMap containing response headers.
*
* @param response_headers Response headers to transform to reflect the passed in response
* options.
* @param response_options Configuration specifying how to transform the header map.
*/
static void applyConfigToResponseHeaders(Envoy::Http::ResponseHeaderMap& response_headers,
nighthawk::server::ResponseOptions& response_options);
};

} // namespace Server
} // namespace Nighthawk
77 changes: 77 additions & 0 deletions source/server/http_dynamic_delay_filter.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#include "server/http_dynamic_delay_filter.h"

#include <string>

#include "envoy/server/filter_config.h"

#include "server/common.h"

#include "absl/strings/str_cat.h"

namespace Nighthawk {
namespace Server {

std::atomic<uint64_t> HttpDynamicDelayDecoderFilterConfig::instances_(0);
Comment thread
oschaaf marked this conversation as resolved.
Outdated

HttpDynamicDelayDecoderFilterConfig::HttpDynamicDelayDecoderFilterConfig(
nighthawk::server::ResponseOptions proto_config)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nighthawk::server::ResponseOptions&& ?

: server_config_(std::move(proto_config)) {}

HttpDynamicDelayDecoderFilter::HttpDynamicDelayDecoderFilter(
HttpDynamicDelayDecoderFilterConfigSharedPtr config)
: config_(std::move(config)) {
config_->incrementInstanceCount();
}

void HttpDynamicDelayDecoderFilter::onDestroy() { config_->decrementInstanceCount(); }

Envoy::Http::FilterHeadersStatus
HttpDynamicDelayDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers, bool) {
base_config_ = config_->server_config();
const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig);
if (request_config_header) {
Utility::mergeJsonConfig(request_config_header->value().getStringView(), base_config_,
error_message_);
}
if (error_message_.has_value()) {
decoder_callbacks_->sendLocalReply(
static_cast<Envoy::Http::Code>(500),
fmt::format("dynamic-delay didn't understand the request: {}", *error_message_), nullptr,
absl::nullopt, "");
return Envoy::Http::FilterHeadersStatus::StopIteration;
}
absl::optional<int64_t> delay;
Comment thread
oschaaf marked this conversation as resolved.
Outdated
if (base_config_.has_static_delay()) {
delay = Envoy::Protobuf::util::TimeUtil::DurationToMilliseconds(base_config_.static_delay());
} else if (base_config_.has_concurrency_based_delay()) {
auto& concurrency = base_config_.concurrency_based_delay();
Comment thread
oschaaf marked this conversation as resolved.
Outdated
const uint64_t current_value = config_->approximateInstances();
delay = Envoy::Protobuf::util::TimeUtil::DurationToMilliseconds(
concurrency.minimal_delay() + (current_value * concurrency.concurrency_delay_factor()));
std::cerr << current_value << ":" << delay.value() << std::endl;
}
if (delay.has_value() && delay > 0) {
// Emit header to communicate the delay we desire to the fault filter extension.
const Envoy::Http::LowerCaseString key("x-envoy-fault-delay-request");
headers.setCopy(key, absl::StrCat(*delay));
}
return Envoy::Http::FilterHeadersStatus::Continue;
}

Envoy::Http::FilterDataStatus HttpDynamicDelayDecoderFilter::decodeData(Envoy::Buffer::Instance&,
bool) {
return Envoy::Http::FilterDataStatus::Continue;
}

Envoy::Http::FilterTrailersStatus
HttpDynamicDelayDecoderFilter::decodeTrailers(Envoy::Http::RequestTrailerMap&) {
return Envoy::Http::FilterTrailersStatus::Continue;
}

void HttpDynamicDelayDecoderFilter::setDecoderFilterCallbacks(
Envoy::Http::StreamDecoderFilterCallbacks& callbacks) {
decoder_callbacks_ = &callbacks;
}

} // namespace Server
} // namespace Nighthawk
51 changes: 51 additions & 0 deletions source/server/http_dynamic_delay_filter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#pragma once

#include <atomic>
#include <string>

#include "envoy/server/filter_config.h"

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

namespace Nighthawk {
namespace Server {

// Basically this is left in as a placeholder for further configuration.
class HttpDynamicDelayDecoderFilterConfig {
public:
HttpDynamicDelayDecoderFilterConfig(nighthawk::server::ResponseOptions proto_config);
const nighthawk::server::ResponseOptions& server_config() { return server_config_; }
void incrementInstanceCount() { instances_++; }
void decrementInstanceCount() { instances_--; }
uint64_t approximateInstances() const { return instances_; }

private:
const nighthawk::server::ResponseOptions server_config_;
static std::atomic<uint64_t> instances_;
};

using HttpDynamicDelayDecoderFilterConfigSharedPtr =
std::shared_ptr<HttpDynamicDelayDecoderFilterConfig>;

class HttpDynamicDelayDecoderFilter : public Envoy::Http::StreamDecoderFilter {
public:
HttpDynamicDelayDecoderFilter(HttpDynamicDelayDecoderFilterConfigSharedPtr);

// Http::StreamFilterBase
void onDestroy() override;

// Http::StreamDecoderFilter
Envoy::Http::FilterHeadersStatus decodeHeaders(Envoy::Http::RequestHeaderMap&, bool) override;
Comment thread
oschaaf marked this conversation as resolved.
Envoy::Http::FilterDataStatus decodeData(Envoy::Buffer::Instance&, bool) override;
Envoy::Http::FilterTrailersStatus decodeTrailers(Envoy::Http::RequestTrailerMap&) override;
void setDecoderFilterCallbacks(Envoy::Http::StreamDecoderFilterCallbacks&) override;

private:
const HttpDynamicDelayDecoderFilterConfigSharedPtr config_;
Envoy::Http::StreamDecoderFilterCallbacks* decoder_callbacks_;
nighthawk::server::ResponseOptions base_config_;
absl::optional<std::string> error_message_;
};

} // namespace Server
} // namespace Nighthawk
55 changes: 55 additions & 0 deletions source/server/http_dynamic_delay_filter_config.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#include <string>

#include "envoy/registry/registry.h"

#include "external/envoy/source/common/protobuf/message_validator_impl.h"

#include "api/server/response_options.pb.h"
#include "api/server/response_options.pb.validate.h"

#include "server/http_dynamic_delay_filter.h"

namespace Nighthawk {
namespace Server {
namespace Configuration {

class HttpDynamicDelayDecoderFilterConfig
Comment thread
oschaaf marked this conversation as resolved.
Outdated
: public Envoy::Server::Configuration::NamedHttpFilterConfigFactory {
public:
Envoy::Http::FilterFactoryCb
createFilterFactoryFromProto(const Envoy::Protobuf::Message& proto_config, const std::string&,
Envoy::Server::Configuration::FactoryContext& context) override {

auto& validation_visitor = Envoy::ProtobufMessage::getStrictValidationVisitor();
return createFilter(
Envoy::MessageUtil::downcastAndValidate<const nighthawk::server::ResponseOptions&>(
proto_config, validation_visitor),
context);
}

Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return Envoy::ProtobufTypes::MessagePtr{new nighthawk::server::ResponseOptions()};
}

std::string name() const override { return "dynamic-delay"; }

private:
Envoy::Http::FilterFactoryCb createFilter(const nighthawk::server::ResponseOptions& proto_config,
Envoy::Server::Configuration::FactoryContext&) {
Nighthawk::Server::HttpDynamicDelayDecoderFilterConfigSharedPtr config =
std::make_shared<Nighthawk::Server::HttpDynamicDelayDecoderFilterConfig>(
Nighthawk::Server::HttpDynamicDelayDecoderFilterConfig(proto_config));

return [config](Envoy::Http::FilterChainFactoryCallbacks& callbacks) -> void {
auto* filter = new Nighthawk::Server::HttpDynamicDelayDecoderFilter(config);
callbacks.addStreamDecoderFilter(Envoy::Http::StreamDecoderFilterSharedPtr{filter});
};
}
};

static Envoy::Registry::RegisterFactory<HttpDynamicDelayDecoderFilterConfig,
Envoy::Server::Configuration::NamedHttpFilterConfigFactory>
register_;
} // namespace Configuration
} // namespace Server
} // namespace Nighthawk
Loading