From 2bacbc43a7a89ab4bb594631a722ec7fa7681376 Mon Sep 17 00:00:00 2001 From: Ben Plotnick Date: Tue, 7 Jan 2025 23:59:10 -0500 Subject: [PATCH] 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 --- .../filters/http/dynamic_modules/filter.cc | 12 +++- .../filters/http/dynamic_modules/filter.h | 9 ++- test/extensions/dynamic_modules/http/BUILD | 13 ++++ .../dynamic_modules/http/integration_test.cc | 59 +++++++++++++++++++ .../dynamic_modules/test_data/rust/http.rs | 15 +++++ 5 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 test/extensions/dynamic_modules/http/integration_test.cc diff --git a/source/extensions/filters/http/dynamic_modules/filter.cc b/source/extensions/filters/http/dynamic_modules/filter.cc index 5fb713d70d04..f622bd6e216c 100644 --- a/source/extensions/filters/http/dynamic_modules/filter.cc +++ b/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) { diff --git a/source/extensions/filters/http/dynamic_modules/filter.h b/source/extensions/filters/http/dynamic_modules/filter.h index 6c3a367d3f00..47bba8f6672f 100644 --- a/source/extensions/filters/http/dynamic_modules/filter.h +++ b/source/extensions/filters/http/dynamic_modules/filter.h @@ -18,7 +18,7 @@ class DynamicModuleHttpFilter : public Http::StreamFilter, public std::enable_shared_from_this { 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(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; }; diff --git a/test/extensions/dynamic_modules/http/BUILD b/test/extensions/dynamic_modules/http/BUILD index a32f37918a53..27d3b2729794 100644 --- a/test/extensions/dynamic_modules/http/BUILD +++ b/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", + ], +) diff --git a/test/extensions/dynamic_modules/http/integration_test.cc b/test/extensions/dynamic_modules/http/integration_test.cc new file mode 100644 index 000000000000..41eab4a09fea --- /dev/null +++ b/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, + 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 diff --git a/test/extensions/dynamic_modules/test_data/rust/http.rs b/test/extensions/dynamic_modules/test_data/rust/http.rs index 04b0a2e8f34d..70089047c242 100644 --- a/test/extensions/dynamic_modules/test_data/rust/http.rs +++ b/test/extensions/dynamic_modules/test_data/rust/http.rs @@ -20,6 +20,7 @@ fn new_http_filter_config_fn( ) -> Option>> { 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 HttpFilter 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 HttpFilterConfig for PassthroughHttpFilterConfig { + fn new_http_filter(&self, _envoy: EnvoyHttpFilterConfig) -> Box> { + Box::new(PassthroughHttpFilter {}) + } +} + +struct PassthroughHttpFilter {} + +impl HttpFilter for PassthroughHttpFilter {} /// A HTTP filter configuration that implements /// [`envoy_proxy_dynamic_modules_rust_sdk::HttpFilterConfig`] to test the dynamic metadata related