http connection manager: Add configurable request path timeout (#4456)

Signed-off-by: Auni Ahsan <[email protected]>

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -20,7 +20,7 @@ import "gogoproto/gogo.proto";
 // [#protodoc-title: HTTP connection manager]
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 
-// [#comment:next free field: 28]
+// [#comment:next free field: 29]
 message HttpConnectionManager {
   enum CodecType {
     option (gogoproto.goproto_enum_prefix) = false;
@@ -165,6 +165,12 @@ message HttpConnectionManager {
   // timeout, although per-route idle timeout overrides will continue to apply.
   google.protobuf.Duration stream_idle_timeout = 24 [(gogoproto.stdduration) = true];
 
+  // A timeout for idle requests managed by the connection manager.
+  // The timer is activated when the request is initiated, and is disarmed when the last byte of the
+  // request is sent upstream (i.e. all decoding filters have processed the request), OR when the
+  // response is initiated. If not specified or set to 0, this timeout is disabled.
+  google.protobuf.Duration request_timeout = 28 [(gogoproto.stdduration) = true];
+
   // The time that Envoy will wait between sending an HTTP/2 “shutdown
   // notification” (GOAWAY frame with max stream ID) and a final GOAWAY frame.
   // This is used so that Envoy provides a grace period for new streams that

docs/root/configuration/http_conn_man/stats.rst

@@ -53,8 +53,9 @@ statistics:
    downstream_rq_4xx, Counter, Total 4xx responses
    downstream_rq_5xx, Counter, Total 5xx responses
    downstream_rq_ws_on_non_ws_route, Counter, Total WebSocket upgrade requests rejected by non WebSocket routes
-   downstream_rq_time, Histogram, Request time milliseconds
+   downstream_rq_time, Histogram, Total time for request and response (milliseconds)
    downstream_rq_idle_timeout, Counter, Total requests closed due to idle timeout
+   downstream_rq_timeout, Counter, Total requests closed due to a timeout on the request path
    downstream_rq_overload_close, Counter, Total requests closed due to envoy overload
    rs_too_large, Counter, Total response errors due to buffering an overly large body
 

docs/root/intro/version_history.rst

@@ -16,6 +16,7 @@ Version history
 * fault: removed integer percentage support.
 * http: Added HTTP/2 WebSocket proxying via :ref:`extended CONNECT <envoy_api_field_core.Http2ProtocolOptions.allow_connect>`
 * http: added limits to the number and length of header modifications in all fields request_headers_to_add and response_headers_to_add. These limits are very high and should only be used as a last-resort safeguard.
+* http: added support for a :ref:`request timeout <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.request_timeout>`. The timeout is disabled by default.
 * http: no longer adding whitespace when appending X-Forwarded-For headers. **Warning**: this is not
   compatible with 1.7.0 builds prior to `9d3a4eb4ac44be9f0651fcc7f87ad98c538b01ee <https://github.com/envoyproxy/envoy/pull/3610>`_.
   See `#3611 <https://github.com/envoyproxy/envoy/issues/3611>`_ for details.

source/common/http/conn_manager_config.h

@@ -62,6 +62,7 @@ namespace Http {
   HISTOGRAM(downstream_rq_time)                                                                    \
   COUNTER  (downstream_rq_idle_timeout)                                                            \
   COUNTER  (downstream_rq_overload_close)                                                          \
+  COUNTER  (downstream_rq_timeout)                                                            \
   COUNTER  (rs_too_large)
 // clang-format on
 
@@ -233,6 +234,12 @@ class ConnectionManagerConfig {
    */
   virtual std::chrono::milliseconds streamIdleTimeout() const PURE;
 
+  /**
+   * @return request timeout for incoming connection manager connections. Zero indicates
+   *         a disabled request timeout.
+   */
+  virtual std::chrono::milliseconds requestTimeout() const PURE;
+
   /**
    * @return delayed close timeout for downstream HTTP connections. Zero indicates a disabled
    *         timeout. See http_connection_manager.proto for a detailed description of this timeout.

source/common/http/conn_manager_impl.cc

@@ -357,8 +357,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
     : connection_manager_(connection_manager),
       snapped_route_config_(connection_manager.config_.routeConfigProvider().config()),
       stream_id_(connection_manager.random_generator_.random()),
-      request_timer_(new Stats::Timespan(connection_manager_.stats_.named_.downstream_rq_time_,
-                                         connection_manager_.timeSystem())),
+      request_response_timespan_(new Stats::Timespan(
+          connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSystem())),
       stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSystem()) {
   connection_manager_.stats_.named_.downstream_rq_total_.inc();
   connection_manager_.stats_.named_.downstream_rq_active_.inc();
@@ -382,6 +382,14 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
         [this]() -> void { onIdleTimeout(); });
     resetIdleTimer();
   }
+
+  if (connection_manager_.config_.requestTimeout().count()) {
+    std::chrono::milliseconds request_timeout_ms_ = connection_manager_.config_.requestTimeout();
+    request_timer_ = connection_manager.read_callbacks_->connection().dispatcher().createTimer(
+        [this]() -> void { onRequestTimeout(); });
+    request_timer_->enableTimer(request_timeout_ms_);
+  }
+
   stream_info_.setRequestedServerName(
       connection_manager_.read_callbacks_->connection().requestedServerName());
 }
@@ -437,6 +445,13 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() {
   }
 }
 
+void ConnectionManagerImpl::ActiveStream::onRequestTimeout() {
+  connection_manager_.stats_.named_.downstream_rq_timeout_.inc();
+  sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_),
+                 Http::Code::RequestTimeout, "request timeout", nullptr, is_head_request_,
+                 absl::nullopt);
+}
+
 void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker(
     StreamDecoderFilterSharedPtr filter, bool dual_filter) {
   ActiveStreamDecoderFilterPtr wrapper(new ActiveStreamDecoderFilter(*this, filter, dual_filter));
@@ -748,6 +763,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte
     (*continue_data_entry)->stopped_ = true;
     (*continue_data_entry)->continueDecoding();
   }
+
+  if (end_stream) {
+    disarmRequestTimeout();
+  }
 }
 
 void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, bool end_stream) {
@@ -813,6 +832,10 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter*
   if (trailers_added_entry != decoder_filters_.end()) {
     decodeTrailers(trailers_added_entry->get(), *request_trailers_);
   }
+
+  if (end_stream) {
+    disarmRequestTimeout();
+  }
 }
 
 HeaderMap& ConnectionManagerImpl::ActiveStream::addDecodedTrailers() {
@@ -879,6 +902,7 @@ void ConnectionManagerImpl::ActiveStream::decodeTrailers(ActiveStreamDecoderFilt
       return;
     }
   }
+  disarmRequestTimeout();
 }
 
 void ConnectionManagerImpl::ActiveStream::maybeEndDecode(bool end_stream) {
@@ -890,6 +914,12 @@ void ConnectionManagerImpl::ActiveStream::maybeEndDecode(bool end_stream) {
   }
 }
 
+void ConnectionManagerImpl::ActiveStream::disarmRequestTimeout() {
+  if (request_timer_) {
+    request_timer_->disableTimer();
+  }
+}
+
 std::list<ConnectionManagerImpl::ActiveStreamEncoderFilterPtr>::iterator
 ConnectionManagerImpl::ActiveStream::commonEncodePrefix(ActiveStreamEncoderFilter* filter,
                                                         bool end_stream) {
@@ -933,6 +963,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply(
     bool is_grpc_request, Code code, const std::string& body,
     std::function<void(HeaderMap& headers)> modify_headers, bool is_head_request,
     const absl::optional<Grpc::Status::GrpcStatus> grpc_status) {
+  ASSERT(response_headers_ == nullptr);
   Utility::sendLocalReply(is_grpc_request,
                           [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
                             if (modify_headers != nullptr) {
@@ -991,6 +1022,7 @@ void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders(
 void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilter* filter,
                                                         HeaderMap& headers, bool end_stream) {
   resetIdleTimer();
+  disarmRequestTimeout();
 
   std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, end_stream);
   std::list<ActiveStreamEncoderFilterPtr>::iterator continue_data_entry = encoder_filters_.end();
@@ -1222,7 +1254,7 @@ void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilt
 void ConnectionManagerImpl::ActiveStream::maybeEndEncode(bool end_stream) {
   if (end_stream) {
     stream_info_.onLastDownstreamTxByteSent();
-    request_timer_->complete();
+    request_response_timespan_->complete();
     connection_manager_.doEndStream(*this);
   }
 }

source/common/http/conn_manager_impl.h

@@ -279,6 +279,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     void decodeHeaders(ActiveStreamDecoderFilter* filter, HeaderMap& headers, bool end_stream);
     void decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream);
     void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers);
+    void disarmRequestTimeout();
     void maybeEndDecode(bool end_stream);
     void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming);
     HeaderMap& addEncodedTrailers();
@@ -379,6 +380,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     void onIdleTimeout();
     // Reset per-stream idle timer.
     void resetIdleTimer();
+    // Per-stream request timeout callback
+    void onRequestTimeout();
 
     ConnectionManagerImpl& connection_manager_;
     Router::ConfigConstSharedPtr snapped_route_config_;
@@ -395,9 +398,11 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     std::list<ActiveStreamDecoderFilterPtr> decoder_filters_;
     std::list<ActiveStreamEncoderFilterPtr> encoder_filters_;
     std::list<AccessLog::InstanceSharedPtr> access_log_handlers_;
-    Stats::TimespanPtr request_timer_;
+    Stats::TimespanPtr request_response_timespan_;
     // Per-stream idle timeout.
     Event::TimerPtr idle_timer_;
+    // Per-stream request timeout.
+    Event::TimerPtr request_timer_;
     std::chrono::milliseconds idle_timeout_ms_{};
     State state_;
     StreamInfo::StreamInfoImpl stream_info_;

source/extensions/filters/network/http_connection_manager/config.cc

@@ -150,6 +150,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
       idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(config, idle_timeout)),
       stream_idle_timeout_(
           PROTOBUF_GET_MS_OR_DEFAULT(config, stream_idle_timeout, StreamIdleTimeoutMs)),
+      request_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, request_timeout, RequestTimeoutMs)),
       drain_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, drain_timeout, 5000)),
       generate_request_id_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, generate_request_id, true)),
       date_provider_(date_provider),

source/extensions/filters/network/http_connection_manager/config.h

@@ -111,6 +111,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   bool generateRequestId() override { return generate_request_id_; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
+  std::chrono::milliseconds requestTimeout() const override { return request_timeout_; }
   Router::RouteConfigProvider& routeConfigProvider() override { return *route_config_provider_; }
   const std::string& serverName() override { return server_name_; }
   Http::ConnectionManagerStats& stats() override { return stats_; }
@@ -167,6 +168,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   absl::optional<std::string> user_agent_;
   absl::optional<std::chrono::milliseconds> idle_timeout_;
   std::chrono::milliseconds stream_idle_timeout_;
+  std::chrono::milliseconds request_timeout_;
   Router::RouteConfigProviderPtr route_config_provider_;
   std::chrono::milliseconds drain_timeout_;
   bool generate_request_id_;
@@ -177,6 +179,8 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
 
   // Default idle timeout is 5 minutes if nothing is specified in the HCM config.
   static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000;
+  // request timeout is disabled by default
+  static const uint64_t RequestTimeoutMs = 0;
 };
 
 } // namespace HttpConnectionManager

source/server/http/admin.h

@@ -100,6 +100,7 @@ class AdminImpl : public Admin,
   bool generateRequestId() override { return false; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return {}; }
+  std::chrono::milliseconds requestTimeout() const override { return {}; }
   std::chrono::milliseconds delayedCloseTimeout() const override { return {}; }
   Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; }
   const std::string& serverName() override { return Http::DefaultServerString::get(); }

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -92,6 +92,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   bool generateRequestId() override { return true; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
+  std::chrono::milliseconds requestTimeout() const override { return request_timeout_; }
   std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; }
   Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; }
   const std::string& serverName() override { return server_name_; }
@@ -131,6 +132,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   ConnectionManagerTracingStats tracing_stats_;
   ConnectionManagerListenerStats listener_stats_;
   std::chrono::milliseconds stream_idle_timeout_{};
+  std::chrono::milliseconds request_timeout_{};
   std::chrono::milliseconds delayed_close_timeout_{};
   bool use_remote_address_{true};
   Http::ForwardClientCertType forward_client_cert_{Http::ForwardClientCertType::Sanitize};