review feedback

oschaaf · oschaaf · commit 8ee78db003c1 · 2020-07-08T11:54:45.000+02:00
Signed-off-by: Otto van der Schaaf &lt;oschaaf@we-amp.com&gt;
diff --git a/source/server/README.md b/source/server/README.md
@@ -77,7 +77,7 @@ admin:
 
 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 a what
+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.
 
diff --git a/source/server/http_dynamic_delay_filter.cc b/source/server/http_dynamic_delay_filter.cc
@@ -55,21 +55,30 @@ HttpDynamicDelayDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& head
     delay_ms = computeDelayMilliseconds(current_value, concurrency_config.minimal_delay(),
                                         concurrency_config.concurrency_delay_factor());
   }
+  maybeRequestFaultFilterDelay(delay_ms, headers);
+  return Envoy::Http::FilterHeadersStatus::Continue;
+}
+
+void HttpDynamicDelayDecoderFilter::maybeRequestFaultFilterDelay(
+    const absl::optional<int64_t> delay_ms, Envoy::Http::RequestHeaderMap& headers) {
   if (delay_ms.has_value() && delay_ms > 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_ms));
   }
-  return Envoy::Http::FilterHeadersStatus::Continue;
 }
 
 Envoy::Http::FilterDataStatus HttpDynamicDelayDecoderFilter::decodeData(Envoy::Buffer::Instance&,
                                                                         bool) {
+  // Minimal implementation that satisfies the contract imposed by Http::StreamDecoderFilter:
+  // we do nothing, and request the state machine that drives us to continue.
   return Envoy::Http::FilterDataStatus::Continue;
 }
 
 Envoy::Http::FilterTrailersStatus
 HttpDynamicDelayDecoderFilter::decodeTrailers(Envoy::Http::RequestTrailerMap&) {
+  // Minimal implementation that satisfies the contract imposed by Http::StreamDecoderFilter:
+  // we do nothing, and request the state machine that drives us to continue.
   return Envoy::Http::FilterTrailersStatus::Continue;
 }
 
diff --git a/source/server/http_dynamic_delay_filter.h b/source/server/http_dynamic_delay_filter.h
@@ -86,6 +86,17 @@ class HttpDynamicDelayDecoderFilter : public Envoy::Http::StreamDecoderFilter {
                       1e6);
   }
 
+  /**
+   * Communicate to the fault filter, which should be running after this filter, that a delay should
+   * be inserted. The request is only made when the passed delay is set to a value > 0.
+   *
+   * @param delay_ms The delay that should be propagated, if any. When not set or <= 0, the call
+   * will be a no-op.
+   * @param request_headers The request headers that will be modified to instruct the faul filter.
+   */
+  static void maybeRequestFaultFilterDelay(const absl::optional<int64_t> delay_ms,
+                                           Envoy::Http::RequestHeaderMap& request_headers);
+
 private:
   const HttpDynamicDelayDecoderFilterConfigSharedPtr config_;
   Envoy::Http::StreamDecoderFilterCallbacks* decoder_callbacks_;
diff --git a/source/server/http_dynamic_delay_filter_config.cc b/source/server/http_dynamic_delay_filter_config.cc
@@ -12,6 +12,7 @@
 namespace Nighthawk {
 namespace Server {
 namespace Configuration {
+namespace {
 
 class HttpDynamicDelayDecoderFilterConfigFactory
     : public Envoy::Server::Configuration::NamedHttpFilterConfigFactory {
@@ -47,6 +48,8 @@ class HttpDynamicDelayDecoderFilterConfigFactory
   }
 };
 
+} // namespace
+
 static Envoy::Registry::RegisterFactory<HttpDynamicDelayDecoderFilterConfigFactory,
                                         Envoy::Server::Configuration::NamedHttpFilterConfigFactory>
     register_;

Original file line number	Diff line number	Diff line change
`@@ -55,21 +55,30 @@ HttpDynamicDelayDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& head`
`55`	`55`	`delay_ms = computeDelayMilliseconds(current_value, concurrency_config.minimal_delay(),`
`56`	`56`	`concurrency_config.concurrency_delay_factor());`
`57`	`57`	`}`
	`58`	`+ maybeRequestFaultFilterDelay(delay_ms, headers);`
	`59`	`+ return Envoy::Http::FilterHeadersStatus::Continue;`
	`60`	`+}`
	`61`	`+`
	`62`	`+void HttpDynamicDelayDecoderFilter::maybeRequestFaultFilterDelay(`
	`63`	`+ const absl::optional<int64_t> delay_ms, Envoy::Http::RequestHeaderMap& headers) {`
`58`	`64`	`if (delay_ms.has_value() && delay_ms > 0) {`
`59`	`65`	`// Emit header to communicate the delay we desire to the fault filter extension.`
`60`	`66`	`const Envoy::Http::LowerCaseString key("x-envoy-fault-delay-request");`
`61`	`67`	`headers.setCopy(key, absl::StrCat(*delay_ms));`
`62`	`68`	`}`
`63`		`- return Envoy::Http::FilterHeadersStatus::Continue;`
`64`	`69`	`}`
`65`	`70`
`66`	`71`	`Envoy::Http::FilterDataStatus HttpDynamicDelayDecoderFilter::decodeData(Envoy::Buffer::Instance&,`
`67`	`72`	`bool) {`
	`73`	`+ // Minimal implementation that satisfies the contract imposed by Http::StreamDecoderFilter:`
	`74`	`+ // we do nothing, and request the state machine that drives us to continue.`
`68`	`75`	`return Envoy::Http::FilterDataStatus::Continue;`
`69`	`76`	`}`
`70`	`77`
`71`	`78`	`Envoy::Http::FilterTrailersStatus`
`72`	`79`	`HttpDynamicDelayDecoderFilter::decodeTrailers(Envoy::Http::RequestTrailerMap&) {`
	`80`	`+ // Minimal implementation that satisfies the contract imposed by Http::StreamDecoderFilter:`
	`81`	`+ // we do nothing, and request the state machine that drives us to continue.`
`73`	`82`	`return Envoy::Http::FilterTrailersStatus::Continue;`
`74`	`83`	`}`
`75`	`84`