From b6cdb252f3d178a3ddbd15b954b415f58bbfed19 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Sat, 18 Jul 2026 08:01:58 +0000 Subject: [PATCH] http: add server factory only factory support to geoip Signed-off-by: wbpcode --- envoy/geoip/geoip_provider_driver.h | 2 +- source/extensions/filters/http/geoip/BUILD | 1 + .../extensions/filters/http/geoip/config.cc | 21 ++++++++-- source/extensions/filters/http/geoip/config.h | 14 +++++++ .../filters/network/geoip/config.cc | 3 +- .../geoip_providers/common/factory_base.h | 4 +- .../geoip_providers/maxmind/config.cc | 16 ++++---- .../geoip_providers/maxmind/config.h | 3 +- .../filters/http/geoip/config_test.cc | 27 +++++++++++++ .../geoip_providers/maxmind/config_test.cc | 40 +++++++++++-------- .../maxmind/geoip_provider_test.cc | 7 ++-- test/mocks/geoip/mocks.h | 2 +- 12 files changed, 101 insertions(+), 39 deletions(-) diff --git a/envoy/geoip/geoip_provider_driver.h b/envoy/geoip/geoip_provider_driver.h index f4184e79f7380..5008f6e245ff2 100644 --- a/envoy/geoip/geoip_provider_driver.h +++ b/envoy/geoip/geoip_provider_driver.h @@ -64,7 +64,7 @@ class GeoipProviderFactory : public Config::TypedFactory { */ virtual DriverSharedPtr createGeoipProviderDriver(const Protobuf::Message& config, const std::string& stat_prefix, - Server::Configuration::FactoryContext& context) PURE; + Server::Configuration::ServerFactoryContext& context) PURE; std::string category() const override { return "envoy.geoip_providers"; } }; diff --git a/source/extensions/filters/http/geoip/BUILD b/source/extensions/filters/http/geoip/BUILD index 7d47565f71d1c..bfa2eab2afd1a 100644 --- a/source/extensions/filters/http/geoip/BUILD +++ b/source/extensions/filters/http/geoip/BUILD @@ -41,6 +41,7 @@ envoy_cc_extension( "//source/common/protobuf:utility_lib", "//source/extensions/filters/http/common:factory_base_lib", "//source/extensions/filters/http/geoip:geoip_filter_lib", + "//source/server:generic_factory_context_lib", "@envoy_api//envoy/extensions/filters/http/geoip/v3:pkg_cc_proto", ], ) diff --git a/source/extensions/filters/http/geoip/config.cc b/source/extensions/filters/http/geoip/config.cc index d178d9322c65d..ea6f4b3f8ea41 100644 --- a/source/extensions/filters/http/geoip/config.cc +++ b/source/extensions/filters/http/geoip/config.cc @@ -5,6 +5,7 @@ #include "source/common/config/utility.h" #include "source/common/protobuf/utility.h" #include "source/extensions/filters/http/geoip/geoip_filter.h" +#include "source/server/generic_factory_context.h" namespace Envoy { namespace Extensions { @@ -23,9 +24,9 @@ absl::Status validateConfig(const envoy::extensions::filters::http::geoip::v3::G } } // namespace -absl::StatusOr GeoipFilterFactory::createFilterFactoryFromProtoTyped( +absl::StatusOr GeoipFilterFactory::createFilterFactory( const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config, - const std::string& stat_prefix, Server::Configuration::FactoryContext& context) { + const std::string& stat_prefix, Server::Configuration::GenericFactoryContext& context) { // Validate configuration before creating the filter. auto status = validateConfig(proto_config); if (!status.ok()) { @@ -41,12 +42,26 @@ absl::StatusOr GeoipFilterFactory::createFilterFactoryFro provider_config); ProtobufTypes::MessagePtr message = Envoy::Config::Utility::translateToFactoryConfig( provider_config, context.messageValidationVisitor(), geo_provider_factory); - auto driver = geo_provider_factory.createGeoipProviderDriver(*message, stat_prefix, context); + auto driver = geo_provider_factory.createGeoipProviderDriver(*message, stat_prefix, + context.serverFactoryContext()); return [filter_config, driver](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(std::make_shared(filter_config, driver)); }; } +absl::StatusOr GeoipFilterFactory::createFilterFactoryFromProtoTyped( + const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config, + const std::string& stat_prefix, Server::Configuration::FactoryContext& context) { + return createFilterFactory(proto_config, stat_prefix, context); +} + +absl::StatusOr GeoipFilterFactory::createHttpFilterFactoryFromProtoTyped( + const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config, + const std::string& stat_prefix, Server::Configuration::ServerFactoryContext& context) { + Server::GenericFactoryContextImpl generic_context(context, context.messageValidationVisitor()); + return createFilterFactory(proto_config, stat_prefix, generic_context); +} + /** * Static registration for geoip filter. @see RegisterFactory. */ diff --git a/source/extensions/filters/http/geoip/config.h b/source/extensions/filters/http/geoip/config.h index a5dc0f69c1d02..9e7f852513ed7 100644 --- a/source/extensions/filters/http/geoip/config.h +++ b/source/extensions/filters/http/geoip/config.h @@ -22,6 +22,20 @@ class GeoipFilterFactory absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; + absl::StatusOr createHttpFilterFactoryFromProtoTyped( + const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config, + const std::string& stats_prefix, + Server::Configuration::ServerFactoryContext& context) override; + +private: + // Shared factory creation used by both the downstream (FactoryContext) and route/vhost-level + // (ServerFactoryContext) paths. A GenericFactoryContext is used so the filter's scope and + // validation visitor stay correct for each path, while the provider driver is created with the + // server factory context. + absl::StatusOr + createFilterFactory(const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config, + const std::string& stat_prefix, + Server::Configuration::GenericFactoryContext& context); }; } // namespace Geoip diff --git a/source/extensions/filters/network/geoip/config.cc b/source/extensions/filters/network/geoip/config.cc index 89e0f518fc35f..c1d20a26ec3d7 100644 --- a/source/extensions/filters/network/geoip/config.cc +++ b/source/extensions/filters/network/geoip/config.cc @@ -37,7 +37,8 @@ absl::StatusOr GeoipFilterFactory::createFilterFactory provider_config); ProtobufTypes::MessagePtr message = Envoy::Config::Utility::translateToFactoryConfig( provider_config, context.messageValidationVisitor(), geo_provider_factory); - auto driver = geo_provider_factory.createGeoipProviderDriver(*message, stat_prefix, context); + auto driver = geo_provider_factory.createGeoipProviderDriver(*message, stat_prefix, + context.serverFactoryContext()); return [filter_config, driver](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(std::make_shared(filter_config, driver)); diff --git a/source/extensions/geoip_providers/common/factory_base.h b/source/extensions/geoip_providers/common/factory_base.h index 9376192e8d666..b3e12f3e381a9 100644 --- a/source/extensions/geoip_providers/common/factory_base.h +++ b/source/extensions/geoip_providers/common/factory_base.h @@ -19,7 +19,7 @@ template class FactoryBase : public Geolocation::GeoipProvid // GeoipProviderFactory Geolocation::DriverSharedPtr createGeoipProviderDriver(const Protobuf::Message& config, const std::string& stat_prefix, - Server::Configuration::FactoryContext& context) override { + Server::Configuration::ServerFactoryContext& context) override { return createGeoipProviderDriverTyped(MessageUtil::downcastAndValidate( config, context.messageValidationVisitor()), stat_prefix, context); @@ -35,7 +35,7 @@ template class FactoryBase : public Geolocation::GeoipProvid private: virtual Geolocation::DriverSharedPtr createGeoipProviderDriverTyped(const ConfigProto& proto_config, const std::string& stat_prefix, - Server::Configuration::FactoryContext& context) PURE; + Server::Configuration::ServerFactoryContext& context) PURE; const std::string name_; }; diff --git a/source/extensions/geoip_providers/maxmind/config.cc b/source/extensions/geoip_providers/maxmind/config.cc index 2d626cb91e1f3..3f840e13af795 100644 --- a/source/extensions/geoip_providers/maxmind/config.cc +++ b/source/extensions/geoip_providers/maxmind/config.cc @@ -25,7 +25,7 @@ class DriverSingleton : public Envoy::Singleton::Instance { std::shared_ptr get(std::shared_ptr singleton, const ConfigProto& proto_config, const std::string& stat_prefix, - Server::Configuration::FactoryContext& context) { + Server::Configuration::ServerFactoryContext& context) { std::shared_ptr driver; const uint64_t key = MessageUtil::hash(proto_config); absl::MutexLock lock(mu_); @@ -35,9 +35,8 @@ class DriverSingleton : public Envoy::Singleton::Instance { } else { const auto& provider_config = std::make_shared(proto_config, stat_prefix, context.scope()); - driver = std::make_shared( - context.serverFactoryContext().mainThreadDispatcher(), - context.serverFactoryContext().api(), singleton, provider_config); + driver = std::make_shared(context.mainThreadDispatcher(), context.api(), + singleton, provider_config); drivers_[key] = driver; } return driver; @@ -58,11 +57,10 @@ MaxmindProviderFactory::MaxmindProviderFactory() : FactoryBase("envoy.geoip_prov DriverSharedPtr MaxmindProviderFactory::createGeoipProviderDriverTyped( const ConfigProto& proto_config, const std::string& stat_prefix, - Server::Configuration::FactoryContext& context) { - std::shared_ptr drivers = - context.serverFactoryContext().singletonManager().getTyped( - SINGLETON_MANAGER_REGISTERED_NAME(maxmind_geolocation_provider_singleton), - [] { return std::make_shared(); }); + Server::Configuration::ServerFactoryContext& context) { + std::shared_ptr drivers = context.singletonManager().getTyped( + SINGLETON_MANAGER_REGISTERED_NAME(maxmind_geolocation_provider_singleton), + [] { return std::make_shared(); }); return drivers->get(drivers, proto_config, stat_prefix, context); } diff --git a/source/extensions/geoip_providers/maxmind/config.h b/source/extensions/geoip_providers/maxmind/config.h index 007bc90936eef..0ca73e16105d9 100644 --- a/source/extensions/geoip_providers/maxmind/config.h +++ b/source/extensions/geoip_providers/maxmind/config.h @@ -26,7 +26,8 @@ class MaxmindProviderFactory // FactoryBase DriverSharedPtr createGeoipProviderDriverTyped( const envoy::extensions::geoip_providers::maxmind::v3::MaxMindConfig& proto_config, - const std::string& stat_prefix, Server::Configuration::FactoryContext& context) override; + const std::string& stat_prefix, + Server::Configuration::ServerFactoryContext& context) override; }; DECLARE_FACTORY(MaxmindProviderFactory); diff --git a/test/extensions/filters/http/geoip/config_test.cc b/test/extensions/filters/http/geoip/config_test.cc index 825a5c370f5aa..9f8198e06b3ba 100644 --- a/test/extensions/filters/http/geoip/config_test.cc +++ b/test/extensions/filters/http/geoip/config_test.cc @@ -113,6 +113,33 @@ TEST(GeoipFilterConfigTest, GeoipFilterConfigWithCorrectProto) { cb(filter_callback); } +TEST(GeoipFilterConfigTest, GeoipFilterConfigWithCorrectProto2) { + TestScopedRuntime scoped_runtime; + Geolocation::DummyGeoipProviderFactory dummy_factory; + Registry::InjectFactory registered(dummy_factory); + std::string filter_config_yaml = R"EOF( + xff_config: + xff_num_trusted_hops: 1 + provider: + name: "envoy.geoip_providers.dummy" + typed_config: + "@type": type.googleapis.com/test.mocks.geoip.DummyProvider + )EOF"; + GeoipFilterConfig filter_config; + TestUtility::loadFromYaml(filter_config_yaml, filter_config); + NiceMock context; + EXPECT_CALL(context.server_factory_context_, messageValidationVisitor()).Times(2); + GeoipFilterFactory factory; + Http::FilterFactoryCb cb = + factory + .createHttpFilterFactoryFromProto(filter_config, "geoip", context.server_factory_context_) + .value(); + Http::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, + addStreamDecoderFilter(AllOf(HasUseXff(true), HasXffNumTrustedHops(1)))); + cb(filter_callback); +} + TEST(GeoipFilterConfigTest, GeoipFilterConfigMissingProvider) { TestScopedRuntime scoped_runtime; Geolocation::DummyGeoipProviderFactory dummy_factory; diff --git a/test/extensions/geoip_providers/maxmind/config_test.cc b/test/extensions/geoip_providers/maxmind/config_test.cc index b48ab9d6c5886..0f8b38bdfc30d 100644 --- a/test/extensions/geoip_providers/maxmind/config_test.cc +++ b/test/extensions/geoip_providers/maxmind/config_test.cc @@ -284,7 +284,7 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithCorrectProto) { TestUtility::loadFromYaml(processed_provider_config_yaml, provider_config); MaxmindProviderFactory factory; Geolocation::DriverSharedPtr driver = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); EXPECT_THAT(driver, AllOf(HasCityDbPath(city_db_path), HasIspDbPath(isp_db_path), HasAnonDbPath(anon_db_path), HasCountryHeader("x-geo-country"), HasCityHeader("x-geo-city"), HasRegionHeader("x-geo-region"), @@ -305,7 +305,9 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithNoDbPaths) { NiceMock context; MaxmindProviderFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createGeoipProviderDriver(provider_config, "maxmind", context), Envoy::EnvoyException, + factory.createGeoipProviderDriver(provider_config, "maxmind", + context.server_factory_context_), + Envoy::EnvoyException, "At least one geolocation database path needs to be configured: " "city_db_path, isp_db_path, asn_db_path, anon_db_path or country_db_path"); } @@ -317,9 +319,10 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithNoGeoHeaders) { MaxmindProviderConfig provider_config; TestUtility::loadFromYaml(provider_config_yaml, provider_config); NiceMock context; - EXPECT_CALL(context, messageValidationVisitor()); + EXPECT_CALL(context.server_factory_context_, messageValidationVisitor()); MaxmindProviderFactory factory; - EXPECT_THROW_WITH_REGEX(factory.createGeoipProviderDriver(provider_config, "maxmind", context), + EXPECT_THROW_WITH_REGEX(factory.createGeoipProviderDriver(provider_config, "maxmind", + context.server_factory_context_), ProtoValidationException, "Proto constraint validation failed.*value is required.*"); } @@ -334,10 +337,11 @@ TEST_F(MaxmindProviderConfigTest, DbPathFormatValidatedWhenNonEmptyValue) { MaxmindProviderConfig provider_config; TestUtility::loadFromYaml(provider_config_yaml, provider_config); NiceMock context; - EXPECT_CALL(context, messageValidationVisitor()); + EXPECT_CALL(context.server_factory_context_, messageValidationVisitor()); MaxmindProviderFactory factory; EXPECT_THROW_WITH_REGEX( - factory.createGeoipProviderDriver(provider_config, "maxmind", context), + factory.createGeoipProviderDriver(provider_config, "maxmind", + context.server_factory_context_), ProtoValidationException, "Proto constraint validation failed.*value does not match regex pattern.*"); } @@ -370,9 +374,9 @@ TEST_F(MaxmindProviderConfigTest, ReusesProviderInstanceForSameProtoConfig) { TestUtility::loadFromYaml(processed_provider_config_yaml, provider_config); MaxmindProviderFactory factory; Geolocation::DriverSharedPtr driver1 = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); Geolocation::DriverSharedPtr driver2 = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); EXPECT_EQ(driver1.get(), driver2.get()); } @@ -416,9 +420,9 @@ TEST_F(MaxmindProviderConfigTest, DifferentProviderInstancesForDifferentProtoCon TestUtility::loadFromYaml(processed_provider_config_yaml2, provider_config2); MaxmindProviderFactory factory; Geolocation::DriverSharedPtr driver1 = - factory.createGeoipProviderDriver(provider_config1, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config1, "maxmind", server_factory_context_); Geolocation::DriverSharedPtr driver2 = - factory.createGeoipProviderDriver(provider_config2, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config2, "maxmind", server_factory_context_); EXPECT_NE(driver1.get(), driver2.get()); } @@ -435,7 +439,7 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithCountryDbPath) { TestUtility::loadFromYaml(processed_provider_config_yaml, provider_config); MaxmindProviderFactory factory; Geolocation::DriverSharedPtr driver = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); // City DB is not configured, so isCityDbPathSet() should return false. EXPECT_THAT(driver, AllOf(HasCountryDbPath(country_db_path), HasCountryHeader("x-geo-country"), IsCityDbPathSet(false))); @@ -458,7 +462,7 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithCountryDbAndCityDbPaths) { TestUtility::loadFromYaml(processed_provider_config_yaml, provider_config); MaxmindProviderFactory factory; Geolocation::DriverSharedPtr driver = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); // Both Country DB and City DB are configured. EXPECT_THAT(driver, AllOf(HasCountryDbPath(country_db_path), HasCityDbPath(city_db_path), HasCountryHeader("x-geo-country"), HasCityHeader("x-geo-city"), @@ -497,7 +501,7 @@ TEST_F(MaxmindProviderConfigTest, EXPECT_LOG_CONTAINS( "warning", "Using deprecated option", Geolocation::DriverSharedPtr driver = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); EXPECT_THAT(driver, AllOf(HasCityDbPath(city_db_path), HasIspDbPath(isp_db_path), HasAnonDbPath(anon_db_path), HasCountryHeader("x-geo-country"), @@ -523,8 +527,8 @@ TEST_F(MaxmindProviderConfigTest, MaxmindProviderFactory factory; // Verify that is_anon field is read and used as anon_header_. EXPECT_LOG_CONTAINS("warning", "Using deprecated option", - Geolocation::DriverSharedPtr driver = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + Geolocation::DriverSharedPtr driver = factory.createGeoipProviderDriver( + provider_config, "maxmind", server_factory_context_); auto provider = std::static_pointer_cast(driver); auto anon_header = GeoipProviderPeer::countryHeader(*provider); // The is_anon fallback should populate the anon header. @@ -547,7 +551,9 @@ TEST_F(MaxmindProviderConfigTest, NiceMock context; MaxmindProviderFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createGeoipProviderDriver(provider_config, "maxmind", context), Envoy::EnvoyException, + factory.createGeoipProviderDriver(provider_config, "maxmind", + context.server_factory_context_), + Envoy::EnvoyException, "At least one geolocation database path needs to be configured: " "city_db_path, isp_db_path, asn_db_path, anon_db_path or country_db_path"); } @@ -574,7 +580,7 @@ TEST_F(MaxmindProviderConfigTest, DEPRECATED_FEATURE_TEST(GeoFieldKeysTakesPrece // geo_field_keys should take precedence, so we should see the "new" values. // The deprecated geo_headers_to_add should be ignored. Geolocation::DriverSharedPtr driver = - factory.createGeoipProviderDriver(provider_config, "maxmind", context_); + factory.createGeoipProviderDriver(provider_config, "maxmind", server_factory_context_); EXPECT_THAT(driver, AllOf(HasCountryHeader("x-geo-country-new"), HasCityHeader("x-geo-city-new"))); // Region should NOT be set because geo_field_keys takes precedence and it doesn't have region. diff --git a/test/extensions/geoip_providers/maxmind/geoip_provider_test.cc b/test/extensions/geoip_providers/maxmind/geoip_provider_test.cc index f2440afba5072..56efd79342133 100644 --- a/test/extensions/geoip_providers/maxmind/geoip_provider_test.cc +++ b/test/extensions/geoip_providers/maxmind/geoip_provider_test.cc @@ -146,9 +146,7 @@ class GeoipProviderTestBase { void initializeProvider(const std::string& yaml, std::optional& conditional) { - EXPECT_CALL(context_, scope()).WillRepeatedly(ReturnRef(*scope_)); - EXPECT_CALL(context_, serverFactoryContext()) - .WillRepeatedly(ReturnRef(server_factory_context_)); + EXPECT_CALL(server_factory_context_, scope()).WillRepeatedly(ReturnRef(*scope_)); EXPECT_CALL(server_factory_context_, api()).WillRepeatedly(ReturnRef(*api_)); EXPECT_CALL(dispatcher_, createFilesystemWatcher_()) .WillRepeatedly(Invoke([this, &conditional] { @@ -172,7 +170,8 @@ class GeoipProviderTestBase { .WillRepeatedly(ReturnRef(dispatcher_)); envoy::extensions::geoip_providers::maxmind::v3::MaxMindConfig config; TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), config); - provider_ = provider_factory_->createGeoipProviderDriver(config, "prefix.", context_); + provider_ = + provider_factory_->createGeoipProviderDriver(config, "prefix.", server_factory_context_); } void expectStats(const absl::string_view& db_type, const uint32_t total_count = 1, diff --git a/test/mocks/geoip/mocks.h b/test/mocks/geoip/mocks.h index aeaf56a7b3047..0c51ba42a4729 100644 --- a/test/mocks/geoip/mocks.h +++ b/test/mocks/geoip/mocks.h @@ -24,7 +24,7 @@ class DummyGeoipProviderFactory : public GeoipProviderFactory { DummyGeoipProviderFactory() : driver_(new MockDriver()) {} Geolocation::DriverSharedPtr createGeoipProviderDriver(const Protobuf::Message&, const std::string&, - Server::Configuration::FactoryContext&) override { + Server::Configuration::ServerFactoryContext&) override { return driver_; }