dym sdk: add network filter sdk for golang/cpp#44678
dym sdk: add network filter sdk for golang/cpp#44678wbpcode wants to merge 2 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
wbpcode
requested review from
agrawroh,
mathetake and
mattklein123
as code owners
|
copilot is used to help develop this feature. Let's me review it carefully first. |
|
/gemini review |
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Network Filter SDK for dynamic modules, providing support for both C++ and Go. It includes the necessary ABI implementations, interfaces for network buffers and filter handles, and a shared scheduler component. Integration tests have been updated to verify the SDK across C++, Go, and Rust. Feedback focuses on improving the robustness of the Go SetMetadata implementation by logging unsupported types and addressing a potential use-after-free risk in the C++ SDK by using std::shared_ptr for HTTP callout callbacks.
| func (h *dymNetworkFilterHandle) SetMetadata(metadataNamespace, key string, value any) { | ||
| var numValue float64 | ||
| var isNum bool | ||
| var strValue string | ||
| var isStr bool | ||
|
|
||
| switch v := value.(type) { | ||
| case uint: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case uint8: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case uint16: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case uint32: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case uint64: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case int: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case int8: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case int16: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case int32: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case int64: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case float32: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case float64: | ||
| numValue = float64(v) | ||
| isNum = true | ||
| case bool: | ||
| C.envoy_dynamic_module_callback_network_set_dynamic_metadata_bool( | ||
| h.hostPluginPtr, | ||
| stringToModuleBuffer(metadataNamespace), | ||
| stringToModuleBuffer(key), | ||
| C.bool(v), | ||
| ) | ||
| runtime.KeepAlive(metadataNamespace) | ||
| runtime.KeepAlive(key) | ||
| return | ||
| case string: | ||
| strValue = v | ||
| isStr = true | ||
| } | ||
|
|
||
| if isNum { | ||
| C.envoy_dynamic_module_callback_network_set_dynamic_metadata_number( | ||
| h.hostPluginPtr, | ||
| stringToModuleBuffer(metadataNamespace), | ||
| stringToModuleBuffer(key), | ||
| C.double(numValue), | ||
| ) | ||
| } else if isStr { | ||
| C.envoy_dynamic_module_callback_network_set_dynamic_metadata_string( | ||
| h.hostPluginPtr, | ||
| stringToModuleBuffer(metadataNamespace), | ||
| stringToModuleBuffer(key), | ||
| stringToModuleBuffer(strValue), | ||
| ) | ||
| } | ||
| runtime.KeepAlive(metadataNamespace) | ||
| runtime.KeepAlive(key) | ||
| runtime.KeepAlive(strValue) | ||
| } |
There was a problem hiding this comment.
The SetMetadata implementation silently ignores unsupported types. While the current list is comprehensive, it might be beneficial to log a warning when an unknown type is passed to help developers debug why their metadata isn't being set.
| if (result == envoy_dynamic_module_type_http_callout_init_result_Success) { | ||
| callout_callbacks_[callout_id_out] = &cb; | ||
| } |
There was a problem hiding this comment.
Storing a raw reference HttpCalloutCallback& cb in the callout_callbacks_ map assumes that the caller ensures the callback's lifetime exceeds the duration of the asynchronous HTTP call. This is a potential source of use-after-free bugs. Per repository guidelines, it is acceptable to keep safety checks for added safety. Consider using std::shared_ptr to manage the callback's lifetime more safely. If using std::shared_ptr, ensure that any argument-providing functions are called before the constructor to maintain exception safety guarantees (i.e., to prevent a custom deleter from running if an argument-providing function throws).
References
- In complex code, it is acceptable to keep cheap safety checks, even if they appear redundant, for added safety.
- A std::shared_ptr constructor's exception guarantee (calling the deleter on the raw pointer if an exception is thrown during construction) only applies to exceptions thrown during the shared_ptr's own construction (e.g., bad_alloc). It does not apply to exceptions thrown when evaluating the arguments to the constructor. To prevent a custom deleter from running when an argument-providing function throws, call that function and store its result before calling the std::shared_ptr constructor.
There was a problem hiding this comment.
Pull request overview
Adds dynamic-modules network filter SDK support for Go and C++, plus new multi-language integration test modules to validate core network-handle operations (read-disable, connection state, half-close, buffer limits).
Changes:
- Introduces Go network filter shared APIs + Go ABI bridge for network filter callbacks.
- Introduces C++ network SDK headers/impl plus C++ ABI bridge for network filters (and shared scheduler refactor).
- Extends dynamic-modules network integration tests and adds Go/C++ test modules to the test data set.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/extensions/dynamic_modules/test_data/go/network_integration_test/network_integration_test.go | New Go dynamic module implementing network-filter assertions for integration coverage. |
| test/extensions/dynamic_modules/test_data/go/BUILD | Adds the new Go test module build target. |
| test/extensions/dynamic_modules/test_data/cpp/network_integration_test.cc | New C++ dynamic module implementing network-filter assertions for integration coverage. |
| test/extensions/dynamic_modules/test_data/cpp/BUILD | Adds the new C++ test module build target. |
| test/extensions/dynamic_modules/network/integration_test.cc | Refactors test base + adds SDK-language parameterized network integration tests. |
| test/extensions/dynamic_modules/network/BUILD | Wires new Go/C++ test modules into the network integration test. |
| source/extensions/dynamic_modules/sdk/go/shared/network_base.go | Adds Go shared network handle/buffer/status types and interfaces. |
| source/extensions/dynamic_modules/sdk/go/shared/network_api.go | Adds Go network filter/factory/config-factory interfaces + empty implementations. |
| source/extensions/dynamic_modules/sdk/go/sdk.go | Adds Go registry and factory creation for network filters. |
| source/extensions/dynamic_modules/sdk/go/abi/network.go | Implements Go ABI exports and host-callback wrappers for network filters. |
| source/extensions/dynamic_modules/sdk/cpp/sdk_network.h | Adds C++ network filter SDK API surface and registry. |
| source/extensions/dynamic_modules/sdk/cpp/sdk_network.cc | Implements C++ network filter registry plumbing. |
| source/extensions/dynamic_modules/sdk/cpp/sdk_internal_network.cc | Implements C++ ABI exports/host-callback wrappers for network filters. |
| source/extensions/dynamic_modules/sdk/cpp/sdk_internal_common.h | Shared templated scheduler implementation for SDK ABIs. |
| source/extensions/dynamic_modules/sdk/cpp/sdk_internal.cc | Refactors HTTP scheduler impl to use shared template. |
| source/extensions/dynamic_modules/sdk/cpp/BUILD | Updates SDK build targets to include network SDK and ABI sources. |
| source/extensions/dynamic_modules/BUILD | Wires new Go shared/ABI sources into Bazel go_library targets. |
| srcs = [ | ||
| "sdk_internal.cc", | ||
| "sdk_internal_common.h", |
| "sdk_internal_common.h", | ||
| "sdk_internal_network.cc", | ||
| ], |
| unparsedConfig []byte) (shared.NetworkFilterFactory, error) { | ||
| configFactory := networkFilterConfigFactoryRegistry[name] | ||
| if configFactory == nil { | ||
| return nil, fmt.Errorf("failed to get network filter config factory") |
| if err != nil || factory == nil { | ||
| configHandle.Log(shared.LogLevelWarn, "Failed to load network filter configuration: %v", err) | ||
| return nil | ||
| } |
| "//test/extensions/dynamic_modules/test_data/go:network_integration_test", | ||
| "//test/extensions/dynamic_modules/test_data/rust:network_integration_test", | ||
| ], | ||
| env = {"GODEBUG": "cgocheck=0"}, |
| : DynamicModulesNetworkIntegrationTestBase(GetParam() == "rust" | ||
| ? Envoy::Network::Address::IpVersion::v4 | ||
| : Envoy::Network::Address::IpVersion::v6) {} | ||
|
|
Commit Message:
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:]