dynamic_modules: don't call on_http_filter_destroy_ if filter is null (…

…#37899)

I tested dynamic modules from rust and was getting a segfault. I noticed
that drop was getting called twice. It turns out that `onDestroy` gets
called twice. The right solution here is probably to prevent
`onDestroy()` from being called twice... but barring that, this change
works.

Commit Message: dynamic_modules: don't call on_http_filter_destroy_ if
filter is null
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Ben Plotnick <[email protected]>

source/extensions/filters/http/dynamic_modules/filter.cc

@@ -7,13 +7,23 @@ namespace Extensions {
 namespace DynamicModules {
 namespace HttpFilters {
 
+DynamicModuleHttpFilter::~DynamicModuleHttpFilter() { destroy(); }
+
 void DynamicModuleHttpFilter::initializeInModuleFilter() {
   in_module_filter_ = config_->on_http_filter_new_(config_->in_module_config_, thisAsVoidPtr());
 }
 
 void DynamicModuleHttpFilter::onStreamComplete() {}
 
-void DynamicModuleHttpFilter::onDestroy() { config_->on_http_filter_destroy_(in_module_filter_); };
+void DynamicModuleHttpFilter::onDestroy() { destroy(); };
+
+void DynamicModuleHttpFilter::destroy() {
+  if (in_module_filter_ == nullptr) {
+    return;
+  }
+  config_->on_http_filter_destroy_(in_module_filter_);
+  in_module_filter_ = nullptr;
+}
 
 FilterHeadersStatus DynamicModuleHttpFilter::decodeHeaders(RequestHeaderMap& headers,
                                                            bool end_of_stream) {

source/extensions/filters/http/dynamic_modules/filter.h

@@ -18,7 +18,7 @@ class DynamicModuleHttpFilter : public Http::StreamFilter,
                                 public std::enable_shared_from_this<DynamicModuleHttpFilter> {
 public:
   DynamicModuleHttpFilter(DynamicModuleHttpFilterConfigSharedPtr config) : config_(config) {}
-  ~DynamicModuleHttpFilter() override = default;
+  ~DynamicModuleHttpFilter() override;
 
   /**
    * Initializes the in-module filter.
@@ -79,6 +79,13 @@ class DynamicModuleHttpFilter : public Http::StreamFilter,
    */
   void* thisAsVoidPtr() { return static_cast<void*>(this); }
 
+  /**
+   * Called when filter is destroyed via onDestroy() or destructor. Forwards the call to the
+   * module via on_http_filter_destroy_ and resets in_module_filter_ to null. Subsequent calls are a
+   * no-op.
+   */
+  void destroy();
+
   const DynamicModuleHttpFilterConfigSharedPtr config_ = nullptr;
   envoy_dynamic_module_type_http_filter_module_ptr in_module_filter_ = nullptr;
 };

test/extensions/dynamic_modules/http/BUILD

@@ -61,3 +61,16 @@ envoy_cc_test(
         "//test/mocks/http:http_mocks",
     ],
 )
+
+envoy_cc_test(
+    name = "integration_test",
+    srcs = ["integration_test.cc"],
+    data = [
+        "//test/extensions/dynamic_modules/test_data/rust:http",
+    ],
+    rbe_pool = "6gig",
+    deps = [
+        "//source/extensions/filters/http/dynamic_modules:factory_registration",
+        "//test/integration:http_integration_lib",
+    ],
+)

test/extensions/dynamic_modules/http/integration_test.cc

@@ -0,0 +1,59 @@
+#include "test/integration/http_integration.h"
+
+namespace Envoy {
+class DynamicModulesIntegrationTest : public testing::TestWithParam<Network::Address::IpVersion>,
+                                      public HttpIntegrationTest {
+public:
+  DynamicModulesIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP2, GetParam()){};
+
+  void initializeFilter(const std::string& module_name, const std::string& filter_name,
+                        const std::string& config = "") {
+    constexpr auto filter_config = R"EOF(
+name: envoy.extensions.filters.http.dynamic_modules
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.filters.http.dynamic_modules.v3.DynamicModuleFilter
+  dynamic_module_config:
+    name: {}
+  filter_name: {}
+  filter_config: {}
+)EOF";
+
+    config_helper_.prependFilter(fmt::format(filter_config, module_name, filter_name, config));
+    initialize();
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(IpVersions, DynamicModulesIntegrationTest,
+                         testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
+                         TestUtility::ipTestParamsToString);
+
+TEST_P(DynamicModulesIntegrationTest, Nop) {
+  TestEnvironment::setEnvVar(
+      "ENVOY_DYNAMIC_MODULES_SEARCH_PATH",
+      TestEnvironment::substitute(
+          "{{ test_rundir }}/test/extensions/dynamic_modules/test_data/rust"),
+      1);
+
+  initializeFilter("http", "passthrough");
+  // Create a client aimed at Envoy’s default HTTP port.
+  codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
+
+  // Create some request headers.
+  Http::TestRequestHeaderMapImpl request_headers{
+      {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}};
+
+  // Send the request headers from the client, wait until they are received upstream. When they
+  // are received, send the default response headers from upstream and wait until they are
+  // received at by client
+  auto response = sendRequestAndWaitForResponse(request_headers, 10, default_response_headers_, 10);
+
+  // Verify the proxied request was received upstream, as expected.
+  EXPECT_TRUE(upstream_request_->complete());
+  EXPECT_EQ(10U, upstream_request_->bodyLength());
+  // Verify the proxied response was received downstream, as expected.
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("200", response->headers().Status()->value().getStringView());
+  EXPECT_EQ(10U, response->body().size());
+}
+
+} // namespace Envoy

test/extensions/dynamic_modules/test_data/rust/http.rs

@@ -20,6 +20,7 @@ fn new_http_filter_config_fn<EHF: EnvoyHttpFilter>(
 ) -> Option<Box<dyn HttpFilterConfig<EHF>>> {
   match name {
     "header_callbacks" => Some(Box::new(HeaderCallbacksFilterConfig {})),
+    "passthrough" => Some(Box::new(PassthroughHttpFilterConfig {})),
     "dynamic_metadata_callbacks" => Some(Box::new(DynamicMetadataCallbacksFilterConfig {})),
     // TODO: add various configs for body, etc.
     _ => panic!("Unknown filter name: {}", name),
@@ -228,6 +229,20 @@ impl<EHF: EnvoyHttpFilter> HttpFilter<EHF> for HeaderCallbacksFilter {
   }
 }
 
+/// A HTTP filter configuration that implements
+/// [`envoy_proxy_dynamic_modules_rust_sdk::HttpFilterConfig`], but simply passes through to
+/// the default implementation.
+struct PassthroughHttpFilterConfig {}
+
+impl<EHF: EnvoyHttpFilter> HttpFilterConfig<EHF> for PassthroughHttpFilterConfig {
+  fn new_http_filter(&self, _envoy: EnvoyHttpFilterConfig) -> Box<dyn HttpFilter<EHF>> {
+    Box::new(PassthroughHttpFilter {})
+  }
+}
+
+struct PassthroughHttpFilter {}
+
+impl<EHF: EnvoyHttpFilter> HttpFilter<EHF> for PassthroughHttpFilter {}
 
 /// A HTTP filter configuration that implements
 /// [`envoy_proxy_dynamic_modules_rust_sdk::HttpFilterConfig`] to test the dynamic metadata related