From f632f69ac2336c32a0bdc48a09a50010ff484ae5 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 30 Nov 2021 17:35:52 -0500 Subject: [PATCH 1/6] fix hcm Signed-off-by: Dan Zhang --- source/common/http/conn_manager_impl.cc | 1 + source/common/http/filter_manager.cc | 16 ++- source/common/http/filter_manager.h | 6 +- test/common/http/conn_manager_impl_test_2.cc | 122 +++++++++++++++++++ 4 files changed, 143 insertions(+), 2 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d9c654a596339..36fcef49a1db2 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1531,6 +1531,7 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason reset_ filter_manager_.streamInfo().setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().Overload); } + filter_manager_.onDownstreamReset(); connection_manager_.doDeferredStreamDestroy(*this); } diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index c49c17829323a..fbd595148414f 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1013,11 +1013,16 @@ void FilterManager::sendDirectLocalReply( state_.non_100_response_headers_encoded_ = true; filter_manager_callbacks_.encodeHeaders(*filter_manager_callbacks_.responseHeaders(), end_stream); - + if (state_.saw_downstream_reset_) { + return; + } maybeEndEncode(end_stream); }, [&](Buffer::Instance& data, bool end_stream) -> void { filter_manager_callbacks_.encodeData(data, end_stream); + if (state_.saw_downstream_reset_) { + return; + } maybeEndEncode(end_stream); }}, Utility::LocalReplyData{state_.is_grpc_request_, code, body, grpc_status, is_head_request}); @@ -1151,6 +1156,9 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); state_.non_100_response_headers_encoded_ = true; filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); + if (state_.saw_downstream_reset_) { + return; + } maybeEndEncode(modified_end_stream); if (!modified_end_stream) { @@ -1290,6 +1298,9 @@ void FilterManager::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instan const bool modified_end_stream = end_stream && trailers_added_entry == encoder_filters_.end(); filter_manager_callbacks_.encodeData(data, modified_end_stream); + if (state_.saw_downstream_reset_) { + return; + } maybeEndEncode(modified_end_stream); // If trailers were adding during encodeData we need to trigger decodeTrailers in order @@ -1332,6 +1343,9 @@ void FilterManager::encodeTrailers(ActiveStreamEncoderFilter* filter, } filter_manager_callbacks_.encodeTrailers(trailers); + if (state_.saw_downstream_reset_) { + return; + } maybeEndEncode(true); } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 758d046e249d0..fb43d5e6cf189 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -945,6 +945,8 @@ class FilterManager : public ScopeTrackedObject, void contextOnContinue(ScopeTrackedObjectStack& tracked_object_stack); + void onDownstreamReset() { state_.saw_downstream_reset_ = true; } + private: // Indicates which filter to start the iteration with. enum class FilterIterationStartState { AlwaysStartFromNext, CanStartFromCurrent }; @@ -1065,7 +1067,8 @@ class FilterManager : public ScopeTrackedObject, : remote_complete_(false), local_complete_(false), has_1xx_headers_(false), created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), non_100_response_headers_encoded_(false), under_on_local_reply_(false), - decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false) {} + decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false), + saw_downstream_reset_(false) {} uint32_t filter_call_state_{0}; @@ -1088,6 +1091,7 @@ class FilterManager : public ScopeTrackedObject, // True when the filter chain iteration was aborted with local reply. bool decoder_filter_chain_aborted_ : 1; bool encoder_filter_chain_aborted_ : 1; + bool saw_downstream_reset_ : 1; // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 44bd0db57ddec..702fda7194226 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -2848,5 +2848,127 @@ TEST_F(HttpConnectionManagerImplTest, RequestRejectedViaIPDetection) { EXPECT_EQ(1U, stats_.named_.downstream_rq_rejected_via_ip_detection_.value()); } +TEST_F(HttpConnectionManagerImplTest, DisconnectDuringEncodeHeader) { + setup(false, "envoy-server-test"); + setupFilterChain(1, 0); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + startRequest(/*end_stream=*/true); + + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + EXPECT_NE(nullptr, headers.Server()); + EXPECT_EQ("envoy-server-test", headers.getServerValue()); + conn_manager_->onEvent(Network::ConnectionEvent::LocalClose); + })); + EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); + EXPECT_CALL(*decoder_filters_[0], onDestroy()); + + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails(""); + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), true, "details"); +} + +TEST_F(HttpConnectionManagerImplTest, DisconnectDuringEncodeBody) { + setup(false, "envoy-server-test"); + setupFilterChain(1, 0); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + startRequest(/*end_stream=*/true); + + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + EXPECT_NE(nullptr, headers.Server()); + EXPECT_EQ("envoy-server-test", headers.getServerValue()); + })); + EXPECT_CALL(response_encoder_, encodeData(_, true)) + .WillOnce(Invoke([&](Buffer::Instance&, bool) -> void { + conn_manager_->onEvent(Network::ConnectionEvent::LocalClose); + })); + EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); + EXPECT_CALL(*decoder_filters_[0], onDestroy()); + + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails(""); + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false, "details"); + Buffer::OwnedImpl response_body("response"); + decoder_filters_[0]->callbacks_->encodeData(response_body, true); +} + +TEST_F(HttpConnectionManagerImplTest, DisconnectDuringEncodeTrailer) { + setup(false, "envoy-server-test"); + setupFilterChain(1, 0); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + startRequest(/*end_stream=*/true); + + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + EXPECT_NE(nullptr, headers.Server()); + EXPECT_EQ("envoy-server-test", headers.getServerValue()); + })); + EXPECT_CALL(response_encoder_, encodeData(_, false)); + EXPECT_CALL(response_encoder_, encodeTrailers(_)) + .WillOnce(Invoke([&](const Http::ResponseTrailerMap&) -> void { + conn_manager_->onEvent(Network::ConnectionEvent::LocalClose); + })); + EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); + EXPECT_CALL(*decoder_filters_[0], onDestroy()); + + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails(""); + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false, "details"); + Buffer::OwnedImpl response_body("response"); + decoder_filters_[0]->callbacks_->encodeData(response_body, false); + decoder_filters_[0]->callbacks_->encodeTrailers( + ResponseTrailerMapPtr{new TestResponseTrailerMapImpl{{"some", "trailer"}}}); +} + +TEST_F(HttpConnectionManagerImplTest, DirectLocalReplyCausesDisconnect) { + initial_buffer_limit_ = 10; + setup(false, ""); + setUpEncoderAndDecoder(false, false); + sendRequestHeadersAndData(); + + // Start the response without processing the request headers through all + // filters. + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails(""); + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false, "details"); + + // Now overload the buffer with response data. The filter returns + // StopIterationAndBuffer, which will trigger an early response. + + expectOnDestroy(); + Buffer::OwnedImpl fake_response("A long enough string to go over watermarks"); + // Fake response starts doing through the filter. + EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) + .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); + std::string response_body; + // The 500 goes directly to the encoder. + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> FilterHeadersStatus { + // Make sure this is a 500 + EXPECT_EQ("500", headers.getStatusValue()); + // Make sure Envoy standard sanitization has been applied. + EXPECT_TRUE(headers.Date() != nullptr); + EXPECT_EQ("response_payload_too_large", + decoder_filters_[0]->callbacks_->streamInfo().responseCodeDetails().value()); + return FilterHeadersStatus::Continue; + })); + EXPECT_CALL(response_encoder_, encodeData(_, true)) + .WillOnce(Invoke([&](Buffer::Instance&, bool) -> void { + conn_manager_->onEvent(Network::ConnectionEvent::LocalClose); + })); + decoder_filters_[0]->callbacks_->encodeData(fake_response, false); + + EXPECT_EQ(1U, stats_.named_.rs_too_large_.value()); +} + } // namespace Http } // namespace Envoy From 14cdfe54867827c3e848bdb023a16efbdf7f9cf1 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 30 Nov 2021 18:22:29 -0500 Subject: [PATCH 2/6] add runtime guard Signed-off-by: Dan Zhang --- source/common/http/conn_manager_impl.cc | 5 ++++- source/common/runtime/runtime_features.cc | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 36fcef49a1db2..81910a2876d06 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1531,7 +1531,10 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason reset_ filter_manager_.streamInfo().setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().Overload); } - filter_manager_.onDownstreamReset(); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.handle_stream_reset_during_hcm_encoding")) { + filter_manager_.onDownstreamReset(); + } connection_manager_.doDeferredStreamDestroy(*this); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index c2559c63db5be..70d9c766a76a6 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -62,6 +62,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.fix_added_trailers", "envoy.reloadable_features.grpc_bridge_stats_disabled", + "envoy.reloadable_features.handle_stream_reset_during_hcm_encoding", "envoy.reloadable_features.hash_multiple_header_values", "envoy.reloadable_features.health_check.graceful_goaway_handling", "envoy.reloadable_features.http2_consume_stream_refused_errors", From d7b4ce439951fad03e03f76c28ab9f922ddc5b48 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 30 Nov 2021 18:28:35 -0500 Subject: [PATCH 3/6] release note Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0830081fae39c..938cbba81bd9b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -26,6 +26,7 @@ Bug Fixes *Changes expected to improve the state of the world and are unlikely to have negative effects* * ext_authz: fix the ext_authz network filter to correctly set response flag and code details to ``UAEX`` when a connection is denied. +* hcm: stop processing the response if encoding it has caused downstream reset. The fix is guarded by ``envoy.reloadable_features.handle_stream_reset_during_hcm_encoding``. * listener: fixed the crash when updating listeners that do not bind to port. * tcp: fixed a bug where upstream circuit breakers applied HTTP per-request bounds to TCP connections. * thrift_proxy: fix the thrift_proxy connection manager to correctly report success/error response metrics when performing :ref:`payload passthrough `. From 6e1ed4b81f319509c608db970267dbdc7df48c27 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 2 Dec 2021 22:32:47 -0500 Subject: [PATCH 4/6] socket fail test Signed-off-by: Dan Zhang --- test/integration/protocol_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 109e84dde5b66..647adc11043c1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -3557,7 +3557,7 @@ TEST_P(DownstreamProtocolIntegrationTest, HandleSocketFail) { socket_swap.write_matcher_->setSourcePort(lookupPort("http")); socket_swap.write_matcher_->setWriteOverride(ebadf); // TODO(danzh) set to true. - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); if (downstreamProtocol() == Http::CodecType::HTTP3) { // For HTTP/3 since the packets are black holed, there is no client side From ab67af1ac05b59bac56d7fd34c0739d862c24a4d Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Dec 2021 11:43:58 -0500 Subject: [PATCH 5/6] release note Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 1057a23656926..3e34a2c90e2ca 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -28,8 +28,6 @@ Bug Fixes * ext_authz: fix the ext_authz http filter to correctly set response flags to ``UAEX`` when a connection is denied. * ext_authz: fix the ext_authz network filter to correctly set response flag and code details to ``UAEX`` when a connection is denied. * hcm: stop processing the response if encoding it has caused downstream reset. The fix is guarded by ``envoy.reloadable_features.handle_stream_reset_during_hcm_encoding``. -* listener: fixed the crash when updating listeners that do not bind to port. -* tcp: fixed a bug where upstream circuit breakers applied HTTP per-request bounds to TCP connections. * listener: fixed issue where more than one listener could listen on the same port if using reuse port, thus randomly accepting connections on different listeners. This configuration is now rejected. * thrift_proxy: do not close downstream connections when an upstream connection overflow happens. * thrift_proxy: fix the thrift_proxy connection manager to correctly report success/error response metrics when performing :ref:`payload passthrough `. From 1b9d79e5f05fc1b823ed63fa22ff29bdca49f31e Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 6 Dec 2021 12:19:14 -0500 Subject: [PATCH 6/6] remove TODO Signed-off-by: Dan Zhang --- test/integration/protocol_integration_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 647adc11043c1..3c9e9b8d4be79 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -3556,7 +3556,6 @@ TEST_P(DownstreamProtocolIntegrationTest, HandleSocketFail) { Network::IoSocketError* ebadf = Network::IoSocketError::getIoSocketEbadfInstance(); socket_swap.write_matcher_->setSourcePort(lookupPort("http")); socket_swap.write_matcher_->setWriteOverride(ebadf); - // TODO(danzh) set to true. upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); if (downstreamProtocol() == Http::CodecType::HTTP3) {