From 8e86b137216ea134e83f4e41abf80d4fa0575af6 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 6 Oct 2020 21:52:07 -0400 Subject: [PATCH 1/2] Proactively disconnect connections flooded in drain timeout Signed-off-by: Yan Avlasov --- source/common/http/http2/codec_impl.cc | 1 + source/common/http/http2/codec_impl_legacy.cc | 1 + test/common/http/http2/codec_impl_test.cc | 40 +++++++++++++++++++ test/integration/http2_integration_test.cc | 25 ++++++++++++ 4 files changed, 67 insertions(+) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 68adc0128603..d806e0a40c66 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -695,6 +695,7 @@ void ConnectionImpl::goAway() { auto status = sendPendingFrames(); // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); + checkProtocolConstraintViolation(); } void ConnectionImpl::shutdownNotice() { diff --git a/source/common/http/http2/codec_impl_legacy.cc b/source/common/http/http2/codec_impl_legacy.cc index 354fb689752c..9249eb46ff4e 100644 --- a/source/common/http/http2/codec_impl_legacy.cc +++ b/source/common/http/http2/codec_impl_legacy.cc @@ -664,6 +664,7 @@ void ConnectionImpl::goAway() { ASSERT(rc == 0); sendPendingFrames(); + checkProtocolConstraintViolation(); } void ConnectionImpl::shutdownNotice() { diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 3e348a12393b..917816f8593f 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -2260,6 +2260,46 @@ TEST_P(Http2CodecImplTest, EmptyDataFloodOverride) { EXPECT_TRUE(status.ok()); } +// Verify that codec detects flood of outbound frames caused by goAway() method +TEST_P(Http2CodecImplTest, GoAwayCausesOutboundFlood) { + initialize(); + + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); + request_encoder_->encodeHeaders(request_headers, false); + + int frame_count = 0; + Buffer::OwnedImpl buffer; + ON_CALL(server_connection_, write(_, _)) + .WillByDefault(Invoke([&buffer, &frame_count](Buffer::Instance& frame, bool) { + ++frame_count; + buffer.move(frame); + })); + + auto* violation_callback = + new NiceMock(&server_connection_.dispatcher_); + + TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + response_encoder_->encodeHeaders(response_headers, false); + // Account for the single HEADERS frame above + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { + Buffer::OwnedImpl data("0"); + EXPECT_NO_THROW(response_encoder_->encodeData(data, false)); + } + + EXPECT_FALSE(violation_callback->enabled_); + + server_->goAway(); + + 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()); +} + // CONNECT without upgrade type gets tagged with "bytestream" TEST_P(Http2CodecImplTest, ConnectTest) { client_http2_options_.set_allow_connect(true); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index d08a282e66a4..ff48895e132c 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1992,6 +1992,31 @@ TEST_P(Http2FloodMitigationTest, RST_STREAM) { test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value()); } +TEST_P(Http2FloodMitigationTest, GoAwayOverflowOnDrainTimeout) { + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + auto* drain_time_out = hcm.mutable_drain_timeout(); + std::chrono::milliseconds timeout(1000); + auto seconds = std::chrono::duration_cast(timeout); + drain_time_out->set_seconds(seconds.count()); + + auto* http_protocol_options = hcm.mutable_common_http_protocol_options(); + auto* idle_time_out = http_protocol_options->mutable_idle_timeout(); + idle_time_out->set_seconds(seconds.count()); + }); + // pre-fill two away from overflow + prefillOutboundDownstreamQueue(AllFrameFloodLimit - 2); + + // connection idle timeout will send first GOAWAY frame and start drain timer + // drain timeout will send second GOAWAY frame which should trigger flood protection + // Wait for connection to be flooded with outbound GOAWAY frame and disconnected. + tcp_client_->waitForDisconnect(); + + // Verify that the flood check was triggered + EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value()); +} + // Verify that the server stop reading downstream connection on protocol error. TEST_P(Http2FloodMitigationTest, TooManyStreams) { config_helper_.addConfigModifier( From 4ac889ca5a9f8c3b56ba88b84be3935cf0411a90 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 6 Oct 2020 21:55:02 -0400 Subject: [PATCH 2/2] format Signed-off-by: Yan Avlasov --- test/integration/http2_integration_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index ff48895e132c..f214694d07b6 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1992,6 +1992,7 @@ TEST_P(Http2FloodMitigationTest, RST_STREAM) { test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value()); } +// Verify detection of frame flood when sending second GOAWAY frame on drain timeout TEST_P(Http2FloodMitigationTest, GoAwayOverflowOnDrainTimeout) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& @@ -2003,7 +2004,7 @@ TEST_P(Http2FloodMitigationTest, GoAwayOverflowOnDrainTimeout) { auto* http_protocol_options = hcm.mutable_common_http_protocol_options(); auto* idle_time_out = http_protocol_options->mutable_idle_timeout(); - idle_time_out->set_seconds(seconds.count()); + idle_time_out->set_seconds(seconds.count()); }); // pre-fill two away from overflow prefillOutboundDownstreamQueue(AllFrameFloodLimit - 2);