diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index c64e0837266de..c4944ca29f1df 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -162,6 +162,10 @@ struct ResponseCodeDetailValues { const std::string FilterChainNotFound = "filter_chain_not_found"; // The client disconnected unexpectedly. const std::string DownstreamRemoteDisconnect = "downstream_remote_disconnect"; + // The client connection was locally closed for an unspecified reason. + const std::string DownstreamLocalDisconnect = "downstream_local_disconnect"; + // The max connection duration was exceeded. + const std::string DurationTimeout = "duration_timeout"; // The response was generated by the admin filter. const std::string AdminFilterResponse = "admin_filter_response"; // The original stream was replaced with an internal redirect. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 5d82151074f4b..1bc13a8a85236 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -182,7 +182,8 @@ ConnectionManagerImpl::~ConnectionManagerImpl() { void ConnectionManagerImpl::checkForDeferredClose() { if (drain_state_ == DrainState::Closing && streams_.empty() && !codec_->wantsToWrite()) { - doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay, absl::nullopt); + doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().DownstreamLocalDisconnect); } } @@ -263,15 +264,14 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod void ConnectionManagerImpl::handleCodecError(absl::string_view error) { ENVOY_CONN_LOG(debug, "dispatch error: {}", read_callbacks_->connection(), error); - read_callbacks_->connection().streamInfo().setResponseCodeDetails( - absl::StrCat("codec error: ", error)); read_callbacks_->connection().streamInfo().setResponseFlag( StreamInfo::ResponseFlag::DownstreamProtocolError); // HTTP/1.1 codec has already sent a 400 response if possible. HTTP/2 codec has already sent // GOAWAY. doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay, - StreamInfo::ResponseFlag::DownstreamProtocolError); + StreamInfo::ResponseFlag::DownstreamProtocolError, + absl::StrCat("codec error: ", error)); } void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { @@ -354,8 +354,8 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { return Network::FilterStatus::StopIteration; } -void ConnectionManagerImpl::resetAllStreams( - absl::optional response_flag) { +void ConnectionManagerImpl::resetAllStreams(absl::optional response_flag, + absl::string_view details) { while (!streams_.empty()) { // Mimic a downstream reset in this case. We must also remove callbacks here. Though we are // about to close the connection and will disable further reads, it is possible that flushing @@ -367,23 +367,16 @@ void ConnectionManagerImpl::resetAllStreams( // codec but there are no easy answers and this seems simpler. auto& stream = *streams_.front(); stream.response_encoder_->getStream().removeCallbacks(stream); - stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view()); + if (!stream.response_encoder_->getStream().responseDetails().empty()) { + stream.filter_manager_.streamInfo().setResponseCodeDetails( + stream.response_encoder_->getStream().responseDetails()); + } else if (!details.empty()) { + stream.filter_manager_.streamInfo().setResponseCodeDetails(details); + } if (response_flag.has_value()) { - // This code duplicates some of the logic in - // onResetStream(). There seems to be no easy way to force - // onResetStream to do the right thing within its current API. - // Encoding DownstreamProtocolError as reason==LocalReset does - // not work because local reset is generated in other places. - // Encoding it in the string_view argument would lead to a hack - // of the form: if parameter is nonempty, use that; else if the - // codec details are nonempty, use those. This hack does not - // seem better than the code duplication, so punt for now. stream.filter_manager_.streamInfo().setResponseFlag(response_flag.value()); - if (*response_flag == StreamInfo::ResponseFlag::DownstreamProtocolError) { - stream.filter_manager_.streamInfo().setResponseCodeDetails( - stream.response_encoder_->getStream().responseDetails()); - } } + stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view()); } } @@ -398,6 +391,10 @@ void ConnectionManagerImpl::onEvent(Network::ConnectionEvent event) { remote_close_ = true; stats_.named_.downstream_cx_destroy_remote_.inc(); } + absl::string_view details = + event == Network::ConnectionEvent::RemoteClose + ? StreamInfo::ResponseCodeDetails::get().DownstreamRemoteDisconnect + : StreamInfo::ResponseCodeDetails::get().DownstreamLocalDisconnect; // TODO(mattklein123): It is technically possible that something outside of the filter causes // a local connection close, so we still guard against that here. A better solution would be to // have some type of "pre-close" callback that we could hook for cleanup that would get called @@ -407,13 +404,13 @@ void ConnectionManagerImpl::onEvent(Network::ConnectionEvent event) { // NOTE: In the case where a local close comes from outside the filter, this will cause any // stream closures to increment remote close stats. We should do better here in the future, // via the pre-close callback mentioned above. - doConnectionClose(absl::nullopt, absl::nullopt); + doConnectionClose(absl::nullopt, absl::nullopt, details); } } void ConnectionManagerImpl::doConnectionClose( absl::optional close_type, - absl::optional response_flag) { + absl::optional response_flag, absl::string_view details) { if (connection_idle_timer_) { connection_idle_timer_->disableTimer(); connection_idle_timer_.reset(); @@ -445,7 +442,7 @@ void ConnectionManagerImpl::doConnectionClose( // Note that resetAllStreams() does not actually write anything to the wire. It just resets // all upstream streams and their filter stacks. Thus, there are no issues around recursive // entry. - resetAllStreams(response_flag); + resetAllStreams(response_flag, details); } if (close_type.has_value()) { @@ -464,7 +461,7 @@ void ConnectionManagerImpl::onIdleTimeout() { if (!codec_) { // No need to delay close after flushing since an idle timeout has already fired. Attempt to // write out buffered data one last time and issue a local close if successful. - doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt); + doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt, ""); } else if (drain_state_ == DrainState::NotDraining) { startDrainSequence(); } @@ -475,7 +472,8 @@ void ConnectionManagerImpl::onConnectionDurationTimeout() { stats_.named_.downstream_cx_max_duration_reached_.inc(); if (!codec_) { // Attempt to write out buffered data one last time and issue a local close if successful. - doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt); + doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().DurationTimeout); } else if (drain_state_ == DrainState::NotDraining) { startDrainSequence(); } @@ -633,7 +631,6 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { filter_manager_.streamInfo().setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().DownstreamRemoteDisconnect); } - if (connection_manager_.codec_->protocol() < Protocol::Http2) { // For HTTP/2 there are still some reset cases where details are not set. // For HTTP/1 there shouldn't be any. Regression-proof this. @@ -2176,7 +2173,6 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl: // If we need to differentiate we need to do it inside the codec. Can start with this. ENVOY_STREAM_LOG(debug, "stream reset", *this); connection_manager_.stats_.named_.downstream_rq_rx_reset_.inc(); - connection_manager_.doDeferredStreamDestroy(*this); // If the codec sets its responseDetails(), impute a // DownstreamProtocolError and propagate the details upwards. @@ -2185,6 +2181,8 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl: filter_manager_.streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); filter_manager_.streamInfo().setResponseCodeDetails(encoder_details); } + + connection_manager_.doDeferredStreamDestroy(*this); } void ConnectionManagerImpl::ActiveStream::onAboveWriteBufferHighWatermark() { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index e453d0271df0d..e3c9d40a10839 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -1007,7 +1007,8 @@ class ConnectionManagerImpl : Logger::Loggable, */ void doEndStream(ActiveStream& stream); - void resetAllStreams(absl::optional response_flag); + void resetAllStreams(absl::optional response_flag, + absl::string_view details); void onIdleTimeout(); void onConnectionDurationTimeout(); void onDrainTimeout(); @@ -1015,7 +1016,8 @@ class ConnectionManagerImpl : Logger::Loggable, Tracing::HttpTracer& tracer() { return *config_.tracer(); } void handleCodecError(absl::string_view error); void doConnectionClose(absl::optional close_type, - absl::optional response_flag); + absl::optional response_flag, + absl::string_view details); enum class DrainState { NotDraining, Draining, Closing }; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index af6780154c2c7..9c7654f4869f4 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -3665,7 +3665,9 @@ TEST_F(HttpConnectionManagerImplTest, TestDownstreamProtocolErrorAfterHeadersAcc // Verify that FrameFloodException causes connection to be closed abortively. TEST_F(HttpConnectionManagerImplTest, FrameFloodError) { - InSequence s; + std::shared_ptr log_handler = + std::make_shared>(); + access_logs_ = {log_handler}; setup(false, ""); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status { @@ -3680,14 +3682,20 @@ TEST_F(HttpConnectionManagerImplTest, FrameFloodError) { EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWriteAndDelay)); + EXPECT_CALL(*log_handler, log(_, _, _, _)) + .WillOnce(Invoke([](const HeaderMap*, const HeaderMap*, const HeaderMap*, + const StreamInfo::StreamInfo& stream_info) { + ASSERT_TRUE(stream_info.responseCodeDetails().has_value()); + EXPECT_EQ("codec error: too many outbound frames.", + stream_info.responseCodeDetails().value()); + })); // Kick off the incoming data. Buffer::OwnedImpl fake_input("1234"); EXPECT_LOG_NOT_CONTAINS("warning", "downstream HTTP flood", conn_manager_->onData(fake_input, false)); + EXPECT_TRUE(filter_callbacks_.connection_.streamInfo().hasResponseFlag( StreamInfo::ResponseFlag::DownstreamProtocolError)); - EXPECT_EQ("codec error: too many outbound frames.", - filter_callbacks_.connection_.streamInfo().responseCodeDetails().value()); } TEST_F(HttpConnectionManagerImplTest, IdleTimeoutNoCodec) {