diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 1445494868d3..72a08094ee47 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -21,6 +21,7 @@ HTTP connection manager "http_codec_options": "...", "server_name": "...", "idle_timeout_s": "...", + "drain_timeout_ms": "...", "access_log": [], "use_remote_address": "..." } @@ -100,8 +101,19 @@ idle_timeout_s *(optional, integer)* The idle timeout in seconds for connections managed by the connection manager. The idle timeout is defined as the period in which there are no active requests. If not set, there is no idle timeout. When the idle timeout is reached the connection will be closed. If - the connection is an HTTP/2 connection a GOAWAY frame will be sent prior to closing - the connection. + the connection is an HTTP/2 connection a drain sequence will occur prior to closing the + connection. See :ref:`drain_timeout_s `. + +.. _config_http_conn_man_drain_timeout_ms: + +drain_timeout_ms + *(optional, integer)* The time in milliseconds 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 race with the final GOAWAY frame. + During this grace period, Envoy will continue to accept new streams. After the grace period, a + final GOAWAY frame is sent and Envoy will start refusing new streams. Draining occurs both + when a connection hits the idle timeout or during general server draining. The default grace + period is 5000 milliseconds (5 seconds) if this option is not specified. :ref:`access_log ` *(optional, array)* Configuration for :ref:`HTTP access logs ` @@ -111,7 +123,7 @@ idle_timeout_s use_remote_address *(optional, boolean)* If set to true, the connection manager will use the real remote address - of the client connection when determining internal versus external origin and manipulating + of the client connection when determining internal versus external origin and manipulating various headers. If set to false or absent, the connection manager will use the :ref:`config_http_conn_man_headers_x-forwarded-for` HTTP header. See the documentation for :ref:`config_http_conn_man_headers_x-forwarded-for`, diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index dab849b40848..619d04d7f06e 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -188,6 +188,12 @@ class Connection { */ virtual const std::string& protocolString() PURE; + /** + * Indicate a "shutdown notice" to the remote. This is a hint that the remote should not send + * any new streams, but if streams do arrive that will not be reset. + */ + virtual void shutdownNotice() PURE; + /** * @return bool whether the codec has data that it wants to write but cannot due to protocol * reasons (e.g, needing window updates). diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 4da0dfca7db7..6826f48d930d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -65,10 +65,10 @@ ConnectionManagerImpl::~ConnectionManagerImpl() { } if (codec_) { - if (codec_->protocolString() == Http::Http1::PROTOCOL_STRING) { + if (codec_->protocolString() == Http1::PROTOCOL_STRING) { config_.stats().named_.downstream_cx_http1_active_.dec(); } else { - ASSERT(codec_->protocolString() == Http::Http2::PROTOCOL_STRING); + ASSERT(codec_->protocolString() == Http2::PROTOCOL_STRING); config_.stats().named_.downstream_cx_http2_active_.dec(); } } @@ -78,7 +78,7 @@ ConnectionManagerImpl::~ConnectionManagerImpl() { } void ConnectionManagerImpl::checkForDeferredClose() { - if (close_when_drained_ && streams_.empty() && !codec_->wantsToWrite()) { + if (drain_state_ == DrainState::Closing && streams_.empty() && !codec_->wantsToWrite()) { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); } } @@ -104,14 +104,14 @@ void ConnectionManagerImpl::destroyStream(ActiveStream& stream) { std::move(stream.removeFromList(streams_))); } - if (reset_stream && !(codec_->features() & Http::CodecFeatures::Multiplexing)) { - close_when_drained_ = true; + if (reset_stream && !(codec_->features() & CodecFeatures::Multiplexing)) { + drain_state_ = DrainState::Closing; } checkForDeferredClose(); // Reading may have been disabled for the non-multiplexing case, so enable it again. - if (!close_when_drained_ && !(codec_->features() & Http::CodecFeatures::Multiplexing) && + if (drain_state_ != DrainState::Closing && !(codec_->features() & CodecFeatures::Multiplexing) && !read_callbacks_->connection().readEnabled()) { read_callbacks_->connection().readDisable(false); } @@ -121,7 +121,7 @@ void ConnectionManagerImpl::destroyStream(ActiveStream& stream) { } } -Http::StreamDecoder& ConnectionManagerImpl::newStream(Http::StreamEncoder& response_encoder) { +StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder) { if (idle_timer_) { idle_timer_->disableTimer(); } @@ -138,11 +138,11 @@ Http::StreamDecoder& ConnectionManagerImpl::newStream(Http::StreamEncoder& respo Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data) { if (!codec_) { codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); - if (codec_->protocolString() == Http::Http1::PROTOCOL_STRING) { + if (codec_->protocolString() == Http1::PROTOCOL_STRING) { config_.stats().named_.downstream_cx_http1_total_.inc(); config_.stats().named_.downstream_cx_http1_active_.inc(); } else { - ASSERT(codec_->protocolString() == Http::Http2::PROTOCOL_STRING); + ASSERT(codec_->protocolString() == Http2::PROTOCOL_STRING); config_.stats().named_.downstream_cx_http2_total_.inc(); config_.stats().named_.downstream_cx_http2_active_.inc(); } @@ -168,7 +168,7 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data) { // The HTTP/1.1 codec will pause dispatch after a single message is complete. We want to // either redispatch if there are no streams and we have more data, or if we have a single // complete stream but have not responded yet we will pause socket reads to apply back pressure. - if (!(codec_->features() & Http::CodecFeatures::Multiplexing)) { + if (!(codec_->features() & CodecFeatures::Multiplexing)) { if (read_callbacks_->connection().state() == Network::Connection::State::Open && data.length() > 0 && streams_.empty()) { redispatch = true; @@ -207,6 +207,11 @@ void ConnectionManagerImpl::onEvent(uint32_t events) { idle_timer_->disableTimer(); idle_timer_.reset(); } + + if (drain_timer_) { + drain_timer_->disableTimer(); + drain_timer_.reset(); + } } if (!streams_.empty()) { @@ -235,11 +240,18 @@ void ConnectionManagerImpl::onGoAway() { void ConnectionManagerImpl::onIdleTimeout() { conn_log_debug("idle timeout", read_callbacks_->connection()); config_.stats().named_.downstream_cx_idle_timeout_.inc(); - if (codec_) { - codec_->goAway(); + if (!codec_) { + read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); + } else if (drain_state_ == DrainState::NotDraining) { + startDrainSequence(); } +} - read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); +void ConnectionManagerImpl::onDrainTimeout() { + ASSERT(drain_state_ != DrainState::NotDraining); + codec_->goAway(); + drain_state_ = DrainState::Closing; + checkForDeferredClose(); } DateFormatter ConnectionManagerImpl::ActiveStream::date_formatter_("%a, %d %b %Y %H:%M:%S GMT"); @@ -251,17 +263,17 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect request_info_(connection_manager_.codec_->protocolString()) { connection_manager_.config_.stats().named_.downstream_rq_total_.inc(); connection_manager_.config_.stats().named_.downstream_rq_active_.inc(); - if (connection_manager_.codec_->protocolString() == Http::Http1::PROTOCOL_STRING) { + if (connection_manager_.codec_->protocolString() == Http1::PROTOCOL_STRING) { connection_manager_.config_.stats().named_.downstream_rq_http1_total_.inc(); } else { - ASSERT(connection_manager_.codec_->protocolString() == Http::Http2::PROTOCOL_STRING); + ASSERT(connection_manager_.codec_->protocolString() == Http2::PROTOCOL_STRING); connection_manager_.config_.stats().named_.downstream_rq_http2_total_.inc(); } } ConnectionManagerImpl::ActiveStream::~ActiveStream() { connection_manager_.config_.stats().named_.downstream_rq_active_.dec(); - for (Http::AccessLog::InstancePtr access_log : connection_manager_.config_.accessLogs()) { + for (AccessLog::InstancePtr access_log : connection_manager_.config_.accessLogs()) { access_log->log(request_headers_.get(), response_headers_.get(), request_info_); } @@ -271,26 +283,24 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { } } -void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilter( - Http::StreamDecoderFilterPtr filter) { +void ConnectionManagerImpl::ActiveStream::addStreamDecoderFilter(StreamDecoderFilterPtr filter) { ActiveStreamDecoderFilterPtr wrapper(new ActiveStreamDecoderFilter(*this, filter)); filter->setDecoderFilterCallbacks(*wrapper); wrapper->moveIntoListBack(std::move(wrapper), decoder_filters_); } -void ConnectionManagerImpl::ActiveStream::addStreamEncoderFilter( - Http::StreamEncoderFilterPtr filter) { +void ConnectionManagerImpl::ActiveStream::addStreamEncoderFilter(StreamEncoderFilterPtr filter) { ActiveStreamEncoderFilterPtr wrapper(new ActiveStreamEncoderFilter(*this, filter)); filter->setEncoderFilterCallbacks(*wrapper); wrapper->moveIntoListBack(std::move(wrapper), encoder_filters_); } -void ConnectionManagerImpl::ActiveStream::addStreamFilter(Http::StreamFilterPtr filter) { +void ConnectionManagerImpl::ActiveStream::addStreamFilter(StreamFilterPtr filter) { addStreamDecoderFilter(filter); addStreamEncoderFilter(filter); } -void ConnectionManagerImpl::ActiveStream::chargeStats(Http::HeaderMap& headers) { +void ConnectionManagerImpl::ActiveStream::chargeStats(HeaderMap& headers) { uint64_t response_code = Utility::getResponseStatus(headers); request_info_.response_code_.value(response_code); @@ -313,8 +323,7 @@ uint64_t ConnectionManagerImpl::ActiveStream::connectionId() { return connection_manager_.read_callbacks_->connection().id(); } -void ConnectionManagerImpl::ActiveStream::decodeHeaders(Http::HeaderMapPtr&& headers, - bool end_stream) { +void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { ASSERT(!state_.remote_complete_); state_.remote_complete_ = end_stream; @@ -334,15 +343,14 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(Http::HeaderMapPtr&& hea const std::string& codec_version = request_headers_->get(Headers::get().Version); if (!(codec_version == "HTTP/1.1" || codec_version == "HTTP/2")) { HeaderMapImpl headers{ - {Headers::get().Status, std::to_string(enumToInt(Http::Code::UpgradeRequired))}}; + {Headers::get().Status, std::to_string(enumToInt(Code::UpgradeRequired))}}; encodeHeaders(nullptr, headers, true); return; } // Require host header. For HTTP/1.1 Host has already been translated to :host. if (request_headers_->get(Headers::get().Host).empty()) { - HeaderMapImpl headers{ - {Headers::get().Status, std::to_string(enumToInt(Http::Code::BadRequest))}}; + HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::BadRequest))}}; encodeHeaders(nullptr, headers, true); return; } @@ -358,8 +366,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(Http::HeaderMapPtr&& hea // here and keep it under 60K. Ultimately it would be nice to bring this to a lower value but // unclear if that is possible or not. if (request_headers_->byteSize() > (60 * 1024)) { - HeaderMapImpl headers{ - {Headers::get().Status, std::to_string(enumToInt(Http::Code::BadRequest))}}; + HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::BadRequest))}}; encodeHeaders(nullptr, headers, true); return; } @@ -370,7 +377,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(Http::HeaderMapPtr&& hea // https://tools.ietf.org/html/rfc7230#section-5.3 if (request_headers_->get(Headers::get().Path).find('/') != 0) { connection_manager_.config_.stats().named_.downstream_rq_non_relative_path_.inc(); - HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Http::Code::NotFound))}}; + HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::NotFound))}}; encodeHeaders(nullptr, headers, true); return; } @@ -384,7 +391,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(Http::HeaderMapPtr&& hea } void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilter* filter, - Http::HeaderMap& headers, bool end_stream) { + HeaderMap& headers, bool end_stream) { std::list::iterator entry; if (!filter) { entry = decoder_filters_.begin(); @@ -393,7 +400,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte } for (; entry != decoder_filters_.end(); entry++) { - Http::FilterHeadersStatus status = (*entry)->handle_->decodeHeaders(headers, end_stream); + FilterHeadersStatus status = (*entry)->handle_->decodeHeaders(headers, end_stream); stream_log_trace("decode headers called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); if (!(*entry)->commonHandleAfterHeadersCallback(status)) { @@ -433,7 +440,7 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter* } for (; entry != decoder_filters_.end(); entry++) { - Http::FilterDataStatus status = (*entry)->handle_->decodeData(data, end_stream); + FilterDataStatus status = (*entry)->handle_->decodeData(data, end_stream); stream_log_trace("decode data called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); if (!(*entry)->commonHandleAfterDataCallback(status, data)) { @@ -464,7 +471,7 @@ void ConnectionManagerImpl::ActiveStream::decodeTrailers(ActiveStreamDecoderFilt } for (; entry != decoder_filters_.end(); entry++) { - Http::FilterTrailersStatus status = (*entry)->handle_->decodeTrailers(trailers); + FilterTrailersStatus status = (*entry)->handle_->decodeTrailers(trailers); stream_log_trace("decode trailers called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); if (!(*entry)->commonHandleAfterTrailersCallback(status)) { @@ -490,11 +497,20 @@ ConnectionManagerImpl::ActiveStream::commonEncodePrefix(ActiveStreamEncoderFilte } } +void ConnectionManagerImpl::startDrainSequence() { + ASSERT(drain_state_ == DrainState::NotDraining); + drain_state_ = DrainState::Draining; + codec_->shutdownNotice(); + drain_timer_ = read_callbacks_->connection().dispatcher().createTimer( + [this]() -> void { onDrainTimeout(); }); + drain_timer_->enableTimer(config_.drainTimeout()); +} + void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilter* filter, - Http::HeaderMap& headers, bool end_stream) { + HeaderMap& headers, bool end_stream) { std::list::iterator entry = commonEncodePrefix(filter, end_stream); for (; entry != encoder_filters_.end(); entry++) { - Http::FilterHeadersStatus status = (*entry)->handle_->encodeHeaders(headers, end_stream); + FilterHeadersStatus status = (*entry)->handle_->encodeHeaders(headers, end_stream); stream_log_trace("encode headers called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); if (!(*entry)->commonHandleAfterHeadersCallback(status)) { @@ -512,9 +528,13 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte // See if we want to drain/close the connection. Send the go away frame prior to encoding the // header block. - if (!connection_manager_.close_when_drained_ && connection_manager_.drain_close_.drainClose()) { - connection_manager_.codec_->goAway(); - connection_manager_.close_when_drained_ = true; + if (connection_manager_.drain_state_ == DrainState::NotDraining && + connection_manager_.drain_close_.drainClose()) { + + // This doesn't really do anything for HTTP/1.1 other then give the connection another boost + // of time to race with incoming requests. It mainly just keeps the logic the same between + // HTTP/1.1 and HTTP/2. + connection_manager_.startDrainSequence(); connection_manager_.config_.stats().named_.downstream_cx_drain_close_.inc(); stream_log_debug("drain closing connection", *this); } @@ -523,15 +543,15 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte // multiplexing, we should disconnect since we don't want to wait around for the request to // finish. if (!state_.remote_complete_) { - if (!(connection_manager_.codec_->features() & Http::CodecFeatures::Multiplexing)) { - connection_manager_.close_when_drained_ = true; + if (!(connection_manager_.codec_->features() & CodecFeatures::Multiplexing)) { + connection_manager_.drain_state_ = DrainState::Closing; } connection_manager_.config_.stats().named_.downstream_rq_response_before_rq_complete_.inc(); } - if (connection_manager_.close_when_drained_ && - !(connection_manager_.codec_->features() & Http::CodecFeatures::Multiplexing)) { + if (connection_manager_.drain_state_ == DrainState::Closing && + !(connection_manager_.codec_->features() & CodecFeatures::Multiplexing)) { headers.addViaCopy(Headers::get().Connection, Headers::get().ConnectionValues.Close); } @@ -552,7 +572,7 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter* Buffer::Instance& data, bool end_stream) { std::list::iterator entry = commonEncodePrefix(filter, end_stream); for (; entry != encoder_filters_.end(); entry++) { - Http::FilterDataStatus status = (*entry)->handle_->encodeData(data, end_stream); + FilterDataStatus status = (*entry)->handle_->encodeData(data, end_stream); stream_log_trace("encode data called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); if (!(*entry)->commonHandleAfterDataCallback(status, data)) { @@ -572,7 +592,7 @@ void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilt HeaderMap& trailers) { std::list::iterator entry = commonEncodePrefix(filter, true); for (; entry != encoder_filters_.end(); entry++) { - Http::FilterTrailersStatus status = (*entry)->handle_->encodeTrailers(trailers); + FilterTrailersStatus status = (*entry)->handle_->encodeTrailers(trailers); stream_log_trace("encode trailers called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); if (!(*entry)->commonHandleAfterTrailersCallback(status)) { @@ -643,16 +663,16 @@ void ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue() { } bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterHeadersCallback( - Http::FilterHeadersStatus status) { + FilterHeadersStatus status) { ASSERT(!headers_continued_); ASSERT(!stopped_); - if (status == Http::FilterHeadersStatus::StopIteration) { + if (status == FilterHeadersStatus::StopIteration) { stopped_ = true; return false; } else { - ASSERT(status == Http::FilterHeadersStatus::Continue); + ASSERT(status == FilterHeadersStatus::Continue); headers_continued_ = true; return true; } @@ -676,9 +696,9 @@ void ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleBufferData( } bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterDataCallback( - Http::FilterDataStatus status, Buffer::Instance& provided_data) { + FilterDataStatus status, Buffer::Instance& provided_data) { - if (status == Http::FilterDataStatus::Continue) { + if (status == FilterDataStatus::Continue) { if (stopped_) { commonHandleBufferData(provided_data); commonContinue(); @@ -688,7 +708,7 @@ bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterDataCallbac } } else { stopped_ = true; - if (status == Http::FilterDataStatus::StopIterationAndBuffer) { + if (status == FilterDataStatus::StopIterationAndBuffer) { commonHandleBufferData(provided_data); } @@ -699,9 +719,9 @@ bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterDataCallbac } bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterTrailersCallback( - Http::FilterTrailersStatus status) { + FilterTrailersStatus status) { - if (status == Http::FilterTrailersStatus::Continue) { + if (status == FilterTrailersStatus::Continue) { if (stopped_) { commonContinue(); return false; @@ -723,13 +743,13 @@ Event::Dispatcher& ConnectionManagerImpl::ActiveStreamFilterBase::dispatcher() { return parent_.connection_manager_.read_callbacks_->connection().dispatcher(); } -Http::AccessLog::RequestInfo& ConnectionManagerImpl::ActiveStreamFilterBase::requestInfo() { +AccessLog::RequestInfo& ConnectionManagerImpl::ActiveStreamFilterBase::requestInfo() { return parent_.request_info_; } void ConnectionManagerImpl::ActiveStreamDecoderFilter::continueDecoding() { commonContinue(); } -void ConnectionManagerImpl::ActiveStreamDecoderFilter::encodeHeaders(Http::HeaderMapPtr&& headers, +void ConnectionManagerImpl::ActiveStreamDecoderFilter::encodeHeaders(HeaderMapPtr&& headers, bool end_stream) { parent_.response_headers_ = std::move(headers); parent_.encodeHeaders(nullptr, *parent_.response_headers_, end_stream); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 46822c7877e5..a182c8318238 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -101,6 +101,12 @@ class ConnectionManagerConfig { const Buffer::Instance& data, ServerConnectionCallbacks& callbacks) PURE; + /** + * @return the time in milliseconds the connection manager will wait betwen issuing a "shutdown + * notice" to the time it will issue a full GOAWAY and not accept any new streams. + */ + virtual std::chrono::milliseconds drainTimeout() PURE; + /** * @return FilterChainFactory& the HTTP level filter factory to build the connection's filter * chain. @@ -375,19 +381,21 @@ class ConnectionManagerImpl : Logger::Loggable, */ void destroyStream(ActiveStream& stream); - /** - * Connection idle timer event. - */ void onIdleTimeout(); + void onDrainTimeout(); + void startDrainSequence(); + + enum class DrainState { NotDraining, Draining, Closing }; ConnectionManagerConfig& config_; ServerConnectionPtr codec_; std::list streams_; Stats::TimespanPtr conn_length_; Network::DrainDecision& drain_close_; - bool close_when_drained_{}; + DrainState drain_state_{DrainState::NotDraining}; UserAgent user_agent_; Event::TimerPtr idle_timer_; + Event::TimerPtr drain_timer_; Runtime::RandomGenerator& random_generator_; Tracing::HttpTracer& tracer_; Runtime::Loader& runtime_; diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index fdf427b2f5c8..3823818013ea 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -127,6 +127,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(frame->hd.type)); - if (frame->hd.type == NGHTTP2_GOAWAY) { + + // Only raise GOAWAY once, since we don't currently expose stream information. Shutdown + // notifications are the same as a normal GOAWAY. + if (frame->hd.type == NGHTTP2_GOAWAY && !raised_goaway_) { ASSERT(frame->hd.stream_id == 0); + raised_goaway_ = true; callbacks().onGoAway(); return 0; } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 75c4185b5788..fef41e472ff1 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -66,6 +66,7 @@ class ConnectionImpl : public virtual Connection, Logger::Loggable, Http::ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& data, Http::ServerConnectionCallbacks& callbacks) override; + std::chrono::milliseconds drainTimeout() override { return drain_timeout_; } FilterChainFactory& filterFactory() override { return *this; } const Optional& idleTimeout() override { return idle_timeout_; } const Router::Config& routeConfig() override { return *route_config_; } @@ -101,6 +102,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Optional user_agent_; Optional idle_timeout_; Router::ConfigPtr route_config_; + std::chrono::milliseconds drain_timeout_; }; /** diff --git a/source/server/http/admin.h b/source/server/http/admin.h index a72bd5899b55..8ee0135d8d14 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -42,6 +42,7 @@ class AdminImpl : public Admin, Http::ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& data, Http::ServerConnectionCallbacks& callbacks) override; + std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } Http::FilterChainFactory& filterFactory() override { return *this; } const Optional& idleTimeout() override { return idle_timeout_; } const Router::Config& routeConfig() override { return *route_config_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index b78fd9d1a444..4ff3c32bbf59 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -67,6 +67,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi ServerConnectionCallbacks&) override { return ServerConnectionPtr{codec_}; } + std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } FilterChainFactory& filterFactory() override { return filter_factory_; } const Optional& idleTimeout() override { return idle_timeout_; } const Router::Config& routeConfig() override { return route_config_; } @@ -219,10 +220,16 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) { conn_manager_->onData(fake_input); Http::HeaderMapPtr response_headers{new HeaderMapImpl{{":status", "300"}}}; + Event::MockTimer* drain_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*drain_timer, enableTimer(_)); EXPECT_CALL(drain_close_, drainClose()).WillOnce(Return(true)); + EXPECT_CALL(*codec_, shutdownNotice()); + filter->callbacks_->encodeHeaders(std::move(response_headers), true); + EXPECT_CALL(*codec_, goAway()); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); - filter->callbacks_->encodeHeaders(std::move(response_headers), true); + EXPECT_CALL(*drain_timer, disableTimer()); + drain_timer->callback_(); EXPECT_EQ(1U, stats_.named_.downstream_cx_drain_close_.value()); EXPECT_EQ(1U, stats_.named_.downstream_rq_3xx_.value()); @@ -372,6 +379,22 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamProtocolError) { conn_manager_->onData(fake_input); } +TEST_F(HttpConnectionManagerImplTest, IdleTimeoutNoCodec) { + // Not used in the test. + delete codec_; + + idle_timeout_.value(std::chrono::milliseconds(10)); + Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*idle_timer, enableTimer(_)); + setup(false, ""); + + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); + EXPECT_CALL(*idle_timer, disableTimer()); + idle_timer->callback_(); + + EXPECT_EQ(1U, stats_.named_.downstream_cx_idle_timeout_.value()); +} + TEST_F(HttpConnectionManagerImplTest, IdleTimeout) { idle_timeout_.value(std::chrono::milliseconds(10)); Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); @@ -411,10 +434,15 @@ TEST_F(HttpConnectionManagerImplTest, IdleTimeout) { Http::HeaderMapPtr response_headers{new HeaderMapImpl{{":status", "200"}}}; filter->callbacks_->encodeHeaders(std::move(response_headers), true); + Event::MockTimer* drain_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*drain_timer, enableTimer(_)); + idle_timer->callback_(); + EXPECT_CALL(*codec_, goAway()); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); EXPECT_CALL(*idle_timer, disableTimer()); - idle_timer->callback_(); + EXPECT_CALL(*drain_timer, disableTimer()); + drain_timer->callback_(); EXPECT_EQ(1U, stats_.named_.downstream_cx_idle_timeout_.value()); } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 3160ec13588c..243d5facd3a6 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -60,6 +60,32 @@ class Http2CodecImplTest : public testing::TestWithParam { ConnectionWrapper server_wrapper_; }; +TEST_P(Http2CodecImplTest, ShutdownNotice) { + MockStreamDecoder response_decoder; + StreamEncoder& request_encoder = client_.newStream(response_decoder); + + MockStreamDecoder request_decoder; + StreamEncoder* response_encoder; + EXPECT_CALL(server_callbacks_, newStream(_)) + .WillOnce(Invoke([&](StreamEncoder& encoder) -> StreamDecoder& { + response_encoder = &encoder; + return request_decoder; + })); + + HeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder, decodeHeaders_(_, true)); + request_encoder.encodeHeaders(request_headers, true); + + EXPECT_CALL(client_callbacks_, onGoAway()); + server_.shutdownNotice(); + server_.goAway(); + + HeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); + response_encoder->encodeHeaders(response_headers, true); +} + TEST_P(Http2CodecImplTest, RefusedStreamReset) { MockStreamDecoder response_decoder; StreamEncoder& request_encoder = client_.newStream(response_decoder); diff --git a/test/config/integration/server.json b/test/config/integration/server.json index 1501e6a0c9c6..89a80cb5e731 100644 --- a/test/config/integration/server.json +++ b/test/config/integration/server.json @@ -21,6 +21,7 @@ "name": "http_connection_manager", "config": { "codec_type": "http1", + "drain_timeout_ms": 10, "access_log": [ { "path": "/dev/null", @@ -101,6 +102,7 @@ "name": "http_connection_manager", "config": { "codec_type": "http1", + "drain_timeout_ms": 10, "access_log": [ { "path": "/dev/null", diff --git a/test/config/integration/server_http2.json b/test/config/integration/server_http2.json index 678404d6f4f6..d264a960aa67 100644 --- a/test/config/integration/server_http2.json +++ b/test/config/integration/server_http2.json @@ -14,6 +14,7 @@ "name": "http_connection_manager", "config": { "codec_type": "http2", + "drain_timeout_ms": 10, "access_log": [ { "path": "/dev/null", @@ -83,6 +84,7 @@ "name": "http_connection_manager", "config": { "codec_type": "http2", + "drain_timeout_ms": 10, "access_log": [ { "path": "/dev/null", diff --git a/test/config/integration/server_http2_upstream.json b/test/config/integration/server_http2_upstream.json index 5d3d6b2c1634..dbe4ad6a35be 100644 --- a/test/config/integration/server_http2_upstream.json +++ b/test/config/integration/server_http2_upstream.json @@ -8,6 +8,7 @@ "name": "http_connection_manager", "config": { "codec_type": "http2", + "drain_timeout_ms": 10, "access_log": [ { "path": "/dev/null", @@ -77,6 +78,7 @@ "name": "http_connection_manager", "config": { "codec_type": "http2", + "drain_timeout_ms": 10, "access_log": [ { "path": "/dev/null", diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 6de18aa3d0de..da3c3d172ca8 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -63,7 +63,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(accessLogs, const std::list&()); MOCK_METHOD3(createCodec_, ServerConnection*(Network::Connection&, const Buffer::Instance&, ServerConnectionCallbacks&)); - + MOCK_METHOD0(drainTimeout, std::chrono::milliseconds()); MOCK_METHOD0(filterFactory, FilterChainFactory&()); MOCK_METHOD0(idleTimeout, const Optional&()); MOCK_METHOD0(routeConfig, Router::Config&()); @@ -158,6 +158,7 @@ class MockServerConnection : public ServerConnection { MOCK_METHOD0(features, uint64_t()); MOCK_METHOD0(goAway, void()); MOCK_METHOD0(protocolString, const std::string&()); + MOCK_METHOD0(shutdownNotice, void()); MOCK_METHOD0(wantsToWrite, bool()); const std::string protocol_{"HTTP/1.1"}; @@ -173,6 +174,7 @@ class MockClientConnection : public ClientConnection { MOCK_METHOD0(features, uint64_t()); MOCK_METHOD0(goAway, void()); MOCK_METHOD0(protocolString, const std::string&()); + MOCK_METHOD0(shutdownNotice, void()); MOCK_METHOD0(wantsToWrite, bool()); // Http::ClientConnection