diff --git a/api/envoy/api/v2/route/route.proto b/api/envoy/api/v2/route/route.proto index 52906b51fed6d..fdafb101da5c3 100644 --- a/api/envoy/api/v2/route/route.proto +++ b/api/envoy/api/v2/route/route.proto @@ -313,7 +313,7 @@ message CorsPolicy { google.protobuf.BoolValue enabled = 7; } -// [#comment:next free field: 24] +// [#comment:next free field: 25] message RouteAction { oneof cluster_specifier { option (validate.required) = true; @@ -401,7 +401,9 @@ message RouteAction { google.protobuf.BoolValue auto_host_rewrite = 7; } - // Specifies the timeout for the route. If not specified, the default is 15s. + // Specifies the upstream timeout for the route. If not specified, the default is 15s. This + // spans between the point at which the entire downstream request (i.e. end-of-stream) has been + // processed and when the upstream response has been completely processed. // // .. note:: // @@ -423,8 +425,8 @@ message RouteAction { // :ref:`config_http_filters_router_x-envoy-max-retries`. google.protobuf.UInt32Value num_retries = 2; - // Specifies a non-zero timeout per retry attempt. This parameter is optional. - // The same conditions documented for + // Specifies a non-zero upstream timeout per retry attempt. This parameter is optional. The + // same conditions documented for // :ref:`config_http_filters_router_x-envoy-upstream-rq-per-try-timeout-ms` apply. // // .. note:: @@ -437,6 +439,28 @@ 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. + // + // The idle timeout is distinct to :ref:`timeout + // `, which provides an upper bound + // on the upstream response time; :ref:`idle_timeout + // ` instead bounds the amount + // of time the request's stream may be idle. + // + // After header decoding, the idle timeout will apply on downstream and + // upstream request events. 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. + google.protobuf.Duration idle_timeout = 24 + [(validate.rules).duration.gt = {}, (gogoproto.stdduration) = true]; + // Indicates that the route has a retry policy. RetryPolicy retry_policy = 9; diff --git a/docs/root/configuration/http_conn_man/stats.rst b/docs/root/configuration/http_conn_man/stats.rst index 1b9b13e1d63ec..9b32590f3362e 100644 --- a/docs/root/configuration/http_conn_man/stats.rst +++ b/docs/root/configuration/http_conn_man/stats.rst @@ -52,6 +52,7 @@ statistics: 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_idle_timeout, Counter, Total requests closed due to idle timeout rs_too_large, Counter, Total response errors due to buffering an overly large body Per user agent statistics diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index f9217f55e99a7..e6f55e59f3c5f 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -14,6 +14,10 @@ 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 :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 + this in duration, you may want to consider setting a non-default per-stream idle timeout. * http: better handling of HEAD requests. Now sending transfer-encoding: chunked rather than content-length: 0. * http: response filters not applied to early error paths such as http_parser generated 400s. * proxy_protocol: added support for HAProxy Proxy Protocol v2 (AF_INET/AF_INET6 only). diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index e9c0f2d848d09..9aa244f29d051 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -472,6 +472,11 @@ class RouteEntry : public ResponseEntry { */ virtual std::chrono::milliseconds timeout() const PURE; + /** + * @return absl::optional the route's idle timeout. + */ + virtual absl::optional idleTimeout() const PURE; + /** * @return absl::optional the maximum allowed timeout value derived * from 'grpc-timeout' header of a gRPC request. Non-present value disables use of 'grpc-timeout' diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 61afc3157078f..a01727ae6e61d 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -195,6 +195,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, return std::chrono::milliseconds(0); } } + absl::optional idleTimeout() const override { return absl::nullopt; } absl::optional maxGrpcTimeout() const override { return absl::nullopt; } diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 0b59bb0bfc061..4d6ca959adbab 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -54,6 +54,7 @@ namespace Http { COUNTER (downstream_rq_4xx) \ COUNTER (downstream_rq_5xx) \ HISTOGRAM(downstream_rq_time) \ + COUNTER (downstream_rq_idle_timeout) \ COUNTER (rs_too_large) // clang-format on diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 6d1e9f909bdc1..f21aa0fab0be4 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -396,6 +396,28 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { ASSERT(state_.filter_call_state_ == 0); } +void ConnectionManagerImpl::ActiveStream::resetIdleTimer() { + if (idle_timer_ != nullptr) { + // TODO(htuch): If this shows up in performance profiles, optimize by only + // updating a timestamp here and doing periodic checks for idle timeouts + // instead, or reducing the accuracy of timers. + idle_timer_->enableTimer(idle_timeout_ms_); + } +} + +void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { + connection_manager_.stats_.named_.downstream_rq_idle_timeout_.inc(); + // If headers have not been sent to the user, send a 408. + if (response_headers_ != nullptr) { + // TODO(htuch): We could send trailers here with an x-envoy timeout header + // or gRPC status code, and/or set H2 RST_STREAM error. + connection_manager_.doEndStream(*this); + } else { + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::RequestTimeout, + "stream timeout", nullptr); + } +} + void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker( StreamDecoderFilterSharedPtr filter, bool dual_filter) { ActiveStreamDecoderFilterPtr wrapper(new ActiveStreamDecoderFilter(*this, filter, dual_filter)); @@ -579,6 +601,16 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Allow non websocket requests to go through websocket enabled routes. } + if (cached_route_.value()) { + 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(); + } + } + // Check if tracing is enabled at all. if (connection_manager_.config_.tracingConfig()) { traceRequest(); @@ -702,6 +734,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, boo void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream) { + resetIdleTimer(); + // If a response is complete or a reset has been sent, filters do not care about further body // data. Just drop it. if (state_.local_complete_) { @@ -750,6 +784,7 @@ void ConnectionManagerImpl::ActiveStream::addDecodedData(ActiveStreamDecoderFilt } void ConnectionManagerImpl::ActiveStream::decodeTrailers(HeaderMapPtr&& trailers) { + resetIdleTimer(); maybeEndDecode(true); request_trailers_ = std::move(trailers); decodeTrailers(nullptr, *request_trailers_); @@ -846,6 +881,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( ActiveStreamEncoderFilter* filter, HeaderMap& headers) { + resetIdleTimer(); ASSERT(connection_manager_.config_.proxy100Continue()); // Make sure commonContinue continues encode100ContinueHeaders. has_continue_headers_ = true; @@ -882,6 +918,8 @@ void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream) { + resetIdleTimer(); + std::list::iterator entry = commonEncodePrefix(filter, end_stream); std::list::iterator continue_data_entry = encoder_filters_.end(); @@ -1019,6 +1057,7 @@ void ConnectionManagerImpl::ActiveStream::addEncodedData(ActiveStreamEncoderFilt void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instance& data, bool end_stream) { + resetIdleTimer(); std::list::iterator entry = commonEncodePrefix(filter, end_stream); for (; entry != encoder_filters_.end(); entry++) { ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeData)); @@ -1042,6 +1081,7 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter* void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilter* filter, HeaderMap& trailers) { + resetIdleTimer(); std::list::iterator entry = commonEncodePrefix(filter, true); for (; entry != encoder_filters_.end(); entry++) { ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeTrailers)); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 49702004c725f..2ccf7b32ce51a 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -362,6 +362,10 @@ class ConnectionManagerImpl : Logger::Loggable, void setBufferLimit(uint32_t limit); // Set up the Encoder/Decoder filter chain. bool createFilterChain(); + // Per-stream idle timeout callback. + void onIdleTimeout(); + // Reset per-stream idle timer. + void resetIdleTimer(); ConnectionManagerImpl& connection_manager_; Router::ConfigConstSharedPtr snapped_route_config_; @@ -379,6 +383,9 @@ class ConnectionManagerImpl : Logger::Loggable, std::list encoder_filters_; std::list access_log_handlers_; Stats::TimespanPtr request_timer_; + // Per-stream idle timeout. + Event::TimerPtr idle_timer_; + std::chrono::milliseconds idle_timeout_ms_{}; State state_; RequestInfo::RequestInfoImpl request_info_; absl::optional cached_route_; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ca884fa92e785..94877655ebed0 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -258,6 +258,8 @@ 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)), 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 cc3fd0509bffe..ac274dd28e7be 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -311,6 +311,10 @@ 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 maxGrpcTimeout() const override { return max_grpc_timeout_; } @@ -395,6 +399,9 @@ class RouteEntryImplBase : public RouteEntry, const RetryPolicy& retryPolicy() const override { return parent_->retryPolicy(); } const ShadowPolicy& shadowPolicy() const override { return parent_->shadowPolicy(); } std::chrono::milliseconds timeout() const override { return parent_->timeout(); } + absl::optional idleTimeout() const override { + return parent_->idleTimeout(); + } absl::optional maxGrpcTimeout() const override { return parent_->maxGrpcTimeout(); } @@ -503,6 +510,9 @@ 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. @@ -512,6 +522,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 max_grpc_timeout_; const absl::optional runtime_; Runtime::Loader& loader_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 714994dff8855..7b4b1a2007bd2 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1093,6 +1093,223 @@ TEST_F(HttpConnectionManagerImplTest, NoPath) { conn_manager_->onData(fake_input, false); } +// No idle timeout when route idle timeout is not configured. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNotConfigured) { + setup(false, ""); + + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, createTimer_(_)).Times(0); + EXPECT_CALL(*codec_, dispatch(_)) + .Times(1) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + 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, ""); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(std::chrono::milliseconds(10))); + + // Codec sends downstream request headers. + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeHeaders(std::move(headers), false); + + // Expect resetIdleTimer() to be called for the response + // encodeHeaders()/encodeData(). + EXPECT_CALL(*idle_timer, enableTimer(_)).Times(2); + idle_timer->callback_(); + + data.drain(4); + })); + + // 408 direct response after timeout. + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("408", headers.Status()->value().c_str()); + })); + std::string response_body; + EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ("stream timeout", response_body); + EXPECT_EQ(1U, stats_.named_.downstream_rq_idle_timeout_.value()); +} + +// Validate the per-stream idle timeout after having sent downstream +// headers+body. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeadersAndBody) { + setup(false, ""); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(std::chrono::milliseconds(10))); + + // Codec sends downstream request headers. + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeHeaders(std::move(headers), false); + + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeData(data, false); + + // Expect resetIdleTimer() to be called for the response + // encodeHeaders()/encodeData(). + EXPECT_CALL(*idle_timer, enableTimer(_)).Times(2); + idle_timer->callback_(); + + data.drain(4); + })); + + // 408 direct response after timeout. + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("408", headers.Status()->value().c_str()); + })); + std::string response_body; + EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ("stream timeout", response_body); + EXPECT_EQ(1U, stats_.named_.downstream_rq_idle_timeout_.value()); +} + +// Validate the per-stream idle timeout after upstream headers have been sent. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterUpstreamHeaders) { + setup(false, ""); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(std::chrono::milliseconds(10))); + + // Store the basic request encoder during filter chain setup. + std::shared_ptr filter(new NiceMock()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(filter); + })); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + // Codec sends downstream request headers, upstream response headers are + // encoded. + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeHeaders(std::move(headers), false); + + HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + filter->callbacks_->encodeHeaders(std::move(response_headers), false); + + idle_timer->callback_(); + + data.drain(4); + })); + + // 200 upstream response. + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("200", headers.Status()->value().c_str()); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(1U, stats_.named_.downstream_rq_idle_timeout_.value()); +} + +// Validate the per-stream idle timeout after a sequence of header/data events. +TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterBidiData) { + setup(false, ""); + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout()) + .WillByDefault(Return(std::chrono::milliseconds(10))); + proxy_100_continue_ = true; + + // Store the basic request encoder during filter chain setup. + std::shared_ptr filter(new NiceMock()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(filter); + })); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + // Codec sends downstream request headers, upstream response headers are + // encoded, data events happen in various directions. + Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + StreamDecoder* decoder; + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(response_encoder_); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeHeaders(std::move(headers), false); + + HeaderMapPtr response_continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + filter->callbacks_->encode100ContinueHeaders(std::move(response_continue_headers)); + + HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + filter->callbacks_->encodeHeaders(std::move(response_headers), false); + + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeData(data, false); + + HeaderMapPtr trailers{new TestHeaderMapImpl{{"foo", "bar"}}}; + EXPECT_CALL(*idle_timer, enableTimer(_)); + decoder->decodeTrailers(std::move(trailers)); + + Buffer::OwnedImpl fake_response("world"); + EXPECT_CALL(*idle_timer, enableTimer(_)); + filter->callbacks_->encodeData(fake_response, false); + + idle_timer->callback_(); + + data.drain(4); + })); + + // 100 continue. + EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)); + + // 200 upstream response. + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("200", headers.Status()->value().c_str()); + })); + + std::string response_body; + EXPECT_CALL(response_encoder_, encodeData(_, false)).WillOnce(AddBufferToString(&response_body)); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(1U, stats_.named_.downstream_rq_idle_timeout_.value()); + EXPECT_EQ("world", response_body); +} + TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) { setup(false, ""); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index e6bd33829d6b5..be75ff20356a9 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4373,6 +4373,65 @@ name: RegexNoMatch } } +TEST(RouteConfigurationV2, NoIdleTimeout) { + const std::string NoIdleTimeot = R"EOF( +name: NoIdleTimeout +virtual_hosts: + - name: regex + domains: [idle.lyft.com] + routes: + - match: { regex: "/regex"} + route: + cluster: some-cluster + idle_timeout: 0s + )EOF"; + + NiceMock factory_context; + ConfigImpl config(parseRouteConfigurationFromV2Yaml(NoIdleTimeot), factory_context, true); + Http::TestHeaderMapImpl headers = genRedirectHeaders("idle.lyft.com", "/regex", true, false); + const RouteEntry* route_entry = config.route(headers, 0)->routeEntry(); + EXPECT_EQ(absl::nullopt, route_entry->idleTimeout()); +} + +TEST(RouteConfigurationV2, DefaultIdleTimeout) { + const std::string DefaultIdleTimeot = R"EOF( +name: NoIdleTimeout +virtual_hosts: + - name: regex + domains: [idle.lyft.com] + routes: + - match: { regex: "/regex"} + route: + cluster: some-cluster + )EOF"; + + NiceMock factory_context; + ConfigImpl config(parseRouteConfigurationFromV2Yaml(DefaultIdleTimeot), 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()); +} + +TEST(RouteConfigurationV2, ExplicitIdleTimeout) { + const std::string ExplicitIdleTimeot = R"EOF( +name: NoIdleTimeout +virtual_hosts: + - name: regex + domains: [idle.lyft.com] + routes: + - match: { regex: "/regex"} + route: + cluster: some-cluster + idle_timeout: 7s + )EOF"; + + NiceMock factory_context; + ConfigImpl config(parseRouteConfigurationFromV2Yaml(ExplicitIdleTimeot), factory_context, true); + Http::TestHeaderMapImpl headers = genRedirectHeaders("idle.lyft.com", "/regex", true, false); + const RouteEntry* route_entry = config.route(headers, 0)->routeEntry(); + EXPECT_EQ(7 * 1000, route_entry->idleTimeout().value().count()); +} + class PerFilterConfigsTest : public testing::Test { public: PerFilterConfigsTest() : factory_(), registered_factory_(factory_) {} @@ -4499,6 +4558,7 @@ name: foo checkEach(yaml, 1213, 1213, 1415); } + } // namespace } // namespace Router } // namespace Envoy diff --git a/test/integration/BUILD b/test/integration/BUILD index 05811781ad872..e2db60fecc283 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -221,6 +221,12 @@ envoy_cc_test_library( ], ) +envoy_cc_test( + name = "idle_timeout_integration_test", + srcs = ["idle_timeout_integration_test.cc"], + deps = [":http_protocol_integration_lib"], +) + envoy_cc_test_library( name = "integration_lib", srcs = [ diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc new file mode 100644 index 0000000000000..97b71aefbb139 --- /dev/null +++ b/test/integration/idle_timeout_integration_test.cc @@ -0,0 +1,143 @@ +#include "test/integration/http_protocol_integration.h" + +namespace Envoy { +namespace { + +class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { +public: + void initialize() override { + 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); + // For validating encode100ContinueHeaders() timer kick. + hcm.set_proxy_100_continue(true); + }); + HttpProtocolIntegrationTest::initialize(); + } + + IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest() { + initialize(); + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":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(); + return response; + } + + void sleep() { std::this_thread::sleep_for(std::chrono::milliseconds(TimeoutMs / 2)); } + + void waitForTimeout(IntegrationStreamDecoder& response) { + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + codec_client_->waitForDisconnect(); + } else { + response.waitForReset(); + codec_client_->close(); + } + EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_idle_timeout")->value()); + } + + // 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; +}; + +INSTANTIATE_TEST_CASE_P(Protocols, IdleTimeoutIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +// Per-stream idle timeout after having sent downstream headers. +TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeaders) { + 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(); + + sleep(); + codec_client_->sendData(*request_encoder_, 1, false); + + waitForTimeout(*response); + + EXPECT_FALSE(upstream_request_->complete()); + EXPECT_EQ(1U, 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 upstream headers have been sent. +TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterUpstreamHeaders) { + auto response = setupPerStreamIdleTimeoutTest(); + + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false); + + waitForTimeout(*response); + + EXPECT_FALSE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + EXPECT_FALSE(response->complete()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); + EXPECT_EQ("", response->body()); +} + +// Per-stream idle timeout after a sequence of header/data events. +TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterBidiData) { + auto response = setupPerStreamIdleTimeoutTest(); + + sleep(); + upstream_request_->encode100ContinueHeaders(Http::TestHeaderMapImpl{{":status", "100"}}); + + sleep(); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false); + + sleep(); + upstream_request_->encodeData(1, false); + + sleep(); + codec_client_->sendData(*request_encoder_, 1, false); + + sleep(); + Http::TestHeaderMapImpl request_trailers{{"request1", "trailer1"}, {"request2", "trailer2"}}; + codec_client_->sendTrailers(*request_encoder_, request_trailers); + + sleep(); + upstream_request_->encodeData(1, false); + + waitForTimeout(*response); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(1U, upstream_request_->bodyLength()); + EXPECT_FALSE(response->complete()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); + EXPECT_EQ("aa", response->body()); +} + +// Successful request/response when per-stream idle timeout is configured. +TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutRequestAndResponse) { + testRouterRequestAndResponseWithBody(1024, 1024, false, nullptr); +} + +} // namespace +} // namespace Envoy diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 3cc41a1d769f4..9040253dc63de 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -229,6 +229,7 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(retryPolicy, const RetryPolicy&()); MOCK_CONST_METHOD0(shadowPolicy, const ShadowPolicy&()); MOCK_CONST_METHOD0(timeout, std::chrono::milliseconds()); + MOCK_CONST_METHOD0(idleTimeout, absl::optional()); MOCK_CONST_METHOD0(maxGrpcTimeout, absl::optional()); MOCK_CONST_METHOD1(virtualCluster, const VirtualCluster*(const Http::HeaderMap& headers)); MOCK_CONST_METHOD0(virtualHostName, const std::string&());