Skip to content

http: add server factory only factory support to geoip #46224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wbpcode wants to merge 1 commit into
base: main
Choose a base branch
from
Open

http: add server factory only factory support to geoip #46224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion envoy/geoip/geoip_provider_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@wbpcode wbpcode Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may break our forks because this API changes if our forks have a custom GeoipProvider implementation, but I have no good idea to keep the compatbility.

I think it's acceptable because this is not an extension point like HTTP filter.

@wbpcode wbpcode Jul 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I checked the existing implementation of this extension point, it's possible the cached driver be shared across listeners, the server context is more appropriate for this API.

Anyway, I will let maintainers to check to this point.

@wbpcode wbpcode Jul 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @yanavlasov @ravenblackx


std::string category() const override { return "envoy.geoip_providers"; }
};
Expand Down
1 change: 1 addition & 0 deletions source/extensions/filters/http/geoip/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
21 changes: 18 additions & 3 deletions source/extensions/filters/http/geoip/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -23,9 +24,9 @@ absl::Status validateConfig(const envoy::extensions::filters::http::geoip::v3::G
}
} // namespace

absl::StatusOr<Http::FilterFactoryCb> GeoipFilterFactory::createFilterFactoryFromProtoTyped(
absl::StatusOr<Http::FilterFactoryCb> 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()) {
Expand All @@ -41,12 +42,26 @@ absl::StatusOr<Http::FilterFactoryCb> 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<GeoipFilter>(filter_config, driver));
};
}

absl::StatusOr<Http::FilterFactoryCb> 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<Http::FilterFactoryCb> 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.
*/
Expand Down
14 changes: 14 additions & 0 deletions source/extensions/filters/http/geoip/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,20 @@ class GeoipFilterFactory
absl::StatusOr<Http::FilterFactoryCb> createFilterFactoryFromProtoTyped(
const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;
absl::StatusOr<Http::FilterFactoryCb> 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<Http::FilterFactoryCb>
createFilterFactory(const envoy::extensions::filters::http::geoip::v3::Geoip& proto_config,
const std::string& stat_prefix,
Server::Configuration::GenericFactoryContext& context);
};

} // namespace Geoip
Expand Down
3 changes: 2 additions & 1 deletion source/extensions/filters/network/geoip/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ absl::StatusOr<Network::FilterFactoryCb> 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<GeoipFilter>(filter_config, driver));
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/geoip_providers/common/factory_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ template <class ConfigProto> 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<const ConfigProto&>(
config, context.messageValidationVisitor()),
stat_prefix, context);
Expand All @@ -35,7 +35,7 @@ template <class ConfigProto> 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_;
};
Expand Down
16 changes: 7 additions & 9 deletions source/extensions/geoip_providers/maxmind/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class DriverSingleton : public Envoy::Singleton::Instance {
std::shared_ptr<GeoipProvider> get(std::shared_ptr<DriverSingleton> singleton,
const ConfigProto& proto_config,
const std::string& stat_prefix,
Server::Configuration::FactoryContext& context) {
Server::Configuration::ServerFactoryContext& context) {
std::shared_ptr<GeoipProvider> driver;
const uint64_t key = MessageUtil::hash(proto_config);
absl::MutexLock lock(mu_);
Expand All @@ -35,9 +35,8 @@ class DriverSingleton : public Envoy::Singleton::Instance {
} else {
const auto& provider_config =
std::make_shared<GeoipProviderConfig>(proto_config, stat_prefix, context.scope());
driver = std::make_shared<GeoipProvider>(
context.serverFactoryContext().mainThreadDispatcher(),
context.serverFactoryContext().api(), singleton, provider_config);
driver = std::make_shared<GeoipProvider>(context.mainThreadDispatcher(), context.api(),
singleton, provider_config);
drivers_[key] = driver;
}
return driver;
Expand All @@ -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<DriverSingleton> drivers =
context.serverFactoryContext().singletonManager().getTyped<DriverSingleton>(
SINGLETON_MANAGER_REGISTERED_NAME(maxmind_geolocation_provider_singleton),
[] { return std::make_shared<DriverSingleton>(); });
Server::Configuration::ServerFactoryContext& context) {
std::shared_ptr<DriverSingleton> drivers = context.singletonManager().getTyped<DriverSingleton>(
SINGLETON_MANAGER_REGISTERED_NAME(maxmind_geolocation_provider_singleton),
[] { return std::make_shared<DriverSingleton>(); });
return drivers->get(drivers, proto_config, stat_prefix, context);
}

Expand Down
3 changes: 2 additions & 1 deletion source/extensions/geoip_providers/maxmind/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
27 changes: 27 additions & 0 deletions test/extensions/filters/http/geoip/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,33 @@ TEST(GeoipFilterConfigTest, GeoipFilterConfigWithCorrectProto) {
cb(filter_callback);
}

TEST(GeoipFilterConfigTest, GeoipFilterConfigWithCorrectProto2) {
TestScopedRuntime scoped_runtime;
Geolocation::DummyGeoipProviderFactory dummy_factory;
Registry::InjectFactory<Geolocation::GeoipProviderFactory> 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<Server::Configuration::MockFactoryContext> 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;
Expand Down
40 changes: 23 additions & 17 deletions test/extensions/geoip_providers/maxmind/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -305,7 +305,9 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithNoDbPaths) {
NiceMock<Server::Configuration::MockFactoryContext> 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");
}
Expand All @@ -317,9 +319,10 @@ TEST_F(MaxmindProviderConfigTest, ProviderConfigWithNoGeoHeaders) {
MaxmindProviderConfig provider_config;
TestUtility::loadFromYaml(provider_config_yaml, provider_config);
NiceMock<Server::Configuration::MockFactoryContext> 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.*");
}
Expand All @@ -334,10 +337,11 @@ TEST_F(MaxmindProviderConfigTest, DbPathFormatValidatedWhenNonEmptyValue) {
MaxmindProviderConfig provider_config;
TestUtility::loadFromYaml(provider_config_yaml, provider_config);
NiceMock<Server::Configuration::MockFactoryContext> 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.*");
}
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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());
}

Expand All @@ -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)));
Expand All @@ -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"),
Expand Down Expand Up @@ -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"),
Expand All @@ -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<GeoipProvider>(driver);
auto anon_header = GeoipProviderPeer::countryHeader(*provider);
// The is_anon fallback should populate the anon header.
Expand All @@ -547,7 +551,9 @@ TEST_F(MaxmindProviderConfigTest,
NiceMock<Server::Configuration::MockFactoryContext> 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");
}
Expand All @@ -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.
Expand Down
7 changes: 3 additions & 4 deletions test/extensions/geoip_providers/maxmind/geoip_provider_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,7 @@ class GeoipProviderTestBase {

void initializeProvider(const std::string& yaml,
std::optional<ConditionalInitializer>& 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] {
Expand All @@ -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,
Expand Down
Loading
Loading