From f7ac6c437feb76d03ed2fbcbf5020928ee6553e3 Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Wed, 20 May 2026 03:05:14 +0000 Subject: [PATCH 1/4] ip tagging: refactor ip tagging to make it lock free Signed-off-by: wbpcode/wangbaiping --- source/common/config/datasource.h | 44 +-- .../http/ip_tagging/ip_tagging_filter.cc | 354 +++++++----------- .../http/ip_tagging/ip_tagging_filter.h | 109 +++--- .../http/ip_tagging/ip_tagging_filter_test.cc | 8 +- 4 files changed, 215 insertions(+), 300 deletions(-) diff --git a/source/common/config/datasource.h b/source/common/config/datasource.h index 4f643b2dfaed6..a30a7efab6dd1 100644 --- a/source/common/config/datasource.h +++ b/source/common/config/datasource.h @@ -68,7 +68,6 @@ absl::optional getPath(const envoy::config::core::v3::DataSource& s template using DataTransform = std::function>(absl::string_view)>; -using DataUpdateCb = std::function; struct ProviderOptions { // Use an empty string if no DataSource case is specified. @@ -94,11 +93,9 @@ template class DynamicData { ThreadLocal::SlotAllocator& tls, Api::Api& api, DataTransform data_transform_cb, const ProviderOptions& options, std::shared_ptr initial_data, uint64_t initial_hash, - absl::AnyInvocable cleanup, absl::Status& creation_status, - absl::optional> data_update_cb = absl::nullopt) + absl::AnyInvocable cleanup, absl::Status& creation_status) : dispatcher_(main_dispatcher), api_(api), options_(options), filename_(source.filename()), - data_transform_cb_(data_transform_cb), data_update_cb_(data_update_cb), hash_(initial_hash), - cleanup_(std::move(cleanup)) { + data_transform_cb_(data_transform_cb), hash_(initial_hash), cleanup_(std::move(cleanup)) { slot_ = ThreadLocal::TypedSlot::ThreadLocalData>::makeUnique(tls); slot_->set([initial_data = std::move(initial_data)](Event::Dispatcher&) { @@ -163,14 +160,11 @@ template class DynamicData { hash_ = new_hash; } - slot_->runOnAllThreads([new_data = std::move(transformed_new_data_or_error.value()), - this](OptRef::ThreadLocalData> obj) { + slot_->runOnAllThreads([new_data = std::move(transformed_new_data_or_error.value())]( + OptRef::ThreadLocalData> obj) { if (obj.has_value()) { obj->data_ = new_data; } - if (data_update_cb_.has_value()) { - (*data_update_cb_)(); - } }); return absl::OkStatus(); } @@ -180,7 +174,6 @@ template class DynamicData { const ProviderOptions options_; const std::string filename_; DataTransform data_transform_cb_; - absl::optional data_update_cb_; uint64_t hash_; absl::AnyInvocable cleanup_; ThreadLocal::TypedSlotPtr slot_; @@ -211,7 +204,6 @@ template class DataSourceProvider { * @param data_transform_cb transforms content of the DataSource (type std::string) * to the desired `DataType` type. * @param max_size max size limit of file to read, default 0 means no limit. - * @param data_update_cb optional callback that can be invoked upon data update in the DataSource. * @return absl::StatusOr with DataSource contents. or an error * status if any error occurs. * NOTE: If file watch is enabled and the new file content does not meet the @@ -220,17 +212,15 @@ template class DataSourceProvider { static absl::StatusOr> create(const ProtoDataSource& source, Event::Dispatcher& main_dispatcher, ThreadLocal::SlotAllocator& tls, Api::Api& api, bool allow_empty, - DataTransform data_transform_cb, uint64_t max_size, - absl::optional data_update_cb = absl::nullopt) { + DataTransform data_transform_cb, uint64_t max_size) { return create(source, main_dispatcher, tls, api, data_transform_cb, - {.allow_empty = allow_empty, .max_size = max_size}, {}, data_update_cb); + {.allow_empty = allow_empty, .max_size = max_size}, {}); } static absl::StatusOr> create(const ProtoDataSource& source, Event::Dispatcher& main_dispatcher, ThreadLocal::SlotAllocator& tls, Api::Api& api, DataTransform data_transform_cb, - const ProviderOptions& options, absl::AnyInvocable cleanup = {}, - absl::optional data_update_cb = absl::nullopt) { + const ProviderOptions& options, absl::AnyInvocable cleanup = {}) { uint64_t max_size = options.max_size; auto initial_data_or_error = read(source, options.allow_empty, api, max_size); RETURN_IF_NOT_OK_REF(initial_data_or_error.status()); @@ -253,11 +243,11 @@ template class DataSourceProvider { absl::Status creation_status = absl::OkStatus(); const uint64_t hash = options.hash_content ? HashUtil::xxHash64(*initial_data_or_error) : 0; - auto ret = std::unique_ptr( - new DataSourceProvider(std::make_unique>( - source, main_dispatcher, tls, api, data_transform_cb, options, - std::move(transformed_data_or_error).value(), hash, std::move(cleanup), creation_status, - data_update_cb))); + auto ret = std::unique_ptr(new DataSourceProvider( + std::make_unique>(source, main_dispatcher, tls, api, + data_transform_cb, options, + std::move(transformed_data_or_error).value(), hash, + std::move(cleanup), creation_status))); RETURN_IF_NOT_OK(creation_status); return std::move(ret); } @@ -301,16 +291,15 @@ class ProviderSingleton : public Singleton::Instance, public: ProviderSingleton(Event::Dispatcher& main_dispatcher, ThreadLocal::SlotAllocator& tls, Api::Api& api, DataTransform data_transform_cb, - const ProviderOptions& options, - absl::optional data_update_cb = absl::nullopt) + const ProviderOptions& options) : dispatcher_(main_dispatcher), tls_(tls), api_(api), data_transform_cb_(data_transform_cb), - data_update_cb_(data_update_cb), options_(options) {} + options_(options) {} absl::StatusOr> getOrCreate(const ProtoDataSource& source) { ASSERT_IS_MAIN_OR_TEST_THREAD(); if (!usesFileWatching(source, options_)) { - return DataSourceProvider::create( - source, dispatcher_, tls_, api_, data_transform_cb_, options_, {}, data_update_cb_); + return DataSourceProvider::create(source, dispatcher_, tls_, api_, + data_transform_cb_, options_, {}); } const size_t config_hash = MessageUtil::hash(source); auto it = dynamic_providers_.find(config_hash); @@ -347,7 +336,6 @@ class ProviderSingleton : public Singleton::Instance, ThreadLocal::SlotAllocator& tls_; Api::Api& api_; DataTransform data_transform_cb_; - absl::optional data_update_cb_; const ProviderOptions options_; absl::flat_hash_map>> dynamic_providers_; }; diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc index 8afed8adbd018..51cf0334d3fd1 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc @@ -14,157 +14,23 @@ namespace Extensions { namespace HttpFilters { namespace IpTagging { -namespace { -static const Network::LcTrie::LcTrie EMPTY_TRIE_{{}}; -} - -IpTagsProvider::IpTagsProvider(const envoy::config::core::v3::DataSource& ip_tags_datasource, - Event::Dispatcher& main_dispatcher, Api::Api& api, - ProtobufMessage::ValidationVisitor& validation_visitor, - ThreadLocal::SlotAllocator& tls, const std::string& stat_prefix, - Stats::ScopeSharedPtr scope, Singleton::InstanceSharedPtr owner, - absl::Status& creation_status) - : api_(api), owner_(owner) { - const auto datasource_filename = ip_tags_datasource.filename(); - if (!datasource_filename.empty()) { - if (!absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Yaml) && - !absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Json)) { - creation_status = absl::InvalidArgumentError( - "Unsupported file format, unable to parse ip tags from file " + datasource_filename); - return; - } - auto provider_or_error = - Config::DataSource::DataSourceProvider>::create( - ip_tags_datasource, main_dispatcher, tls, api_, false, - // data_transform_cb - [this, datasource_filename, &validation_visitor, scope, - stat_prefix](absl::string_view new_data) -> absl::StatusOr { - IPTagsProto ip_tags_proto; - 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 - // string_view. - MessageUtil::loadFromYamlNoThrow(data, ip_tags_proto, validation_visitor); - if (!load_status.ok()) { - return load_status; - } - } else if (absl::EndsWith(datasource_filename, - MessageUtil::FileExtensions::get().Json)) { - bool has_unknown_field; - auto load_status = - MessageUtil::loadFromJsonNoThrow(new_data, ip_tags_proto, has_unknown_field); - if (!load_status.ok()) { - return load_status; - } - } - auto new_loader = std::make_shared(stat_prefix, scope); - auto result = new_loader->parseIpTagsAsProto(ip_tags_proto.ip_tags()); - if (result.ok()) { - absl::MutexLock lock(&tags_loader_mu_); - tags_loader_ = std::move(new_loader); - } - return result; - }, - 0, - // data_update_cb - absl::make_optional([this]() { getTagsLoader()->incIpTagsReloadSuccess(); })); - if (!provider_or_error.status().ok()) { - creation_status = absl::InvalidArgumentError( - fmt::format("unable to create data source '{}'", provider_or_error.status().message())); - return; - } - data_source_provider_ = std::move(provider_or_error.value()); - } else { - creation_status = - absl::InvalidArgumentError("Cannot load tags from empty filename in datasource."); - } -} - -IpTagsProvider::~IpTagsProvider() { ENVOY_LOG(debug, "Shutting down ip tags provider"); }; - -LcTrieSharedPtr IpTagsProvider::ipTags() { - return data_source_provider_->data() == nullptr - ? std::make_shared>() - : data_source_provider_->data(); -} - -std::shared_ptr IpTagsProvider::getTagsLoader() { - absl::MutexLock lock(&tags_loader_mu_); - return tags_loader_; -} - -void IpTagsProvider::incHit(absl::string_view tag) { - if (auto loader = getTagsLoader()) { - loader->incHit(tag); - } -} - -void IpTagsProvider::incTotal() { - if (auto loader = getTagsLoader()) { - loader->incTotal(); - } -} - -void IpTagsProvider::incNoHit() { - if (auto loader = getTagsLoader()) { - loader->incNoHit(); - } -} - -absl::StatusOr> IpTagsRegistrySingleton::getOrCreateProvider( - const envoy::config::core::v3::DataSource& ip_tags_datasource, Api::Api& api, - ProtobufMessage::ValidationVisitor& validation_visitor, ThreadLocal::SlotAllocator& tls, - Event::Dispatcher& main_dispatcher, const std::string& stat_prefix, Stats::Scope& scope, - std::shared_ptr singleton) { - if (scope_ == nullptr) { - scope_ = scope.createScope(""); - } - std::shared_ptr ip_tags_provider; - absl::Status creation_status = absl::OkStatus(); - const uint64_t key = std::hash()(ip_tags_datasource.filename()); - auto it = ip_tags_registry_.find(key); - if (it != ip_tags_registry_.end()) { - if (std::shared_ptr provider = it->second.lock()) { - ip_tags_provider = provider; - } else { - ip_tags_provider = std::make_shared(ip_tags_datasource, main_dispatcher, api, - validation_visitor, tls, stat_prefix, - scope_, singleton, creation_status); - if (!creation_status.ok()) { - return creation_status; - } - ip_tags_registry_[key] = ip_tags_provider; - } - } else { - ip_tags_provider = std::make_shared(ip_tags_datasource, main_dispatcher, api, - validation_visitor, tls, stat_prefix, - scope_, singleton, creation_status); - if (!creation_status.ok()) { - return creation_status; - } - ip_tags_registry_[key] = ip_tags_provider; - } - return ip_tags_provider; -} - -IpTagsLoader::IpTagsLoader(const std::string& stat_prefix, Stats::ScopeSharedPtr scope) - : scope_(scope), stat_name_set_(scope->symbolTable().makeSet("IpTaggingReload")), +IpTagsStats::IpTagsStats(const std::string& stat_prefix, Stats::ScopeSharedPtr scope) + : scope_(std::move(scope)), stat_name_set_(scope_->symbolTable().makeSet("IpTagging")), stats_prefix_(stat_name_set_->add(stat_prefix + "ip_tagging")), unknown_tag_(stat_name_set_->add("unknown_tag.hit")), total_(stat_name_set_->add("total")), no_hit_(stat_name_set_->add("no_hit")), reload_success_(stat_name_set_->add("reload_success")) {} -void IpTagsLoader::incHit(absl::string_view tag) { +void IpTagsStats::incHit(absl::string_view tag) { incCounter(stat_name_set_->getBuiltin(absl::StrCat(tag, ".hit"), unknown_tag_)); } -void IpTagsLoader::incCounter(Stats::StatName name) { +void IpTagsStats::incCounter(Stats::StatName name) { Stats::SymbolTable::StoragePtr storage = scope_->symbolTable().join({stats_prefix_, name}); scope_->counterFromStatName(Stats::StatName(storage.get())).inc(); } -absl::StatusOr IpTagsLoader::parseIpTagsAsProto( +absl::StatusOr IpTagsStats::parseIpTagsAsProto( const Protobuf::RepeatedPtrField< envoy::extensions::filters::http::ip_tagging::v3::IPTagging::IPTag>& ip_tags) { std::vector>> tag_data; @@ -189,6 +55,107 @@ absl::StatusOr IpTagsLoader::parseIpTagsAsProto( return std::make_shared>(tag_data); } +IpTagsProvider::IpTagsProvider(const envoy::config::core::v3::DataSource& ip_tags_datasource, + Event::Dispatcher& main_dispatcher, Api::Api& api, + ProtobufMessage::ValidationVisitor& validation_visitor, + ThreadLocal::SlotAllocator& tls, const std::string& stat_prefix, + Stats::ScopeSharedPtr scope, Singleton::InstanceSharedPtr owner, + absl::Status& creation_status) + : owner_(owner) { + const auto& datasource_filename = ip_tags_datasource.filename(); + if (datasource_filename.empty()) { + creation_status = + absl::InvalidArgumentError("Cannot load tags from empty filename in datasource."); + return; + } + if (!absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Yaml) && + !absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Json)) { + creation_status = absl::InvalidArgumentError( + "Unsupported file format, unable to parse ip tags from file " + datasource_filename); + return; + } + + // Each successful load (initial + every file-watcher reload) atomically produces a + // new LoadedIpTags containing a fresh trie and matching IpTagsStats, which the + // DataSourceProvider publishes via data(). `first_load` skips the reload_success + // increment on the initial load so the counter only reflects subsequent reloads. The + // `scope` shared_ptr is captured by value so the stats scope outlives any individual + // listener that bootstrapped this provider (providers are shared across listeners + // via IpTagsRegistrySingleton). + auto provider_or_error = Config::DataSource::DataSourceProvider::create( + ip_tags_datasource, main_dispatcher, tls, api, /*allow_empty=*/false, + [stat_prefix, scope, &validation_visitor, datasource_filename, first_load = true]( + absl::string_view new_data) mutable -> absl::StatusOr> { + IPTagsProto ip_tags_proto; + 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 + // string_view. + MessageUtil::loadFromYamlNoThrow(data, ip_tags_proto, validation_visitor); + if (!load_status.ok()) { + return load_status; + } + } else if (absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Json)) { + bool has_unknown_field; + auto load_status = + MessageUtil::loadFromJsonNoThrow(new_data, ip_tags_proto, has_unknown_field); + if (!load_status.ok()) { + return load_status; + } + } + auto stats = std::make_shared(stat_prefix, scope); + auto trie_or = stats->parseIpTagsAsProto(ip_tags_proto.ip_tags()); + if (!trie_or.ok()) { + return trie_or.status(); + } + if (first_load) { + first_load = false; + } else { + stats->incIpTagsReloadSuccess(); + } + return std::make_shared(LoadedIpTags{*std::move(trie_or), std::move(stats)}); + }, + 0); + if (!provider_or_error.status().ok()) { + creation_status = absl::InvalidArgumentError( + fmt::format("unable to create data source '{}'", provider_or_error.status().message())); + return; + } + data_source_provider_ = std::move(provider_or_error.value()); +} + +IpTagsProvider::~IpTagsProvider() { ENVOY_LOG(debug, "Shutting down ip tags provider"); } + +absl::StatusOr> IpTagsRegistrySingleton::getOrCreateProvider( + const envoy::config::core::v3::DataSource& ip_tags_datasource, Api::Api& api, + ProtobufMessage::ValidationVisitor& validation_visitor, ThreadLocal::SlotAllocator& tls, + Event::Dispatcher& main_dispatcher, const std::string& stat_prefix, Stats::Scope& scope, + std::shared_ptr singleton) { + // Lazily create a singleton-owned scope on first use. Reusing it across providers + // keeps the stats scope alive even after the listener that first created the + // provider goes away, since other listeners may still share this provider. + if (scope_ == nullptr) { + scope_ = scope.createScope(""); + } + const uint64_t key = std::hash()(ip_tags_datasource.filename()); + auto it = ip_tags_registry_.find(key); + if (it != ip_tags_registry_.end()) { + if (std::shared_ptr provider = it->second.lock()) { + return provider; + } + } + absl::Status creation_status = absl::OkStatus(); + auto ip_tags_provider = + std::make_shared(ip_tags_datasource, main_dispatcher, api, validation_visitor, + tls, stat_prefix, scope_, singleton, creation_status); + if (!creation_status.ok()) { + return creation_status; + } + ip_tags_registry_[key] = ip_tags_provider; + return ip_tags_provider; +} + SINGLETON_MANAGER_REGISTRATION(ip_tags_registry); absl::StatusOr IpTaggingFilterConfig::create( @@ -210,11 +177,7 @@ IpTaggingFilterConfig::IpTaggingFilterConfig( Runtime::Loader& runtime, Api::Api& api, ThreadLocal::SlotAllocator& tls, Event::Dispatcher& dispatcher, ProtobufMessage::ValidationVisitor& validation_visitor, absl::Status& creation_status) - : request_type_(requestTypeEnum(config.request_type())), scope_(scope), runtime_(runtime), - stats_prefix_str_(stat_prefix), stat_name_set_(scope_.symbolTable().makeSet("IpTagging")), - stats_prefix_(stat_name_set_->add(stats_prefix_str_ + "ip_tagging")), - no_hit_(stat_name_set_->add("no_hit")), total_(stat_name_set_->add("total")), - unknown_tag_(stat_name_set_->add("unknown_tag.hit")), + : request_type_(requestTypeEnum(config.request_type())), runtime_(runtime), ip_tag_header_(config.has_ip_tag_header() ? config.ip_tag_header().header() : ""), ip_tag_header_action_(config.has_ip_tag_header() ? config.ip_tag_header().action() @@ -225,86 +188,35 @@ IpTaggingFilterConfig::IpTaggingFilterConfig( if (config.ip_tags().empty() && !config.has_ip_tags_datasource()) { creation_status = absl::InvalidArgumentError( "HTTP IP Tagging Filter requires either ip_tags or ip_tags_datasource to be specified."); + return; } if (!config.ip_tags().empty() && config.has_ip_tags_datasource()) { creation_status = absl::InvalidArgumentError("Only one of ip_tags or ip_tags_datasource can be configured."); + return; } - RETURN_ONLY_IF_NOT_OK_REF(creation_status); if (!config.ip_tags().empty()) { - std::vector>> tag_data; - for (const auto& ip_tag : config.ip_tags()) { - std::vector cidr_set; - cidr_set.reserve(ip_tag.ip_list().size()); - for (const envoy::config::core::v3::CidrRange& entry : ip_tag.ip_list()) { - absl::StatusOr cidr_or_error = - Network::Address::CidrRange::create(entry); - if (cidr_or_error.status().ok()) { - cidr_set.emplace_back(std::move(cidr_or_error.value())); - } else { - creation_status = absl::InvalidArgumentError( - fmt::format("invalid ip/mask combo '{}/{}' (format is /<# mask bits>)", - entry.address_prefix(), entry.prefix_len().value())); - return; - } - } - tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set); - stat_name_set_->rememberBuiltin(absl::StrCat(ip_tag.ip_tag_name(), ".hit")); - trie_ = std::make_unique>(tag_data); - } - } else { - auto provider_or_error = ip_tags_registry_->getOrCreateProvider( - config.ip_tags_datasource(), api, validation_visitor, tls, dispatcher, stat_prefix, scope, - ip_tags_registry_); - if (provider_or_error.status().ok()) { - provider_ = provider_or_error.value(); - } else { - creation_status = provider_or_error.status(); + auto stats = std::make_shared(stat_prefix, scope.createScope("")); + auto trie_or = stats->parseIpTagsAsProto(config.ip_tags()); + if (!trie_or.ok()) { + creation_status = trie_or.status(); return; } - trie_ = provider_->ipTags(); - } -} - -void IpTaggingFilterConfig::incCounter(Stats::StatName name) { - Stats::SymbolTable::StoragePtr storage = scope_.symbolTable().join({stats_prefix_, name}); - scope_.counterFromStatName(Stats::StatName(storage.get())).inc(); -} - -void IpTaggingFilterConfig::incHit(absl::string_view tag) { - if (provider_) { - provider_->incHit(tag); - } else { - incCounter(stat_name_set_->getBuiltin(absl::StrCat(tag, ".hit"), unknown_tag_)); - } -} - -void IpTaggingFilterConfig::incNoHit() { - if (provider_) { - provider_->incNoHit(); - } else { - incCounter(no_hit_); + static_ip_tags_ = + std::make_shared(LoadedIpTags{*std::move(trie_or), std::move(stats)}); + return; } -} -void IpTaggingFilterConfig::incTotal() { - if (provider_) { - provider_->incTotal(); - } else { - incCounter(total_); - } -} - -const Network::LcTrie::LcTrie& IpTaggingFilterConfig::trie() const { - if (provider_) { - return *provider_->ipTags(); - } - if (trie_ != nullptr) { - return *trie_; + auto provider_or_error = ip_tags_registry_->getOrCreateProvider( + config.ip_tags_datasource(), api, validation_visitor, tls, dispatcher, stat_prefix, scope, + ip_tags_registry_); + if (!provider_or_error.status().ok()) { + creation_status = provider_or_error.status(); + return; } - return EMPTY_TRIE_; + provider_ = std::move(provider_or_error.value()); } OptRef IpTaggingFilterConfig::ipTagHeader() const { @@ -332,8 +244,18 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::RequestHeaderMap& return Http::FilterHeadersStatus::Continue; } - std::vector tags = - config_->trie().getData(callbacks_->streamInfo().downstreamAddressProvider().remoteAddress()); + // Pull the current snapshot once so the trie lookup and stats updates that follow + // come from the same reload, even if a file watcher publishes a new snapshot mid-call. + LoadedIpTagsSharedPtr loaded = config_->loadedIpTags(); + if (loaded == nullptr) { + return Http::FilterHeadersStatus::Continue; + } + + std::vector tags; + if (loaded->trie != nullptr) { + tags = + loaded->trie->getData(callbacks_->streamInfo().downstreamAddressProvider().remoteAddress()); + } // Used for testing. synchronizer_.syncPoint("_trie_lookup_complete"); @@ -343,12 +265,12 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::RequestHeaderMap& // If there are use cases with a large set of tags, a way to opt into these stats // should be exposed and other observability options like logging tags need to be implemented. for (const std::string& tag : tags) { - config_->incHit(tag); + loaded->stats->incHit(tag); } } else { - config_->incNoHit(); + loaded->stats->incNoHit(); } - config_->incTotal(); + loaded->stats->incTotal(); return Http::FilterHeadersStatus::Continue; } diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h index 4a19662bb7877..5a33698dff349 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h @@ -30,28 +30,35 @@ namespace IpTagging { using IPTagsProto = envoy::extensions::filters::http::ip_tagging::v3::IPTagging::IPTags; using LcTrieSharedPtr = std::shared_ptr>; -class IpTagsLoader : public Logger::Loggable, - public std::enable_shared_from_this { +/** + * Stats container for a single loaded set of ip tags. Owns the StatNameSet used to + * intern per-tag `.hit` builtins, plus the shared `total`, `no_hit`, `unknown_tag.hit` + * and `reload_success` stat names. Also exposes the proto-to-trie parser, which is the + * one place tag names are interned as builtins. + */ +class IpTagsStats : public Logger::Loggable { public: - IpTagsLoader(const std::string& stat_prefix, Stats::ScopeSharedPtr scope); - - void incIpTagsReloadSuccess() { incCounter(reload_success_); } + IpTagsStats(const std::string& stat_prefix, Stats::ScopeSharedPtr scope); void incHit(absl::string_view tag); - void incNoHit() { incCounter(no_hit_); } void incTotal() { incCounter(total_); } + void incIpTagsReloadSuccess() { incCounter(reload_success_); } /** - * Parses ip tags in a proto format into a trie structure. - * @param ip_tags Collection of ip tags in proto format. - * @return Valid LcTrieSharedPtr if parsing succeeded or error status otherwise. + * Parses ip tags in proto format into a trie. Interns each `.hit` as a builtin + * on this instance's StatNameSet so subsequent `incHit` calls go through the fast path. + * @return Valid LcTrieSharedPtr on success or an error status otherwise. */ absl::StatusOr parseIpTagsAsProto(const Protobuf::RepeatedPtrField< envoy::extensions::filters::http::ip_tagging::v3::IPTagging::IPTag>& ip_tags); private: + void incCounter(Stats::StatName name); + + // Owned by this instance so the stats remain valid even if the caller that bootstrapped + // them goes away (relevant for providers shared across listeners). Stats::ScopeSharedPtr scope_; Stats::StatNameSetPtr stat_name_set_; const Stats::StatName stats_prefix_; @@ -59,12 +66,26 @@ class IpTagsLoader : public Logger::Loggable, const Stats::StatName total_; const Stats::StatName no_hit_; const Stats::StatName reload_success_; - void incCounter(Stats::StatName name); }; +using IpTagsStatsSharedPtr = std::shared_ptr; + +/** + * A single immutable snapshot of the parsed ip tags: the trie used for lookup and the + * stats container whose StatNameSet has the matching tag-name builtins interned. A + * snapshot is produced atomically on every load and handed to the filter as a unit, so + * a request never sees a trie from one reload paired with stats from another. + */ +struct LoadedIpTags { + LcTrieSharedPtr trie; + IpTagsStatsSharedPtr stats; +}; + +using LoadedIpTagsSharedPtr = std::shared_ptr; + /** - * This class owns ip tags trie structure for a configured absolute file path and provides access to - * the ip tags data. It also performs periodic refresh of ip tags data. + * Owns the file-watched DataSourceProvider that produces a new LoadedIpTags on every + * successful reload. */ class IpTagsProvider : public Logger::Loggable { public: @@ -77,34 +98,23 @@ class IpTagsProvider : public Logger::Loggable { ~IpTagsProvider(); - LcTrieSharedPtr ipTags(); - - void incHit(absl::string_view tag); - void incNoHit(); - void incTotal(); + LoadedIpTagsSharedPtr loadedIpTags() const { return data_source_provider_->data(); } private: - std::shared_ptr getTagsLoader(); - - Api::Api& api_; - Envoy::Config::DataSource::DataSourceProviderPtr> - data_source_provider_; - // A shared_ptr to keep the provider singleton alive as long as any of its providers are in use. + Envoy::Config::DataSource::DataSourceProviderPtr data_source_provider_; + // Keeps the registry singleton alive as long as any provider is in use. const Singleton::InstanceSharedPtr owner_; - mutable absl::Mutex tags_loader_mu_; - std::shared_ptr tags_loader_ ABSL_GUARDED_BY(tags_loader_mu_); }; using IpTagsProviderSharedPtr = std::shared_ptr; /** - * A singleton for file based loading of ip tags and looking up parsed trie data structures with Ip - * tags. When given equivalent file paths to the Ip tags, the singleton returns pointers to the same - * trie structure. + * A singleton that de-duplicates IpTagsProvider instances by datasource filename so two + * filter configs pointing at the same file share one provider (and thus one parsed trie). */ class IpTagsRegistrySingleton : public Envoy::Singleton::Instance { public: - IpTagsRegistrySingleton() {} + IpTagsRegistrySingleton() = default; absl::StatusOr> getOrCreateProvider(const envoy::config::core::v3::DataSource& ip_tags_datasource, Api::Api& api, @@ -114,11 +124,12 @@ class IpTagsRegistrySingleton : public Envoy::Singleton::Instance { std::shared_ptr singleton); private: - // Each provider stores shared_ptrs to this singleton, which keeps the singleton - // from being destroyed unless it's no longer keeping track of any providers. - // Each entry in this map consists of a key (hash of an absolute file path to ip tags file) - // and and value (instance of `IpTagsProvider` that owns ip tags data). + // Each provider stores a shared_ptr to this singleton, keeping the singleton alive + // until no provider remains. Key is a hash of the datasource filename. absl::flat_hash_map> ip_tags_registry_; + // Shared stats scope used by every provider this singleton creates. Lazily + // initialized as a child of the first caller's scope and kept alive by the + // singleton so a provider can outlive any individual listener that asked for it. Stats::ScopeSharedPtr scope_; }; @@ -144,16 +155,18 @@ class IpTaggingFilterConfig : public std::enable_shared_from_this& trie() const; + + // Returns the current snapshot. For the inline (static) path this is the snapshot + // built at construction; for the datasource path it is the snapshot currently + // published by the file watcher. The filter pulls this once per request so the trie + // and stats it uses come from the same reload. + LoadedIpTagsSharedPtr loadedIpTags() const { + return provider_ ? provider_->loadedIpTags() : static_ip_tags_; + } OptRef ipTagHeader() const; HeaderAction ipTagHeaderAction() const { return ip_tag_header_action_; } - void incHit(absl::string_view tag); - - void incNoHit(); - void incTotal(); - private: IpTaggingFilterConfig(const envoy::extensions::filters::http::ip_tagging::v3::IPTagging& config, const std::string& stat_prefix, Singleton::Manager& singleton_manager, @@ -176,25 +189,18 @@ class IpTaggingFilterConfig : public std::enable_shared_from_this ip_tags_registry_; - LcTrieSharedPtr trie_; + // Set only on the inline path. + LoadedIpTagsSharedPtr static_ip_tags_; + // Set only on the datasource path. std::shared_ptr provider_; }; @@ -226,7 +232,6 @@ class IpTaggingFilter : public Http::StreamDecoderFilter { Http::StreamDecoderFilterCallbacks* callbacks_{}; // Used for testing only. mutable Thread::ThreadSynchronizer synchronizer_; - // Allow the unit test to have access to private members. friend class IpTaggingFilterPeer; }; diff --git a/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc b/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc index 106d7911dc434..0599b148516a8 100644 --- a/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc +++ b/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc @@ -39,8 +39,8 @@ class IpTaggingFilterConfigPeer { } static void resetTrie(IpTaggingFilterConfig& filter_config) { - if (filter_config.trie_) { - filter_config.trie_.reset(); + if (filter_config.static_ip_tags_) { + filter_config.static_ip_tags_->trie.reset(); } } }; @@ -474,7 +474,7 @@ TEST_F(IpTaggingFilterTest, ReusesIpTagsProviderInstanceForSameFilePath) { auto provider2 = IpTaggingFilterConfigPeer::ipTagsProvider(*config2); EXPECT_NE(nullptr, provider1); EXPECT_NE(nullptr, provider2); - EXPECT_EQ(provider1->ipTags(), provider2->ipTags()); + EXPECT_EQ(provider1->loadedIpTags()->trie, provider2->loadedIpTags()->trie); } TEST_F(IpTaggingFilterTest, DifferentIpTagsProviderInstanceForDifferentFilePath) { @@ -501,7 +501,7 @@ TEST_F(IpTaggingFilterTest, DifferentIpTagsProviderInstanceForDifferentFilePath) auto provider2 = IpTaggingFilterConfigPeer::ipTagsProvider(*config2); EXPECT_NE(nullptr, provider1); EXPECT_NE(nullptr, provider2); - EXPECT_NE(provider1->ipTags(), provider2->ipTags()); + EXPECT_NE(provider1->loadedIpTags()->trie, provider2->loadedIpTags()->trie); ::testing::Mock::VerifyAndClearExpectations(&filter_callbacks_); } From db236356c7e5f68022822b4a26dba1428b9cbb99 Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Wed, 20 May 2026 06:18:32 +0000 Subject: [PATCH 2/4] fix format and address AI comment Signed-off-by: wbpcode/wangbaiping --- .../filters/http/ip_tagging/ip_tagging_filter.cc | 2 +- .../filters/http/ip_tagging/ip_tagging_filter.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc index 51cf0334d3fd1..eee209d36d26d 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc @@ -138,7 +138,7 @@ absl::StatusOr> IpTagsRegistrySingleton::getOrCr if (scope_ == nullptr) { scope_ = scope.createScope(""); } - const uint64_t key = std::hash()(ip_tags_datasource.filename()); + const size_t key = std::hash()(ip_tags_datasource.filename()); auto it = ip_tags_registry_.find(key); if (it != ip_tags_registry_.end()) { if (std::shared_ptr provider = it->second.lock()) { diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h index 5a33698dff349..bfb8b547be406 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h @@ -109,7 +109,7 @@ class IpTagsProvider : public Logger::Loggable { using IpTagsProviderSharedPtr = std::shared_ptr; /** - * A singleton that de-duplicates IpTagsProvider instances by datasource filename so two + * A singleton that de-duplicates IpTagsProvider instances by data source filename so two * filter configs pointing at the same file share one provider (and thus one parsed trie). */ class IpTagsRegistrySingleton : public Envoy::Singleton::Instance { @@ -125,7 +125,7 @@ class IpTagsRegistrySingleton : public Envoy::Singleton::Instance { private: // Each provider stores a shared_ptr to this singleton, keeping the singleton alive - // until no provider remains. Key is a hash of the datasource filename. + // until no provider remains. Key is a hash of the data source filename. absl::flat_hash_map> ip_tags_registry_; // Shared stats scope used by every provider this singleton creates. Lazily // initialized as a child of the first caller's scope and kept alive by the @@ -157,7 +157,7 @@ class IpTaggingFilterConfig : public std::enable_shared_from_this ip_tags_registry_; // Set only on the inline path. LoadedIpTagsSharedPtr static_ip_tags_; - // Set only on the datasource path. + // Set only on the data source path. std::shared_ptr provider_; }; From 19b4574ec2029abf0cf6aaef81a8bf6ea4445d95 Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Wed, 20 May 2026 09:32:51 +0000 Subject: [PATCH 3/4] fix test Signed-off-by: wbpcode/wangbaiping --- .../filters/http/ip_tagging/ip_tagging_integration_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/http/ip_tagging/ip_tagging_integration_test.cc b/test/extensions/filters/http/ip_tagging/ip_tagging_integration_test.cc index 82ed4bd4f5d33..0251ecd16b428 100644 --- a/test/extensions/filters/http/ip_tagging/ip_tagging_integration_test.cc +++ b/test/extensions/filters/http/ip_tagging/ip_tagging_integration_test.cc @@ -96,7 +96,8 @@ TEST_P(IpTaggingIntegrationTest, FileBasedIpTaggingWithReload) { TestEnvironment::temporaryPath("ip_tagging_test/watcher_new_target.yaml"), TestEnvironment::temporaryPath("ip_tagging_test/watcher_target.yaml")); - test_server_->waitForCounter("http.config_test.ip_tagging.reload_success", testing::Ge(2)); + // There is only one useful reload in this test. + test_server_->waitForCounter("http.config_test.ip_tagging.reload_success", testing::Ge(1)); response = codec_client_->makeHeaderOnlyRequest( Http::TestRequestHeaderMapImpl{{":method", "GET"}, From 088e84fd1aa69967c15db26da78e6c464e7ca90e Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Sat, 23 May 2026 15:00:45 +0000 Subject: [PATCH 4/4] address comments Signed-off-by: wbpcode/wangbaiping --- .../filters/http/ip_tagging/ip_tagging_filter.cc | 6 +++--- .../filters/http/ip_tagging/ip_tagging_filter.h | 9 +++++---- .../filters/http/ip_tagging/ip_tagging_filter_test.cc | 3 +-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc index eee209d36d26d..84194ffffbcb6 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc @@ -88,10 +88,10 @@ IpTagsProvider::IpTagsProvider(const envoy::config::core::v3::DataSource& ip_tag absl::string_view new_data) mutable -> absl::StatusOr> { IPTagsProto ip_tags_proto; if (absl::EndsWith(datasource_filename, MessageUtil::FileExtensions::get().Yaml)) { + // TODO(nezdolik) remove string casting once yaml utility has been migrated to + // string_view. auto data = std::string(new_data); auto load_status = - // TODO(nezdolik) remove string casting once yaml utility has been migrated to - // string_view. MessageUtil::loadFromYamlNoThrow(data, ip_tags_proto, validation_visitor); if (!load_status.ok()) { return load_status; @@ -246,7 +246,7 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::RequestHeaderMap& // Pull the current snapshot once so the trie lookup and stats updates that follow // come from the same reload, even if a file watcher publishes a new snapshot mid-call. - LoadedIpTagsSharedPtr loaded = config_->loadedIpTags(); + LoadedIpTagsConstSharedPtr loaded = config_->loadedIpTags(); if (loaded == nullptr) { return Http::FilterHeadersStatus::Continue; } diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h index bfb8b547be406..2c1531da1bf75 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h @@ -81,7 +81,7 @@ struct LoadedIpTags { IpTagsStatsSharedPtr stats; }; -using LoadedIpTagsSharedPtr = std::shared_ptr; +using LoadedIpTagsConstSharedPtr = std::shared_ptr; /** * Owns the file-watched DataSourceProvider that produces a new LoadedIpTags on every @@ -98,7 +98,7 @@ class IpTagsProvider : public Logger::Loggable { ~IpTagsProvider(); - LoadedIpTagsSharedPtr loadedIpTags() const { return data_source_provider_->data(); } + LoadedIpTagsConstSharedPtr loadedIpTags() const { return data_source_provider_->data(); } private: Envoy::Config::DataSource::DataSourceProviderPtr data_source_provider_; @@ -160,7 +160,7 @@ class IpTaggingFilterConfig : public std::enable_shared_from_thisloadedIpTags() : static_ip_tags_; } @@ -199,7 +199,7 @@ class IpTaggingFilterConfig : public std::enable_shared_from_this ip_tags_registry_; // Set only on the inline path. - LoadedIpTagsSharedPtr static_ip_tags_; + LoadedIpTagsConstSharedPtr static_ip_tags_; // Set only on the data source path. std::shared_ptr provider_; }; @@ -232,6 +232,7 @@ class IpTaggingFilter : public Http::StreamDecoderFilter { Http::StreamDecoderFilterCallbacks* callbacks_{}; // Used for testing only. mutable Thread::ThreadSynchronizer synchronizer_; + // Allow the unit test to have access to private members. friend class IpTaggingFilterPeer; }; diff --git a/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc b/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc index 0599b148516a8..d86a77741cf09 100644 --- a/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc +++ b/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc @@ -20,7 +20,6 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::InvokeWithoutArgs; using testing::Return; namespace Envoy { @@ -40,7 +39,7 @@ class IpTaggingFilterConfigPeer { static void resetTrie(IpTaggingFilterConfig& filter_config) { if (filter_config.static_ip_tags_) { - filter_config.static_ip_tags_->trie.reset(); + const_cast(filter_config.static_ip_tags_.get())->trie.reset(); } } };