Refactor filter configuration to make the different test server filters independent

CHANGELOG.md

@@ -0,0 +1,99 @@
+# Changelog
+
+All breaking changes to this project will be documented in this file, most recent changes at the top.
+
+## 0.5.0
+
+In an effort to clean up the previous change and improve it, we re-modified the dynamic-delay and time-tracking filters to extricate their configuration entirely from nighthawk.server.ResponseOptions.
+
+If you are converting from the previous configuration with `experimental_response_options`, such as:
+
+```
+          http_filters:
+          - name: time-tracking
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.TimeTrackingConfiguration
+              experimental_response_options:
+                emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+          - name: dynamic-delay
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.DynamicDelayConfiguration
+              experimental_response_options:
+                static_delay: 1.33s
+          - name: test-server
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.ResponseOptions
+              response_body_size: 10
+              static_delay: 1.33s
+              emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+              v3_response_headers:
+              - { header: { key: "x-nh", value: "1"}}
+```
+
+You should now specify only fields related to each filter in their configuration, and you can do so at the top-level of those protos:
+
+```
+          http_filters:
+          - name: time-tracking
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.TimeTrackingConfiguration
+              emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+          - name: dynamic-delay
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.DynamicDelayConfiguration
+              static_delay: 1.33s
+          - name: test-server
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.ResponseOptions
+              response_body_size: 10
+              v3_response_headers:
+              - { header: { key: "x-nh", value: "1"}}
+```
+
+This change does NOT affect how headers update the base configurations.
+
+## 0.4.0
+
+Due to [upstream envoy change](https://github.com/envoyproxy/nighthawk/commit/4919c54202329adc3875eb1bce074af33d54e26d), we modified the dynamic-delay and time-tracking filters' configuration protos to have their own configuration protos wrapping nighthawk.server.ResponseOptions.
+
+For base yaml configuration files, if you previously had:
+
+```
+          http_filters:
+          - name: time-tracking
+          - name: dynamic-delay
+          - name: test-server
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.ResponseOptions
+              response_body_size: 10
+              static_delay: 1.33s
+              emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+              v3_response_headers:
+              - { header: { key: "x-nh", value: "1"}}
+```
+
+You should now explicitly specify the types and values of the protos as such:
+
+```
+          http_filters:
+          - name: time-tracking
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.TimeTrackingConfiguration
+              experimental_response_options:
+                emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+          - name: dynamic-delay
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.DynamicDelayConfiguration
+              experimental_response_options:
+                static_delay: 1.33s
+          - name: test-server
+            typed_config:
+              "@type": type.googleapis.com/nighthawk.server.ResponseOptions
+              response_body_size: 10
+              static_delay: 1.33s
+              emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+              v3_response_headers:
+              - { header: { key: "x-nh", value: "1"}}
+```
+
+This change does NOT affect how headers update the base configurations.

api/server/BUILD

@@ -10,7 +10,11 @@ envoy_package()
 
 api_cc_py_proto_library(
     name = "response_options_proto",
-    srcs = ["response_options.proto"],
+    srcs = [
+        "dynamic_delay.proto",
+        "response_options.proto",
+        "time_tracking.proto",
+    ],
     deps = [
         "@envoy_api//envoy/api/v2/core:pkg",
         "@envoy_api//envoy/config/core/v3:pkg",

api/server/dynamic_delay.proto

@@ -0,0 +1,20 @@
+// Proto file supporting any configuration related to the dynamic delay filter.
+syntax = "proto3";
+
+import "api/server/response_options.proto";
+import "google/protobuf/duration.proto";
+import "validate/validate.proto";
+
+package nighthawk.server;
+
+// Configures the dynamic-delay filter.
+message DynamicDelayConfiguration {
+  oneof oneof_delay_options {
+    // Static delay duration.
+    google.protobuf.Duration static_delay = 2 [(validate.rules).duration.gte.nanos = 0];
+    // Concurrency based linear delay configuration.
+    ConcurrencyBasedLinearDelay concurrency_based_linear_delay = 3;
+  }
+
+  reserved 1;
+}

api/server/response_options.proto

@@ -35,30 +35,25 @@ message ResponseOptions {
   // If true, then echo request headers in the response body.
   bool echo_request_headers = 3;
 
+  // IMPORTANT:
+  // The below fields are only for use in the x-nighthawk-test-server-config header.
+  // They do not have any behavior on the test server filter, but rather the dynamic-delay
+  // and time-tracking filters. For server-level configuration, please provide the
+  // configuration in the appropriate configuration proto.
+
+  // Provides request-level configuration that overrides DynamicDelayConfiguration.
   oneof oneof_delay_options {
     // Static delay duration.
     google.protobuf.Duration static_delay = 4 [(validate.rules).duration.gte.nanos = 0];
     // Concurrency based linear delay configuration.
     ConcurrencyBasedLinearDelay concurrency_based_linear_delay = 5;
   }
+
+  // Provides request-level configuration that overrides TimeTrackingConfiguration.
   // If set, makes the extension include timing data in the supplied response header name.
   // For example, when set to "x-abc", and 3 requests are performed, the test server will respond
   // with: Response 1: No x-abc header because there's no previous response. Response 2: Header
   // x-abc: <ns elapsed between responses 2 and 1>. Response 3: Header x-abc: <ns elapsed between
   // responses 3 and 2>.
   string emit_previous_request_delta_in_response_header = 6;
 }
-
-// Configures the dynamic-delay test filter.
-message DynamicDelayConfiguration {
-  // This is a temporary solution to allow this functionality to continue, but will likely be
-  // reconfigured soon.
-  ResponseOptions experimental_response_options = 1;
-}
-
-// Configures the time-tracking test filter
-message TimeTrackingConfiguration {
-  // This is a temporary solution to allow this functionality to continue, but will likely be
-  // reconfigured soon.
-  ResponseOptions experimental_response_options = 1;
-}

api/server/time_tracking.proto

@@ -0,0 +1,16 @@
+// Proto file supporting any configuration related to the time tracking filter.
+syntax = "proto3";
+
+package nighthawk.server;
+
+// Configures the time-tracking filter.
+message TimeTrackingConfiguration {
+  // If set, makes the extension include timing data in the supplied response header name.
+  // For example, when set to "x-abc", and 3 requests are performed, the test server will respond
+  // with: Response 1: No x-abc header because there's no previous response. Response 2: Header
+  // x-abc: <ns elapsed between responses 2 and 1>. Response 3: Header x-abc: <ns elapsed between
+  // responses 3 and 2>.
+  string emit_previous_request_delta_in_response_header = 2;
+
+  reserved 1;
+}

docs/root/examples/HEADER_BASED_LATENCY.md

@@ -24,8 +24,7 @@ Another use-case is tracking request-arrival time deltas using a feature of the
     - name: time-tracking
     typed_config:
         "@type": type.googleapis.com/nighthawk.server.TimeTrackingConfiguration
-        experimental_response_options:
-          emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
+        emit_previous_request_delta_in_response_header: x-origin-request-receipt-delta
     - name: test-server
     typed_config:
         "@type": type.googleapis.com/nighthawk.server.ResponseOptions

source/server/BUILD

@@ -50,6 +50,7 @@ envoy_cc_library(
     deps = [
         ":configuration_lib",
         "//api/server:response_options_proto_cc_proto",
+        "@envoy//source/common/common:statusor_lib_with_external_headers",
         "@envoy//source/exe:envoy_common_lib_with_external_headers",
     ],
 )
@@ -63,6 +64,7 @@ envoy_cc_library(
         ":configuration_lib",
         "//api/server:response_options_proto_cc_proto",
         "//source/common:thread_safe_monotonic_time_stopwatch_lib",
+        "@envoy//source/common/common:statusor_lib_with_external_headers",
         "@envoy//source/exe:envoy_common_lib_with_external_headers",
         "@envoy//source/extensions/filters/http/common:pass_through_filter_lib_with_external_headers",
     ],
@@ -76,6 +78,7 @@ envoy_cc_library(
     deps = [
         ":configuration_lib",
         "//api/server:response_options_proto_cc_proto",
+        "@envoy//source/common/common:statusor_lib_with_external_headers",
         "@envoy//source/exe:envoy_common_lib_with_external_headers",
         "@envoy//source/extensions/filters/http/fault:fault_filter_lib_with_external_headers",
     ],

source/server/README.md

@@ -50,8 +50,7 @@ static_resources:
                   - name: dynamic-delay
                     typed_config:
                       "@type": type.googleapis.com/nighthawk.server.DynamicDelayConfiguration
-                      experimental_response_options:
-                        static_delay: 0.5s
+                      static_delay: 0.5s
                   - name: test-server # before envoy.router because order matters!
                     typed_config:
                       "@type": type.googleapis.com/nighthawk.server.ResponseOptions

source/server/http_dynamic_delay_filter.cc

@@ -4,22 +4,85 @@
 
 #include "envoy/server/filter_config.h"
 
+#include "api/server/dynamic_delay.pb.validate.h"
+
 #include "source/server/configuration.h"
 #include "source/server/well_known_headers.h"
 
 #include "absl/strings/str_cat.h"
 
 namespace Nighthawk {
 namespace Server {
+namespace {
+
+using ::nighthawk::server::DynamicDelayConfiguration;
+using ::nighthawk::server::ResponseOptions;
+
+// Cherry-picks the relevant fields from the header_json, which should be a ResponseOptions proto,
+// and merges them into the base_config.
+absl::Status cherryPickDynamicDelayConfiguration(absl::string_view header_json,
+                                                 DynamicDelayConfiguration& base_config) {
+  try {
+    ResponseOptions response_options;
+    auto& validation_visitor = Envoy::ProtobufMessage::getStrictValidationVisitor();
+    Envoy::MessageUtil::loadFromJson(std::string(header_json), response_options,
+                                     validation_visitor);
+    switch (response_options.oneof_delay_options_case()) {
+    case ResponseOptions::OneofDelayOptionsCase::kStaticDelay:
+      *base_config.mutable_static_delay() = response_options.static_delay();
+      break;
+    case ResponseOptions::OneofDelayOptionsCase::kConcurrencyBasedLinearDelay:
+      *base_config.mutable_concurrency_based_linear_delay() =
+          response_options.concurrency_based_linear_delay();
+      break;
+    case ResponseOptions::OneofDelayOptionsCase::ONEOF_DELAY_OPTIONS_NOT_SET:
+      break; // No action required.
+    }
+  } catch (const Envoy::EnvoyException& exception) {
+    return absl::InvalidArgumentError(
+        fmt::format("Error merging json config: {}", exception.what()));
+  }
+  return absl::OkStatus();
+}
+
+const absl::StatusOr<std::shared_ptr<const DynamicDelayConfiguration>>
+computeEffectiveConfiguration(std::shared_ptr<const DynamicDelayConfiguration> base_filter_config,
+                              const Envoy::Http::RequestHeaderMap& request_headers) {
+  const auto& request_config_header =
+      request_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.
+    DynamicDelayConfiguration modified_filter_config = *base_filter_config;
+    absl::Status cherry_pick_status = cherryPickDynamicDelayConfiguration(
+        request_config_header[0]->value().getStringView(), modified_filter_config);
+    if (cherry_pick_status.ok()) {
+      return std::make_shared<const DynamicDelayConfiguration>(std::move(modified_filter_config));
+    } else {
+      return cherry_pick_status;
+    }
+  } else if (request_config_header.size() > 1) {
+    return absl::InvalidArgumentError(
+        "Received multiple configuration headers in the request, expected only one.");
+  }
+  return base_filter_config;
+}
+
+} // namespace
 
 HttpDynamicDelayDecoderFilterConfig::HttpDynamicDelayDecoderFilterConfig(
-    const nighthawk::server::DynamicDelayConfiguration& proto_config,
-    Envoy::Runtime::Loader& runtime, const std::string& stats_prefix, Envoy::Stats::Scope& scope,
-    Envoy::TimeSource& time_source)
-    : FilterConfigurationBase(proto_config.experimental_response_options(), "dynamic-delay"),
-      runtime_(runtime),
+    const DynamicDelayConfiguration& proto_config, Envoy::Runtime::Loader& runtime,
+    const std::string& stats_prefix, Envoy::Stats::Scope& scope, Envoy::TimeSource& time_source)
+    : FilterConfigurationBase("dynamic-delay"), runtime_(runtime),
       stats_prefix_(absl::StrCat(stats_prefix, fmt::format("{}.", filter_name()))), scope_(scope),
-      time_source_(time_source) {}
+      time_source_(time_source),
+      server_config_(std::make_shared<DynamicDelayConfiguration>(proto_config)) {}
+
+std::shared_ptr<const DynamicDelayConfiguration>
+HttpDynamicDelayDecoderFilterConfig::getStartupFilterConfiguration() {
+  return server_config_;
+}
 
 HttpDynamicDelayDecoderFilter::HttpDynamicDelayDecoderFilter(
     HttpDynamicDelayDecoderFilterConfigSharedPtr config)
@@ -42,14 +105,15 @@ void HttpDynamicDelayDecoderFilter::onDestroy() {
 Envoy::Http::FilterHeadersStatus
 HttpDynamicDelayDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers,
                                              bool end_stream) {
-  effective_config_ = config_->computeEffectiveConfiguration(headers);
+  effective_config_ =
+      computeEffectiveConfiguration(config_->getStartupFilterConfiguration(), headers);
   if (effective_config_.ok()) {
     const absl::optional<int64_t> delay_ms =
         computeDelayMs(*effective_config_.value(), config_->approximateFilterInstances());
     maybeRequestFaultFilterDelay(delay_ms, headers);
   } else {
     if (end_stream) {
-      config_->validateOrSendError(effective_config_, *decoder_callbacks_);
+      config_->validateOrSendError(effective_config_.status(), *decoder_callbacks_);
       return Envoy::Http::FilterHeadersStatus::StopIteration;
     }
     return Envoy::Http::FilterHeadersStatus::Continue;
@@ -61,23 +125,23 @@ Envoy::Http::FilterDataStatus
 HttpDynamicDelayDecoderFilter::decodeData(Envoy::Buffer::Instance& buffer, bool end_stream) {
   if (!effective_config_.ok()) {
     if (end_stream) {
-      config_->validateOrSendError(effective_config_, *decoder_callbacks_);
+      config_->validateOrSendError(effective_config_.status(), *decoder_callbacks_);
       return Envoy::Http::FilterDataStatus::StopIterationNoBuffer;
     }
     return Envoy::Http::FilterDataStatus::Continue;
   }
   return Envoy::Extensions::HttpFilters::Fault::FaultFilter::decodeData(buffer, end_stream);
 }
 
-absl::optional<int64_t> HttpDynamicDelayDecoderFilter::computeDelayMs(
-    const nighthawk::server::ResponseOptions& response_options, const uint64_t concurrency) {
+absl::optional<int64_t>
+HttpDynamicDelayDecoderFilter::computeDelayMs(const DynamicDelayConfiguration& config,
+                                              const uint64_t concurrency) {
   absl::optional<int64_t> delay_ms;
-  if (response_options.has_static_delay()) {
-    delay_ms =
-        Envoy::Protobuf::util::TimeUtil::DurationToMilliseconds(response_options.static_delay());
-  } else if (response_options.has_concurrency_based_linear_delay()) {
+  if (config.has_static_delay()) {
+    delay_ms = Envoy::Protobuf::util::TimeUtil::DurationToMilliseconds(config.static_delay());
+  } else if (config.has_concurrency_based_linear_delay()) {
     const nighthawk::server::ConcurrencyBasedLinearDelay& concurrency_config =
-        response_options.concurrency_based_linear_delay();
+        config.concurrency_based_linear_delay();
     delay_ms = computeConcurrencyBasedLinearDelayMs(concurrency, concurrency_config.minimal_delay(),
                                                     concurrency_config.concurrency_delay_factor());
   }