From 03d4b1721955c04f8ebb7a8903dfec433149ed5a Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 2 Oct 2020 15:23:32 -0400 Subject: [PATCH] Proactively disconnect connections flooded when encoding headers Signed-off-by: Yan Avlasov --- source/common/http/http2/codec_impl.cc | 5 +- source/common/http/http2/codec_impl.h | 6 +- source/common/http/http2/codec_impl_legacy.cc | 5 +- source/common/http/http2/codec_impl_legacy.h | 6 +- test/common/http/http2/codec_impl_test.cc | 11 +- test/integration/autonomous_upstream.cc | 15 +- test/integration/autonomous_upstream.h | 2 + test/integration/http2_integration_test.cc | 150 +++++++++++++++--- test/integration/http2_integration_test.h | 1 + 9 files changed, 158 insertions(+), 43 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e02f8af85b54..68adc0128603 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -176,6 +176,7 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector // process termination from unhandled exception with the RELEASE_ASSERT. // Further work will replace this RELEASE_ASSERT with proper error handling. RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); + parent_.checkProtocolConstraintViolation(); } void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, @@ -488,7 +489,7 @@ void ConnectionImpl::StreamImpl::encodeDataHelper(Buffer::Instance& data, bool e auto status = parent_.sendPendingFrames(); // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); - parent_.checkProtocolConstrainViolation(); + parent_.checkProtocolConstraintViolation(); if (local_end_stream_ && pending_send_data_.length() > 0) { createPendingFlushTimer(); @@ -1506,7 +1507,7 @@ ServerConnectionImpl::trackOutboundFrames(bool is_outbound_flood_monitored_contr return releasor; } -void ServerConnectionImpl::checkProtocolConstrainViolation() { +void ServerConnectionImpl::checkProtocolConstraintViolation() { if (!protocol_constraints_.checkOutboundFrameLimits().ok()) { scheduleProtocolConstraintViolationCallback(); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index be9cf67a1f7a..72bbcc44e764 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -462,7 +462,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable local_end_stream_ = end_stream; submitHeaders(final_headers, end_stream ? nullptr : &provider); parent_.sendPendingFrames(); + parent_.checkProtocolConstraintViolation(); } void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, @@ -470,7 +471,7 @@ void ConnectionImpl::StreamImpl::encodeDataHelper(Buffer::Instance& data, bool e } parent_.sendPendingFrames(); - parent_.checkProtocolConstrainViolation(); + parent_.checkProtocolConstraintViolation(); if (local_end_stream_ && pending_send_data_.length() > 0) { createPendingFlushTimer(); @@ -1457,7 +1458,7 @@ ServerConnectionImpl::trackOutboundFrames(bool is_outbound_flood_monitored_contr return releasor; } -void ServerConnectionImpl::checkProtocolConstrainViolation() { +void ServerConnectionImpl::checkProtocolConstraintViolation() { if (!protocol_constraints_.checkOutboundFrameLimits().ok()) { scheduleProtocolConstraintViolationCallback(); } diff --git a/source/common/http/http2/codec_impl_legacy.h b/source/common/http/http2/codec_impl_legacy.h index b9344bea8557..edacf8ae8b04 100644 --- a/source/common/http/http2/codec_impl_legacy.h +++ b/source/common/http/http2/codec_impl_legacy.h @@ -445,7 +445,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1; ++i) { EXPECT_NO_THROW(response_encoder_->encodeHeaders(response_headers, false)); } - // Presently flood mitigation is done only when processing downstream data - // So we need to send stream from downstream client to trigger mitigation - EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError, - "Too many frames in the outbound queue."); + + EXPECT_TRUE(violation_callback->enabled_); + EXPECT_CALL(server_connection_, close(Envoy::Network::ConnectionCloseType::NoFlush)); + violation_callback->invokeCallback(); EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1); EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value()); diff --git a/test/integration/autonomous_upstream.cc b/test/integration/autonomous_upstream.cc index 2fa38ccd54f9..ead74585f9a7 100644 --- a/test/integration/autonomous_upstream.cc +++ b/test/integration/autonomous_upstream.cc @@ -21,6 +21,7 @@ const char AutonomousStream::RESPONSE_SIZE_BYTES[] = "response_size_bytes"; const char AutonomousStream::RESPONSE_DATA_BLOCKS[] = "response_data_blocks"; const char AutonomousStream::EXPECT_REQUEST_SIZE_BYTES[] = "expect_request_size_bytes"; const char AutonomousStream::RESET_AFTER_REQUEST[] = "reset_after_request"; +const char AutonomousStream::NO_TRAILERS[] = "no_trailers"; AutonomousStream::AutonomousStream(FakeHttpConnection& parent, Http::ResponseEncoder& encoder, AutonomousUpstream& upstream, bool allow_incomplete_streams) @@ -63,11 +64,17 @@ void AutonomousStream::sendResponse() { int32_t response_data_blocks = 1; HeaderToInt(RESPONSE_DATA_BLOCKS, response_data_blocks, headers); - encodeHeaders(upstream_.responseHeaders(), false); - for (int32_t i = 0; i < response_data_blocks; ++i) { - encodeData(response_body_length, false); + const bool send_trailers = headers.get_(NO_TRAILERS).empty(); + const bool headers_only_response = !send_trailers && response_data_blocks == 0; + encodeHeaders(upstream_.responseHeaders(), headers_only_response); + if (!headers_only_response) { + for (int32_t i = 0; i < response_data_blocks; ++i) { + encodeData(response_body_length, i == (response_data_blocks - 1) && !send_trailers); + } + if (send_trailers) { + encodeTrailers(upstream_.responseTrailers()); + } } - encodeTrailers(upstream_.responseTrailers()); } AutonomousHttpConnection::AutonomousHttpConnection(AutonomousUpstream& autonomous_upstream, diff --git a/test/integration/autonomous_upstream.h b/test/integration/autonomous_upstream.h index 69814394be35..1ee1a5f3a280 100644 --- a/test/integration/autonomous_upstream.h +++ b/test/integration/autonomous_upstream.h @@ -21,6 +21,8 @@ class AutonomousStream : public FakeStream { // If set, the stream will reset when the request is complete, rather than // sending a response. static const char RESET_AFTER_REQUEST[]; + // Prevents upstream from sending trailers. + static const char NO_TRAILERS[]; AutonomousStream(FakeHttpConnection& parent, Http::ResponseEncoder& encoder, AutonomousUpstream& upstream, bool allow_incomplete_streams); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index fda15444c193..d08a282e66a4 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1725,6 +1725,34 @@ void Http2FloodMitigationTest::floodServer(absl::string_view host, absl::string_ test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value()); } +void Http2FloodMitigationTest::prefillOutboundDownstreamQueue(uint32_t data_frame_count) { + // Set large buffer limits so the test is not affected by the flow control. + config_helper_.setBufferLimits(1024 * 1024 * 1024, 1024 * 1024 * 1024); + autonomous_upstream_ = true; + autonomous_allow_incomplete_streams_ = true; + beginSession(); + + // Do not read from the socket and send request that causes autonomous upstream to respond + // with the specified number of DATA frames. This pre-fills downstream outbound frame queue + // such the the next response triggers flood protection. + // Simulate TCP push back on the Envoy's downstream network socket, so that outbound frames + // start to accumulate in the transport socket buffer. + writev_matcher_->setWritevReturnsEgain(); + + const auto request = Http2Frame::makeRequest( + Http2Frame::makeClientStreamId(0), "host", "/test/long/url", + {Http2Frame::Header("response_data_blocks", absl::StrCat(data_frame_count)), + Http2Frame::Header("no_trailers", "0")}); + sendFrame(request); + + // Wait for some data to arrive and then wait for the upstream_rq_active to flip to 0 to indicate + // that the first request has completed. + test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_rx_bytes_total", 10000); + test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_active", 0); + // Verify that pre-fill did not trigger flood protection + EXPECT_EQ(0, test_server_->counter("http2.outbound_flood")->value()); +} + INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FloodMitigationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -1773,7 +1801,8 @@ TEST_P(Http2FloodMitigationTest, Data) { writev_matcher_->setWritevReturnsEgain(); const auto request = Http2Frame::makeRequest( - 1, "host", "/test/long/url", {Http2Frame::Header("response_data_blocks", "1000")}); + 1, "host", "/test/long/url", + {Http2Frame::Header("response_data_blocks", "1000"), Http2Frame::Header("no_trailers", "0")}); sendFrame(request); // Wait for connection to be flooded with outbound DATA frames and disconnected. @@ -1793,8 +1822,6 @@ TEST_P(Http2FloodMitigationTest, Data) { // This test also verifies that RELEASE_ASSERT in the ConnectionImpl::StreamImpl::encodeDataHelper() // is not fired when it is called by the sendLocalReply() in the dispatching context. TEST_P(Http2FloodMitigationTest, DataOverflowFromDecoderFilterSendLocalReply) { - // Set large buffer limits so the test is not affected by the flow control. - config_helper_.setBufferLimits(1024 * 1024 * 1024, 1024 * 1024 * 1024); config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { @@ -1812,27 +1839,8 @@ name: send_local_reply_filter hcm.mutable_http_filters()->SwapElements(size - 2, size - 1); }); - autonomous_upstream_ = true; - autonomous_allow_incomplete_streams_ = true; - beginSession(); - - // Do not read from the socket and send request that causes autonomous upstream - // to respond with 1000 DATA frames. The Http2FloodMitigationTest::beginSession() - // sets 1000 flood limit for all frame types. Including 1 HEADERS response frame - // 997 DATA frames should make just 2 frames under the flood limit. - // Simulate TCP push back on the Envoy's downstream network socket, so that outbound frames start - // to accumulate in the transport socket buffer. - writev_matcher_->setWritevReturnsEgain(); - - auto request = - Http2Frame::makeRequest(Http2Frame::makeClientStreamId(0), "host", "/test/long/url", - {Http2Frame::Header("response_data_blocks", "997")}); - sendFrame(request); - - // Wait for some data to arrive and then wait for the upstream_rq_active to flip to 0 to indicate - // that the first request has completed. - test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_rx_bytes_total", 1000); - test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_active", 0); + // pre-fill 2 away from overflow + prefillOutboundDownstreamQueue(AllFrameFloodLimit - 2); // At this point the outbound downstream frame queue should be 2 away from overflowing. // Make the SetResponseCodeFilterConfig decoder filter call sendLocalReply with body. @@ -1852,6 +1860,100 @@ name: send_local_reply_filter EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value()); } +// Verify that the server can detect flood of response HEADERS frames +TEST_P(Http2FloodMitigationTest, Headers) { + // pre-fill one away from overflow + prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1); + + // Send second request which should trigger headers only response. + // Verify that connection was disconnected and appropriate counters were set. + auto request2 = Http2Frame::makeRequest( + Http2Frame::makeClientStreamId(1), "host", "/test/long/url", + {Http2Frame::Header("response_data_blocks", "0"), Http2Frame::Header("no_trailers", "0")}); + sendFrame(request2); + + // Wait for connection to be flooded with outbound HEADERS frame and disconnected. + tcp_client_->waitForDisconnect(); + + // If the server codec had incorrectly thrown an exception on flood detection it would cause + // the entire upstream to be disconnected. Verify it is still active, and there are no destroyed + // connections. + ASSERT_EQ(1, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value()); + ASSERT_EQ(0, test_server_->counter("cluster.cluster_0.upstream_cx_destroy")->value()); + // Verify that the flood check was triggered + EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value()); +} + +// Verify that the server can detect overflow by 100 continue response sent by Envoy itself +TEST_P(Http2FloodMitigationTest, Envoy100ContinueHeaders) { + // pre-fill one away from overflow + prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1); + + // Send second request which should trigger Envoy to respond with 100 continue. + // Verify that connection was disconnected and appropriate counters were set. + auto request2 = Http2Frame::makeRequest( + Http2Frame::makeClientStreamId(1), "host", "/test/long/url", + {Http2Frame::Header("response_data_blocks", "0"), Http2Frame::Header("no_trailers", "0"), + Http2Frame::Header("expect", "100-continue")}); + sendFrame(request2); + + // Wait for connection to be flooded with outbound HEADERS frame and disconnected. + tcp_client_->waitForDisconnect(); + + // If the server codec had incorrectly thrown an exception on flood detection it would cause + // the entire upstream to be disconnected. Verify it is still active, and there are no destroyed + // connections. + ASSERT_EQ(1, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value()); + ASSERT_EQ(0, test_server_->counter("cluster.cluster_0.upstream_cx_destroy")->value()); + // The second upstream request should be reset since it is disconnected when sending 100 continue + // response + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_rq_tx_reset")->value()); + // Verify that the flood check was triggered + EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value()); +} + +// Verify that the server can detect flood triggered by a HEADERS frame from a decoder filter call +// to sendLocalReply(). +// This test also verifies that RELEASE_ASSERT in the +// ConnectionImpl::StreamImpl::encodeHeadersBase() is not fired when it is called by the +// sendLocalReply() in the dispatching context. +TEST_P(Http2FloodMitigationTest, HeadersOverflowFromDecoderFilterSendLocalReply) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + const std::string yaml_string = R"EOF( +name: send_local_reply_filter +typed_config: + "@type": type.googleapis.com/test.integration.filters.SetResponseCodeFilterConfig + prefix: "/call_send_local_reply" + code: 404 + )EOF"; + TestUtility::loadFromYaml(yaml_string, *hcm.add_http_filters()); + // keep router the last + auto size = hcm.http_filters_size(); + hcm.mutable_http_filters()->SwapElements(size - 2, size - 1); + }); + + // pre-fill one away from overflow + prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1); + + // At this point the outbound downstream frame queue should be 1 away from overflowing. + // Make the SetResponseCodeFilterConfig decoder filter call sendLocalReply without body. + // Verify that connection was disconnected and appropriate counters were set. + auto request2 = + Http2Frame::makeRequest(Http2Frame::makeClientStreamId(1), "host", "/call_send_local_reply"); + sendFrame(request2); + + // Wait for connection to be flooded with outbound HEADERS frame and disconnected. + tcp_client_->waitForDisconnect(); + + // Verify that the upstream connection is still alive. + ASSERT_EQ(1, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value()); + ASSERT_EQ(0, test_server_->counter("cluster.cluster_0.upstream_cx_destroy")->value()); + // Verify that the flood check was triggered + EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value()); +} + // TODO(yanavlasov): add the same tests as above for the encoder filters. // This is currently blocked by the https://github.com/envoyproxy/envoy/pull/13256 diff --git a/test/integration/http2_integration_test.h b/test/integration/http2_integration_test.h index afd4c527c10f..c6b56d6037ca 100644 --- a/test/integration/http2_integration_test.h +++ b/test/integration/http2_integration_test.h @@ -138,5 +138,6 @@ class Http2FloodMitigationTest : public SocketInterfaceSwap, public Http2FrameIn void setNetworkConnectionBufferSize(); void beginSession() override; + void prefillOutboundDownstreamQueue(uint32_t data_frame_count); }; } // namespace Envoy