comment, fix bug and add test

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
envoyproxy · Aug 16, 2019 · 32ec517 · 32ec517
1 parent 5cdc543
commit 32ec517
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 32 deletions.
diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc
@@ -72,45 +72,46 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) {
   ENVOY_LOG(debug, "tls inspector: new connection accepted");
   Network::ConnectionSocket& socket = cb.socket();
   ASSERT(file_event_ == nullptr);
+  cb_ = &cb;
 
   ParseState parse_state = onRead();
   switch (parse_state) {
   case ParseState::Error:
+    // As per discussion in https://github.com/envoyproxy/envoy/issues/7864
+    // we don't add new enum in FilterStatus so we have to signal the caller
+    // the new condition.
     cb.socket().close();
     return Network::FilterStatus::StopIteration;
   case ParseState::Done:
     return Network::FilterStatus::Continue;
   case ParseState::Continue:
     // do nothing but create the event
-    break;
+    file_event_ = cb.dispatcher().createFileEvent(
+        socket.ioHandle().fd(),
+        [this](uint32_t events) {
+          if (events & Event::FileReadyType::Closed) {
+            config_->stats().connection_closed_.inc();
+            done(false);
+            return;
+          }
+
+          ASSERT(events == Event::FileReadyType::Read);
+          ParseState parse_state = onRead();
+          switch (parse_state) {
+          case ParseState::Error:
+            done(false);
+            break;
+          case ParseState::Done:
+            done(true);
+            break;
+          case ParseState::Continue:
+            // do nothing but wait for the next event
+            break;
+          }
+        },
+        Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Closed);
+    return Network::FilterStatus::StopIteration;
   }
-  file_event_ = cb.dispatcher().createFileEvent(
-      socket.ioHandle().fd(),
-      [this](uint32_t events) {
-        if (events & Event::FileReadyType::Closed) {
-          config_->stats().connection_closed_.inc();
-          done(false);
-          return;
-        }
-
-        ASSERT(events == Event::FileReadyType::Read);
-        ParseState parse_state = onRead();
-        switch (parse_state) {
-        case ParseState::Error:
-          done(false);
-          break;
-        case ParseState::Done:
-          done(true);
-          break;
-        case ParseState::Continue:
-          // do nothing but wait for the next event
-          break;
-        }
-      },
-      Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Closed);
-
-  cb_ = &cb;
-  return Network::FilterStatus::StopIteration;
 }
 
 void Filter::onALPN(const unsigned char* data, unsigned int len) {

diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h
@@ -36,7 +36,14 @@ struct TlsInspectorStats {
   ALL_TLS_INSPECTOR_STATS(GENERATE_COUNTER_STRUCT)
 };
 
-enum class ParseState { Done, Continue, Error };
+enum class ParseState {
+  // Parse result is out. It could be tls or not.
+  Done,
+  // Parser expects more data.
+  Continue,
+  // Parser reports unrecoverable error.
+  Error
+};
 /**
  * Global configuration for TLS inspector.
  */

diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc
@@ -184,6 +184,7 @@ void ConnectionHandlerImpl::ActiveSocket::unlink() {
 
 void ConnectionHandlerImpl::ActiveSocket::continueFilterChain(bool success) {
   if (success) {
+    bool no_error = true;
     if (iter_ == accept_filters_.end()) {
       iter_ = accept_filters_.begin();
     } else {
@@ -196,15 +197,19 @@ void ConnectionHandlerImpl::ActiveSocket::continueFilterChain(bool success) {
         // The filter is responsible for calling us again at a later time to continue the filter
         // chain from the next filter.
         if (!socket().ioHandle().isOpen()) {
-          return;
+          // break the loop but should not create new connection
+          no_error = false;
+          break;
         } else {
-          unlink();
+          // Blocking at the filter but no error
           return;
         }
       }
     }
     // Successfully ran all the accept filters.
-    newConnection();
+    if (no_error) {
+      newConnection();
+    }
   }
 
   // Filter execution concluded, unlink and delete this ActiveSocket if it was linked.

diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc
@@ -43,6 +43,13 @@ class TlsInspectorTest : public testing::Test {
     EXPECT_CALL(cb_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_));
     EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_));
 
+    // Prepare the first recv attempt during
+    EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
+        .WillOnce(
+            Invoke([](int fd, void* buffer, size_t length, int flag) -> Api::SysCallSizeResult {
+              ENVOY_LOG_MISC(error, "In mock syscall recv {} {} {} {}", fd, buffer, length, flag);
+              return Api::SysCallSizeResult{static_cast<ssize_t>(0), 0};
+            }));
     EXPECT_CALL(dispatcher_,
                 createFileEvent_(_, _, Event::FileTriggerType::Edge,
                                  Event::FileReadyType::Read | Event::FileReadyType::Closed))
@@ -231,6 +238,36 @@ TEST_F(TlsInspectorTest, NotSsl) {
   EXPECT_EQ(1, cfg_->stats().tls_not_found_.value());
 }
 
+TEST_F(TlsInspectorTest, InlineReadSucceed) {
+  filter_ = std::make_unique<Filter>(cfg_);
+
+  EXPECT_CALL(cb_, socket()).WillRepeatedly(ReturnRef(socket_));
+  EXPECT_CALL(cb_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_));
+  EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_));
+  const std::vector<absl::string_view> alpn_protos = {absl::string_view("h2")};
+  const std::string servername("example.com");
+  std::vector<uint8_t> client_hello = Tls::Test::generateClientHello(servername, "\x02h2");
+
+  EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
+      .WillOnce(Invoke(
+          [&client_hello](int fd, void* buffer, size_t length, int flag) -> Api::SysCallSizeResult {
+            ENVOY_LOG_MISC(trace, "In mock syscall recv {} {} {} {}", fd, buffer, length, flag);
+            ASSERT(length >= client_hello.size());
+            memcpy(buffer, client_hello.data(), client_hello.size());
+            return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0};
+          }));
+
+  // No event is created if the inline recv parse the hello.
+  EXPECT_CALL(dispatcher_,
+              createFileEvent_(_, _, Event::FileTriggerType::Edge,
+                               Event::FileReadyType::Read | Event::FileReadyType::Closed))
+      .Times(0);
+
+  EXPECT_CALL(socket_, setRequestedServerName(Eq(servername)));
+  EXPECT_CALL(socket_, setRequestedApplicationProtocols(alpn_protos));
+  EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls")));
+  EXPECT_EQ(Network::FilterStatus::Continue, filter_->onAccept(cb_));
+}
 } // namespace
 } // namespace TlsInspector
 } // namespace ListenerFilters