From a8293b730371bcf3a0f54003fa1841beda947502 Mon Sep 17 00:00:00 2001 From: "Antonio V. Leonti" <53806445+antoniovleonti@users.noreply.github.com> Date: Mon, 12 Aug 2024 10:07:24 -0400 Subject: [PATCH] test interaction between drain upon completion & onDrainTimeout (#35568) Commit Message: test interaction between drain upon completion & onDrainTimeout Additional Description: adds unit & integration tests that demonstrate it is safe to use max_requests_per_connection with max_connection_duration, idle_timeout, and max_stream_duration. (idle_timeout is not explicitly tested but triggers the same exact drain behavior as max_connection_duration.) Risk Level: none Testing: test-only PR --------- Signed-off-by: antoniovleonti --- test/common/http/conn_manager_impl_test_2.cc | 52 +++++++++++ test/integration/protocol_integration_test.cc | 89 +++++++++++++++++++ 2 files changed, 141 insertions(+) diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index fead362c3328..526a874ee330 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -479,6 +479,58 @@ TEST_F(HttpConnectionManagerImplTest, MaxRequests) { conn_manager_->onEvent(Network::ConnectionEvent::RemoteClose); } +// max_requests_per_connection is met first then the drain timer fires. Drain timer should be +// ignored. +TEST_F(HttpConnectionManagerImplTest, DrainConnectionUponCompletionVsOnDrainTimeoutHttp11) { + // Http1.1 is used for this test because it defaults to keeping the connection alive. + EXPECT_CALL(*codec_, protocol()).WillRepeatedly(Return(Protocol::Http11)); + max_requests_per_connection_ = 2; + max_connection_duration_ = std::chrono::milliseconds(10); + + Event::MockTimer* connection_duration_timer = setUpTimer(); + EXPECT_CALL(*connection_duration_timer, enableTimer(_, _)); + // Set up connection. + setup(false, ""); + + // Create a filter so we can encode responses. + MockStreamDecoderFilter* filter = new NiceMock(); + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { + auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); + manager.applyFilterFactoryCb({}, factory); + return true; + })); + + startRequest(true); + // Encode response, connection will not be closed since we're using http 1.1. + filter->callbacks_->streamInfo().setResponseCodeDetails(""); + filter->callbacks_->encodeHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true, "details"); + response_encoder_.stream_.codec_callbacks_->onCodecEncodeComplete(); + + // Now connection is established and codec is not nullptr. This should start the drain timer. + Event::MockTimer* drain_timer = setUpTimer(); + EXPECT_CALL(*drain_timer, enableTimer(_, _)); + connection_duration_timer->invokeCallback(); + EXPECT_EQ(1U, stats_.named_.downstream_cx_max_duration_reached_.value()); + + // Get a fresh mock filter. + filter = new NiceMock(); + // Send a second request. This will cause max_requests_per_connection limit to be reached. + // Connection drain state will be set to closing. + startRequest(true); + EXPECT_EQ(1U, stats_.named_.downstream_cx_max_requests_reached_.value()); + + drain_timer->invokeCallback(); + + // Send the last response. The drain timer having already fired should not be an issue. + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)); + filter->callbacks_->streamInfo().setResponseCodeDetails(""); + filter->callbacks_->encodeHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true, "details"); + response_encoder_.stream_.codec_callbacks_->onCodecEncodeComplete(); +} + TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) { max_connection_duration_ = (std::chrono::milliseconds(10)); Event::MockTimer* connection_duration_timer = setUpTimer(); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index f45f177a5b2e..90edb9b80931 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -3102,6 +3102,95 @@ TEST_P(DownstreamProtocolIntegrationTest, MaxRequestsPerConnectionReached) { ASSERT_TRUE(codec_client_->waitForDisconnect()); } +// Test that onDrainTimeout allows current stream to finish before closing connection for http1. +TEST_P(DownstreamProtocolIntegrationTest, MaxRequestsPerConnectionVsMaxConnectionDuration) { + config_helper_.setDownstreamMaxRequestsPerConnection(2); + config_helper_.setDownstreamMaxConnectionDuration(std::chrono::milliseconds(500)); + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { hcm.mutable_drain_timeout()->set_nanos(500'000'000 /*=500ms*/); }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_max_requests_reached")->value(), + 0); + + test_server_->waitForCounterGe("http.config_test.downstream_cx_max_duration_reached", 1); + // http1 is not closed at this point because envoy needs to send a response with the + // connection:close response header to be able to safely close the connection. For other protocols + // it's safe for envoy to just close the connection, so they do so. + if (downstream_protocol_ != Http::CodecType::HTTP1) { + ASSERT_TRUE(codec_client_->waitForDisconnect()); + EXPECT_TRUE(codec_client_->sawGoAway()); + // The rest of the test is only for http1. + return; + } + + // Sending second request. + auto response_2 = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + + // Before sending the response, sleep past the drain timer. Nothing should happen. + timeSystem().advanceTimeWait(Seconds(1)); + EXPECT_FALSE(codec_client_->sawGoAway()); + + upstream_request_->encodeHeaders(default_response_headers_, true); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + + ASSERT_TRUE(response_2->waitForEndStream()); + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_TRUE(response_2->complete()); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_max_requests_reached")->value(), + 1); + + if (downstream_protocol_ == Http::CodecType::HTTP1) { + ASSERT_NE(nullptr, response_2->headers().Connection()); + EXPECT_EQ("close", response_2->headers().getConnectionValue()); + } else { + EXPECT_TRUE(codec_client_->sawGoAway()); + } + ASSERT_TRUE(codec_client_->waitForDisconnect()); +} + +// Test that max stream duration still works after max requests per connection is reached (i.e. the +// final response is still time bounded). Also, if if max_stream_duration is triggered, it should +// add the connection:close header if the downstream protocol is http1 and this will be the last +// response! +TEST_P(DownstreamProtocolIntegrationTest, MaxRequestsPerConnectionVsMaxStreamDuration) { + config_helper_.setDownstreamMaxRequestsPerConnection(2); + config_helper_.setDownstreamMaxStreamDuration(std::chrono::milliseconds(500)); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + // Sending first request and waiting to complete the response. + sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_max_requests_reached")->value(), + 0); + + // Sending second request and waiting to complete the response. + auto response_2 = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_max_requests_reached")->value(), + 1); + + // Don't send a response. HCM should sendLocalReply after max stream duration has elapsed. + test_server_->waitForCounterGe("http.config_test.downstream_rq_max_duration_reached", 1); + + if (downstream_protocol_ == Http::CodecType::HTTP1) { + ASSERT_TRUE(codec_client_->waitForDisconnect()); + ASSERT_TRUE(response_2->complete()); + // This will be the last request / response; envoy's going to close the connection after this + // stream ends. We should notify the client. + EXPECT_EQ("close", response_2->headers().getConnectionValue()); + } else { + ASSERT_TRUE(response_2->waitForEndStream()); + codec_client_->close(); + } +} + // Make sure that invalid authority headers get blocked at or before the HCM. TEST_P(DownstreamProtocolIntegrationTest, InvalidAuthority) { disable_client_header_validation_ = true;