From b969a6b5fc39fb930b9d9dfbb94a23251c0669e9 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Tue, 10 Mar 2020 02:57:42 +0000 Subject: [PATCH 01/13] add max stream duration Signed-off-by: shikugawa --- api/envoy/api/v2/core/protocol.proto | 5 ++++- api/envoy/config/core/v3/protocol.proto | 5 ++++- .../envoy/api/v2/core/protocol.proto | 5 ++++- .../envoy/config/core/v3/protocol.proto | 5 ++++- source/common/http/conn_manager_config.h | 5 +++++ source/common/http/conn_manager_impl.cc | 14 ++++++++++++ source/common/http/conn_manager_impl.h | 2 ++ .../network/http_connection_manager/config.cc | 2 ++ .../network/http_connection_manager/config.h | 4 ++++ source/server/http/admin.h | 3 +++ test/common/http/conn_manager_impl_test.cc | 22 +++++++++++++++++++ 11 files changed, 68 insertions(+), 4 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 53b6ae8746794..7508b440a5a54 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -112,7 +112,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values @@ -216,6 +216,9 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index b3263a1fdde6e..d3fc7ccec491d 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -128,7 +128,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; @@ -235,6 +235,9 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 53b6ae8746794..7508b440a5a54 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -112,7 +112,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values @@ -216,6 +216,9 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index b3263a1fdde6e..d3fc7ccec491d 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -128,7 +128,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; @@ -235,6 +235,9 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index f81b67f976e12..420ff4b305432 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -272,6 +272,11 @@ class ConnectionManagerConfig { */ virtual std::chrono::milliseconds delayedCloseTimeout() const PURE; + /** + * @return maximum duration time to keep alive stream + */ + virtual absl::optional maxStreamDuration() const PURE; + /** * @return Router::RouteConfigProvider* the configuration provider used to acquire a route * config for each request flow. Pointer ownership is _not_ transferred to the caller of diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 946d61a779b7b..1faa3b6fbd207 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -602,6 +602,15 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect request_timer_->enableTimer(request_timeout_ms_, this); } + if (connection_manager_.config_.maxStreamDuration().has_value()) { + std::chrono::milliseconds max_stream_duration_ms_ = + connection_manager_.config_.maxStreamDuration().value(); + max_stream_duration_timer_ = + connection_manager.read_callbacks_->connection().dispatcher().createTimer([this]() -> void { + onResetStream(StreamResetReason::ConnectionTermination, absl::string_view()); + }); + max_stream_duration_timer_->enableTimer(max_stream_duration_ms_, this); + } stream_info_.setRequestedServerName( connection_manager_.read_callbacks_->connection().requestedServerName()); } @@ -1907,6 +1916,11 @@ bool ConnectionManagerImpl::ActiveStream::handleDataIfStopAll(ActiveStreamFilter } void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl::string_view) { + if (max_stream_duration_timer_) { + max_stream_duration_timer_->disableTimer(); + max_stream_duration_timer_.reset(); + } + // NOTE: This function gets called in all of the following cases: // 1) We TX an app level reset // 2) The codec TX a codec level reset diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index cf2311b099cf4..b446e6fc790e2 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -701,6 +701,8 @@ class ConnectionManagerImpl : Logger::Loggable, Event::TimerPtr stream_idle_timer_; // Per-stream request timeout. Event::TimerPtr request_timer_; + // Per-stream alive duration. + Event::TimerPtr max_stream_duration_timer_; std::chrono::milliseconds idle_timeout_ms_{}; State state_; StreamInfo::StreamInfoImpl stream_info_; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index fdda63afc7e56..fcec861eb2fc9 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -173,6 +173,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), idle_timeout)), max_connection_duration_( PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), max_connection_duration)), + max_stream_duration_( + PROTOBUF_GET_OPTIONAL_MS(config.http2_protocol_options(), max_stream_duration)), stream_idle_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, stream_idle_timeout, StreamIdleTimeoutMs)), request_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, request_timeout, RequestTimeoutMs)), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 56993962cf2aa..f1493ee967569 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -113,6 +113,9 @@ class HttpConnectionManagerConfig : Logger::Loggable, } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } + absl::optional maxStreamDuration() const override { + return max_stream_duration_; + } Router::RouteConfigProvider* routeConfigProvider() override { return route_config_provider_.get(); } @@ -185,6 +188,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const uint32_t max_request_headers_count_; absl::optional idle_timeout_; absl::optional max_connection_duration_; + absl::optional max_stream_duration_; std::chrono::milliseconds stream_idle_timeout_; std::chrono::milliseconds request_timeout_; Router::RouteConfigProviderSharedPtr route_config_provider_; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 8679ec1863d11..2e4f8482f549f 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -124,6 +124,9 @@ class AdminImpl : public Admin, std::chrono::milliseconds streamIdleTimeout() const override { return {}; } std::chrono::milliseconds requestTimeout() const override { return {}; } std::chrono::milliseconds delayedCloseTimeout() const override { return {}; } + absl::optional maxStreamDuration() const override { + return absl::make_optional({}); + } Router::RouteConfigProvider* routeConfigProvider() override { return &route_config_provider_; } Config::ConfigProvider* scopedRouteConfigProvider() override { return &scoped_route_config_provider_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 451adf7fe90b8..3ceffe093b76d 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -304,6 +304,9 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } + absl::optional maxStreamDuration() const override { + return max_stream_duration_timer_; + } bool use_srds_{}; Router::RouteConfigProvider* routeConfigProvider() override { if (use_srds_) { @@ -378,6 +381,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::chrono::milliseconds stream_idle_timeout_{}; std::chrono::milliseconds request_timeout_{}; std::chrono::milliseconds delayed_close_timeout_{}; + absl::optional max_stream_duration_timer_{}; NiceMock random_; NiceMock local_info_; NiceMock factory_context_; @@ -2398,6 +2402,24 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermin EXPECT_EQ(0U, stats_.named_.downstream_rq_timeout_.value()); } +TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { + max_stream_duration_timer_ = absl::make_optional(10); + setup(false, ""); + Event::MockTimer* duration_timer = setUpTimer(); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + EXPECT_CALL(*duration_timer, enableTimer(max_stream_duration_timer_.value(), _)).Times(1); + conn_manager_->newStream(response_encoder_); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); // kick off request + + EXPECT_CALL(*duration_timer, disableTimer()).Times(1); + duration_timer->invokeCallback(); + EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); +} + TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) { setup(false, ""); RequestDecoder* decoder = nullptr; From 7a32f71b93dafc204a6d15efe88aa2ac0e757c38 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Thu, 12 Mar 2020 15:18:45 +0000 Subject: [PATCH 02/13] h2 -> common Signed-off-by: shikugawa --- api/envoy/api/v2/core/protocol.proto | 8 ++++---- api/envoy/config/core/v3/protocol.proto | 8 ++++---- .../envoy/api/v2/core/protocol.proto | 8 ++++---- .../envoy/config/core/v3/protocol.proto | 8 ++++---- source/common/http/conn_manager_impl.cc | 18 +++++++++--------- .../network/http_connection_manager/config.cc | 2 +- source/server/http/admin.h | 3 ++- test/common/http/conn_manager_impl_test.cc | 8 ++++---- test/config/utility.cc | 10 ++++++++++ test/config/utility.h | 3 +++ test/integration/protocol_integration_test.cc | 13 +++++++++++++ 11 files changed, 58 insertions(+), 31 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 7508b440a5a54..386fee3f2276f 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -59,6 +59,9 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 4; } // [#next-free-field: 6] @@ -112,7 +115,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 14] +// [#next-free-field: 13] message Http2ProtocolOptions { // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values @@ -216,9 +219,6 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - - // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. - google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index d3fc7ccec491d..0568a6b3b5915 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -67,6 +67,9 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 4; } // [#next-free-field: 6] @@ -128,7 +131,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 14] +// [#next-free-field: 13] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; @@ -235,9 +238,6 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - - // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. - google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 7508b440a5a54..386fee3f2276f 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -59,6 +59,9 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 4; } // [#next-free-field: 6] @@ -112,7 +115,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 14] +// [#next-free-field: 13] message Http2ProtocolOptions { // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values @@ -216,9 +219,6 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - - // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. - google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index d3fc7ccec491d..0568a6b3b5915 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -67,6 +67,9 @@ message HttpProtocolOptions { // maximum number of request headers allowed is 100. Requests that exceed this limit will receive // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; + + // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + google.protobuf.Duration max_stream_duration = 4; } // [#next-free-field: 6] @@ -128,7 +131,7 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 14] +// [#next-free-field: 13] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; @@ -235,9 +238,6 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - - // Specify duration to keep alive HTTP/2 stream. If not specified, this value is not set. - google.protobuf.Duration max_stream_duration = 13; } // [#not-implemented-hide:] diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 1faa3b6fbd207..34cfe339a9f3d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -187,6 +187,11 @@ void ConnectionManagerImpl::checkForDeferredClose() { } void ConnectionManagerImpl::doEndStream(ActiveStream& stream) { + if (stream.max_stream_duration_timer_) { + stream.max_stream_duration_timer_->disableTimer(); + stream.max_stream_duration_timer_.reset(); + } + // The order of what happens in this routine is important and a little complicated. We first see // if the stream needs to be reset. If it needs to be, this will end up invoking reset callbacks // and then moving the stream to the deferred destruction list. If the stream has not been reset, @@ -602,15 +607,15 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect request_timer_->enableTimer(request_timeout_ms_, this); } - if (connection_manager_.config_.maxStreamDuration().has_value()) { + if (connection_manager_.config_.maxStreamDuration()) { std::chrono::milliseconds max_stream_duration_ms_ = connection_manager_.config_.maxStreamDuration().value(); max_stream_duration_timer_ = - connection_manager.read_callbacks_->connection().dispatcher().createTimer([this]() -> void { - onResetStream(StreamResetReason::ConnectionTermination, absl::string_view()); - }); + connection_manager.read_callbacks_->connection().dispatcher().createTimer( + [this]() -> void { connection_manager_.doEndStream(*this); }); max_stream_duration_timer_->enableTimer(max_stream_duration_ms_, this); } + stream_info_.setRequestedServerName( connection_manager_.read_callbacks_->connection().requestedServerName()); } @@ -1916,11 +1921,6 @@ bool ConnectionManagerImpl::ActiveStream::handleDataIfStopAll(ActiveStreamFilter } void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl::string_view) { - if (max_stream_duration_timer_) { - max_stream_duration_timer_->disableTimer(); - max_stream_duration_timer_.reset(); - } - // NOTE: This function gets called in all of the following cases: // 1) We TX an app level reset // 2) The codec TX a codec level reset diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 611dc539fafcc..02c7e4468f4e9 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -186,7 +186,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( max_connection_duration_( PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), max_connection_duration)), max_stream_duration_( - PROTOBUF_GET_OPTIONAL_MS(config.http2_protocol_options(), max_stream_duration)), + 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)), request_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, request_timeout, RequestTimeoutMs)), diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 08eb09d4f7015..abbac234f3c44 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -125,7 +125,7 @@ class AdminImpl : public Admin, std::chrono::milliseconds requestTimeout() const override { return {}; } std::chrono::milliseconds delayedCloseTimeout() const override { return {}; } absl::optional maxStreamDuration() const override { - return absl::make_optional({}); + return max_stream_duration_; } Router::RouteConfigProvider* routeConfigProvider() override { return &route_config_provider_; } Config::ConfigProvider* scopedRouteConfigProvider() override { @@ -460,6 +460,7 @@ class AdminImpl : public Admin, const uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; absl::optional idle_timeout_; absl::optional max_connection_duration_; + absl::optional max_stream_duration_; absl::optional user_agent_; Http::SlowDateProviderImpl date_provider_; std::vector set_current_client_cert_details_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 464b8ff354071..f38472e6f823b 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -303,7 +303,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } absl::optional maxStreamDuration() const override { - return max_stream_duration_timer_; + return max_stream_duration_; } bool use_srds_{}; Router::RouteConfigProvider* routeConfigProvider() override { @@ -379,7 +379,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::chrono::milliseconds stream_idle_timeout_{}; std::chrono::milliseconds request_timeout_{}; std::chrono::milliseconds delayed_close_timeout_{}; - absl::optional max_stream_duration_timer_{}; + absl::optional max_stream_duration_{}; NiceMock random_; NiceMock local_info_; NiceMock factory_context_; @@ -2403,12 +2403,12 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermin } TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { - max_stream_duration_timer_ = absl::make_optional(10); + max_stream_duration_ = std::chrono::milliseconds(10); setup(false, ""); Event::MockTimer* duration_timer = setUpTimer(); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { - EXPECT_CALL(*duration_timer, enableTimer(max_stream_duration_timer_.value(), _)).Times(1); + EXPECT_CALL(*duration_timer, enableTimer(max_stream_duration_.value(), _)).Times(1); conn_manager_->newStream(response_encoder_); })); diff --git a/test/config/utility.cc b/test/config/utility.cc index 10d0249336511..c22b07f52f62d 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -611,6 +611,16 @@ void ConfigHelper::setDownstreamMaxConnectionDuration(std::chrono::milliseconds }); } +void ConfigHelper::setDownstreamMaxStreamDuration(std::chrono::milliseconds timeout) { + addConfigModifier( + [timeout]( + envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + hcm.mutable_common_http_protocol_options()->mutable_max_stream_duration()->MergeFrom( + ProtobufUtil::TimeUtil::MillisecondsToDuration(timeout.count())); + }); +} + void ConfigHelper::setConnectTimeout(std::chrono::milliseconds timeout) { RELEASE_ASSERT(!finalized_, ""); diff --git a/test/config/utility.h b/test/config/utility.h index e476019198668..785f3a050fedb 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -123,6 +123,9 @@ class ConfigHelper { // Set the max connection duration for downstream connections through the HttpConnectionManager. void setDownstreamMaxConnectionDuration(std::chrono::milliseconds max_connection_duration); + // Set the max stream duration for downstream connections through the HttpConnectionManager. + void setDownstreamMaxStreamDuration(std::chrono::milliseconds max_stream_duration); + // Set the connect timeout on upstream connections. void setConnectTimeout(std::chrono::milliseconds timeout); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 905e84a94a2af..6bb643a9dd21a 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1493,6 +1493,19 @@ TEST_P(ProtocolIntegrationTest, ConnDurationTimeoutNoHttpRequest) { test_server_->waitForCounterGe("http.config_test.downstream_cx_max_duration_reached", 1); } +TEST_P(DownstreamProtocolIntegrationTest, BasicMaxStreamTimeout) { + config_helper_.setDownstreamMaxStreamDuration(std::chrono::milliseconds(500)); + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->startRequest(default_request_headers_); + timeSystem().sleep(std::chrono::milliseconds(550)); + + codec_client_->close(); + EXPECT_FALSE(response.second->complete()); +} + // Make sure that invalid authority headers get blocked at or before the HCM. TEST_P(DownstreamProtocolIntegrationTest, InvalidAuthority) { initialize(); From 57f5ffbaeec79643a0c33b2d00f7801116df3fc1 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Thu, 12 Mar 2020 16:02:25 +0000 Subject: [PATCH 03/13] add doc Signed-off-by: shikugawa --- docs/root/faq/configuration/timeouts.rst | 8 ++++++++ docs/root/intro/version_history.rst | 1 + 2 files changed, 9 insertions(+) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index 33af5873b8885..710f69421d60d 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -53,6 +53,14 @@ context request/stream is interchangeable. is the amount of time that the connection manager will allow a stream to exist with no upstream or downstream activity. The default stream idle timeout is *5 minutes*. This timeout is strongly recommended for streaming APIs (requests or responses that never end). +* The HTTP protocol :ref:`max_stream_duration ` + is defined in a generic message used by both the HTTP + connection manager as well as upstream cluster HTTP connections. The max stream duration is the time which + is used to specify existing span of stream. This is differ from :ref:`request_timeout + ` + from the point of view that :ref:`request_timeout + ` is + disarmed from active stream when the first response message was received. Route timeouts ^^^^^^^^^^^^^^ diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 6a38480b032d7..a0654dc6960fb 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -24,6 +24,7 @@ Version history * http: fixing a bug in HTTP/1.0 responses where Connection: keep-alive was not appended for connections which were kept alive. * http: fixed a bug that could send extra METADATA frames and underflow memory when encoding METADATA frames on a connection that was dispatching data. * http: connection header sanitizing has been modified to always sanitize if there is no upgrade, including when an h2c upgrade attempt has been removed. +* http: added :ref:`max_stream_duration ` to specify the duration of existing streams. See :ref:`connection and stream timeouts `. * listener filters: listener filter extensions use the "envoy.filters.listener" name space. A mapping of extension names is available in the :ref:`deprecated ` documentation. * listeners: fixed issue where :ref:`TLS inspector listener filter ` could have been bypassed by a client using only TLS 1.3. From 396a9857d3c76aea80a2a2d2d0603adfee10b275 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Fri, 13 Mar 2020 14:26:07 +0000 Subject: [PATCH 04/13] apply proto format Signed-off-by: shikugawa --- .../http/http_conn_man/stats.rst | 1 + source/common/http/conn_manager_config.h | 1 + source/common/http/conn_manager_impl.cc | 21 +++++++++++-------- source/common/http/conn_manager_impl.h | 3 ++- test/common/http/conn_manager_impl_test.cc | 21 ++++++++++++++++++- test/integration/protocol_integration_test.cc | 12 +++++++---- 6 files changed, 44 insertions(+), 15 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/stats.rst b/docs/root/configuration/http/http_conn_man/stats.rst index 6d46fb937bd0b..d58c4bc359b86 100644 --- a/docs/root/configuration/http/http_conn_man/stats.rst +++ b/docs/root/configuration/http/http_conn_man/stats.rst @@ -56,6 +56,7 @@ statistics: downstream_rq_ws_on_non_ws_route, Counter, Total WebSocket upgrade requests rejected by non WebSocket routes downstream_rq_time, Histogram, Total time for request and response (milliseconds) downstream_rq_idle_timeout, Counter, Total requests closed due to idle timeout + downstream_rq_max_duration_reached, Counter, Total requests closed due to max duration reached downstream_rq_timeout, Counter, Total requests closed due to a timeout on the request path downstream_rq_overload_close, Counter, Total requests closed due to Envoy overload rs_too_large, Counter, Total response errors due to buffering an overly large body diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 31b796240fb0d..602eccd095c0e 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -58,6 +58,7 @@ namespace Http { COUNTER(downstream_rq_too_large) \ COUNTER(downstream_rq_total) \ COUNTER(downstream_rq_tx_reset) \ + COUNTER(downstream_rq_max_duration_reached) \ COUNTER(downstream_rq_ws_on_non_ws_route) \ COUNTER(rs_too_large) \ GAUGE(downstream_cx_active, Accumulate) \ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 34cfe339a9f3d..761b37c9e47a4 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -187,11 +187,6 @@ void ConnectionManagerImpl::checkForDeferredClose() { } void ConnectionManagerImpl::doEndStream(ActiveStream& stream) { - if (stream.max_stream_duration_timer_) { - stream.max_stream_duration_timer_->disableTimer(); - stream.max_stream_duration_timer_.reset(); - } - // The order of what happens in this routine is important and a little complicated. We first see // if the stream needs to be reset. If it needs to be, this will end up invoking reset callbacks // and then moving the stream to the deferred destruction list. If the stream has not been reset, @@ -235,6 +230,10 @@ void ConnectionManagerImpl::doEndStream(ActiveStream& stream) { } void ConnectionManagerImpl::doDeferredStreamDestroy(ActiveStream& stream) { + if (stream.max_stream_duration_timer_) { + stream.max_stream_duration_timer_->disableTimer(); + stream.max_stream_duration_timer_ = nullptr; + } if (stream.stream_idle_timer_ != nullptr) { stream.stream_idle_timer_->disableTimer(); stream.stream_idle_timer_ = nullptr; @@ -608,12 +607,11 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect } if (connection_manager_.config_.maxStreamDuration()) { - std::chrono::milliseconds max_stream_duration_ms_ = - connection_manager_.config_.maxStreamDuration().value(); max_stream_duration_timer_ = connection_manager.read_callbacks_->connection().dispatcher().createTimer( - [this]() -> void { connection_manager_.doEndStream(*this); }); - max_stream_duration_timer_->enableTimer(max_stream_duration_ms_, this); + [this]() -> void { onStreamMaxDurationReached(); }); + max_stream_duration_timer_->enableTimer(connection_manager_.config_.maxStreamDuration().value(), + this); } stream_info_.setRequestedServerName( @@ -687,6 +685,11 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout); } +void ConnectionManagerImpl::ActiveStream::onStreamMaxDurationReached() { + connection_manager_.stats_.named_.downstream_rq_max_duration_reached_.inc(); + connection_manager_.doEndStream(*this); +} + void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilterWorker( StreamDecoderFilterSharedPtr filter, bool dual_filter) { ActiveStreamDecoderFilterPtr wrapper(new ActiveStreamDecoderFilter(*this, filter, dual_filter)); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 194a333049187..bb3292dc0b876 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -659,7 +659,8 @@ class ConnectionManagerImpl : Logger::Loggable, void resetIdleTimer(); // Per-stream request timeout callback void onRequestTimeout(); - + // Per-stream alive duration reached. + void onStreamMaxDurationReached(); bool hasCachedRoute() { return cached_route_.has_value() && cached_route_.value(); } friend std::ostream& operator<<(std::ostream& os, const ActiveStream& s) { diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index f38472e6f823b..fa4144e47e945 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2415,11 +2415,30 @@ TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); // kick off request - EXPECT_CALL(*duration_timer, disableTimer()).Times(1); + EXPECT_CALL(*duration_timer, disableTimer()); duration_timer->invokeCallback(); EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); } +TEST_F(HttpConnectionManagerImplTest, NotInvokeDeferredStreamDeletion) { + max_stream_duration_ = std::chrono::milliseconds(5000); + setup(false, ""); + Event::MockTimer* duration_timer = setUpTimer(); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + EXPECT_CALL(*duration_timer, enableTimer(max_stream_duration_.value(), _)).Times(1); + conn_manager_->newStream(response_encoder_); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); // kick off request + + EXPECT_CALL(*duration_timer, disableTimer()); + conn_manager_->onEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); +} + TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) { setup(false, ""); RequestDecoder* decoder = nullptr; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 6bb643a9dd21a..722baa02a8557 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1499,11 +1499,15 @@ TEST_P(DownstreamProtocolIntegrationTest, BasicMaxStreamTimeout) { fake_upstreams_[0]->set_allow_unexpected_disconnects(true); codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->startRequest(default_request_headers_); - timeSystem().sleep(std::chrono::milliseconds(550)); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); - codec_client_->close(); - EXPECT_FALSE(response.second->complete()); + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + + test_server_->waitForCounterGe("http.config_test.downstream_rq_max_duration_reached", 1); + response->waitForReset(); + EXPECT_FALSE(response->complete()); } // Make sure that invalid authority headers get blocked at or before the HCM. From e0a00649e687b0a30bd38e5050246bbeb6889f4c Mon Sep 17 00:00:00 2001 From: shikugawa Date: Fri, 13 Mar 2020 16:45:31 +0000 Subject: [PATCH 05/13] fix mock Signed-off-by: shikugawa --- test/common/http/conn_manager_impl_fuzz_test.cc | 4 ++++ test/common/http/conn_manager_utility_test.cc | 1 + 2 files changed, 5 insertions(+) diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 6e68da183de51..87a264140764b 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -105,6 +105,9 @@ class FuzzConfig : public ConnectionManagerConfig { absl::optional maxConnectionDuration() const override { return max_connection_duration_; } + absl::optional maxStreamDuration() const override { + return max_stream_duration_; + } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } @@ -171,6 +174,7 @@ class FuzzConfig : public ConnectionManagerConfig { uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; absl::optional idle_timeout_; absl::optional max_connection_duration_; + absl::optional max_stream_duration_; std::chrono::milliseconds stream_idle_timeout_{}; std::chrono::milliseconds request_timeout_{}; std::chrono::milliseconds delayed_close_timeout_{}; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index ffa321d48170d..b2301a03648a0 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -65,6 +65,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD(absl::optional, idleTimeout, (), (const)); MOCK_METHOD(bool, isRoutable, (), (const)); MOCK_METHOD(absl::optional, maxConnectionDuration, (), (const)); + MOCK_METHOD(absl::optional, maxStreamDuration, (), (const)); MOCK_METHOD(std::chrono::milliseconds, streamIdleTimeout, (), (const)); MOCK_METHOD(std::chrono::milliseconds, requestTimeout, (), (const)); MOCK_METHOD(std::chrono::milliseconds, delayedCloseTimeout, (), (const)); From f72ca5619ebfca887ce0d3dc58726eeb1392bbaa Mon Sep 17 00:00:00 2001 From: shikugawa Date: Mon, 16 Mar 2020 06:40:59 +0000 Subject: [PATCH 06/13] add waiting header Signed-off-by: shikugawa --- docs/root/faq/configuration/timeouts.rst | 16 +++++++++------- source/common/http/conn_manager_impl.cc | 1 + test/integration/protocol_integration_test.cc | 2 ++ 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index 710f69421d60d..8c697a7f95648 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -54,13 +54,15 @@ context request/stream is interchangeable. or downstream activity. The default stream idle timeout is *5 minutes*. This timeout is strongly recommended for streaming APIs (requests or responses that never end). * The HTTP protocol :ref:`max_stream_duration ` - is defined in a generic message used by both the HTTP - connection manager as well as upstream cluster HTTP connections. The max stream duration is the time which - is used to specify existing span of stream. This is differ from :ref:`request_timeout - ` - from the point of view that :ref:`request_timeout - ` is - disarmed from active stream when the first response message was received. + is defined in a generic message used by the HTTP connection manager. The max stream duration is the + maximum time that a stream's lifetime will span. You can use this functionality when you want to kill + HTTP request/response stream periodically. You can't use :ref:`request_timeout + ` + in this situation because this timer will be disarmed if a response header is arrived to the request/response stream. + + .. attention:: + + Now, We can apply this functionality to downstream only. Route timeouts ^^^^^^^^^^^^^^ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 9f7e8cc5db36e..8b0f01851635a 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -686,6 +686,7 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() { } void ConnectionManagerImpl::ActiveStream::onStreamMaxDurationReached() { + ENVOY_STREAM_LOG(debug, "Stream max duration time reached", *this); connection_manager_.stats_.named_.downstream_rq_max_duration_reached_.inc(); connection_manager_.doEndStream(*this); } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 722baa02a8557..c43c96e537c52 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1504,6 +1504,8 @@ TEST_P(DownstreamProtocolIntegrationTest, BasicMaxStreamTimeout) { auto response = std::move(encoder_decoder.second); ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); test_server_->waitForCounterGe("http.config_test.downstream_rq_max_duration_reached", 1); response->waitForReset(); From d6cea7299736dd177afd7dbaa8eea24e1b7aafbf Mon Sep 17 00:00:00 2001 From: shikugawa Date: Mon, 16 Mar 2020 07:36:51 +0000 Subject: [PATCH 07/13] add todo Signed-off-by: shikugawa --- api/envoy/api/v2/core/protocol.proto | 4 +++- api/envoy/config/core/v3/protocol.proto | 4 +++- generated_api_shadow/envoy/api/v2/core/protocol.proto | 4 +++- generated_api_shadow/envoy/config/core/v3/protocol.proto | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 386fee3f2276f..1042c5c2f07ba 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -60,7 +60,9 @@ message HttpProtocolOptions { // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; - // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be + // reset independent of any other timeouts. If not specified, this value is not set. + // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 0568a6b3b5915..2b3cd36fc763e 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -68,7 +68,9 @@ message HttpProtocolOptions { // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; - // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be + // reset independent of any other timeouts. If not specified, this value is not set. + // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 386fee3f2276f..1042c5c2f07ba 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -60,7 +60,9 @@ message HttpProtocolOptions { // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; - // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be + // reset independent of any other timeouts. If not specified, this value is not set. + // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 0568a6b3b5915..2b3cd36fc763e 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -68,7 +68,9 @@ message HttpProtocolOptions { // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2. google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; - // Specify duration to keep alive HTTP/2 stream and request/response pair. If not specified, this value is not set. + // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be + // reset independent of any other timeouts. If not specified, this value is not set. + // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } From 3cd199fa3a6dc61830d89c090cf8fbfde2d2918c Mon Sep 17 00:00:00 2001 From: shikugawa Date: Mon, 16 Mar 2020 12:20:03 +0000 Subject: [PATCH 08/13] add test Signed-off-by: shikugawa --- source/common/http/conn_manager_impl.cc | 3 ++- test/common/http/conn_manager_impl_test.cc | 31 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 8b0f01851635a..438c309aaa298 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -606,7 +606,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect request_timer_->enableTimer(request_timeout_ms_, this); } - if (connection_manager_.config_.maxStreamDuration()) { + const auto max_stream_duration = connection_manager_.config_.maxStreamDuration(); + if (max_stream_duration.has_value() && max_stream_duration.value().count()) { max_stream_duration_timer_ = connection_manager.read_callbacks_->connection().dispatcher().createTimer( [this]() -> void { onStreamMaxDurationReached(); }); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 38a980b3b623b..326b53c1fcc34 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2401,6 +2401,34 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermin EXPECT_EQ(0U, stats_.named_.downstream_rq_timeout_.value()); } +TEST_F(HttpConnectionManagerImplTest, NotInvokeStreamDeletionConfiguredZero) { + max_stream_duration_ = std::chrono::milliseconds(0); + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, createTimer_).Times(0); + conn_manager_->newStream(response_encoder_); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); // kick off request +} + +TEST_F(HttpConnectionManagerImplTest, StreamDeletionConfigiredValidly) { + max_stream_duration_ = std::chrono::milliseconds(10); + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + Event::MockTimer* duration_timer = setUpTimer(); + + EXPECT_CALL(*duration_timer, enableTimer(max_stream_duration_.value(), _)); + conn_manager_->newStream(response_encoder_); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); // kick off request +} + TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { max_stream_duration_ = std::chrono::milliseconds(10); setup(false, ""); @@ -2416,6 +2444,8 @@ TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { EXPECT_CALL(*duration_timer, disableTimer()); duration_timer->invokeCallback(); + + EXPECT_EQ(1U, stats_.named_.downstream_rq_max_duration_reached_.value()); EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); } @@ -2435,6 +2465,7 @@ TEST_F(HttpConnectionManagerImplTest, NotInvokeDeferredStreamDeletion) { EXPECT_CALL(*duration_timer, disableTimer()); conn_manager_->onEvent(Network::ConnectionEvent::RemoteClose); + EXPECT_EQ(0U, stats_.named_.downstream_rq_max_duration_reached_.value()); EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); } From 6978cc35238c081265dd025b33915c70f596b9b8 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Tue, 17 Mar 2020 14:45:32 +0000 Subject: [PATCH 09/13] fix doc Signed-off-by: shikugawa --- api/envoy/api/v2/core/protocol.proto | 1 + api/envoy/config/core/v3/protocol.proto | 1 + docs/root/faq/configuration/timeouts.rst | 8 ++++---- generated_api_shadow/envoy/api/v2/core/protocol.proto | 1 + generated_api_shadow/envoy/config/core/v3/protocol.proto | 1 + 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 1042c5c2f07ba..b7acac7375080 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -62,6 +62,7 @@ message HttpProtocolOptions { // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be // reset independent of any other timeouts. If not specified, this value is not set. + // The current implementation implements this timeout on downstream connections only. // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 2b3cd36fc763e..b7ad4b7d8bd64 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -70,6 +70,7 @@ message HttpProtocolOptions { // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be // reset independent of any other timeouts. If not specified, this value is not set. + // The current implementation implements this timeout on downstream connections only. // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index 8c697a7f95648..65f3e9efce2c0 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -55,14 +55,14 @@ context request/stream is interchangeable. recommended for streaming APIs (requests or responses that never end). * The HTTP protocol :ref:`max_stream_duration ` is defined in a generic message used by the HTTP connection manager. The max stream duration is the - maximum time that a stream's lifetime will span. You can use this functionality when you want to kill - HTTP request/response stream periodically. You can't use :ref:`request_timeout + maximum time that a stream's lifetime will span. You can use this functionality when you want to reset + HTTP request/response streams periodically. You can't use :ref:`request_timeout ` - in this situation because this timer will be disarmed if a response header is arrived to the request/response stream. + in this situation because this timer will be disarmed if a response header is received on the request/response streams. .. attention:: - Now, We can apply this functionality to downstream only. + The current implementation implements this timeout on downstream connections only. Route timeouts ^^^^^^^^^^^^^^ diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 1042c5c2f07ba..b7acac7375080 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -62,6 +62,7 @@ message HttpProtocolOptions { // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be // reset independent of any other timeouts. If not specified, this value is not set. + // The current implementation implements this timeout on downstream connections only. // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 2b3cd36fc763e..b7ad4b7d8bd64 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -70,6 +70,7 @@ message HttpProtocolOptions { // Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be // reset independent of any other timeouts. If not specified, this value is not set. + // The current implementation implements this timeout on downstream connections only. // [#comment:TODO(shikugawa): add this functionality to upstream.] google.protobuf.Duration max_stream_duration = 4; } From c74b0a0801dd635d28d968405d80a9ed02daeabb Mon Sep 17 00:00:00 2001 From: shikugawa Date: Tue, 17 Mar 2020 17:51:46 +0000 Subject: [PATCH 10/13] Kick CI Signed-off-by: shikugawa From 37f44d361c7d8c65c9d093450d68330475f55559 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Wed, 18 Mar 2020 01:21:04 +0000 Subject: [PATCH 11/13] rename ambigous test name Signed-off-by: shikugawa --- test/common/http/conn_manager_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 326b53c1fcc34..aeb1052941f1a 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2401,7 +2401,7 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermin EXPECT_EQ(0U, stats_.named_.downstream_rq_timeout_.value()); } -TEST_F(HttpConnectionManagerImplTest, NotInvokeStreamDeletionConfiguredZero) { +TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationDisabledIfSetToZero) { max_stream_duration_ = std::chrono::milliseconds(0); setup(false, ""); From 562df0dfece1fd115c8e53184f73846bb6026551 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Thu, 19 Mar 2020 06:00:47 +0000 Subject: [PATCH 12/13] fix test name Signed-off-by: shikugawa --- test/common/http/conn_manager_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index aeb1052941f1a..f2b566eb624c6 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2414,7 +2414,7 @@ TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationDisabledIfSetToZero) { conn_manager_->onData(fake_input, false); // kick off request } -TEST_F(HttpConnectionManagerImplTest, StreamDeletionConfigiredValidly) { +TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationVaildlyConfigured) { max_stream_duration_ = std::chrono::milliseconds(10); setup(false, ""); @@ -2429,7 +2429,7 @@ TEST_F(HttpConnectionManagerImplTest, StreamDeletionConfigiredValidly) { conn_manager_->onData(fake_input, false); // kick off request } -TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { +TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationCallbackResetStream) { max_stream_duration_ = std::chrono::milliseconds(10); setup(false, ""); Event::MockTimer* duration_timer = setUpTimer(); @@ -2449,7 +2449,7 @@ TEST_F(HttpConnectionManagerImplTest, StreamAliveDurationExpired) { EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); } -TEST_F(HttpConnectionManagerImplTest, NotInvokeDeferredStreamDeletion) { +TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationCallbackNotCalledIfResetStreamValidly) { max_stream_duration_ = std::chrono::milliseconds(5000); setup(false, ""); Event::MockTimer* duration_timer = setUpTimer(); From 292cab9ccd3e23839201f8299d6292a954ed42b6 Mon Sep 17 00:00:00 2001 From: shikugawa Date: Thu, 19 Mar 2020 17:42:32 +0000 Subject: [PATCH 13/13] typo fix Signed-off-by: shikugawa --- test/common/http/conn_manager_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index f2b566eb624c6..c7890bcedbb53 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2414,7 +2414,7 @@ TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationDisabledIfSetToZero) { conn_manager_->onData(fake_input, false); // kick off request } -TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationVaildlyConfigured) { +TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationValidlyConfigured) { max_stream_duration_ = std::chrono::milliseconds(10); setup(false, "");