test interaction between drain upon completion & onDrainTimeout (envo…

…yproxy#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 <[email protected]>

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<MockStreamDecoderFilter>();
+  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<MockStreamDecoderFilter>();
+  // 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();

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;