diff --git a/api/envoy/api/v2/route/route.proto b/api/envoy/api/v2/route/route.proto index 1ac5b46e4ba2a..0a3bb20ee7491 100644 --- a/api/envoy/api/v2/route/route.proto +++ b/api/envoy/api/v2/route/route.proto @@ -439,12 +439,11 @@ message RouteAction { google.protobuf.Duration per_try_timeout = 3 [(gogoproto.stdduration) = true]; } - // Specifies the idle timeout for the route. If not specified, this defaults - // to 5 minutes. The default value was select so as not to interfere with any - // smaller configured timeouts that may have existed in configurations prior - // to the introduction of this feature, while introducing robustness to TCP - // connections that terminate without FIN. A value of 0 will completely - // disable the idle timeout. + // Specifies the idle timeout for the route. If not specified, there is no per-route idle timeout + // specified, although the connection manager wide :ref:`stream_idle_timeout + // ` + // will still apply. A value of 0 will completely disable the route's idle timeout, even if a + // connection manager stream idle timeout is configured. // // The idle timeout is distinct to :ref:`timeout // `, which provides an upper bound diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index d4c8cd3077d13..1e6999cf2077a 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -19,7 +19,7 @@ import "gogoproto/gogo.proto"; // [#protodoc-title: HTTP connection manager] // HTTP connection manager :ref:`configuration overview `. -// [#comment:next free field: 24] +// [#comment:next free field: 25] message HttpConnectionManager { enum CodecType { option (gogoproto.goproto_enum_prefix) = false; @@ -137,6 +137,33 @@ message HttpConnectionManager { // `. google.protobuf.Duration idle_timeout = 11 [(gogoproto.stdduration) = true]; + // The stream idle timeout for connections managed by the connection manager. + // If not specified, this defaults to 5 minutes. The default value was selected + // so as not to interfere with any smaller configured timeouts that may have + // existed in configurations prior to the introduction of this feature, while + // introducing robustness to TCP connections that terminate without a FIN. + // + // This idle timeout applies to new streams and is overridable by the + // :ref:`route-level idle_timeout + // `. Even on a stream in + // which the override applies, prior to receipt of the initial request + // headers, the :ref:`stream_idle_timeout + // ` + // applies. Each time an encode/decode event for headers or data is processed + // for the stream, the timer will be reset. If the timeout fires, the stream + // is terminated with a 408 Request Timeout error code if no upstream response + // header has been received, otherwise a stream reset occurs. + // + // Note that it is possible to idle timeout even if the wire traffic for a stream is non-idle, due + // to the granularity of events presented to the connection manager. For example, while receiving + // very large request headers, it may be the case that there is traffic regularly arriving on the + // wire while the connection manage is only able to observe the end-of-headers event, hence the + // stream may still idle timeout. + // + // A value of 0 will completely disable the connection manager stream idle + // timeout, although per-route idle timeout overrides will continue to apply. + google.protobuf.Duration stream_idle_timeout = 24 [(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 diff --git a/docs/root/intro/arch_overview/http_connection_management.rst b/docs/root/intro/arch_overview/http_connection_management.rst index 4f1d415b48e3d..40415481322fc 100644 --- a/docs/root/intro/arch_overview/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http_connection_management.rst @@ -42,3 +42,27 @@ table `. The route table can be specified in one of * Statically. * Dynamically via the :ref:`RDS API `. + +Timeouts +-------- + +Various configurable timeouts apply to an HTTP connection and its constituent streams: + +* Connection-level :ref:`idle timeout + `: + this applies to the idle period where no streams are active. +* Connection-level :ref:`drain timeout + `: + this spans between an Envoy originated GOAWAY and connection termination. +* Stream-level idle timeout: this applies to each individual stream. It may be configured at both + the :ref:`connection manager + ` + and :ref:`per-route ` granularity. + Header/data/trailer events on the stream reset the idle timeout. +* Stream-level :ref:`per-route upstream timeout `: this + applies to the upstream response, i.e. a maximum bound on the time from the end of the downstream + request until the end of the upstream response. This may also be specified at the :ref:`per-retry + ` granularity. +* Stream-level :ref:`per-route gRPC max timeout + `: this bounds the upstream timeout and allows + the timeout to be overriden via the *grpc-timeout* request header. diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index eb2bfe7f6752c..f1c728ea040a7 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -14,6 +14,12 @@ Version history * health check: added support for :ref:`custom health check `. * health check: added support for :ref:`specifying jitter as a percentage `. * health_check: added support for :ref:`health check event logging `. +* http: added support for a per-stream idle timeout. This applies at both :ref:`connection manager + ` + and :ref:`per-route granularity `. The timeout + defaults to 5 minutes; if you have other timeouts (e.g. connection idle timeout, upstream + response per-retry) that are longer than this in duration, you may want to consider setting a + non-default per-stream idle timeout. * http: added support for a :ref:`per-stream idle timeout `. This defaults to 5 minutes; if you have other timeouts (e.g. connection idle timeout, upstream response per-retry) that are longer than diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 9aa244f29d051..a69c402251910 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -473,7 +473,8 @@ class RouteEntry : public ResponseEntry { virtual std::chrono::milliseconds timeout() const PURE; /** - * @return absl::optional the route's idle timeout. + * @return optional the route's idle timeout. Zero indicates a + * disabled idle timeout, while nullopt indicates deference to the global timeout. */ virtual absl::optional idleTimeout() const PURE; diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 4d6ca959adbab..7c172903f7a44 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -192,7 +192,13 @@ class ConnectionManagerConfig { /** * @return optional idle timeout for incoming connection manager connections. */ - virtual const absl::optional& idleTimeout() PURE; + virtual absl::optional idleTimeout() const PURE; + + /** + * @return per-stream idle timeout for incoming connection manager connections. Zero indicates a + * disabled idle timeout. + */ + virtual std::chrono::milliseconds streamIdleTimeout() const PURE; /** * @return Router::RouteConfigProvider& the configuration provider used to acquire a route diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index c05ffcbdf7266..aeadafa948ff2 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -369,6 +369,13 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect // prevents surprises for logging code in edge cases. request_info_.setDownstreamRemoteAddress( connection_manager_.read_callbacks_->connection().remoteAddress()); + + if (connection_manager_.config_.streamIdleTimeout().count()) { + idle_timeout_ms_ = connection_manager_.config_.streamIdleTimeout(); + idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( + [this]() -> void { onIdleTimeout(); }); + resetIdleTimer(); + } } ConnectionManagerImpl::ActiveStream::~ActiveStream() { @@ -605,9 +612,18 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, const Router::RouteEntry* route_entry = cached_route_.value()->routeEntry(); if (route_entry != nullptr && route_entry->idleTimeout()) { idle_timeout_ms_ = route_entry->idleTimeout().value(); - idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( - [this]() -> void { onIdleTimeout(); }); - resetIdleTimer(); + if (idle_timeout_ms_.count()) { + // If we have a route-level idle timeout but no global stream idle timeout, create a timer. + if (idle_timer_ == nullptr) { + idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( + [this]() -> void { onIdleTimeout(); }); + } + } else if (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. + idle_timer_->disableTimer(); + idle_timer_ = nullptr; + } } } @@ -617,6 +633,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } decodeHeaders(nullptr, *request_headers_, end_stream); + + // Reset it here for both global and overriden cases. + resetIdleTimer(); } void ConnectionManagerImpl::ActiveStream::traceRequest() { diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index cb0b726f91825..5288a8143640b 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -258,8 +258,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, cluster_not_found_response_code_(ConfigUtility::parseClusterNotFoundResponseCode( route.route().cluster_not_found_response_code())), timeout_(PROTOBUF_GET_MS_OR_DEFAULT(route.route(), timeout, DEFAULT_ROUTE_TIMEOUT_MS)), - idle_timeout_( - PROTOBUF_GET_MS_OR_DEFAULT(route.route(), idle_timeout, DEFAULT_ROUTE_IDLE_TIMEOUT_MS)), + idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(route.route(), idle_timeout)), max_grpc_timeout_(PROTOBUF_GET_OPTIONAL_MS(route.route(), max_grpc_timeout)), runtime_(loadRuntimeData(route.match())), loader_(factory_context.runtime()), host_redirect_(route.redirect().host_redirect()), diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index ac274dd28e7be..e5c79a6c62229 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -311,10 +311,7 @@ class RouteEntryImplBase : public RouteEntry, return vhost_.virtualClusterFromEntries(headers); } std::chrono::milliseconds timeout() const override { return timeout_; } - absl::optional idleTimeout() const override { - return idle_timeout_.count() == 0 ? absl::nullopt - : absl::optional(idle_timeout_); - } + absl::optional idleTimeout() const override { return idle_timeout_; } absl::optional maxGrpcTimeout() const override { return max_grpc_timeout_; } @@ -510,9 +507,6 @@ class RouteEntryImplBase : public RouteEntry, // Default timeout is 15s if nothing is specified in the route config. static const uint64_t DEFAULT_ROUTE_TIMEOUT_MS = 15000; - // Default idle timeout is 5 minutes if nothing is specified in the route config. - static const uint64_t DEFAULT_ROUTE_IDLE_TIMEOUT_MS = 5 * 60 * 1000; - std::unique_ptr cors_policy_; const VirtualHostImpl& vhost_; // See note in RouteEntryImplBase::clusterEntry() on why raw ref // to virtual host is currently safe. @@ -522,7 +516,7 @@ class RouteEntryImplBase : public RouteEntry, const Http::LowerCaseString cluster_header_name_; const Http::Code cluster_not_found_response_code_; const std::chrono::milliseconds timeout_; - const std::chrono::milliseconds idle_timeout_; + const absl::optional idle_timeout_; const absl::optional max_grpc_timeout_; const absl::optional runtime_; Runtime::Loader& loader_; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index d80645d018492..0503e5296b9a6 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -129,6 +129,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( route_config_provider_manager_(route_config_provider_manager), http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())), + idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(config, idle_timeout)), + stream_idle_timeout_( + PROTOBUF_GET_MS_OR_DEFAULT(config, stream_idle_timeout, StreamIdleTimeoutMs)), 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), @@ -216,10 +219,6 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( overall_sampling})); } - if (config.has_idle_timeout()) { - idle_timeout_ = std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, idle_timeout)); - } - for (const auto& access_log : config.access_log()) { AccessLog::InstanceSharedPtr current_access_log = AccessLog::AccessLogFactory::fromProto(access_log, context_); diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index f8f5c6fd4cb08..f4d173059c376 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -87,7 +87,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, std::chrono::milliseconds drainTimeout() override { return drain_timeout_; } FilterChainFactory& filterFactory() override { return *this; } bool generateRequestId() override { return generate_request_id_; } - const absl::optional& idleTimeout() override { return idle_timeout_; } + absl::optional idleTimeout() const override { return idle_timeout_; } + std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } Router::RouteConfigProvider& routeConfigProvider() override { return *route_config_provider_; } const std::string& serverName() override { return server_name_; } Http::ConnectionManagerStats& stats() override { return stats_; } @@ -137,12 +138,16 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::TracingConnectionManagerConfigPtr tracing_config_; absl::optional user_agent_; absl::optional idle_timeout_; + std::chrono::milliseconds stream_idle_timeout_; Router::RouteConfigProviderSharedPtr route_config_provider_; std::chrono::milliseconds drain_timeout_; bool generate_request_id_; Http::DateProvider& date_provider_; Http::ConnectionManagerListenerStats listener_stats_; const bool proxy_100_continue_; + + // Default idle timeout is 5 minutes if nothing is specified in the HCM config. + static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000; }; } // namespace HttpConnectionManager diff --git a/source/server/http/admin.h b/source/server/http/admin.h index e5e798851d49c..b16a58e2b2858 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -89,7 +89,8 @@ class AdminImpl : public Admin, std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } Http::FilterChainFactory& filterFactory() override { return *this; } bool generateRequestId() override { return false; } - const absl::optional& idleTimeout() override { return idle_timeout_; } + absl::optional idleTimeout() const override { return idle_timeout_; } + std::chrono::milliseconds streamIdleTimeout() const override { return {}; } Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } const std::string& serverName() override { return Http::DefaultServerString::get(); } Http::ConnectionManagerStats& stats() override { return stats_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 7b4b1a2007bd2..10ed29faa0ef5 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -254,7 +254,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } FilterChainFactory& filterFactory() override { return filter_factory_; } bool generateRequestId() override { return true; } - const absl::optional& idleTimeout() override { return idle_timeout_; } + absl::optional idleTimeout() const override { return idle_timeout_; } + std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } const std::string& serverName() override { return server_name_; } ConnectionManagerStats& stats() override { return stats_; } @@ -294,6 +295,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi std::vector set_current_client_cert_details_; absl::optional user_agent_; absl::optional idle_timeout_; + std::chrono::milliseconds stream_idle_timeout_{}; NiceMock random_; NiceMock local_info_; NiceMock factory_context_; @@ -1093,7 +1095,9 @@ TEST_F(HttpConnectionManagerImplTest, NoPath) { conn_manager_->onData(fake_input, false); } -// No idle timeout when route idle timeout is not configured. +// No idle timeout when route idle timeout is implied at both global and +// per-route level. The connection manager config is responsible for managing +// the default configuration aspects. TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNotConfigured) { setup(false, ""); @@ -1115,6 +1119,89 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNotConfigured) { EXPECT_EQ(0U, stats_.named_.downstream_rq_idle_timeout_.value()); } +// When the global timeout is configured, the timer is enabled before we receive +// headers. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutGlobal) { + stream_idle_timeout_ = std::chrono::milliseconds(10); + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)) + .Times(1) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + Event::MockTimer* idle_timer = + new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(10))); + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(10))); + decoder->decodeHeaders(std::move(headers), false); + + data.drain(4); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(0U, stats_.named_.downstream_rq_idle_timeout_.value()); +} + +// Per-route timeouts override the global stream idle timeout. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteOverride) { + stream_idle_timeout_ = std::chrono::milliseconds(10); + setup(false, ""); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(std::chrono::milliseconds(30))); + + EXPECT_CALL(*codec_, dispatch(_)) + .Times(1) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + Event::MockTimer* idle_timer = + new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(10))); + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(30))); + decoder->decodeHeaders(std::move(headers), false); + + data.drain(4); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(0U, stats_.named_.downstream_rq_idle_timeout_.value()); +} + +// Per-route zero timeout overrides the global stream idle timeout. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteZeroOverride) { + stream_idle_timeout_ = std::chrono::milliseconds(10); + setup(false, ""); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(std::chrono::milliseconds(0))); + + EXPECT_CALL(*codec_, dispatch(_)) + .Times(1) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + Event::MockTimer* idle_timer = + new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(10))); + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, disableTimer()); + decoder->decodeHeaders(std::move(headers), false); + + data.drain(4); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(0U, stats_.named_.downstream_rq_idle_timeout_.value()); +} + // Validate the per-stream idle timeout after having sent downstream headers. TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders) { setup(false, ""); diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 83d7116ddfebd..91780c603c1d2 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -44,7 +44,8 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(drainTimeout, std::chrono::milliseconds()); MOCK_METHOD0(filterFactory, FilterChainFactory&()); MOCK_METHOD0(generateRequestId, bool()); - MOCK_METHOD0(idleTimeout, const absl::optional&()); + MOCK_CONST_METHOD0(idleTimeout, absl::optional()); + MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds()); MOCK_METHOD0(routeConfigProvider, Router::RouteConfigProvider&()); MOCK_METHOD0(serverName, const std::string&()); MOCK_METHOD0(stats, ConnectionManagerStats&()); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index e8d82f40e0aa2..63c942be9dc6a 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4383,7 +4383,6 @@ name: NoIdleTimeout - match: { regex: "/regex"} route: cluster: some-cluster - idle_timeout: 0s )EOF"; NiceMock factory_context; @@ -4393,9 +4392,9 @@ name: NoIdleTimeout EXPECT_EQ(absl::nullopt, route_entry->idleTimeout()); } -TEST(RouteConfigurationV2, DefaultIdleTimeout) { - const std::string DefaultIdleTimeot = R"EOF( -name: NoIdleTimeout +TEST(RouteConfigurationV2, ZeroIdleTimeout) { + const std::string ZeroIdleTimeot = R"EOF( +name: ZeroIdleTimeout virtual_hosts: - name: regex domains: [idle.lyft.com] @@ -4403,18 +4402,19 @@ name: NoIdleTimeout - match: { regex: "/regex"} route: cluster: some-cluster + idle_timeout: 0s )EOF"; NiceMock factory_context; - ConfigImpl config(parseRouteConfigurationFromV2Yaml(DefaultIdleTimeot), factory_context, true); + ConfigImpl config(parseRouteConfigurationFromV2Yaml(ZeroIdleTimeot), factory_context, true); Http::TestHeaderMapImpl headers = genRedirectHeaders("idle.lyft.com", "/regex", true, false); const RouteEntry* route_entry = config.route(headers, 0)->routeEntry(); - EXPECT_EQ(5 * 60 * 1000, route_entry->idleTimeout().value().count()); + EXPECT_EQ(0, route_entry->idleTimeout().value().count()); } TEST(RouteConfigurationV2, ExplicitIdleTimeout) { const std::string ExplicitIdleTimeot = R"EOF( -name: NoIdleTimeout +name: ExplicitIdleTimeout virtual_hosts: - name: regex domains: [idle.lyft.com] 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 819f4221731e5..1fc646bdba6f0 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -35,6 +35,14 @@ parseHttpConnectionManagerFromJson(const std::string& json_string) { return http_connection_manager; } +envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager +parseHttpConnectionManagerFromV2Yaml(const std::string& yaml) { + envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager + http_connection_manager; + MessageUtil::loadFromYaml(yaml, http_connection_manager); + return http_connection_manager; +} + class HttpConnectionManagerConfigTest : public testing::Test { public: NiceMock context_; @@ -120,6 +128,23 @@ TEST_F(HttpConnectionManagerConfigTest, MiscConfig) { ContainerEq(config.tracingConfig()->request_headers_for_tags_)); EXPECT_EQ(*context_.local_info_.address_, config.localAddress()); EXPECT_EQ("foo", config.serverName()); + EXPECT_EQ(5 * 60 * 1000, config.streamIdleTimeout().count()); +} + +// Validated that an explicit zero stream idle timeout disables. +TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + stream_idle_timeout: 0s + route_config: + name: local_route + http_filters: + - name: envoy.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_); + EXPECT_EQ(0, config.streamIdleTimeout().count()); } TEST_F(HttpConnectionManagerConfigTest, SingleDateProvider) { diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 97b71aefbb139..6aa5fa3b82cdf 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -9,11 +9,17 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { config_helper_.addConfigModifier( [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) -> void { - auto* route_config = hcm.mutable_route_config(); - auto* virtual_host = route_config->mutable_virtual_hosts(0); - auto* route = virtual_host->mutable_routes(0)->mutable_route(); - route->mutable_idle_timeout()->set_seconds(0); - route->mutable_idle_timeout()->set_nanos(TimeoutMs * 1000 * 1000); + if (enable_global_idle_timeout_) { + hcm.mutable_stream_idle_timeout()->set_seconds(0); + hcm.mutable_stream_idle_timeout()->set_nanos(TimeoutMs * 1000 * 1000); + } + if (enable_per_stream_idle_timeout_) { + auto* route_config = hcm.mutable_route_config(); + auto* virtual_host = route_config->mutable_virtual_hosts(0); + auto* route = virtual_host->mutable_routes(0)->mutable_route(); + route->mutable_idle_timeout()->set_seconds(0); + route->mutable_idle_timeout()->set_nanos(TimeoutMs * 1000 * 1000); + } // For validating encode100ContinueHeaders() timer kick. hcm.set_proxy_100_continue(true); }); @@ -22,6 +28,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest() { initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); auto encoder_decoder = codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "GET"}, @@ -30,7 +37,6 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { {":authority", "host"}}); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); - fake_upstreams_[0]->set_allow_unexpected_disconnects(true); fake_upstream_connection_ = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_); upstream_request_ = fake_upstream_connection_->waitForNewStream(*dispatcher_); upstream_request_->waitForHeadersComplete(); @@ -52,6 +58,8 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { // TODO(htuch): This might require scaling for TSAN/ASAN/Valgrind/etc. Bump if // this is the cause of flakes. static constexpr uint64_t TimeoutMs = 200; + bool enable_global_idle_timeout_{}; + bool enable_per_stream_idle_timeout_{true}; }; INSTANTIATE_TEST_CASE_P(Protocols, IdleTimeoutIntegrationTest, @@ -71,6 +79,21 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeaders) { EXPECT_EQ("stream timeout", response->body()); } +// Global per-stream idle timeout applies if there is no per-stream idle timeout. +TEST_P(IdleTimeoutIntegrationTest, GlobalPerStreamIdleTimeoutAfterDownstreamHeaders) { + enable_global_idle_timeout_ = true; + enable_per_stream_idle_timeout_ = false; + auto response = setupPerStreamIdleTimeoutTest(); + + waitForTimeout(*response); + + EXPECT_FALSE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + EXPECT_TRUE(response->complete()); + EXPECT_STREQ("408", response->headers().Status()->value().c_str()); + EXPECT_EQ("stream timeout", response->body()); +} + // Per-stream idle timeout after having sent downstream headers+body. TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeadersAndBody) { auto response = setupPerStreamIdleTimeoutTest();