Address code review feedback

Signed-off-by: Nathan Perry <[email protected]>
envoyproxy · Jun 6, 2022 · d0d52be · d0d52be
1 parent 713ef14
commit d0d52be
Show file tree

Hide file tree

Showing 9 changed files with 114 additions and 11 deletions.
diff --git a/CHANGELOG.md b/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.
diff --git a/api/server/dynamic_delay.proto b/api/server/dynamic_delay.proto
@@ -1,3 +1,4 @@
+// Proto file supporting any configuration related to the dynamic delay filter.
 syntax = "proto3";
 
 import "api/server/response_options.proto";

diff --git a/api/server/time_tracking.proto b/api/server/time_tracking.proto
@@ -1,7 +1,6 @@
+// Proto file supporting any configuration related to the time tracking filter.
 syntax = "proto3";
 
-import "api/server/response_options.proto";
-
 package nighthawk.server;
 
 // Configures the time-tracking filter.

diff --git a/source/server/http_dynamic_delay_filter.cc b/source/server/http_dynamic_delay_filter.cc
@@ -80,7 +80,7 @@ HttpDynamicDelayDecoderFilterConfig::HttpDynamicDelayDecoderFilterConfig(
       server_config_(std::make_shared<DynamicDelayConfiguration>(proto_config)) {}
 
 std::shared_ptr<const DynamicDelayConfiguration>
-HttpDynamicDelayDecoderFilterConfig::getServerConfig() {
+HttpDynamicDelayDecoderFilterConfig::getStartupFilterConfiguration() {
   return server_config_;
 }
 
@@ -105,7 +105,7 @@ void HttpDynamicDelayDecoderFilter::onDestroy() {
 Envoy::Http::FilterHeadersStatus
 HttpDynamicDelayDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers,
                                              bool end_stream) {
-  effective_config_ = computeEffectiveConfiguration(config_->getServerConfig(), headers);
+  effective_config_ = computeEffectiveConfiguration(config_->getStartupFilterConfiguration(), headers);
   if (effective_config_.ok()) {
     const absl::optional<int64_t> delay_ms =
         computeDelayMs(*effective_config_.value(), config_->approximateFilterInstances());

diff --git a/source/server/http_dynamic_delay_filter.h b/source/server/http_dynamic_delay_filter.h
@@ -80,7 +80,7 @@ class HttpDynamicDelayDecoderFilterConfig : public FilterConfigurationBase {
    * @return std::shared_ptr<const nighthawk::server::DynamicDelayConfiguration> the base
    * configuration for this filter.
    */
-  std::shared_ptr<const nighthawk::server::DynamicDelayConfiguration> getServerConfig();
+  std::shared_ptr<const nighthawk::server::DynamicDelayConfiguration> getStartupFilterConfiguration();
 
 private:
   static std::atomic<uint64_t>& instances() {

diff --git a/source/server/http_test_server_filter.cc b/source/server/http_test_server_filter.cc
@@ -48,7 +48,7 @@ HttpTestServerDecoderFilterConfig::HttpTestServerDecoderFilterConfig(
     : FilterConfigurationBase("test-server"),
       server_config_(std::make_shared<ResponseOptions>(proto_config)) {}
 
-std::shared_ptr<const ResponseOptions> HttpTestServerDecoderFilterConfig::getServerConfig() {
+std::shared_ptr<const ResponseOptions> HttpTestServerDecoderFilterConfig::getStartupFilterConfiguration() {
   return server_config_;
 }
 
@@ -74,7 +74,7 @@ void HttpTestServerDecoderFilter::sendReply(const ResponseOptions& options) {
 Envoy::Http::FilterHeadersStatus
 HttpTestServerDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers,
                                            bool end_stream) {
-  effective_config_ = computeEffectiveConfiguration(config_->getServerConfig(), headers);
+  effective_config_ = computeEffectiveConfiguration(config_->getStartupFilterConfiguration(), headers);
   if (end_stream) {
     if (!config_->validateOrSendError(effective_config_.status(), *decoder_callbacks_)) {
       if (effective_config_.value()->echo_request_headers()) {

diff --git a/source/server/http_test_server_filter.h b/source/server/http_test_server_filter.h
@@ -18,7 +18,11 @@ class HttpTestServerDecoderFilterConfig : public FilterConfigurationBase {
 public:
   HttpTestServerDecoderFilterConfig(const nighthawk::server::ResponseOptions& proto_config);
 
-  std::shared_ptr<const nighthawk::server::ResponseOptions> getServerConfig();
+  /**
+   * @return std::shared_ptr<const nighthawk::server::TimeTrackingConfiguration> the base
+   * configuration for this filter.
+   */
+  std::shared_ptr<const nighthawk::server::ResponseOptions> getStartupFilterConfiguration();
 
 private:
   std::shared_ptr<const nighthawk::server::ResponseOptions> server_config_;

diff --git a/source/server/http_time_tracking_filter.cc b/source/server/http_time_tracking_filter.cc
@@ -82,7 +82,7 @@ HttpTimeTrackingFilterConfig::getElapsedNanosSinceLastRequest(Envoy::TimeSource&
   return stopwatch_->getElapsedNsAndReset(time_source);
 }
 
-std::shared_ptr<const TimeTrackingConfiguration> HttpTimeTrackingFilterConfig::getServerConfig() {
+std::shared_ptr<const TimeTrackingConfiguration> HttpTimeTrackingFilterConfig::getStartupFilterConfiguration() {
   return server_config_;
 }
 
@@ -91,7 +91,7 @@ HttpTimeTrackingFilter::HttpTimeTrackingFilter(HttpTimeTrackingFilterConfigShare
 
 Envoy::Http::FilterHeadersStatus
 HttpTimeTrackingFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers, bool end_stream) {
-  effective_config_ = computeEffectiveConfiguration(config_->getServerConfig(), headers);
+  effective_config_ = computeEffectiveConfiguration(config_->getStartupFilterConfiguration(), headers);
   if (end_stream && config_->validateOrSendError(effective_config_.status(), *decoder_callbacks_)) {
     return Envoy::Http::FilterHeadersStatus::StopIteration;
   }

diff --git a/source/server/http_time_tracking_filter.h b/source/server/http_time_tracking_filter.h
@@ -46,7 +46,7 @@ class HttpTimeTrackingFilterConfig : public FilterConfigurationBase {
    * @return std::shared_ptr<const nighthawk::server::TimeTrackingConfiguration> the base
    * configuration for this filter.
    */
-  std::shared_ptr<const nighthawk::server::TimeTrackingConfiguration> getServerConfig();
+  std::shared_ptr<const nighthawk::server::TimeTrackingConfiguration> getStartupFilterConfiguration();
 
 private:
   std::unique_ptr<Stopwatch> stopwatch_;