Refactor filter configuration to make the different test server filte…

…rs independent (#855) This is a followup to #852. In that change, we duplicated ResponseOptions onto separate DynamicDelayConfiguration and TimeTrackingConfiguration protos. However, this resulted in many fields on those protos that were not relevant to each filter. In this PR, we make it so that only the relevant fields exist on each proto. Resulting changes: 1. Moved DynamicDelayConfiguration and TimeTrackingConfiguration into their own files, and moved their fields into those protos directly. 2. In order to make request headers still work, there is now manual logic for taking in a ResponseOptions proto via json and cherrypicking the fields relevant to each filter. 3. The generic method computeEffectiveConfiguration was moved to each filter's anonymous namespace. 4. All configs, tests, and markdown files updated to use the new protos. 5. Some tests have been removed that are no longer necessary, or never provided additional coverage, including some that are less feasible now. - HeaderMerge was removed, but everything it tested for was moved into new unit tests of MergeJsonConfig. Signed-off-by: Nathan Perry <[email protected]>
envoyproxy · Jun 7, 2022 · 7290824 · 7290824
1 parent 28955cd
commit 7290824
Show file tree

Hide file tree

Showing 30 changed files with 540 additions and 260 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -0,0 +1,120 @@
+# 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](https://github.com/envoyproxy/nighthawk/blob/main/api/server/response_options.proto). The new configurations are:
+
+- [nighthawk.server.DynamicDelayConfiguration](https://github.com/envoyproxy/nighthawk/blob/main/api/server/dynamic_delay.proto)
+- [nighthawk.server.TimeTrackingConfiguration](https://github.com/envoyproxy/nighthawk/blob/main/api/server/time_tracking.proto)
+
+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]([nighthawk.server.ResponseOptions](https://github.com/envoyproxy/nighthawk/blob/0.4.0/api/server/response_options.proto)).
+DynamicDelayConfiguration and TimeTrackingConfiguration definitions can be
+found at the bottom of that file as well.
+.
+
+For yaml bootstrap configuration files that defined
+[filter configuration](https://github.com/envoyproxy/nighthawk/blob/main/source/server/README.md#nighthawk-test-server)
+for the `test-server`, `dynamic-delay`, or `time-tracking` filters, 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,
+wrapping the `dynamic-delay` and `time-tracking` configurations in a new field,
+`experimental_response_options`:
+
+```
+          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.
diff --git a/api/server/BUILD b/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",

diff --git a/api/server/dynamic_delay.proto b/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;
+}
diff --git a/api/server/response_options.proto b/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;
-}
diff --git a/api/server/time_tracking.proto b/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;
+}
diff --git a/docs/root/examples/HEADER_BASED_LATENCY.md b/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

diff --git a/source/server/BUILD b/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",
     ],

diff --git a/source/server/README.md b/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
@@ -75,13 +74,19 @@ admin:
       port_value: 8081
 ```
 
-## Response Options config
+## Nighthawk Test Server Filter Configurations
 
-The [ResponseOptions proto](/api/server/response_options.proto) is shared by
-the `Test Server` and `Dynamic Delay` filter extensions. Each filter will
-interpret the parts that are relevant to it. This allows specifying what
-a response should look like in a single message, which can be done at request
-time via the optional `x-nighthawk-test-server-config` request-header.
+The [ResponseOptions proto](/api/server/response_options.proto) used to be 
+shared by the `Test Server`, `Dynamic Delay`, and `Time Tracking` filter
+extensions. Now, each of these filters has its own primary configuration, respectively:
+
+- [ResponseOptions](/api/server/response_options.proto) for `Test Server` 
+- [DynamicDelayConfiguration](/api/server/dynamic_delay.proto) for `Dynamic Delay`
+- [TimeTrackingConfiguration](/api/server/time_tracking.proto) for `Time Tracking`
+
+However, currently, each filter still uses the same `x-nighthawk-test-server-config`
+request-header, which is a ResponseOptions proto. When this header is provided, each
+filter will interpret only the parts that are relevant to it.
 
 ### Test Server
 
@@ -135,7 +140,9 @@ This example shows that intermediate proxy has added `x-forwarded-proto` and
 
 ### Dynamic Delay
 
-The Dynamic Delay interprets the `oneof_delay_options` part in the [ResponseOptions proto](/api/server/response_options.proto). If specified, it can be used to:
+The Dynamic Delay interprets the `DynamicDelayConfiguration` for startup configuration, and the
+`oneof_delay_options` part in the [ResponseOptions proto](/api/server/response_options.proto) for
+`x-nighthawk-test-server-config`. If specified, it can be used to:
 
 - Configure a static delay via `static_delay`.
 - Configure a delay which linearly increase as the number of active requests grows, representing a simplified model of an overloaded server, via `concurrency_based_linear_delay`.