thrift: refactor build files and imports (#4271)

Refactors build dependencies and imports to make inter-protocol dependencies easier. Moves the auto-detecting protocol and transports to better-named files. Splits DecoderEventHandler out of DecoderFilter to allow most implementors to exclude some filter-specific functions that are not required for Thrift decoding. Provides an implmentation of DecoderEventHandler that delegates to another implementation. Generally cleans up some #include directives. The impetus for this change becomes apparent when introducing a new protocol that directly responds to a downstream in order to negotiate protocol options. Risk Level: low, no code changes Testing: existing tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Stephan Zuercher <[email protected]>
istio · Aug 28, 2018 · 68d20b4 · 68d20b4
1 parent 5fa8192
commit 68d20b4
Show file tree

Hide file tree

Showing 34 changed files with 782 additions and 651 deletions.
diff --git a/source/extensions/filters/network/thrift_proxy/BUILD b/source/extensions/filters/network/thrift_proxy/BUILD
@@ -38,7 +38,7 @@ envoy_cc_library(
         ":app_exception_lib",
         ":conn_manager_lib",
         ":decoder_lib",
-        ":protocol_lib",
+        ":protocol_interface",
         "//include/envoy/registry",
         "//source/common/config:utility_lib",
         "//source/extensions/filters/network:well_known_names",
@@ -76,14 +76,24 @@ envoy_cc_library(
     ],
 )
 
+envoy_cc_library(
+    name = "decoder_events_lib",
+    hdrs = ["decoder_events.h"],
+    deps = [
+        ":metadata_lib",
+        ":thrift_lib",
+    ],
+)
+
 envoy_cc_library(
     name = "decoder_lib",
     srcs = ["decoder.cc"],
     hdrs = ["decoder.h"],
     deps = [
-        ":protocol_lib",
+        ":app_exception_lib",
+        ":protocol_interface",
         ":stats_lib",
-        ":transport_lib",
+        ":transport_interface",
         "//source/common/buffer:buffer_lib",
         "//source/extensions/filters/network/thrift_proxy/filters:filter_interface",
     ],
@@ -105,9 +115,9 @@ envoy_cc_library(
         "protocol_converter.h",
     ],
     deps = [
+        ":decoder_events_lib",
         ":protocol_interface",
         "//include/envoy/buffer:buffer_interface",
-        "//source/extensions/filters/network/thrift_proxy/filters:filter_interface",
     ],
 )
 
@@ -118,6 +128,7 @@ envoy_cc_library(
     ],
     external_deps = ["abseil_optional"],
     deps = [
+        ":decoder_events_lib",
         ":metadata_lib",
         ":thrift_lib",
         "//include/envoy/buffer:buffer_interface",
@@ -129,22 +140,59 @@ envoy_cc_library(
 )
 
 envoy_cc_library(
-    name = "protocol_lib",
+    name = "auto_protocol_lib",
+    srcs = [
+        "auto_protocol_impl.cc",
+    ],
+    hdrs = [
+        "auto_protocol_impl.h",
+    ],
+    deps = [
+        ":binary_protocol_lib",
+        ":buffer_helper_lib",
+        ":compact_protocol_lib",
+        ":protocol_interface",
+        "//source/common/common:macros",
+    ],
+)
+
+envoy_cc_library(
+    name = "binary_protocol_lib",
     srcs = [
         "binary_protocol_impl.cc",
-        "compact_protocol_impl.cc",
-        "protocol_impl.cc",
     ],
     hdrs = [
         "binary_protocol_impl.h",
+    ],
+    deps = [
+        ":buffer_helper_lib",
+        ":protocol_interface",
+        "//source/common/common:macros",
+    ],
+)
+
+envoy_cc_library(
+    name = "compact_protocol_lib",
+    srcs = [
+        "compact_protocol_impl.cc",
+    ],
+    hdrs = [
         "compact_protocol_impl.h",
-        "protocol_impl.h",
     ],
     external_deps = ["abseil_optional"],
     deps = [
         ":buffer_helper_lib",
         ":protocol_interface",
-        "//source/common/singleton:const_singleton",
+        "//source/common/common:macros",
+    ],
+)
+
+envoy_cc_library(
+    name = "protocol_lib",
+    deps = [
+        ":auto_protocol_lib",
+        ":binary_protocol_lib",
+        ":compact_protocol_lib",
     ],
 )
 
@@ -183,26 +231,76 @@ envoy_cc_library(
 )
 
 envoy_cc_library(
-    name = "transport_lib",
+    name = "auto_transport_lib",
+    srcs = [
+        "auto_transport_impl.cc",
+    ],
+    hdrs = [
+        "auto_transport_impl.h",
+    ],
+    deps = [
+        ":buffer_helper_lib",
+        ":framed_transport_lib",
+        ":header_transport_lib",
+        ":protocol_lib",
+        ":transport_interface",
+        ":unframed_transport_lib",
+        "//source/common/common:assert_lib",
+    ],
+)
+
+envoy_cc_library(
+    name = "framed_transport_lib",
     srcs = [
         "framed_transport_impl.cc",
-        "header_transport_impl.cc",
-        "transport_impl.cc",
-        "unframed_transport_impl.cc",
     ],
     hdrs = [
         "framed_transport_impl.h",
+    ],
+    deps = [
+        ":buffer_helper_lib",
+        ":transport_interface",
+        "//source/common/common:assert_lib",
+    ],
+)
+
+envoy_cc_library(
+    name = "header_transport_lib",
+    srcs = [
+        "header_transport_impl.cc",
+    ],
+    hdrs = [
         "header_transport_impl.h",
-        "transport_impl.h",
-        "unframed_transport_impl.h",
     ],
     deps = [
         ":app_exception_lib",
         ":buffer_helper_lib",
-        ":metadata_lib",
-        ":protocol_lib",
         ":transport_interface",
         "//source/common/common:assert_lib",
-        "//source/common/singleton:const_singleton",
+    ],
+)
+
+envoy_cc_library(
+    name = "unframed_transport_lib",
+    srcs = [
+        "unframed_transport_impl.cc",
+    ],
+    hdrs = [
+        "unframed_transport_impl.h",
+    ],
+    deps = [
+        ":buffer_helper_lib",
+        ":transport_interface",
+        "//source/common/common:assert_lib",
+    ],
+)
+
+envoy_cc_library(
+    name = "transport_lib",
+    deps = [
+        ":auto_transport_lib",
+        ":framed_transport_lib",
+        ":header_transport_lib",
+        ":unframed_transport_lib",
     ],
 )
diff --git a/...ers/network/thrift_proxy/protocol_impl.cc → ...etwork/thrift_proxy/auto_protocol_impl.cc b/...ers/network/thrift_proxy/protocol_impl.cc → ...etwork/thrift_proxy/auto_protocol_impl.cc
@@ -1,4 +1,4 @@
-#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
+#include "extensions/filters/network/thrift_proxy/auto_protocol_impl.h"
 
 #include <algorithm>
 

diff --git a/...ters/network/thrift_proxy/protocol_impl.h → ...network/thrift_proxy/auto_protocol_impl.h b/...ters/network/thrift_proxy/protocol_impl.h → ...network/thrift_proxy/auto_protocol_impl.h
diff --git a/...rs/network/thrift_proxy/transport_impl.cc → ...twork/thrift_proxy/auto_transport_impl.cc b/...rs/network/thrift_proxy/transport_impl.cc → ...twork/thrift_proxy/auto_transport_impl.cc
@@ -1,4 +1,4 @@
-#include "extensions/filters/network/thrift_proxy/transport_impl.h"
+#include "extensions/filters/network/thrift_proxy/auto_transport_impl.h"
 
 #include "envoy/common/exception.h"
 

diff --git a/...ers/network/thrift_proxy/transport_impl.h → ...etwork/thrift_proxy/auto_transport_impl.h b/...ers/network/thrift_proxy/transport_impl.h → ...etwork/thrift_proxy/auto_transport_impl.h
@@ -8,8 +8,6 @@
 
 #include "extensions/filters/network/thrift_proxy/transport.h"
 
-#include "absl/types/optional.h"
-
 namespace Envoy {
 namespace Extensions {
 namespace NetworkFilters {

diff --git a/source/extensions/filters/network/thrift_proxy/binary_protocol_impl.h b/source/extensions/filters/network/thrift_proxy/binary_protocol_impl.h
@@ -5,7 +5,7 @@
 #include "envoy/buffer/buffer.h"
 #include "envoy/common/pure.h"
 
-#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
+#include "extensions/filters/network/thrift_proxy/protocol.h"
 
 namespace Envoy {
 namespace Extensions {

diff --git a/source/extensions/filters/network/thrift_proxy/compact_protocol_impl.h b/source/extensions/filters/network/thrift_proxy/compact_protocol_impl.h
@@ -6,7 +6,7 @@
 #include "envoy/buffer/buffer.h"
 #include "envoy/common/pure.h"
 
-#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
+#include "extensions/filters/network/thrift_proxy/protocol.h"
 
 #include "absl/types/optional.h"
 

diff --git a/source/extensions/filters/network/thrift_proxy/config.cc b/source/extensions/filters/network/thrift_proxy/config.cc
@@ -8,15 +8,15 @@
 
 #include "common/config/utility.h"
 
+#include "extensions/filters/network/thrift_proxy/auto_protocol_impl.h"
+#include "extensions/filters/network/thrift_proxy/auto_transport_impl.h"
 #include "extensions/filters/network/thrift_proxy/binary_protocol_impl.h"
 #include "extensions/filters/network/thrift_proxy/compact_protocol_impl.h"
 #include "extensions/filters/network/thrift_proxy/decoder.h"
 #include "extensions/filters/network/thrift_proxy/filters/filter_config.h"
 #include "extensions/filters/network/thrift_proxy/filters/well_known_names.h"
 #include "extensions/filters/network/thrift_proxy/framed_transport_impl.h"
-#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
 #include "extensions/filters/network/thrift_proxy/stats.h"
-#include "extensions/filters/network/thrift_proxy/transport_impl.h"
 #include "extensions/filters/network/thrift_proxy/unframed_transport_impl.h"
 
 namespace Envoy {

diff --git a/source/extensions/filters/network/thrift_proxy/conn_manager.cc b/source/extensions/filters/network/thrift_proxy/conn_manager.cc
@@ -38,8 +38,8 @@ void ConnectionManager::dispatch() {
   try {
     bool underflow = false;
     while (!underflow) {
-      ThriftFilters::FilterStatus status = decoder_->onData(request_buffer_, underflow);
-      if (status == ThriftFilters::FilterStatus::StopIteration) {
+      FilterStatus status = decoder_->onData(request_buffer_, underflow);
+      if (status == FilterStatus::StopIteration) {
         stopped_ = true;
         break;
       }
@@ -122,7 +122,7 @@ void ConnectionManager::onEvent(Network::ConnectionEvent event) {
   }
 }
 
-ThriftFilters::DecoderFilter& ConnectionManager::newDecoderFilter() {
+DecoderEventHandler& ConnectionManager::newDecoderEventHandler() {
   ENVOY_LOG(trace, "new decoder filter");
 
   ActiveRpcPtr new_rpc(new ActiveRpc(*this));
@@ -141,17 +141,16 @@ bool ConnectionManager::ResponseDecoder::onData(Buffer::Instance& data) {
   return complete_;
 }
 
-ThriftFilters::FilterStatus
-ConnectionManager::ResponseDecoder::messageBegin(MessageMetadataSharedPtr metadata) {
+FilterStatus ConnectionManager::ResponseDecoder::messageBegin(MessageMetadataSharedPtr metadata) {
   metadata_ = metadata;
   first_reply_field_ =
       (metadata->hasMessageType() && metadata->messageType() == MessageType::Reply);
   return ProtocolConverter::messageBegin(metadata);
 }
 
-ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::fieldBegin(absl::string_view name,
-                                                                           FieldType field_type,
-                                                                           int16_t field_id) {
+FilterStatus ConnectionManager::ResponseDecoder::fieldBegin(absl::string_view name,
+                                                            FieldType field_type,
+                                                            int16_t field_id) {
   if (first_reply_field_) {
     // Reply messages contain a struct where field 0 is the call result and fields 1+ are
     // exceptions, if defined. At most one field may be set. Therefore, the very first field we
@@ -163,7 +162,7 @@ ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::fieldBegin(absl:
   return ProtocolConverter::fieldBegin(name, field_type, field_id);
 }
 
-ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::transportEnd() {
+FilterStatus ConnectionManager::ResponseDecoder::transportEnd() {
   ASSERT(metadata_ != nullptr);
 
   ConnectionManager& cm = parent_.parent_;
@@ -205,11 +204,12 @@ ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::transportEnd() {
     break;
   }
 
-  return ThriftFilters::FilterStatus::Continue;
+  return FilterStatus::Continue;
 }
 
-ThriftFilters::FilterStatus ConnectionManager::ActiveRpc::transportEnd() {
-  ASSERT(metadata_ != nullptr && metadata_->hasMessageType());
+FilterStatus ConnectionManager::ActiveRpc::transportEnd() {
+  ASSERT(metadata_ != nullptr);
+  ASSERT(metadata_->hasMessageType());
 
   parent_.stats_.request_.inc();
 
@@ -233,6 +233,12 @@ ThriftFilters::FilterStatus ConnectionManager::ActiveRpc::transportEnd() {
   return decoder_filter_->transportEnd();
 }
 
+FilterStatus ConnectionManager::ActiveRpc::messageBegin(MessageMetadataSharedPtr metadata) {
+  metadata_ = metadata;
+
+  return event_handler_->messageBegin(metadata);
+}
+
 void ConnectionManager::ActiveRpc::createFilterChain() {
   parent_.config_.filterFactory().createFilterChain(*this);
 }