diff --git a/source/extensions/dynamic_modules/abi.h b/source/extensions/dynamic_modules/abi.h index badf6b962e4a..bdefa5e9ec76 100644 --- a/source/extensions/dynamic_modules/abi.h +++ b/source/extensions/dynamic_modules/abi.h @@ -22,13 +22,46 @@ extern "C" { // ----------------------------------------------------------------------------- // // Types used in the ABI. The name of a type must be prefixed with "envoy_dynamic_module_type_". +// Types with "_module_ptr" suffix are pointers owned by the module, i.e. memory space allocated by +// the module. Types with "_envoy_ptr" suffix are pointers owned by Envoy, i.e. memory space +// allocated by Envoy. /** - * envoy_dynamic_module_type_abi_version represents a null-terminated string that contains the ABI - * version of the dynamic module. This is used to ensure that the dynamic module is built against - * the compatible version of the ABI. + * envoy_dynamic_module_type_abi_version_envoy_ptr represents a null-terminated string that + * contains the ABI version of the dynamic module. This is used to ensure that the dynamic module is + * built against the compatible version of the ABI. + * + * OWNERSHIP: Envoy owns the pointer. + */ +typedef const char* // NOLINT(modernize-use-using) + envoy_dynamic_module_type_abi_version_envoy_ptr; + +/** + * envoy_dynamic_module_type_http_filter_config_envoy_ptr is a raw pointer to + * the DynamicModuleHttpFilterConfig class in Envoy. This is passed to the module when + * creating a new in-module HTTP filter configuration and used to access the HTTP filter-scoped + * information such as metadata, metrics, etc. + * + * This has 1:1 correspondence with envoy_dynamic_module_type_http_filter_config_module_ptr in + * the module. + * + * OWNERSHIP: Envoy owns the pointer. + */ +typedef const void* // NOLINT(modernize-use-using) + envoy_dynamic_module_type_http_filter_config_envoy_ptr; + +/** + * envoy_dynamic_module_type_http_filter_config_module_ptr is a pointer to an in-module HTTP + * configuration corresponding to an Envoy HTTP filter configuration. The config is responsible for + * creating a new HTTP filter that corresponds to each HTTP stream. + * + * This has 1:1 correspondence with the DynamicModuleHttpFilterConfig class in Envoy. + * + * OWNERSHIP: The module is responsible for managing the lifetime of the pointer. The pointer can be + * released when envoy_dynamic_module_on_http_filter_config_destroy is called for the same pointer. */ -typedef const char* envoy_dynamic_module_type_abi_version; // NOLINT(modernize-use-using) +typedef const void* // NOLINT(modernize-use-using) + envoy_dynamic_module_type_http_filter_config_module_ptr; // ----------------------------------------------------------------------------- // ------------------------------- Event Hooks --------------------------------- @@ -54,10 +87,40 @@ typedef const char* envoy_dynamic_module_type_abi_version; // NOLINT(modernize-u * to check compatibility and gracefully fail the initialization because there is no way to * report an error to Envoy. * - * @return envoy_dynamic_module_type_abi_version is the ABI version of the dynamic module. Null - * means the error and the module will be unloaded immediately. + * @return envoy_dynamic_module_type_abi_version_envoy_ptr is the ABI version of the dynamic + * module. Null means the error and the module will be unloaded immediately. + */ +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init(); + +/** + * envoy_dynamic_module_on_http_filter_config_new is called by the main thread when the http + * filter config is loaded. The function returns a + * envoy_dynamic_module_type_http_filter_config_module_ptr for given name and config. + * + * @param filter_config_envoy_ptr is the pointer to the DynamicModuleHttpFilterConfig object for the + * corresponding config. + * @param name_ptr is the name of the filter. + * @param name_size is the size of the name. + * @param config_ptr is the configuration for the module. + * @param config_size is the size of the configuration. + * @return envoy_dynamic_module_type_http_filter_config_module_ptr is the pointer to the + * in-module HTTP filter configuration. Returning nullptr indicates a failure to initialize the + * module. When it fails, the filter configuration will be rejected. + */ +envoy_dynamic_module_type_http_filter_config_module_ptr +envoy_dynamic_module_on_http_filter_config_new( + envoy_dynamic_module_type_http_filter_config_envoy_ptr filter_config_envoy_ptr, + const char* name_ptr, int name_size, const char* config_ptr, int config_size); + +/** + * envoy_dynamic_module_on_http_filter_config_destroy is called when the HTTP filter configuration + * is destroyed in Envoy. The module should release any resources associated with the corresponding + * in-module HTTP filter configuration. + * @param filter_config_ptr is a pointer to the in-module HTTP filter configuration whose + * corresponding Envoy HTTP filter configuration is being destroyed. */ -envoy_dynamic_module_type_abi_version envoy_dynamic_module_on_program_init(); +void envoy_dynamic_module_on_http_filter_config_destroy( + envoy_dynamic_module_type_http_filter_config_module_ptr filter_config_ptr); #ifdef __cplusplus } diff --git a/source/extensions/dynamic_modules/abi_version.h b/source/extensions/dynamic_modules/abi_version.h index 1e17a9a9384b..e9ccc2f60070 100644 --- a/source/extensions/dynamic_modules/abi_version.h +++ b/source/extensions/dynamic_modules/abi_version.h @@ -6,7 +6,7 @@ namespace DynamicModules { #endif // This is the ABI version calculated as a sha256 hash of the ABI header files. When the ABI // changes, this value must change, and the correctness of this value is checked by the test. -const char* kAbiVersion = "4293760426255b24c25b97a18d9fd31b4d1956f10ba0ff2f723580a46ee8fa21"; +const char* kAbiVersion = "164a60ff214ca3cd62526ddb7c3fe21cf943e8721a115c87feca81a58510072c"; #ifdef __cplusplus } // namespace DynamicModules diff --git a/source/extensions/dynamic_modules/dynamic_modules.cc b/source/extensions/dynamic_modules/dynamic_modules.cc index 756c1c5b17ff..3d14796fba47 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.cc +++ b/source/extensions/dynamic_modules/dynamic_modules.cc @@ -16,8 +16,8 @@ namespace DynamicModules { constexpr char DYNAMIC_MODULES_SEARCH_PATH[] = "ENVOY_DYNAMIC_MODULES_SEARCH_PATH"; -absl::StatusOr newDynamicModule(const absl::string_view object_file_path, - const bool do_not_close) { +absl::StatusOr newDynamicModule(const absl::string_view object_file_path, + const bool do_not_close) { // RTLD_LOCAL is always needed to avoid collisions between multiple modules. // RTLD_LAZY is required for not only performance but also simply to load the module, otherwise // dlopen results in Invalid argument. @@ -33,7 +33,7 @@ absl::StatusOr newDynamicModule(const absl::string_view absl::StrCat("Failed to load dynamic module: ", object_file_path, " : ", dlerror())); } - DynamicModuleSharedPtr dynamic_module = std::make_shared(handle); + DynamicModulePtr dynamic_module = std::make_unique(handle); const auto init_function = dynamic_module->getFunctionPointer( @@ -41,7 +41,7 @@ absl::StatusOr newDynamicModule(const absl::string_view if (init_function == nullptr) { return absl::InvalidArgumentError( - absl::StrCat("Failed to resolve envoy_dynamic_module_on_program_init: ", dlerror())); + "Failed to resolve symbol envoy_dynamic_module_on_program_init"); } const char* abi_version = (*init_function)(); @@ -57,8 +57,8 @@ absl::StatusOr newDynamicModule(const absl::string_view return dynamic_module; } -absl::StatusOr newDynamicModuleByName(const absl::string_view module_name, - const bool do_not_close) { +absl::StatusOr newDynamicModuleByName(const absl::string_view module_name, + const bool do_not_close) { const char* module_search_path = getenv(DYNAMIC_MODULES_SEARCH_PATH); if (module_search_path == nullptr) { return absl::InvalidArgumentError(absl::StrCat("Failed to load dynamic module: ", module_name, diff --git a/source/extensions/dynamic_modules/dynamic_modules.h b/source/extensions/dynamic_modules/dynamic_modules.h index be1446f9df57..ed4e42aa024a 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.h +++ b/source/extensions/dynamic_modules/dynamic_modules.h @@ -47,7 +47,7 @@ class DynamicModule { void* handle_; }; -using DynamicModuleSharedPtr = std::shared_ptr; +using DynamicModulePtr = std::unique_ptr; /** * Creates a new DynamicModule. This is mainly exposed for testing purposes. Use @@ -58,8 +58,8 @@ using DynamicModuleSharedPtr = std::shared_ptr; * terminated. For example, c-shared objects compiled by Go doesn't support dlclose * https://github.com/golang/go/issues/11100. */ -absl::StatusOr newDynamicModule(const absl::string_view object_file_path, - const bool do_not_close); +absl::StatusOr newDynamicModule(const absl::string_view object_file_path, + const bool do_not_close); /** * Creates a new DynamicModule by name under the search path specified by the environment variable @@ -70,8 +70,8 @@ absl::StatusOr newDynamicModule(const absl::string_view * will not be destroyed. This is useful when an object has some global state that should not be * terminated. */ -absl::StatusOr newDynamicModuleByName(const absl::string_view module_name, - const bool do_not_close); +absl::StatusOr newDynamicModuleByName(const absl::string_view module_name, + const bool do_not_close); } // namespace DynamicModules } // namespace Extensions diff --git a/source/extensions/dynamic_modules/sdk/rust/src/lib.rs b/source/extensions/dynamic_modules/sdk/rust/src/lib.rs index 5796d12fe9b1..864ab825558c 100644 --- a/source/extensions/dynamic_modules/sdk/rust/src/lib.rs +++ b/source/extensions/dynamic_modules/sdk/rust/src/lib.rs @@ -3,6 +3,8 @@ #![allow(non_snake_case)] #![allow(dead_code)] +use std::sync::OnceLock; + /// This module contains the generated bindings for the envoy dynamic modules ABI. /// /// This is not meant to be used directly. @@ -10,28 +12,42 @@ pub mod abi { include!(concat!(env!("OUT_DIR"), "/bindings.rs")); } -/// Declare the init function for the dynamic module. This function is called when the dynamic module is loaded. -/// The function must return true on success, and false on failure. When it returns false, -/// the dynamic module will not be loaded. +/// Declare the init functions for the dynamic module. /// -/// This is useful to perform any process-wide initialization that the dynamic module needs. +/// The first argument has [`ProgramInitFunction`] type, and it is called when the dynamic module is loaded. +/// +/// The second argument has [`NewHttpFilterConfigFunction`] type, and it is called when the new HTTP filter configuration is created. /// /// # Example /// /// ``` -/// use envoy_proxy_dynamic_modules_rust_sdk::declare_program_init; +/// use envoy_proxy_dynamic_modules_rust_sdk::*; /// -/// declare_program_init!(my_program_init); +/// declare_init_functions!(my_program_init, my_new_http_filter_config_fn); /// /// fn my_program_init() -> bool { /// true /// } +/// +/// fn my_new_http_filter_config_fn( +/// _envoy_filter_config: EnvoyHttpFilterConfig, +/// _name: &str, +/// _config: &str, +/// ) -> Option> { +/// Some(Box::new(MyHttpFilterConfig {})) +/// } +/// +/// struct MyHttpFilterConfig {} +/// +/// impl HttpFilterConfig for MyHttpFilterConfig {} /// ``` #[macro_export] -macro_rules! declare_program_init { - ($f:ident) => { +macro_rules! declare_init_functions { + ($f:ident,$new_http_filter_config_fn:expr) => { #[no_mangle] pub extern "C" fn envoy_dynamic_module_on_program_init() -> *const ::std::os::raw::c_char { + envoy_proxy_dynamic_modules_rust_sdk::NEW_HTTP_FILTER_CONFIG_FUNCTION + .get_or_init(|| $new_http_filter_config_fn); if ($f()) { envoy_proxy_dynamic_modules_rust_sdk::abi::kAbiVersion.as_ptr() as *const ::std::os::raw::c_char @@ -41,3 +57,181 @@ macro_rules! declare_program_init { } }; } + +/// The function signature for the program init function. +/// +/// This is called when the dynamic module is loaded, and it must return true on success, and false on failure. When it returns false, +/// the dynamic module will not be loaded. +/// +/// This is useful to perform any process-wide initialization that the dynamic module needs. +pub type ProgramInitFunction = fn() -> bool; + +/// The function signature for the new HTTP filter configuration function. +/// +/// This is called when a new HTTP filter configuration is created, and it must return a new instance of the [`HttpFilterConfig`] object. +/// Returning `None` will cause the HTTP filter configuration to be rejected. +// +// TODO(@mathetake): I guess there would be a way to avoid the use of dyn in the first place. +// E.g. one idea is to accept all concrete type parameters for HttpFilterConfig and HttpFilter traits in declare_init_functions!, +// and generate the match statement based on that. +pub type NewHttpFilterConfigFunction = fn( + envoy_filter_config: EnvoyHttpFilterConfig, + name: &str, + config: &str, +) -> Option>; + +/// The global init function for HTTP filter configurations. This is set via the `declare_init_functions` macro, +/// and is not intended to be set directly. +pub static NEW_HTTP_FILTER_CONFIG_FUNCTION: OnceLock = OnceLock::new(); + +/// The trait that represents the configuration for an Envoy Http filter configuration. +/// This has one to one mapping with the [`EnvoyHttpFilterConfig`] object. +/// +/// The object is created when the corresponding Envoy Http filter config is created, and it is +/// dropped when the corresponding Envoy Http filter config is destroyed. Therefore, the imlementation +/// is recommended to implement the [`Drop`] trait to handle the necessary cleanup. +pub trait HttpFilterConfig { + /// This is called when a HTTP filter chain is created for a new stream. + fn new_http_filter(&self) -> Box { + unimplemented!() // TODO. + } +} + +/// The trait that represents an Envoy Http filter for each stream. +pub trait HttpFilter {} // TODO. + +/// An opaque object that represents the underlying Envoy Http filter config. This has one to one +/// mapping with the Envoy Http filter config object as well as [`HttpFilterConfig`] object. +/// +/// This is a shallow wrapper around the raw pointer to the Envoy HTTP filter config object, and it +/// can be copied and used up until the corresponding [`HttpFilterConfig`] is dropped. +// +// TODO(@mathetake): make this only avaialble for non-test code, and provide a mock for testing so that users +// can write unit tests for their HttpFilterConfig implementations. +#[derive(Debug, Clone, Copy)] +pub struct EnvoyHttpFilterConfig { + raw_ptr: abi::envoy_dynamic_module_type_http_filter_config_envoy_ptr, +} + +#[no_mangle] +unsafe extern "C" fn envoy_dynamic_module_on_http_filter_config_new( + envoy_filter_config_ptr: abi::envoy_dynamic_module_type_http_filter_config_envoy_ptr, + name_ptr: *const u8, + name_size: usize, + config_ptr: *const u8, + config_size: usize, +) -> abi::envoy_dynamic_module_type_http_filter_config_module_ptr { + // This assumes that the name and config are valid UTF-8 strings. Should we relax? At the moment, both are String at protobuf level. + let name = + std::str::from_utf8(std::slice::from_raw_parts(name_ptr, name_size)).unwrap_or_default(); + let config = std::str::from_utf8(std::slice::from_raw_parts(config_ptr, config_size)) + .unwrap_or_default(); + + let envoy_filter_config = EnvoyHttpFilterConfig { + raw_ptr: envoy_filter_config_ptr, + }; + + envoy_dynamic_module_on_http_filter_config_new_impl( + envoy_filter_config, + name, + config, + NEW_HTTP_FILTER_CONFIG_FUNCTION + .get() + .expect("NEW_HTTP_FILTER_CONFIG_FUNCTION must be set"), + ) +} + +fn envoy_dynamic_module_on_http_filter_config_new_impl( + envoy_filter_config: EnvoyHttpFilterConfig, + name: &str, + config: &str, + new_fn: &NewHttpFilterConfigFunction, +) -> abi::envoy_dynamic_module_type_http_filter_config_module_ptr { + if let Some(config) = new_fn(envoy_filter_config, name, config) { + let boxed_filter_config_ptr = Box::into_raw(Box::new(config)); + boxed_filter_config_ptr as abi::envoy_dynamic_module_type_http_filter_config_module_ptr + } else { + std::ptr::null() + } +} + +#[no_mangle] +unsafe extern "C" fn envoy_dynamic_module_on_http_filter_config_destroy( + config_ptr: abi::envoy_dynamic_module_type_http_filter_config_module_ptr, +) { + let config = config_ptr as *mut *mut dyn HttpFilterConfig; + + // Drop the Box<*mut dyn HttpFilterConfig>, and then the Box, which also drops the underlying object. + let _outer = Box::from_raw(config); + let _inner = Box::from_raw(*config); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_envoy_dynamic_module_on_http_filter_config_new_impl() { + struct TestHttpFilterConfig; + impl HttpFilterConfig for TestHttpFilterConfig {} + + let envoy_filter_config = EnvoyHttpFilterConfig { + raw_ptr: std::ptr::null_mut(), + }; + let mut new_fn: NewHttpFilterConfigFunction = + |_, _, _| Some(Box::new(TestHttpFilterConfig)); + let result = envoy_dynamic_module_on_http_filter_config_new_impl( + envoy_filter_config, + "test_name", + "test_config", + &new_fn, + ); + assert!(!result.is_null()); + + unsafe { + envoy_dynamic_module_on_http_filter_config_destroy(result); + } + + // None should result in null pointer. + new_fn = |_, _, _| None; + let result = envoy_dynamic_module_on_http_filter_config_new_impl( + envoy_filter_config, + "test_name", + "test_config", + &new_fn, + ); + assert!(result.is_null()); + } + + #[test] + fn test_envoy_dynamic_module_on_http_filter_config_destroy() { + use std::sync::atomic::AtomicBool; + + // This test is mainly to check if the drop is called correctly after wrapping/unwrapping the Box. + static DROPPED: AtomicBool = AtomicBool::new(false); + struct TestHttpFilterConfig; + impl HttpFilterConfig for TestHttpFilterConfig {} + impl Drop for TestHttpFilterConfig { + fn drop(&mut self) { + DROPPED.store(true, std::sync::atomic::Ordering::SeqCst); + } + } + + // This is a sort of round-trip to ensure the same control flow as the actual usage. + let new_fn: NewHttpFilterConfigFunction = |_, _, _| Some(Box::new(TestHttpFilterConfig)); + let config_ptr = envoy_dynamic_module_on_http_filter_config_new_impl( + EnvoyHttpFilterConfig { + raw_ptr: std::ptr::null_mut(), + }, + "test_name", + "test_config", + &new_fn, + ); + + unsafe { + envoy_dynamic_module_on_http_filter_config_destroy(config_ptr); + } + // Now that the drop is called, DROPPED must be set to true. + assert!(DROPPED.load(std::sync::atomic::Ordering::SeqCst)); + } +} diff --git a/source/extensions/filters/http/dynamic_modules/factory.cc b/source/extensions/filters/http/dynamic_modules/factory.cc index 29060729d6f9..4d2b45dea960 100644 --- a/source/extensions/filters/http/dynamic_modules/factory.cc +++ b/source/extensions/filters/http/dynamic_modules/factory.cc @@ -14,20 +14,28 @@ absl::StatusOr DynamicModuleConfigFactory::createFilterFa raw_config, context.messageValidationVisitor()); const auto& module_config = proto_config.dynamic_module_config(); - const auto dynamic_module = Extensions::DynamicModules::newDynamicModuleByName( + auto dynamic_module = Extensions::DynamicModules::newDynamicModuleByName( module_config.name(), module_config.do_not_close()); if (!dynamic_module.ok()) { return absl::InvalidArgumentError("Failed to load dynamic module: " + std::string(dynamic_module.status().message())); } - auto filter_config = std::make_shared< - Envoy::Extensions::DynamicModules::HttpFilters::DynamicModuleHttpFilterConfig>( - proto_config.filter_name(), proto_config.filter_config(), dynamic_module.value()); - return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { + absl::StatusOr< + Envoy::Extensions::DynamicModules::HttpFilters::DynamicModuleHttpFilterConfigSharedPtr> + filter_config = + Envoy::Extensions::DynamicModules::HttpFilters::newDynamicModuleHttpFilterConfig( + proto_config.filter_name(), proto_config.filter_config(), + std::move(dynamic_module.value())); + + if (!filter_config.ok()) { + return absl::InvalidArgumentError("Failed to create filter config: " + + std::string(filter_config.status().message())); + } + return [config = filter_config.value()](Http::FilterChainFactoryCallbacks& callbacks) -> void { auto filter = std::make_shared( - filter_config); + config); callbacks.addStreamDecoderFilter(filter); callbacks.addStreamEncoderFilter(filter); }; diff --git a/source/extensions/filters/http/dynamic_modules/filter_config.cc b/source/extensions/filters/http/dynamic_modules/filter_config.cc index db3cc5094fa7..e3b8f579459c 100644 --- a/source/extensions/filters/http/dynamic_modules/filter_config.cc +++ b/source/extensions/filters/http/dynamic_modules/filter_config.cc @@ -7,10 +7,50 @@ namespace HttpFilters { DynamicModuleHttpFilterConfig::DynamicModuleHttpFilterConfig( const absl::string_view filter_name, const absl::string_view filter_config, - Extensions::DynamicModules::DynamicModuleSharedPtr dynamic_module) - : filter_name_(filter_name), filter_config_(filter_config), dynamic_module_(dynamic_module){}; + Extensions::DynamicModules::DynamicModulePtr dynamic_module) + : filter_name_(filter_name), filter_config_(filter_config), + dynamic_module_(std::move(dynamic_module)){}; -DynamicModuleHttpFilterConfig::~DynamicModuleHttpFilterConfig() = default; +DynamicModuleHttpFilterConfig::~DynamicModuleHttpFilterConfig() { + (*in_module_config_destroy_)(in_module_config_); +}; + +absl::StatusOr +newDynamicModuleHttpFilterConfig(const absl::string_view filter_name, + const absl::string_view filter_config, + Extensions::DynamicModules::DynamicModulePtr dynamic_module) { + auto constructor = + dynamic_module->getFunctionPointer( + "envoy_dynamic_module_on_http_filter_config_new"); + if (constructor == nullptr) { + return absl::InvalidArgumentError( + "Failed to resolve symbol envoy_dynamic_module_on_http_filter_config_new"); + } + + auto destroy = + dynamic_module + ->getFunctionPointer( + "envoy_dynamic_module_on_http_filter_config_destroy"); + if (destroy == nullptr) { + return absl::InvalidArgumentError( + "Failed to resolve symbol envoy_dynamic_module_on_http_filter_config_destroy"); + } + + auto config = std::make_shared(filter_name, filter_config, + std::move(dynamic_module)); + + const void* filter_config_envoy_ptr = + (*constructor)(static_cast(config.get()), filter_name.data(), filter_name.size(), + filter_config.data(), filter_config.size()); + + if (filter_config_envoy_ptr == nullptr) { + return absl::InvalidArgumentError("Failed to initialize dynamic module"); + } + + config->in_module_config_ = filter_config_envoy_ptr; + config->in_module_config_destroy_ = destroy; + return config; +} } // namespace HttpFilters } // namespace DynamicModules diff --git a/source/extensions/filters/http/dynamic_modules/filter_config.h b/source/extensions/filters/http/dynamic_modules/filter_config.h index 90a72098dcf3..8eb4ec592c25 100644 --- a/source/extensions/filters/http/dynamic_modules/filter_config.h +++ b/source/extensions/filters/http/dynamic_modules/filter_config.h @@ -1,5 +1,6 @@ #pragma once +#include "source/extensions/dynamic_modules/abi.h" #include "source/extensions/dynamic_modules/dynamic_modules.h" namespace Envoy { @@ -21,10 +22,16 @@ class DynamicModuleHttpFilterConfig { */ DynamicModuleHttpFilterConfig(const absl::string_view filter_name, const absl::string_view filter_config, - Extensions::DynamicModules::DynamicModuleSharedPtr dynamic_module); + Extensions::DynamicModules::DynamicModulePtr dynamic_module); ~DynamicModuleHttpFilterConfig(); + // The corresponding in-module configuration. + envoy_dynamic_module_type_http_filter_config_module_ptr in_module_config_ = nullptr; + + // The function pointer to the module's destroy function, resolved in the constructor. + decltype(&envoy_dynamic_module_on_http_filter_config_destroy) in_module_config_destroy_ = nullptr; + private: // The name of the filter passed in the constructor. const std::string filter_name_; @@ -33,11 +40,23 @@ class DynamicModuleHttpFilterConfig { const std::string filter_config_; // The handle for the module. - Extensions::DynamicModules::DynamicModuleSharedPtr dynamic_module_; + Extensions::DynamicModules::DynamicModulePtr dynamic_module_; }; using DynamicModuleHttpFilterConfigSharedPtr = std::shared_ptr; +/** + * Creates a new DynamicModuleHttpFilterConfig for given configuration. + * @param filter_name the name of the filter. + * @param filter_config the configuration for the module. + * @param dynamic_module the dynamic module to use. + * @return a shared pointer to the new config object or an error if the module could not be loaded. + */ +absl::StatusOr +newDynamicModuleHttpFilterConfig(const absl::string_view filter_name, + const absl::string_view filter_config, + Extensions::DynamicModules::DynamicModulePtr dynamic_module); + } // namespace HttpFilters } // namespace DynamicModules } // namespace Extensions diff --git a/test/extensions/dynamic_modules/BUILD b/test/extensions/dynamic_modules/BUILD index bba4dfe4fe95..6d3cc1ab65cb 100644 --- a/test/extensions/dynamic_modules/BUILD +++ b/test/extensions/dynamic_modules/BUILD @@ -1,3 +1,10 @@ +load( + "@rules_rust//rust:defs.bzl", + "rust_clippy", + "rust_doc_test", + "rust_test", + "rustfmt_test", +) load( "//bazel:envoy_build_system.bzl", "envoy_cc_test", @@ -54,3 +61,41 @@ envoy_cc_test_library( "//test/test_common:utility_lib", ], ) + +# We have targets for the tests of SDK itself here so that //test/... will be able to run them. +rust_test( + name = "rust_sdk_test", + crate = "//source/extensions/dynamic_modules/sdk/rust:envoy_proxy_dynamic_modules_rust_sdk", + tags = [ + # It is a known issue that TSAN detectes a false positive in the test runner of Rust toolchain: + # https://github.com/rust-lang/rust/issues/39608 + # To avoid this issue, we need to use nightly and pass RUSTFLAGS="-Zsanitizer=thread" to this target only + # when we run the test with TSAN: https://github.com/rust-lang/rust/commit/4b91729df22015bd412f6fc0fa397785d1e2159c + # However, that causes symbol conflicts between the sanitizer built by Rust and the one built by Bazel. + # Moreover, we also need to rebuild the Rust std-lib with the cargo option "-Zbuild-std", but that is + # not supported by the rules_rust yet: https://github.com/bazelbuild/rules_rust/issues/2068 + # So, we disable TSAN for now. In contrast, ASAN works without any issue. + "no_tsan", + "nocoverage", + ], +) + +rust_doc_test( + name = "rust_sdk_doc_test", + crate = "//source/extensions/dynamic_modules/sdk/rust:envoy_proxy_dynamic_modules_rust_sdk", + tags = ["nocoverage"], +) + +# As per the discussion in https://github.com/envoyproxy/envoy/pull/35627, +# we set the rust_fmt and clippy target here instead of the part of //tools/code_format target for now. +rustfmt_test( + name = "rust_sdk_fmt", + tags = ["nocoverage"], + targets = ["//source/extensions/dynamic_modules/sdk/rust:envoy_proxy_dynamic_modules_rust_sdk"], +) + +rust_clippy( + name = "rust_sdk_clippy", + tags = ["nocoverage"], + deps = ["//source/extensions/dynamic_modules/sdk/rust:envoy_proxy_dynamic_modules_rust_sdk"], +) diff --git a/test/extensions/dynamic_modules/dynamic_modules_test.cc b/test/extensions/dynamic_modules/dynamic_modules_test.cc index 30106f9282cb..fb70026c5ec3 100644 --- a/test/extensions/dynamic_modules/dynamic_modules_test.cc +++ b/test/extensions/dynamic_modules/dynamic_modules_test.cc @@ -10,7 +10,7 @@ namespace Extensions { namespace DynamicModules { TEST(DynamicModuleTestGeneral, InvalidPath) { - absl::StatusOr result = newDynamicModule("invalid_name", false); + absl::StatusOr result = newDynamicModule("invalid_name", false); EXPECT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); } @@ -21,7 +21,7 @@ INSTANTIATE_TEST_SUITE_P(LanguageTests, DynamicModuleTestLanguages, testing::Val TEST_P(DynamicModuleTestLanguages, DoNotClose) { std::string language = GetParam(); using GetSomeVariableFuncType = int (*)(); - absl::StatusOr module = + absl::StatusOr module = newDynamicModule(testSharedObjectPath("no_op", language), false); EXPECT_TRUE(module.ok()); const auto getSomeVariable = @@ -59,24 +59,24 @@ TEST_P(DynamicModuleTestLanguages, DoNotClose) { TEST_P(DynamicModuleTestLanguages, LoadNoOp) { std::string language = GetParam(); - absl::StatusOr module = + absl::StatusOr module = newDynamicModule(testSharedObjectPath("no_op", language), false); EXPECT_TRUE(module.ok()); } TEST_P(DynamicModuleTestLanguages, NoProgramInit) { std::string language = GetParam(); - absl::StatusOr result = + absl::StatusOr result = newDynamicModule(testSharedObjectPath("no_program_init", language), false); EXPECT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_THAT(result.status().message(), - testing::HasSubstr("undefined symbol: envoy_dynamic_module_on_program_init")); + testing::HasSubstr("Failed to resolve symbol envoy_dynamic_module_on_program_init")); } TEST_P(DynamicModuleTestLanguages, ProgramInitFail) { std::string language = GetParam(); - absl::StatusOr result = + absl::StatusOr result = newDynamicModule(testSharedObjectPath("program_init_fail", language), false); EXPECT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); @@ -86,7 +86,7 @@ TEST_P(DynamicModuleTestLanguages, ProgramInitFail) { TEST_P(DynamicModuleTestLanguages, ABIVersionMismatch) { std::string language = GetParam(); - absl::StatusOr result = + absl::StatusOr result = newDynamicModule(testSharedObjectPath("abi_version_mismatch", language), false); EXPECT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); @@ -101,14 +101,14 @@ TEST(CreateDynamicModulesByName, OK) { "{{ test_rundir }}/test/extensions/dynamic_modules/test_data/rust"), 1); - absl::StatusOr module = newDynamicModuleByName("no_op", false); + absl::StatusOr module = newDynamicModuleByName("no_op", false); EXPECT_TRUE(module.ok()); TestEnvironment::unsetEnvVar("ENVOY_DYNAMIC_MODULES_SEARCH_PATH"); } TEST(CreateDynamicModulesByName, EnvVarNotSet) { // Without setting the search path, this should fail. - absl::StatusOr module = newDynamicModuleByName("no_op", false); + absl::StatusOr module = newDynamicModuleByName("no_op", false); EXPECT_FALSE(module.ok()); EXPECT_EQ(module.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_THAT(module.status().message(), diff --git a/test/extensions/dynamic_modules/http/BUILD b/test/extensions/dynamic_modules/http/BUILD index d0add5f413f9..85f9279c1fc3 100644 --- a/test/extensions/dynamic_modules/http/BUILD +++ b/test/extensions/dynamic_modules/http/BUILD @@ -12,7 +12,9 @@ envoy_cc_test( name = "factory_test", srcs = ["factory_test.cc"], data = [ - "//test/extensions/dynamic_modules/test_data/rust:no_op", + "//test/extensions/dynamic_modules/test_data/c:no_http_config_destory", + "//test/extensions/dynamic_modules/test_data/c:no_http_config_new", + "//test/extensions/dynamic_modules/test_data/c:no_op", ], # factory_context_mocks needs this. rbe_pool = "6gig", diff --git a/test/extensions/dynamic_modules/http/factory_test.cc b/test/extensions/dynamic_modules/http/factory_test.cc index 3aa0f85201f1..e6e3485833b4 100644 --- a/test/extensions/dynamic_modules/http/factory_test.cc +++ b/test/extensions/dynamic_modules/http/factory_test.cc @@ -19,8 +19,7 @@ TEST(DynamicModuleConfigFactory, Overrides) { TEST(DynamicModuleConfigFactory, LoadOK) { TestEnvironment::setEnvVar( "ENVOY_DYNAMIC_MODULES_SEARCH_PATH", - TestEnvironment::substitute( - "{{ test_rundir }}/test/extensions/dynamic_modules/test_data/rust"), + TestEnvironment::substitute("{{ test_rundir }}/test/extensions/dynamic_modules/test_data/c"), 1); envoy::extensions::filters::http::dynamic_modules::v3::DynamicModuleFilter config; @@ -49,6 +48,12 @@ filter_config: bar } TEST(DynamicModuleConfigFactory, LoadError) { + TestEnvironment::setEnvVar( + "ENVOY_DYNAMIC_MODULES_SEARCH_PATH", + TestEnvironment::substitute("{{ test_rundir }}/test/extensions/dynamic_modules/test_data/c"), + 1); + + // Non existent module. envoy::extensions::filters::http::dynamic_modules::v3::DynamicModuleFilter config; const std::string yaml = R"EOF( dynamic_module_config: @@ -67,6 +72,42 @@ filter_config: bar EXPECT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_THAT(result.status().message(), testing::HasSubstr("Failed to load dynamic module:")); + + // The init function envoy_dynamic_module_on_http_filter_config_new is not defined. + const std::string yaml2 = R"EOF( +dynamic_module_config: + name: no_http_config_new +filter_name: foo +filter_config: bar +)EOF"; + + envoy::extensions::filters::http::dynamic_modules::v3::DynamicModuleFilter proto_config2; + TestUtility::loadFromYamlAndValidate(yaml2, proto_config2); + + auto result2 = factory.createFilterFactoryFromProto(proto_config2, "", context); + EXPECT_FALSE(result2.ok()); + EXPECT_EQ(result2.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_THAT(result2.status().message(), + testing::HasSubstr( + "Failed to resolve symbol envoy_dynamic_module_on_http_filter_config_new")); + + // The destroy function envoy_dynamic_module_on_http_filter_config_destroy is not defined. + const std::string yaml3 = R"EOF( +dynamic_module_config: + name: no_http_config_destory +filter_name: foo +filter_config: bar +)EOF"; + + envoy::extensions::filters::http::dynamic_modules::v3::DynamicModuleFilter proto_config3; + TestUtility::loadFromYamlAndValidate(yaml3, proto_config3); + + auto result3 = factory.createFilterFactoryFromProto(proto_config3, "", context); + EXPECT_FALSE(result3.ok()); + EXPECT_EQ(result3.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_THAT(result3.status().message(), + testing::HasSubstr( + "Failed to resolve symbol envoy_dynamic_module_on_http_filter_config_destroy")); } } // namespace HttpFilters diff --git a/test/extensions/dynamic_modules/http/filter_test.cc b/test/extensions/dynamic_modules/http/filter_test.cc index a409d48fdd10..49f470b0b032 100644 --- a/test/extensions/dynamic_modules/http/filter_test.cc +++ b/test/extensions/dynamic_modules/http/filter_test.cc @@ -20,13 +20,12 @@ TEST_P(DynamicModuleTestLanguages, Nop) { auto dynamic_module = newDynamicModule(testSharedObjectPath("no_op", language), false); EXPECT_TRUE(dynamic_module.ok()); - auto filter_config_ptr = std::make_shared< - Envoy::Extensions::DynamicModules::HttpFilters::DynamicModuleHttpFilterConfig>( - filter_name, filter_config, dynamic_module.value()); + auto filter_config_or_status = + Envoy::Extensions::DynamicModules::HttpFilters::newDynamicModuleHttpFilterConfig( + filter_name, filter_config, std::move(dynamic_module.value())); + EXPECT_TRUE(filter_config_or_status.ok()); - auto filter = - std::make_shared( - filter_config_ptr); + auto filter = std::make_shared(filter_config_or_status.value()); // The followings are mostly for coverage at the moment. NiceMock decoder_callbacks; diff --git a/test/extensions/dynamic_modules/test_data/c/BUILD b/test/extensions/dynamic_modules/test_data/c/BUILD index a302ec55f3ce..43bf756dba1f 100644 --- a/test/extensions/dynamic_modules/test_data/c/BUILD +++ b/test/extensions/dynamic_modules/test_data/c/BUILD @@ -14,3 +14,7 @@ test_program(name = "no_program_init") test_program(name = "program_init_fail") test_program(name = "abi_version_mismatch") + +test_program(name = "no_http_config_new") + +test_program(name = "no_http_config_destory") diff --git a/test/extensions/dynamic_modules/test_data/c/abi_version_mismatch.c b/test/extensions/dynamic_modules/test_data/c/abi_version_mismatch.c index c93056aafe9a..eb1391db94f8 100644 --- a/test/extensions/dynamic_modules/test_data/c/abi_version_mismatch.c +++ b/test/extensions/dynamic_modules/test_data/c/abi_version_mismatch.c @@ -1,5 +1,5 @@ #include "source/extensions/dynamic_modules/abi.h" -envoy_dynamic_module_type_abi_version envoy_dynamic_module_on_program_init() { +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init() { return "invalid-version-hash"; } diff --git a/test/extensions/dynamic_modules/test_data/c/no_http_config_destory.c b/test/extensions/dynamic_modules/test_data/c/no_http_config_destory.c new file mode 100644 index 000000000000..69a90e06bb7b --- /dev/null +++ b/test/extensions/dynamic_modules/test_data/c/no_http_config_destory.c @@ -0,0 +1,14 @@ +#include "source/extensions/dynamic_modules/abi.h" +#include "source/extensions/dynamic_modules/abi_version.h" + +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init() { + return kAbiVersion; +} + +envoy_dynamic_module_type_http_filter_config_module_ptr +envoy_dynamic_module_on_http_filter_config_new( + envoy_dynamic_module_type_http_filter_config_envoy_ptr filter_config_envoy_ptr, + const char* name_ptr, int name_size, const char* config_ptr, int config_size) { + static int some_variable = 0; + return &some_variable; +} diff --git a/test/extensions/dynamic_modules/test_data/c/no_http_config_new.c b/test/extensions/dynamic_modules/test_data/c/no_http_config_new.c new file mode 100644 index 000000000000..218e9550b01c --- /dev/null +++ b/test/extensions/dynamic_modules/test_data/c/no_http_config_new.c @@ -0,0 +1,6 @@ +#include "source/extensions/dynamic_modules/abi.h" +#include "source/extensions/dynamic_modules/abi_version.h" + +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init() { + return kAbiVersion; +} diff --git a/test/extensions/dynamic_modules/test_data/c/no_op.c b/test/extensions/dynamic_modules/test_data/c/no_op.c index 0993c38f09ee..61443600b8bd 100644 --- a/test/extensions/dynamic_modules/test_data/c/no_op.c +++ b/test/extensions/dynamic_modules/test_data/c/no_op.c @@ -1,10 +1,27 @@ +#include + #include "source/extensions/dynamic_modules/abi.h" #include "source/extensions/dynamic_modules/abi_version.h" +static int some_variable = 0; + int getSomeVariable() { - static int some_variable = 0; some_variable++; return some_variable; } -envoy_dynamic_module_type_abi_version envoy_dynamic_module_on_program_init() { return kAbiVersion; } +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init() { + return kAbiVersion; +} + +envoy_dynamic_module_type_http_filter_config_module_ptr +envoy_dynamic_module_on_http_filter_config_new( + envoy_dynamic_module_type_http_filter_config_envoy_ptr filter_config_envoy_ptr, + const char* name_ptr, int name_size, const char* config_ptr, int config_size) { + return &some_variable; +} + +void envoy_dynamic_module_on_http_filter_config_destroy( + envoy_dynamic_module_type_http_filter_config_module_ptr filter_config_ptr) { + assert(filter_config_ptr == &some_variable); +} diff --git a/test/extensions/dynamic_modules/test_data/c/program_init_fail.c b/test/extensions/dynamic_modules/test_data/c/program_init_fail.c index 0a00ce7d439b..ee03632c4e8a 100644 --- a/test/extensions/dynamic_modules/test_data/c/program_init_fail.c +++ b/test/extensions/dynamic_modules/test_data/c/program_init_fail.c @@ -2,4 +2,6 @@ #include "source/extensions/dynamic_modules/abi.h" -envoy_dynamic_module_type_abi_version envoy_dynamic_module_on_program_init() { return NULL; } +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init() { + return NULL; +} diff --git a/test/extensions/dynamic_modules/test_data/rust/no_op.rs b/test/extensions/dynamic_modules/test_data/rust/no_op.rs index 309744dd86da..3c095d46a6c7 100644 --- a/test/extensions/dynamic_modules/test_data/rust/no_op.rs +++ b/test/extensions/dynamic_modules/test_data/rust/no_op.rs @@ -1,8 +1,9 @@ -use envoy_proxy_dynamic_modules_rust_sdk::declare_program_init; +use envoy_proxy_dynamic_modules_rust_sdk::declare_init_functions; use std::sync::atomic::{AtomicI32, Ordering}; -declare_program_init!(init); +declare_init_functions!(init, new_nop_http_filter_config_fn); +/// This implements the [`envoy_proxy_dynamic_modules_rust_sdk::ProgramInitFunction`] signature. fn init() -> bool { true } @@ -13,3 +14,30 @@ static SOME_VARIABLE: AtomicI32 = AtomicI32::new(1); pub extern "C" fn getSomeVariable() -> i32 { SOME_VARIABLE.fetch_add(1, Ordering::SeqCst) } + +/// This implements the [`envoy_proxy_dynamic_modules_rust_sdk::NewHttpFilterConfigFunction`] signature. +fn new_nop_http_filter_config_fn( + _envoy_filter_factory: envoy_proxy_dynamic_modules_rust_sdk::EnvoyHttpFilterConfig, + name: &str, + config: &str, +) -> Option> { + let name = name.to_string(); + let config = config.to_string(); + Some(Box::new(NopHttpFilterConfig { name, config })) +} + +/// A no-op HTTP filter configuration that implements [`envoy_proxy_dynamic_modules_rust_sdk::HttpFilterConfig`] +/// as well as the [`Drop`] to test the cleanup of the configuration. +struct NopHttpFilterConfig { + name: String, + config: String, +} + +impl envoy_proxy_dynamic_modules_rust_sdk::HttpFilterConfig for NopHttpFilterConfig {} + +impl Drop for NopHttpFilterConfig { + fn drop(&mut self) { + assert_eq!(self.name, "foo"); + assert_eq!(self.config, "bar"); + } +}