tap: fix various issues (#13719)

1) Test flakes 2) LDS reload / new listener needs to connect to an existing admin tap. 3) Crash during admin disconnection if multiple configs are connected to the tap. Signed-off-by: Matt Klein <[email protected]>
envoyproxy · Oct 23, 2020 · 768518e · 768518e
1 parent 9d9e81d
commit 768518e
Show file tree

Hide file tree

Showing 15 changed files with 83 additions and 25 deletions.
diff --git a/source/extensions/common/tap/admin.cc b/source/extensions/common/tap/admin.cc
@@ -65,7 +65,7 @@ Http::Code AdminHandler::handler(absl::string_view, Http::HeaderMap&, Buffer::In
                               tap_request.config_id()));
   }
   for (auto config : config_id_map_[tap_request.config_id()]) {
-    config->newTapConfig(std::move(*tap_request.mutable_tap_config()), this);
+    config->newTapConfig(tap_request.tap_config(), this);
   }
 
   admin_stream.setEndStreamOnComplete(false);
@@ -74,10 +74,10 @@ Http::Code AdminHandler::handler(absl::string_view, Http::HeaderMap&, Buffer::In
       ENVOY_LOG(debug, "detach tap admin request for config_id={}",
                 attached_request_.value().config_id_);
       config->clearTapConfig();
-      attached_request_ = absl::nullopt;
     }
+    attached_request_ = absl::nullopt;
   });
-  attached_request_.emplace(tap_request.config_id(), &admin_stream);
+  attached_request_.emplace(tap_request.config_id(), tap_request.tap_config(), &admin_stream);
   return Http::Code::OK;
 }
 
@@ -91,6 +91,9 @@ void AdminHandler::registerConfig(ExtensionConfig& config, const std::string& co
   ASSERT(!config_id.empty());
   ASSERT(config_id_map_[config_id].count(&config) == 0);
   config_id_map_[config_id].insert(&config);
+  if (attached_request_.has_value() && attached_request_.value().config_id_ == config_id) {
+    config.newTapConfig(attached_request_.value().config_, this);
+  }
 }
 
 void AdminHandler::unregisterConfig(ExtensionConfig& config) {

diff --git a/source/extensions/common/tap/admin.h b/source/extensions/common/tap/admin.h
@@ -68,10 +68,12 @@ class AdminHandler : public Singleton::Instance,
   };
 
   struct AttachedRequest {
-    AttachedRequest(std::string config_id, Server::AdminStream* admin_stream)
-        : config_id_(std::move(config_id)), admin_stream_(admin_stream) {}
+    AttachedRequest(std::string config_id, const envoy::config::tap::v3::TapConfig& config,
+                    Server::AdminStream* admin_stream)
+        : config_id_(std::move(config_id)), config_(config), admin_stream_(admin_stream) {}
 
     const std::string config_id_;
+    const envoy::config::tap::v3::TapConfig config_;
     const Server::AdminStream* admin_stream_;
   };
 

diff --git a/source/extensions/common/tap/extension_config_base.cc b/source/extensions/common/tap/extension_config_base.cc
@@ -36,7 +36,7 @@ ExtensionConfigBase::ExtensionConfigBase(
       throw EnvoyException(
           fmt::format("Error: Specifying admin streaming output without configuring admin."));
     }
-    installNewTap(envoy::config::tap::v3::TapConfig(proto_config_.static_config()), nullptr);
+    installNewTap(proto_config_.static_config(), nullptr);
     ENVOY_LOG(debug, "initializing tap extension with static config");
     break;
   }
@@ -67,20 +67,20 @@ void ExtensionConfigBase::clearTapConfig() {
   });
 }
 
-void ExtensionConfigBase::installNewTap(envoy::config::tap::v3::TapConfig&& proto_config,
+void ExtensionConfigBase::installNewTap(const envoy::config::tap::v3::TapConfig& proto_config,
                                         Sink* admin_streamer) {
   TapConfigSharedPtr new_config =
-      config_factory_->createConfigFromProto(std::move(proto_config), admin_streamer);
+      config_factory_->createConfigFromProto(proto_config, admin_streamer);
   tls_slot_->runOnAllThreads([new_config](ThreadLocal::ThreadLocalObjectSharedPtr object)
                                  -> ThreadLocal::ThreadLocalObjectSharedPtr {
     object->asType<TlsFilterConfig>().config_ = new_config;
     return object;
   });
 }
 
-void ExtensionConfigBase::newTapConfig(envoy::config::tap::v3::TapConfig&& proto_config,
+void ExtensionConfigBase::newTapConfig(const envoy::config::tap::v3::TapConfig& proto_config,
                                        Sink* admin_streamer) {
-  installNewTap(envoy::config::tap::v3::TapConfig(proto_config), admin_streamer);
+  installNewTap(proto_config, admin_streamer);
 }
 
 } // namespace Tap

diff --git a/source/extensions/common/tap/extension_config_base.h b/source/extensions/common/tap/extension_config_base.h
@@ -20,7 +20,7 @@ class ExtensionConfigBase : public ExtensionConfig, Logger::Loggable<Logger::Id:
   // Extensions::Common::Tap::ExtensionConfig
   void clearTapConfig() override;
   const absl::string_view adminId() override;
-  void newTapConfig(envoy::config::tap::v3::TapConfig&& proto_config,
+  void newTapConfig(const envoy::config::tap::v3::TapConfig& proto_config,
                     Sink* admin_streamer) override;
 
 protected:
@@ -40,7 +40,7 @@ class ExtensionConfigBase : public ExtensionConfig, Logger::Loggable<Logger::Id:
 private:
   // Holds the functionality of installing a new tap config. This is the underlying method to the
   // virtual method newTapConfig.
-  void installNewTap(envoy::config::tap::v3::TapConfig&& proto_config, Sink* admin_streamer);
+  void installNewTap(const envoy::config::tap::v3::TapConfig& proto_config, Sink* admin_streamer);
 
   struct TlsFilterConfig : public ThreadLocal::ThreadLocalObject {
     TapConfigSharedPtr config_;

diff --git a/source/extensions/common/tap/tap.h b/source/extensions/common/tap/tap.h
@@ -100,7 +100,7 @@ class ExtensionConfig {
    *        specifies that output type. May not be used if the configuration does not specify
    *        admin output. May be nullptr if admin is not used to supply the config.
    */
-  virtual void newTapConfig(envoy::config::tap::v3::TapConfig&& proto_config,
+  virtual void newTapConfig(const envoy::config::tap::v3::TapConfig& proto_config,
                             Sink* admin_streamer) PURE;
 };
 
@@ -167,8 +167,9 @@ class TapConfigFactory {
    * @return a new configuration given a raw tap service config proto. See
    * ExtensionConfig::newTapConfig() for param info.
    */
-  virtual TapConfigSharedPtr createConfigFromProto(envoy::config::tap::v3::TapConfig&& proto_config,
-                                                   Sink* admin_streamer) PURE;
+  virtual TapConfigSharedPtr
+  createConfigFromProto(const envoy::config::tap::v3::TapConfig& proto_config,
+                        Sink* admin_streamer) PURE;
 };
 
 using TapConfigFactoryPtr = std::unique_ptr<TapConfigFactory>;

diff --git a/source/extensions/common/tap/tap_config_base.cc b/source/extensions/common/tap/tap_config_base.cc
@@ -46,7 +46,7 @@ bool Utility::addBufferToProtoBytes(envoy::data::tap::v3::Body& output_body,
   }
 }
 
-TapConfigBaseImpl::TapConfigBaseImpl(envoy::config::tap::v3::TapConfig&& proto_config,
+TapConfigBaseImpl::TapConfigBaseImpl(const envoy::config::tap::v3::TapConfig& proto_config,
                                      Common::Tap::Sink* admin_streamer)
     : max_buffered_rx_bytes_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
           proto_config.output_config(), max_buffered_rx_bytes, DefaultMaxBufferedBytes)),

diff --git a/source/extensions/common/tap/tap_config_base.h b/source/extensions/common/tap/tap_config_base.h
@@ -101,7 +101,7 @@ class TapConfigBaseImpl : public virtual TapConfig {
   bool streaming() const override { return streaming_; }
 
 protected:
-  TapConfigBaseImpl(envoy::config::tap::v3::TapConfig&& proto_config,
+  TapConfigBaseImpl(const envoy::config::tap::v3::TapConfig& proto_config,
                     Common::Tap::Sink* admin_streamer);
 
 private:

diff --git a/source/extensions/filters/http/tap/config.cc b/source/extensions/filters/http/tap/config.cc
@@ -17,7 +17,7 @@ class HttpTapConfigFactoryImpl : public Extensions::Common::Tap::TapConfigFactor
 public:
   // TapConfigFactory
   Extensions::Common::Tap::TapConfigSharedPtr
-  createConfigFromProto(envoy::config::tap::v3::TapConfig&& proto_config,
+  createConfigFromProto(const envoy::config::tap::v3::TapConfig& proto_config,
                         Extensions::Common::Tap::Sink* admin_streamer) override {
     return std::make_shared<HttpTapConfigImpl>(std::move(proto_config), admin_streamer);
   }

diff --git a/source/extensions/filters/http/tap/tap_config_impl.cc b/source/extensions/filters/http/tap/tap_config_impl.cc
@@ -26,7 +26,7 @@ fillHeaderList(Protobuf::RepeatedPtrField<envoy::config::core::v3::HeaderValue>*
 }
 } // namespace
 
-HttpTapConfigImpl::HttpTapConfigImpl(envoy::config::tap::v3::TapConfig&& proto_config,
+HttpTapConfigImpl::HttpTapConfigImpl(const envoy::config::tap::v3::TapConfig& proto_config,
                                      Common::Tap::Sink* admin_streamer)
     : TapCommon::TapConfigBaseImpl(std::move(proto_config), admin_streamer) {}
 

diff --git a/source/extensions/filters/http/tap/tap_config_impl.h b/source/extensions/filters/http/tap/tap_config_impl.h
@@ -19,7 +19,7 @@ class HttpTapConfigImpl : public Extensions::Common::Tap::TapConfigBaseImpl,
                           public HttpTapConfig,
                           public std::enable_shared_from_this<HttpTapConfigImpl> {
 public:
-  HttpTapConfigImpl(envoy::config::tap::v3::TapConfig&& proto_config,
+  HttpTapConfigImpl(const envoy::config::tap::v3::TapConfig& proto_config,
                     Extensions::Common::Tap::Sink* admin_streamer);
 
   // TapFilter::HttpTapConfig

diff --git a/source/extensions/transport_sockets/tap/config.cc b/source/extensions/transport_sockets/tap/config.cc
@@ -22,7 +22,7 @@ class SocketTapConfigFactoryImpl : public Extensions::Common::Tap::TapConfigFact
 
   // TapConfigFactory
   Extensions::Common::Tap::TapConfigSharedPtr
-  createConfigFromProto(envoy::config::tap::v3::TapConfig&& proto_config,
+  createConfigFromProto(const envoy::config::tap::v3::TapConfig& proto_config,
                         Extensions::Common::Tap::Sink* admin_streamer) override {
     return std::make_shared<SocketTapConfigImpl>(std::move(proto_config), admin_streamer,
                                                  time_source_);

diff --git a/source/extensions/transport_sockets/tap/tap_config_impl.h b/source/extensions/transport_sockets/tap/tap_config_impl.h
@@ -50,7 +50,7 @@ class SocketTapConfigImpl : public Extensions::Common::Tap::TapConfigBaseImpl,
                             public SocketTapConfig,
                             public std::enable_shared_from_this<SocketTapConfigImpl> {
 public:
-  SocketTapConfigImpl(envoy::config::tap::v3::TapConfig&& proto_config,
+  SocketTapConfigImpl(const envoy::config::tap::v3::TapConfig& proto_config,
                       Extensions::Common::Tap::Sink* admin_streamer, TimeSource& time_system)
       : Extensions::Common::Tap::TapConfigBaseImpl(std::move(proto_config), admin_streamer),
         time_source_(time_system) {}

diff --git a/test/extensions/common/tap/admin_test.cc b/test/extensions/common/tap/admin_test.cc
@@ -23,7 +23,7 @@ class MockExtensionConfig : public ExtensionConfig {
   MOCK_METHOD(const absl::string_view, adminId, ());
   MOCK_METHOD(void, clearTapConfig, ());
   MOCK_METHOD(void, newTapConfig,
-              (envoy::config::tap::v3::TapConfig && proto_config, Sink* admin_streamer));
+              (const envoy::config::tap::v3::TapConfig& proto_config, Sink* admin_streamer));
 };
 
 class AdminHandlerTest : public testing::Test {

diff --git a/test/extensions/filters/http/tap/tap_filter_integration_test.cc b/test/extensions/filters/http/tap/tap_filter_integration_test.cc
@@ -256,6 +256,9 @@ config_id: test_config_id
 
   // Second request/response with no tap.
   makeRequest(request_headers_tap_, {}, nullptr, response_headers_no_tap_, {}, nullptr);
+  // The above admin tap close can race with the request that follows and we don't have any way
+  // to synchronize it, so just count the number of taps after the request for use below.
+  const auto current_tapped = test_server_->counter("http.config_test.tap.rq_tapped")->value();
 
   // Setup the tap again and leave it open.
   startAdminRequest(admin_request_yaml);
@@ -328,10 +331,59 @@ config_id: test_config_id
   TestUtility::loadFromYaml(admin_response_->body(), trace);
 
   admin_client_->close();
-  EXPECT_EQ(3UL, test_server_->counter("http.config_test.tap.rq_tapped")->value());
+  EXPECT_EQ(current_tapped + 3UL, test_server_->counter("http.config_test.tap.rq_tapped")->value());
   test_server_->waitForGaugeEq("http.admin.downstream_rq_active", 0);
 }
 
+// Make sure that an admin tap works correctly across an LDS reload.
+TEST_P(TapIntegrationTest, AdminLdsReload) {
+  initializeFilter(admin_filter_config_);
+
+  const std::string admin_request_yaml =
+      R"EOF(
+config_id: test_config_id
+tap_config:
+  match:
+    and_match:
+      rules:
+        - http_request_trailers_match:
+            headers:
+              - name: foo_trailer
+                exact_match: bar
+        - http_response_trailers_match:
+            headers:
+              - name: bar_trailer
+                exact_match: baz
+  output_config:
+    sinks:
+      - streaming_admin: {}
+)EOF";
+
+  startAdminRequest(admin_request_yaml);
+
+  ConfigHelper new_config_helper(version_, *api_,
+                                 MessageUtil::getJsonStringFromMessage(config_helper_.bootstrap()));
+  new_config_helper.addFilter(admin_filter_config_);
+  new_config_helper.renameListener("foo");
+  new_config_helper.setLds("1");
+  test_server_->waitForCounterGe("listener_manager.listener_create_success", 2);
+  registerTestServerPorts({"http"});
+
+  codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http")));
+  makeRequest(request_headers_no_tap_, {}, &request_trailers_, response_headers_no_tap_, {},
+              &response_trailers_);
+
+  envoy::data::tap::v3::TraceWrapper trace;
+  admin_response_->waitForBodyData(1);
+  TestUtility::loadFromYaml(admin_response_->body(), trace);
+  EXPECT_EQ("bar",
+            findHeader("foo_trailer", trace.http_buffered_trace().request().trailers())->value());
+  EXPECT_EQ("baz",
+            findHeader("bar_trailer", trace.http_buffered_trace().response().trailers())->value());
+
+  admin_client_->close();
+}
+
 // Verify both request and response trailer matching works.
 TEST_P(TapIntegrationTest, AdminTrailers) {
   initializeFilter(admin_filter_config_);

diff --git a/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc b/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc
@@ -678,7 +678,7 @@ TEST_P(SslTapIntegrationTest, RequestWithStreamingUpstreamTap) {
   std::vector<envoy::data::tap::v3::TraceWrapper> traces =
       Extensions::Common::Tap::readTracesFromFile(
           fmt::format("{}_{}.pb_length_delimited", path_prefix_, id));
-  ASSERT_EQ(4, traces.size());
+  ASSERT_GE(traces.size(), 4);
 
   // The initial connection message has no local address, but has a remote address (not connected
   // yet).