diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index b9607c9c3b39d..1b286f911ff78 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -793,7 +793,7 @@ message CorsPolicy { google.protobuf.BoolValue forward_not_matching_preflights = 13; } -// [#next-free-field: 42] +// [#next-free-field: 43] message RouteAction { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RouteAction"; @@ -1318,8 +1318,28 @@ message RouteAction { // If the :ref:`overload action ` "envoy.overload_actions.reduce_timeouts" // is configured, this timeout is scaled according to the value for // :ref:`HTTP_DOWNSTREAM_STREAM_IDLE `. + // + // This timeout may also be used in place of ``flush_timeout`` in very specific cases. See the + // documentation for ``flush_timeout`` for more details. google.protobuf.Duration idle_timeout = 24; + // Specifies the codec stream flush timeout for the route. + // + // If not specified, the first preference is the global :ref:`stream_flush_timeout + // `, + // but only if explicitly configured. + // + // If neither the explicit HCM-wide flush timeout nor this route-specific flush timeout is configured, + // the route's stream idle timeout is reused for this timeout. This is for + // backwards compatibility since both behaviors were historically controlled by the one timeout. + // + // If the route also does not have an idle timeout configured, the global :ref:`stream_idle_timeout + // `. used, again + // for backwards compatibility. That timeout defaults to 5 minutes. + // + // A value of 0 via any of the above paths will completely disable the timeout for a given route. + google.protobuf.Duration flush_timeout = 42; + // Specifies how to send request over TLS early data. // If absent, allows `safe HTTP requests `_ to be sent on early data. // [#extension-category: envoy.route.early_data_policy] diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 32389d90bf30f..730e065e6c417 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -37,7 +37,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // HTTP connection manager :ref:`configuration overview `. // [#extension: envoy.filters.network.http_connection_manager] -// [#next-free-field: 59] +// [#next-free-field: 60] message HttpConnectionManager { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager"; @@ -527,16 +527,6 @@ message HttpConnectionManager { // is terminated with a 408 Request Timeout error code if no upstream response // header has been received, otherwise a stream reset occurs. // - // This timeout also specifies the amount of time that Envoy will wait for the peer to open enough - // window to write any remaining stream data once the entirety of stream data (local end stream is - // true) has been buffered pending available window. In other words, this timeout defends against - // a peer that does not release enough window to completely write the stream, even though all - // data has been proxied within available flow control windows. If the timeout is hit in this - // case, the :ref:`tx_flush_timeout ` counter will be - // incremented. Note that :ref:`max_stream_duration - // ` does not apply to - // this corner case. - // // If the :ref:`overload action ` "envoy.overload_actions.reduce_timeouts" // is configured, this timeout is scaled according to the value for // :ref:`HTTP_DOWNSTREAM_STREAM_IDLE `. @@ -549,9 +539,29 @@ message HttpConnectionManager { // // A value of 0 will completely disable the connection manager stream idle // timeout, although per-route idle timeout overrides will continue to apply. + // + // This timeout is also used as the default value for :ref:`stream_flush_timeout + // `. google.protobuf.Duration stream_idle_timeout = 24 [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // The stream flush timeout for connections managed by the connection manager. + // + // If not specified, the value of stream_idle_timeout is used. This is for backwards compatibility + // since this was the original behavior. In essence this timeout is an override for the + // stream_idle_timeout that applies specifically to the end of stream flush case. + // + // This timeout specifies the amount of time that Envoy will wait for the peer to open enough + // window to write any remaining stream data once the entirety of stream data (local end stream is + // true) has been buffered pending available window. In other words, this timeout defends against + // a peer that does not release enough window to completely write the stream, even though all + // data has been proxied within available flow control windows. If the timeout is hit in this + // case, the :ref:`tx_flush_timeout ` counter will be + // incremented. Note that :ref:`max_stream_duration + // ` does not apply to + // this corner case. + google.protobuf.Duration stream_flush_timeout = 59; + // The amount of time that Envoy will wait for the entire request to be received. // 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 diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 3b380f3efb75c..50df7d115762e 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -296,6 +296,11 @@ new_features: - area: http change: | Added ``upstream_rq_per_cx`` histogram to track requests per connection for monitoring connection reuse efficiency. - +- area: http + change: | + Added + :ref:`stream_flush_timeout + ` + to allow for configuring a stream flush timeout independently from the stream idle timeout. deprecated: diff --git a/envoy/router/router.h b/envoy/router/router.h index 2fe9d575342d4..0569f793f42cd 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -1033,6 +1033,12 @@ class RouteEntry : public ResponseEntry { */ virtual absl::optional idleTimeout() const PURE; + /** + * @return optional the route's flush timeout. Zero indicates a + * disabled idle timeout, while nullopt indicates deference to the global timeout. + */ + virtual absl::optional flushTimeout() const PURE; + /** * @return true if new style max_stream_duration config should be used over the old style. */ diff --git a/source/common/http/codec_helper.h b/source/common/http/codec_helper.h index a6ee26e1db765..aec7224bfac5f 100644 --- a/source/common/http/codec_helper.h +++ b/source/common/http/codec_helper.h @@ -89,11 +89,11 @@ class StreamCallbackHelper { class MultiplexedStreamImplBase : public Stream, public StreamCallbackHelper { public: MultiplexedStreamImplBase(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} - ~MultiplexedStreamImplBase() override { ASSERT(stream_idle_timer_ == nullptr); } + ~MultiplexedStreamImplBase() override { ASSERT(stream_flush_timer_ == nullptr); } // TODO(mattklein123): Optimally this would be done in the destructor but there are currently // deferred delete lifetime issues that need sorting out if the destructor of the stream is // going to be able to refer to the parent connection. - virtual void destroy() { disarmStreamIdleTimer(); } + virtual void destroy() { disarmStreamFlushTimer(); } void onLocalEndStream() { ASSERT(local_end_stream_); @@ -102,11 +102,11 @@ class MultiplexedStreamImplBase : public Stream, public StreamCallbackHelper { } } - void disarmStreamIdleTimer() { - if (stream_idle_timer_ != nullptr) { + void disarmStreamFlushTimer() { + if (stream_flush_timer_ != nullptr) { // To ease testing and the destructor assertion. - stream_idle_timer_->disableTimer(); - stream_idle_timer_.reset(); + stream_flush_timer_->disableTimer(); + stream_flush_timer_.reset(); } } @@ -117,18 +117,18 @@ class MultiplexedStreamImplBase : public Stream, public StreamCallbackHelper { protected: void setFlushTimeout(std::chrono::milliseconds timeout) override { - stream_idle_timeout_ = timeout; + stream_flush_timeout_ = timeout; } void createPendingFlushTimer() { - ASSERT(stream_idle_timer_ == nullptr); - if (stream_idle_timeout_.count() > 0) { - stream_idle_timer_ = dispatcher_.createTimer([this] { onPendingFlushTimer(); }); - stream_idle_timer_->enableTimer(stream_idle_timeout_); + ASSERT(stream_flush_timer_ == nullptr); + if (stream_flush_timeout_.count() > 0) { + stream_flush_timer_ = dispatcher_.createTimer([this] { onPendingFlushTimer(); }); + stream_flush_timer_->enableTimer(stream_flush_timeout_); } } - virtual void onPendingFlushTimer() { stream_idle_timer_.reset(); } + virtual void onPendingFlushTimer() { stream_flush_timer_.reset(); } virtual bool hasPendingData() PURE; @@ -136,9 +136,9 @@ class MultiplexedStreamImplBase : public Stream, public StreamCallbackHelper { private: Event::Dispatcher& dispatcher_; - // See HttpConnectionManager.stream_idle_timeout. - std::chrono::milliseconds stream_idle_timeout_{}; - Event::TimerPtr stream_idle_timer_; + // See HttpConnectionManager.stream_flush_timeout. + std::chrono::milliseconds stream_flush_timeout_{}; + Event::TimerPtr stream_flush_timer_; }; } // namespace Http diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index c80f6bf90deda..071b6464af3fb 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -311,6 +311,12 @@ class ConnectionManagerConfig { */ virtual std::chrono::milliseconds streamIdleTimeout() const PURE; + /** + * @return per-stream flush timeout for incoming connection manager connections. Zero indicates a + * disabled idle timeout. + */ + virtual absl::optional streamFlushTimeout() const PURE; + /** * @return request timeout for incoming connection manager connections. Zero indicates * a disabled request timeout. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 81df610026576..4e12fff583b7e 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -431,7 +431,12 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod new_stream->response_encoder_ = &response_encoder; new_stream->response_encoder_->getStream().addCallbacks(*new_stream); new_stream->response_encoder_->getStream().registerCodecEventCallbacks(new_stream.get()); - new_stream->response_encoder_->getStream().setFlushTimeout(new_stream->idle_timeout_ms_); + if (config_->streamFlushTimeout().has_value()) { + new_stream->response_encoder_->getStream().setFlushTimeout( + config_->streamFlushTimeout().value()); + } else { + new_stream->response_encoder_->getStream().setFlushTimeout(config_->streamIdleTimeout()); + } new_stream->streamInfo().setDownstreamBytesMeter(response_encoder.getStream().bytesMeter()); // If the network connection is backed up, the stream should be made aware of it on creation. // Both HTTP/1.x and HTTP/2 codecs handle this in StreamCallbackHelper::addCallbacksHelper. @@ -846,6 +851,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect connection_manager_.overload_manager_), request_response_timespan_(new Stats::HistogramCompletableTimespanImpl( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), + has_explicit_global_flush_timeout_( + connection_manager.config_->streamFlushTimeout().has_value()), header_validator_( connection_manager.config_->makeHeaderValidator(connection_manager.codec_->protocol())), trace_refresh_after_route_refresh_(Runtime::runtimeFeatureEnabled( @@ -2216,30 +2223,43 @@ void ConnectionManagerImpl::ActiveStream::setVirtualHostRoute( refreshTracing(); refreshDurationTimeout(); - refreshIdleTimeout(); + refreshIdleAndFlushTimeouts(); } -void ConnectionManagerImpl::ActiveStream::refreshIdleTimeout() { - if (hasCachedRoute()) { - const Router::RouteEntry* route_entry = cached_route_.value()->routeEntry(); - if (route_entry != nullptr && route_entry->idleTimeout()) { - idle_timeout_ms_ = route_entry->idleTimeout().value(); - response_encoder_->getStream().setFlushTimeout(idle_timeout_ms_); - if (idle_timeout_ms_.count()) { - // If we have a route-level idle timeout but no global stream idle timeout, create a timer. - if (stream_idle_timer_ == nullptr) { - stream_idle_timer_ = connection_manager_.dispatcher_->createScaledTimer( - Event::ScaledTimerType::HttpDownstreamIdleStreamTimeout, - [this]() -> void { onIdleTimeout(); }); - } - } else if (stream_idle_timer_ != nullptr) { - // If we had a global stream idle timeout but the route-level idle timeout is set to zero - // (to override), we disable the idle timer. - stream_idle_timer_->disableTimer(); - stream_idle_timer_ = nullptr; +void ConnectionManagerImpl::ActiveStream::refreshIdleAndFlushTimeouts() { + if (!hasCachedRoute()) { + return; + } + const Router::RouteEntry* route_entry = cached_route_.value()->routeEntry(); + if (route_entry == nullptr) { + return; + } + + if (route_entry->idleTimeout().has_value()) { + idle_timeout_ms_ = route_entry->idleTimeout().value(); + if (idle_timeout_ms_.count()) { + // If we have a route-level idle timeout but no global stream idle timeout, create a timer. + if (stream_idle_timer_ == nullptr) { + stream_idle_timer_ = connection_manager_.dispatcher_->createScaledTimer( + Event::ScaledTimerType::HttpDownstreamIdleStreamTimeout, + [this]() -> void { onIdleTimeout(); }); } + } else if (stream_idle_timer_ != nullptr) { + // If we had a global stream idle timeout but the route-level idle timeout is set to zero + // (to override), we disable the idle timer. + stream_idle_timer_->disableTimer(); + stream_idle_timer_ = nullptr; } } + + if (route_entry->flushTimeout().has_value()) { + response_encoder_->getStream().setFlushTimeout(route_entry->flushTimeout().value()); + } else if (!has_explicit_global_flush_timeout_ && route_entry->idleTimeout().has_value()) { + // If there is no route-level flush timeout, and the global flush timeout was also inherited + // from the idle timeout, also inherit the route-level idle timeout. This is for backwards + // compatibility. + response_encoder_->getStream().setFlushTimeout(idle_timeout_ms_); + } } void ConnectionManagerImpl::ActiveStream::refreshAccessLogFlushTimer() { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index db0aad3e6ed6c..4a6743e0e27fa 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -333,7 +333,7 @@ class ConnectionManagerImpl : Logger::Loggable, void refreshCachedRoute(const Router::RouteCallback& cb); void refreshDurationTimeout(); - void refreshIdleTimeout(); + void refreshIdleAndFlushTimeouts(); void refreshAccessLogFlushTimer(); void refreshTracing(); @@ -473,6 +473,11 @@ class ConnectionManagerImpl : Logger::Loggable, Event::TimerPtr access_log_flush_timer_; std::chrono::milliseconds idle_timeout_ms_{}; + // If an explicit global flush timeout is set, never override it with the route entry idle + // timeout. If there is no explicit global flush timeout, then override with the route entry + // idle timeout if it exists. This is to prevent breaking existing user expectations that the + // flush timeout is the same as the idle timeout. + const bool has_explicit_global_flush_timeout_{false}; State state_; // Snapshot of the route configuration at the time of request is started. This is used to ensure diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 713eb309af3e8..40b20b20dd590 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -1234,7 +1234,7 @@ int ConnectionImpl::onFrameSend(int32_t stream_id, size_t length, uint8_t type, // teardown. As part of the work to remove exceptions we should aim to clean up all of this // error handling logic and only handle this type of case at the end of dispatch. for (auto& stream : active_streams_) { - stream->disarmStreamIdleTimer(); + stream->disarmStreamFlushTimer(); } return ERR_CALLBACK_FAILURE; } diff --git a/source/common/http/null_route_impl.h b/source/common/http/null_route_impl.h index 2043bf37267b0..7e9c16fe203c0 100644 --- a/source/common/http/null_route_impl.h +++ b/source/common/http/null_route_impl.h @@ -174,6 +174,7 @@ struct RouteEntryImpl : public Router::RouteEntry { } bool usingNewTimeouts() const override { return false; } absl::optional idleTimeout() const override { return absl::nullopt; } + absl::optional flushTimeout() const override { return absl::nullopt; } absl::optional maxStreamDuration() const override { return absl::nullopt; } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index df46ccc0c604b..ae6df86f80d08 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1188,6 +1188,7 @@ RouteEntryImplBase::OptionalTimeouts RouteEntryImplBase::buildOptionalTimeouts( // Calculate how many values are actually set, to initialize `OptionalTimeouts` packed_struct, // avoiding memory re-allocation on each set() call. int num_timeouts_set = route.has_idle_timeout() ? 1 : 0; + num_timeouts_set += route.has_flush_timeout() ? 1 : 0; num_timeouts_set += route.has_max_grpc_timeout() ? 1 : 0; num_timeouts_set += route.has_grpc_timeout_offset() ? 1 : 0; if (route.has_max_stream_duration()) { @@ -1200,6 +1201,10 @@ RouteEntryImplBase::OptionalTimeouts RouteEntryImplBase::buildOptionalTimeouts( timeouts.set( std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(route, idle_timeout))); } + if (route.has_flush_timeout()) { + timeouts.set( + std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(route, flush_timeout))); + } if (route.has_max_grpc_timeout()) { timeouts.set( std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(route, max_grpc_timeout))); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 39c48c059c159..0ccf576b8d76e 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -747,13 +747,17 @@ class RouteEntryImplBase : public RouteEntryAndRoute, GrpcTimeoutHeaderMax, GrpcTimeoutHeaderOffset, MaxGrpcTimeout, - GrpcTimeoutOffset + GrpcTimeoutOffset, + FlushTimeout, }; - using OptionalTimeouts = PackedStruct; + using OptionalTimeouts = PackedStruct; absl::optional idleTimeout() const override { return getOptionalTimeout(); } + absl::optional flushTimeout() const override { + return getOptionalTimeout(); + } absl::optional maxStreamDuration() const override { return getOptionalTimeout(); } diff --git a/source/common/router/delegating_route_impl.cc b/source/common/router/delegating_route_impl.cc index c43d527dae4f0..fe1fb3f64c47e 100644 --- a/source/common/router/delegating_route_impl.cc +++ b/source/common/router/delegating_route_impl.cc @@ -93,6 +93,10 @@ absl::optional DelegatingRouteEntry::idleTimeout() co return base_route_entry_->idleTimeout(); } +absl::optional DelegatingRouteEntry::flushTimeout() const { + return base_route_entry_->flushTimeout(); +} + bool DelegatingRouteEntry::usingNewTimeouts() const { return base_route_entry_->usingNewTimeouts(); } diff --git a/source/common/router/delegating_route_impl.h b/source/common/router/delegating_route_impl.h index e720b6940d29e..bb3ef724189d1 100644 --- a/source/common/router/delegating_route_impl.h +++ b/source/common/router/delegating_route_impl.h @@ -108,6 +108,7 @@ class DelegatingRouteEntry : public DelegatingRouteBase { const std::vector& shadowPolicies() const override; std::chrono::milliseconds timeout() const override; absl::optional idleTimeout() const override; + absl::optional flushTimeout() const override; bool usingNewTimeouts() const override; absl::optional maxStreamDuration() const override; absl::optional grpcTimeoutHeaderMax() const override; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index cf1c3bae5d74c..7f59880891068 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -395,6 +395,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), max_stream_duration)), stream_idle_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, stream_idle_timeout, StreamIdleTimeoutMs)), + stream_flush_timeout_( + PROTOBUF_GET_MS_OR_DEFAULT(config, stream_flush_timeout, stream_idle_timeout_.count())), request_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, request_timeout, RequestTimeoutMs)), request_headers_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, request_headers_timeout, RequestHeaderTimeoutMs)), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 51e4e651f404d..60c8c8daf7087 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -184,6 +184,9 @@ class HttpConnectionManagerConfig : Logger::Loggable, return http1_safe_max_connection_duration_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } + absl::optional streamFlushTimeout() const override { + return stream_flush_timeout_; + } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds requestHeadersTimeout() const override { return request_headers_timeout_; @@ -330,6 +333,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const bool http1_safe_max_connection_duration_; absl::optional max_stream_duration_; std::chrono::milliseconds stream_idle_timeout_; + absl::optional stream_flush_timeout_; std::chrono::milliseconds request_timeout_; std::chrono::milliseconds request_headers_timeout_; Router::RouteConfigProviderSharedPtr route_config_provider_; diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 2bf3706e07100..25af385aff56c 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -152,6 +152,9 @@ class AdminImpl : public Admin, uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; } uint32_t maxRequestHeadersCount() const override { return max_request_headers_count_; } std::chrono::milliseconds streamIdleTimeout() const override { return {}; } + absl::optional streamFlushTimeout() const override { + return std::nullopt; + } std::chrono::milliseconds requestTimeout() const override { return {}; } std::chrono::milliseconds requestHeadersTimeout() const override { return {}; } std::chrono::milliseconds delayedCloseTimeout() const override { return {}; } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 53bc9d6ba4f5b..d7f367977e27c 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -160,6 +160,9 @@ class FuzzConfig : public ConnectionManagerConfig { return max_stream_duration_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } + absl::optional streamFlushTimeout() const override { + return stream_flush_timeout_; + } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds requestHeadersTimeout() const override { return request_headers_timeout_; @@ -282,6 +285,7 @@ class FuzzConfig : public ConnectionManagerConfig { bool http1_safe_max_connection_duration_{false}; absl::optional max_stream_duration_; std::chrono::milliseconds stream_idle_timeout_{}; + std::chrono::milliseconds stream_flush_timeout_{}; std::chrono::milliseconds request_timeout_{}; std::chrono::milliseconds request_headers_timeout_{}; std::chrono::milliseconds delayed_close_timeout_{}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a951ab5f7dfaa..c30e487cb005c 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -3472,6 +3472,104 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteOverride) { filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); } +// Per-stream flush and idle timeouts have a somewhat complex precedence order, so here we test +// every combination of global and per-route flush and idle timeouts. Note that global idle timeout +// not being set or unset doesn't affect the behavior of the flush timeout since it's the same as +// the idle timeout being set explicitly to the default value. For this reason it's a constant in +// the below tests. +class IdleAndFlushTimeoutTestFixture : public HttpConnectionManagerImplMixin, + public testing::TestWithParam> { +public: + IdleAndFlushTimeoutTestFixture() + : global_flush_timeout_set_(std::get<0>(GetParam())), + route_flush_timeout_set_(std::get<1>(GetParam())), + route_idle_timeout_set_(std::get<2>(GetParam())) { + if (route_flush_timeout_set_) { + route_flush_timeout_ = std::chrono::milliseconds(30); + } + if (route_idle_timeout_set_) { + route_idle_timeout_ = std::chrono::milliseconds(40); + } + } + +protected: + const bool global_flush_timeout_set_; + const bool route_flush_timeout_set_; + const bool route_idle_timeout_set_; + absl::optional global_flush_timeout_{absl::nullopt}; + absl::optional route_flush_timeout_{absl::nullopt}; + absl::optional route_idle_timeout_{absl::nullopt}; +}; + +INSTANTIATE_TEST_SUITE_P(IdleAndFlushTimeoutTestFixture, IdleAndFlushTimeoutTestFixture, + testing::Combine(testing::Bool(), testing::Bool(), testing::Bool()), + [](const testing::TestParamInfo>& info) { + return absl::StrCat(std::get<0>(info.param) ? "GlobalFlushTimeoutSet" + : "NoGlobalFlushTimeout", + std::get<1>(info.param) ? "RouteFlushTimeoutSet" + : "NoRouteFlushTimeout", + std::get<2>(info.param) ? "RouteIdleTimeoutSet" + : "NoRouteIdleTimeout"); + }); + +TEST_P(IdleAndFlushTimeoutTestFixture, TestAllCases) { + stream_idle_timeout_ = std::chrono::milliseconds(10); // Constant across all cases. + if (global_flush_timeout_set_) { + stream_flush_timeout_ = std::chrono::milliseconds(20); + } + setup(); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, flushTimeout()) + .WillByDefault(Return(route_flush_timeout_)); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(route_idle_timeout_)); + + EXPECT_CALL(*codec_, dispatch(_)) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { + // Both timers will get initialized in all cases here. The value of the flush timeout + // just depends on whether it was set explicitly or inherited from the idle timeout. + EXPECT_CALL(response_encoder_.stream_, + setFlushTimeout(global_flush_timeout_set_ ? stream_flush_timeout_.value() + : stream_idle_timeout_)); + Event::MockTimer* idle_timer = setUpTimer(); + EXPECT_CALL(*idle_timer, enableTimer(stream_idle_timeout_, _)); + decoder_ = &conn_manager_->newStream(response_encoder_); + + RequestHeaderMapPtr headers{new TestRequestHeaderMapImpl{ + {":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; + + if (route_flush_timeout_set_) { + // If a route flush timeout is set it will ALWAYS be used. + EXPECT_CALL(response_encoder_.stream_, setFlushTimeout(route_flush_timeout_.value())); + } else if (!global_flush_timeout_set_ && route_idle_timeout_set_) { + // If no route flush timeout is set and the global flush timeout was inherited from the + // idle timeout, adopt the route idle timeout. This is for backwards compatibility with + // existing Envoy behavior. + EXPECT_CALL(response_encoder_.stream_, setFlushTimeout(route_idle_timeout_.value())); + } else { + // One of the following is true: + // 1. No route flush or idle timeout is set, so there's nothing to do here. + // 2. The global flush timeout is set explicitly, so the route idle timeout is ignored. + EXPECT_CALL(response_encoder_.stream_, setFlushTimeout(_)).Times(0); + } + + EXPECT_CALL(*idle_timer, disableTimer()); + EXPECT_CALL(*idle_timer, enableTimer(route_idle_timeout_set_ ? route_idle_timeout_.value() + : stream_idle_timeout_, + _)); + + decoder_->decodeHeaders(std::move(headers), false); + + data.drain(4); + return Http::okStatus(); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(0U, stats_.named_.downstream_rq_idle_timeout_.value()); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); +} + // Per-route zero timeout overrides the global stream idle timeout. TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteZeroOverride) { stream_idle_timeout_ = std::chrono::milliseconds(10); diff --git a/test/common/http/conn_manager_impl_test_base.cc b/test/common/http/conn_manager_impl_test_base.cc index 36fb2509d7f5e..e40871cbe3c1f 100644 --- a/test/common/http/conn_manager_impl_test_base.cc +++ b/test/common/http/conn_manager_impl_test_base.cc @@ -61,6 +61,9 @@ class ConnectionManagerConfigProxyObject : public ConnectionManagerConfig { std::chrono::milliseconds streamIdleTimeout() const override { return parent_.streamIdleTimeout(); } + absl::optional streamFlushTimeout() const override { + return parent_.streamFlushTimeout(); + } std::chrono::milliseconds requestTimeout() const override { return parent_.requestTimeout(); } std::chrono::milliseconds requestHeadersTimeout() const override { return parent_.requestHeadersTimeout(); diff --git a/test/common/http/conn_manager_impl_test_base.h b/test/common/http/conn_manager_impl_test_base.h index 3e830da57b1e9..dd5d0ebad5b57 100644 --- a/test/common/http/conn_manager_impl_test_base.h +++ b/test/common/http/conn_manager_impl_test_base.h @@ -122,6 +122,9 @@ class HttpConnectionManagerImplMixin : public ConnectionManagerConfig { return http1_safe_max_connection_duration_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } + absl::optional streamFlushTimeout() const override { + return stream_flush_timeout_; + } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds requestHeadersTimeout() const override { return request_headers_timeout_; @@ -285,6 +288,7 @@ class HttpConnectionManagerImplMixin : public ConnectionManagerConfig { absl::optional max_connection_duration_; bool http1_safe_max_connection_duration_{false}; std::chrono::milliseconds stream_idle_timeout_{}; + absl::optional stream_flush_timeout_{}; std::chrono::milliseconds request_timeout_{}; std::chrono::milliseconds request_headers_timeout_{}; std::chrono::milliseconds delayed_close_timeout_{}; diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index f7fe6b9eb9aa1..56120bd3d5bcc 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -906,6 +906,119 @@ TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) { EXPECT_EQ(0, config.streamIdleTimeout().count()); } +TEST_F(HttpConnectionManagerConfigTest, StreamIdleTimeoutDefault) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + // 5 minutes -> ms. + EXPECT_EQ(5 * 60 * 1000, config.streamIdleTimeout().count()); +} + +// Tracks stream_idle_timeout. If neither stream_idle_timeout nor stream_flush_timeout are set, +// stream_flush_timeout should default to stream_idle_timeout's default. +TEST_F(HttpConnectionManagerConfigTest, StreamFlushTimeoutDefault) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + ASSERT_TRUE(config.streamFlushTimeout().has_value()); + // 5 minutes. + EXPECT_EQ(5 * 60 * 1000, config.streamFlushTimeout().value().count()); +} + +// If stream_idle_timeout is set and stream_flush_timeout is not set, stream_flush_timeout should +// default to stream_idle_timeout. +TEST_F(HttpConnectionManagerConfigTest, StreamFlushTimeoutDefaultStreamIdleTimeoutSet) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + stream_idle_timeout: 10s + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + ASSERT_TRUE(config.streamFlushTimeout().has_value()); + // 10 seconds. + EXPECT_EQ(10 * 1000, config.streamFlushTimeout().value().count()); +} + +// Validate that an explicit zero stream flush timeout disables it. +TEST_F(HttpConnectionManagerConfigTest, DisabledStreamFlushTimeout) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + stream_flush_timeout: 0s + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + ASSERT_TRUE(config.streamFlushTimeout().has_value()); + EXPECT_EQ(0, config.streamFlushTimeout().value().count()); +} + +// Validate that the flush timeout and idle timeout can be set independently. +TEST_F(HttpConnectionManagerConfigTest, StreamFlushTimeoutAndStreamIdleTimeoutSet) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + stream_idle_timeout: 10s + stream_flush_timeout: 20s + route_config: + name: local_route + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + EXPECT_EQ(10 * 1000, config.streamIdleTimeout().count()); + ASSERT_TRUE(config.streamFlushTimeout().has_value()); + EXPECT_EQ(20 * 1000, config.streamFlushTimeout().value().count()); +} + // Validate that idle_timeout set in common_http_protocol_options is used. TEST_F(HttpConnectionManagerConfigTest, CommonHttpProtocolIdleTimeout) { const std::string yaml_string = R"EOF( diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 558974d6380cc..8173e20caf91a 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -254,6 +254,51 @@ TEST_P(MultiplexedIntegrationTest, CodecStreamIdleTimeout) { ASSERT_TRUE(response->waitForReset()); } +// Test that the codec stream flush timeout can be overridden independently from +// the connection manager stream idle timeout. +TEST_P(MultiplexedIntegrationTest, CodecStreamIdleTimeoutOverride) { + config_helper_.setBufferLimits(1024, 1024); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + // Disable the generic stream idle timeout. This will be overridden by the + // stream_flush_timeout and the test should work exactly the same as the + // CodecStreamIdleTimeout test. + hcm.mutable_stream_idle_timeout()->set_seconds(0); + hcm.mutable_stream_idle_timeout()->set_nanos(0); + + hcm.mutable_stream_flush_timeout()->set_seconds(0); + constexpr uint64_t FlushTimeoutMs = 400; + hcm.mutable_stream_flush_timeout()->set_nanos(FlushTimeoutMs * 1000 * 1000); + }); + initialize(); + const size_t stream_flow_control_window = + downstream_protocol_ == Http::CodecType::HTTP3 ? 32 * 1024 : 65535; + envoy::config::core::v3::Http2ProtocolOptions http2_options = + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions()) + .value(); + http2_options.mutable_initial_stream_window_size()->set_value(stream_flow_control_window); +#ifdef ENVOY_ENABLE_QUIC + if (downstream_protocol_ == Http::CodecType::HTTP3) { + dynamic_cast(*quic_connection_persistent_info_) + .quic_config_.SetInitialStreamFlowControlWindowToSend(stream_flow_control_window); + dynamic_cast(*quic_connection_persistent_info_) + .quic_config_.SetInitialSessionFlowControlWindowToSend(stream_flow_control_window); + } +#endif + codec_client_ = makeRawHttpConnection(makeClientConnection(lookupPort("http")), http2_options); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, false); + upstream_request_->encodeData(stream_flow_control_window + 2000, true); + std::string flush_timeout_counter(downstreamProtocol() == Http::CodecType::HTTP3 + ? "http3.tx_flush_timeout" + : "http2.tx_flush_timeout"); + test_server_->waitForCounterEq(flush_timeout_counter, 1); + ASSERT_TRUE(response->waitForReset()); +} + TEST_P(MultiplexedIntegrationTest, Http2DownstreamKeepalive) { EXCLUDE_DOWNSTREAM_HTTP3; // Http3 keepalive doesn't timeout and close connection. constexpr uint64_t interval_ms = 1; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 88ef777490e26..51ab250d9e6a6 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -652,6 +652,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD(bool, http1SafeMaxConnectionDuration, (), (const)); MOCK_METHOD(absl::optional, maxStreamDuration, (), (const)); MOCK_METHOD(std::chrono::milliseconds, streamIdleTimeout, (), (const)); + MOCK_METHOD(absl::optional, streamFlushTimeout, (), (const)); MOCK_METHOD(std::chrono::milliseconds, requestTimeout, (), (const)); MOCK_METHOD(std::chrono::milliseconds, requestHeadersTimeout, (), (const)); MOCK_METHOD(std::chrono::milliseconds, delayedCloseTimeout, (), (const)); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index f715a0cb79ebe..cf3be8346367d 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -436,6 +436,7 @@ class MockRouteEntry : public RouteEntry { MOCK_METHOD(const std::vector&, shadowPolicies, (), (const)); MOCK_METHOD(std::chrono::milliseconds, timeout, (), (const)); MOCK_METHOD(absl::optional, idleTimeout, (), (const)); + MOCK_METHOD(absl::optional, flushTimeout, (), (const)); MOCK_METHOD(bool, usingNewTimeouts, (), (const)); MOCK_METHOD(absl::optional, maxStreamDuration, (), (const)); MOCK_METHOD(absl::optional, grpcTimeoutHeaderMax, (), (const)); @@ -556,6 +557,7 @@ class MockRoute : public RouteEntryAndRoute { MOCK_METHOD(const std::vector&, shadowPolicies, (), (const)); MOCK_METHOD(std::chrono::milliseconds, timeout, (), (const)); MOCK_METHOD(absl::optional, idleTimeout, (), (const)); + MOCK_METHOD(absl::optional, flushTimeout, (), (const)); MOCK_METHOD(bool, usingNewTimeouts, (), (const)); MOCK_METHOD(absl::optional, maxStreamDuration, (), (const)); MOCK_METHOD(absl::optional, grpcTimeoutHeaderMax, (), (const));