tls: fix detection of the upstream connection close event.

[PiotrSikora]

Fixes #13856.

Signed-off-by: Piotr Sikora piotrsikora@google.com

[ggreenway]

As @antoniovicente noted, please add a test.

[ggreenway]

This looks good overall. Thanks for adding the comments and tests.

[ggreenway]

To ensure this always happens gracefully, close with FlushWrite. Otherwise, it's best-effort to send the close notify.

[PiotrSikora]

I don't believe that NoFlush is best-effort in this case, and the test is not flaky at all:

$ bazel test --config=libc++ //test/extensions/transport_sockets/tls:ssl_socket_test --test_arg="--gtest_filter=*SslSocketTest.Shutdown*" --runs_per_test=1000

//test/extensions/transport_sockets/tls:ssl_socket_test                  PASSED in 1.7s
  Stats over 4000 runs: max = 1.7s, min = 0.5s, avg = 0.9s, dev = 0.2s

$ rg 'SSL_ERROR_ZERO_RETURN|SSL_ERROR_SYSCALL_EOF' bazel-testlogs/test/extensions/transport_sockets/tls/ssl_socket_test/ | cut -d" " -f5 | sort | uniq -c

   2000 SSL_ERROR_SYSCALL_EOF
   2000 SSL_ERROR_ZERO_RETURN

Also, changing this to FlushWrite:

--- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc
+++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
@@ -2603,8 +2603,8 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) {
   EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::Connected))
       .WillOnce(Invoke([&](Network::ConnectionEvent) -> void {
         Buffer::OwnedImpl data("hello");
-        server_connection->write(data, false);
-        server_connection->close(Network::ConnectionCloseType::NoFlush);
+        server_connection->write(data, true);
+        server_connection->close(Network::ConnectionCloseType::FlushWrite);
       }));

   EXPECT_CALL(*client_read_filter, onNewConnection())
@@ -2613,11 +2613,11 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) {
   EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("hello"), true))
       .WillOnce(Invoke([&](Buffer::Instance& read_buffer, bool) -> Network::FilterStatus {
         read_buffer.drain(read_buffer.length());
-        client_connection->close(Network::ConnectionCloseType::NoFlush);
+        client_connection->close(Network::ConnectionCloseType::FlushWrite);
         return Network::FilterStatus::StopIteration;
       }));

-  EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose));
+  EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose));
   EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose))
       .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); }));

results in "hello" being lost, and only onData("", true) being delivered. It looks like there is another bug there, but it's unrelated to this PR.

[PiotrSikora]

See: #13890.

[ggreenway]

Sorry, I forgot that we're using half-open here. close() should not be called at all on the server connection. However, with that change, I'm still seeing some odd behavior in the test. I'm still trying to figure out what's wrong.

[PiotrSikora]

I don't think that we can test this behavior without close().

[ggreenway]

I think this patch makes the test correctly cover what we want it to cover:

diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
index 0ef42abeba..6fc875eea1 100644
--- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc
+++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
@@ -2600,11 +2600,11 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) {
         server_connection->addReadFilter(server_read_filter);
         server_connection->addConnectionCallbacks(server_connection_callbacks);
       }));
+  EXPECT_CALL(*server_read_filter, onNewConnection());
   EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::Connected))
       .WillOnce(Invoke([&](Network::ConnectionEvent) -> void {
         Buffer::OwnedImpl data("hello");
-        server_connection->write(data, false);
-        server_connection->close(Network::ConnectionCloseType::NoFlush);
+        server_connection->write(data, true);
       }));
 
   EXPECT_CALL(*client_read_filter, onNewConnection())
@@ -2616,10 +2616,14 @@ TEST_P(SslSocketTest, ShutdownWithCloseNotify) {
         client_connection->close(Network::ConnectionCloseType::NoFlush);
         return Network::FilterStatus::StopIteration;
       }));
+  EXPECT_CALL(*server_read_filter, onData(_, true));
 
-  EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose));
-  EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose))
-      .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); }));
+  EXPECT_CALL(client_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose));
+  EXPECT_CALL(server_connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose))
+      .WillOnce(Invoke([&](Network::ConnectionEvent) -> void {
+        server_connection->close(Network::ConnectionCloseType::NoFlush);
+        dispatcher_->exit();
+      }));
 
   dispatcher_->run(Event::Dispatcher::RunType::Block);
 }

[ggreenway]

The extra call to onData("", true) was actually on the server connection, in response to the client finishing the close.

[ggreenway]

close() isn't needed when half-close is enabled. The true for end_stream is the close.

[PiotrSikora]

Patch applied, thanks!

[PiotrSikora]

/backport