buffer filter: Deprecate max_request_time timeout (#5290)

Description: The new request_timeout in http connection manager covers the filter chain.
Risk Level: N/A
Fixes #4830
Signed-off-by: Auni Ahsan <auni@google.com>

DEPRECATED.md

@@ -16,6 +16,7 @@ A logged warning is expected for each deprecated item that is in deprecation win
 * Use of `runtime_key` in `RequestMirrorPolicy`, found in
   [route.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/route/route.proto)
   is deprecated. Set the `runtime_fraction` field instead.
+* Use of buffer filter `max_request_time` is deprecated in favor of the request timeout found in [HttpConnectionManager](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto)
 
 ## Version 1.8.0 (Oct 4, 2018)
 

api/envoy/config/filter/http/buffer/v2/buffer.proto

@@ -19,13 +19,10 @@ message Buffer {
 
   // The maximum number of seconds that the filter will wait for a complete
   // request before returning a 408 response.
-  google.protobuf.Duration max_request_time = 2 [
-    (validate.rules).duration = {
-      required: true,
-      gt: {}
-    },
-    (gogoproto.stdduration) = true
-  ];
+  // deprecated in favor of http connection manager of :ref:request timeouts
+  // <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.request_timeout>
+  google.protobuf.Duration max_request_time = 2
+      [deprecated = true, (validate.rules).duration = {gt: {}}, (gogoproto.stdduration) = true];
 }
 
 message BufferPerRoute {

source/common/json/config_schemas.cc

@@ -936,7 +936,7 @@ const std::string Json::Schema::BUFFER_HTTP_FILTER_SCHEMA(R"EOF(
       "max_request_bytes" : {"type" : "integer"},
       "max_request_time_s" : {"type" : "integer"}
     },
-    "required" : ["max_request_bytes", "max_request_time_s"],
+    "required" : ["max_request_bytes"],
     "additionalProperties" : false
   }
   )EOF");

source/extensions/filters/http/buffer/buffer_filter.cc

@@ -23,8 +23,8 @@ BufferFilterSettings::BufferFilterSettings(
     const envoy::config::filter::http::buffer::v2::Buffer& proto_config)
     : disabled_(false),
       max_request_bytes_(static_cast<uint64_t>(proto_config.max_request_bytes().value())),
-      max_request_time_(
-          std::chrono::seconds(PROTOBUF_GET_SECONDS_REQUIRED(proto_config, max_request_time))) {}
+      max_request_time_(std::chrono::seconds(
+          DurationUtil::durationToSeconds((proto_config).max_request_time()))) {}
 
 BufferFilterSettings::BufferFilterSettings(
     const envoy::config::filter::http::buffer::v2::BufferPerRoute& proto_config)
@@ -35,7 +35,7 @@ BufferFilterSettings::BufferFilterSettings(
               : 0),
       max_request_time_(std::chrono::seconds(
           proto_config.has_buffer()
-              ? PROTOBUF_GET_SECONDS_REQUIRED(proto_config.buffer(), max_request_time)
+              ? DurationUtil::durationToSeconds(proto_config.buffer().max_request_time())
               : 0)) {}
 
 BufferFilterConfig::BufferFilterConfig(
@@ -82,7 +82,9 @@ Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap&, bool end
 
   callbacks_->setDecoderBufferLimit(settings_->maxRequestBytes());
   request_timeout_ = callbacks_->dispatcher().createTimer([this]() -> void { onRequestTimeout(); });
-  request_timeout_->enableTimer(settings_->maxRequestTime());
+  if (settings_->maxRequestTime().count() != 0) {
+    request_timeout_->enableTimer(settings_->maxRequestTime());
+  }
 
   return Http::FilterHeadersStatus::StopIteration;
 }

source/extensions/filters/http/buffer/config.cc

@@ -20,7 +20,6 @@ Http::FilterFactoryCb BufferFilterFactory::createFilterFactoryFromProtoTyped(
     const envoy::config::filter::http::buffer::v2::Buffer& proto_config,
     const std::string& stats_prefix, Server::Configuration::FactoryContext& context) {
   ASSERT(proto_config.has_max_request_bytes());
-  ASSERT(proto_config.has_max_request_time());
 
   BufferFilterConfigSharedPtr filter_config(
       new BufferFilterConfig(proto_config, stats_prefix, context.scope()));

test/extensions/filters/http/buffer/buffer_filter_test.cc

@@ -79,6 +79,20 @@ TEST_F(BufferFilterTest, RequestWithData) {
   EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data2, true));
 }
 
+TEST_F(BufferFilterTest, RequestTimeoutNotEnabledWithZero) {
+  InSequence s;
+
+  expectTimerCreate();
+
+  // timer config is 0 by default
+  EXPECT_CALL(*timer_, enableTimer(_)).Times(0);
+  Http::TestHeaderMapImpl headers;
+  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(headers, false));
+
+  filter_.onDestroy();
+  EXPECT_EQ(0U, config_->stats().rq_timeout_.value());
+}
+
 TEST_F(BufferFilterTest, RequestTimeout) {
   InSequence s;
 

test/extensions/filters/http/buffer/config_test.cc

@@ -16,11 +16,20 @@ namespace Extensions {
 namespace HttpFilters {
 namespace BufferFilter {
 
-TEST(BufferFilterFactoryTest, ValidateFail) {
+TEST(BufferFilterFactoryTest, BufferFilterCorrectWithoutRequestTime) {
+  std::string json_string = R"EOF(
+  {
+    "max_request_bytes" : 1028
+  }
+  )EOF";
+
+  Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string);
   NiceMock<Server::Configuration::MockFactoryContext> context;
-  EXPECT_THROW(BufferFilterFactory().createFilterFactoryFromProto(
-                   envoy::config::filter::http::buffer::v2::Buffer(), "stats", context),
-               ProtoValidationException);
+  BufferFilterFactory factory;
+  Http::FilterFactoryCb cb = factory.createFilterFactory(*json_config, "stats", context);
+  Http::MockFilterChainFactoryCallbacks filter_callback;
+  EXPECT_CALL(filter_callback, addStreamDecoderFilter(_));
+  cb(filter_callback);
 }
 
 TEST(BufferFilterFactoryTest, BufferFilterCorrectJson) {