ip tagging: refactor ip tagging to make it lock free#45166
Conversation
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
… dev-make-ip-tag-better
|
/gemini review |
![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 refactors the IP tagging filter to use atomic snapshots for tries and statistics, ensuring that lookups and stat increments remain consistent during file reloads. It also simplifies the general DataSourceProvider by removing the data_update_cb mechanism. A high-severity issue was identified where validation_visitor is captured by reference in a lambda within a singleton-managed provider, potentially leading to use-after-free bugs if the provider outlives the configuration.
There was a problem hiding this comment.
Pull request overview
Refactors the HTTP IP tagging filter to publish immutable “loaded snapshot” objects (trie + matching stats container) so request-path lookups and stat updates can be performed without locking, and removes the unused/incorrect DataSourceProvider update callback mechanism.
Changes:
- Introduce
LoadedIpTagssnapshots andIpTagsStatsto keep trie + per-tag builtin stat names consistent across reloads. - Rework
IpTagsProviderto publishLoadedIpTagsatomically from the datasource transform callback (enabling lock-free request path). - Remove
DataUpdateCbsupport fromConfig::DataSource::DataSourceProvider/DynamicData.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc | Updates tests to validate sharing behavior via the new LoadedIpTags snapshot/trie access. |
| source/extensions/filters/http/ip_tagging/ip_tagging_filter.h | Introduces IpTagsStats + LoadedIpTags and updates provider/config APIs to use snapshots. |
| source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc | Implements snapshot creation on load/reload and updates filter request path to use a single per-request snapshot. |
| source/common/config/datasource.h | Removes DataUpdateCb plumbing from DynamicData/DataSourceProvider and associated invocation logic. |
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/retest |
| const FilterRequestType request_type_; | ||
| Stats::Scope& scope_; | ||
| Runtime::Loader& runtime_; | ||
| const std::string stats_prefix_str_; |
| Http::StreamDecoderFilterCallbacks* callbacks_{}; | ||
| // Used for testing only. | ||
| mutable Thread::ThreadSynchronizer synchronizer_; | ||
| // Allow the unit test to have access to private members. |
| if (absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Yaml)) { | ||
| auto data = std::string(new_data); | ||
| auto load_status = | ||
| // TODO(nezdolik) remove string casting once yaml utility has been migrated to |
There was a problem hiding this comment.
now looking at this comment, looks like it better suits to be above line 91. (can fix it separately)
| IpTagsStatsSharedPtr stats; | ||
| }; | ||
|
|
||
| using LoadedIpTagsSharedPtr = std::shared_ptr<LoadedIpTags>; |
There was a problem hiding this comment.
should we add const here?
using LoadedIpTagsSharedPtr = std::shared_ptr<const LoadedIpTags>;
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
… dev-make-ip-tag-better
Commit Message: ip tagging: refactor ip tagging to make it lock free
Additional Description:
Make the ip tagging lock free and removed unnecessary update callback of data source.
Risk Level: mid.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.