add maxmind country db support by hwyuan · Pull Request #31147 · envoyproxy/envoy

hwyuan · 2023-12-02T20:30:58Z

Commit Message: add maxmind country db support
Additional Description:
Support maxmind country db in geoip maxmind provider.
Since both country db and city db can populate country info, we allow at most one of country db and city db can be configured to avoid ambiguity.
Risk Level: Low
Testing: unit test
Docs Changes: TODO
Release Notes: TODO
Platform Specific Features:
[Optional Fixes #Issue] #30956

repokitteh-read-only · 2023-12-02T20:31:04Z

Hi @hwyuan, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #31147 was opened by hwyuan.

see: more, trace.

repokitteh-read-only · 2023-12-02T20:31:05Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #31147 was opened by hwyuan.

see: more, trace.

repokitteh-read-only · 2023-12-02T20:31:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #31147 was opened by hwyuan.

see: more, trace.

hwyuan · 2023-12-05T08:17:04Z

/retest

hwyuan · 2023-12-05T18:18:39Z

cc @JuniorHsu

JuniorHsu

also needs changelogs/current.yaml change

JuniorHsu · 2023-12-05T19:31:31Z

by seeing this comment, should we do oneof in proto so we only allow either country or city map?

makes sense, let me update the PR to try oneof

An oneof change of the existing non-oneof field may break the backward compatibility in ProtoBuf, you should not change it and add configuration time exception to notice the user setting both country and city databases is not valid.

sure, let's throw Exception in the constructor of GeoipProviderConfig

since we are adding a new field country_db_path_ in this PR, and users will only be able to pick country_db_path_ or city_db_path after this change, i.e., currently they would either configure city_db_path_ or not using it, would we still have backward compatibility issues of we can adopt oneof directly in proto definition in this PR?

It does not depend on how you use the field. The ProtoBuf does not support oneof backward compatibility and it may cause certain issues. Please refer to the ProtoBuf doc and Envoy API style (Prefer multiple fields with defined precedence over boolean overloads of fields or oneof).

#30851 This is a pretty recent policy change for discouraging oneof. Thanks for the pointer.

As my understanding, some config generation tools have different setup with oneof and non-oneof so moving in/out oneof might causing compatibility issue as well.

got it, thank you both for the context. will update the PR to add the config time exception

JuniorHsu · 2023-12-05T19:35:30Z

this function has lots of similarity with lookupInCityDb. could we refactor to achieve DRY if it's not too hard?

there are indeed similarities in these lookup function and ideally be better abstracted, wonder if we could do the refacor in a seperate PR so that this PR focuses on adding country db support

i'll defer to code owner @nezdolik @ravenblackx

Given that there is already similar duplicated code prior to this PR and PR adds more duplication, is good opportunity to tackle refactor here.

let me give it a try, was worried that refactoring here might make this PR complicated to review, maybe could discuss more once there is a draft commit

seems one possible approach is to have a common lookupInMaxmindDb function, and then we pass the db_type and maxmind_db database pointer to this function, where lookupInMaxmindDb will call related isLookupEnableForDbType and populateGeoLookupResultForDbType functions, where db-specific logics are handled.

What do you think about the approach? also still feels like might be better to do in a separate PR to it's cleaner and easier to review (and to revert if ever needed) as other db lookup functions will be touched, thoughts? @nezdolik

pseudocode

void GeoipProvider::lookupInMaxmindDb( const Network::Address::InstanceConstSharedPtr& remote_address, absl::flat_hash_map<std::string, std::string>& lookup_result, const std::string& db_type /* we can make db_type a enum */, const std::string& stat_prefix /* e.g. city for city_db */, const MaxmindDbPtr& maxmind_db /* passing db pointer */, ) const { if (isLookupEnableForDbType(db_type)) ASSERT(maxmind_db, fmt.format("Maxmind {} database is not initialised for performing lookups", stat_prefix)); int mmdb_error; const uint32_t n_prev_hits = lookup_result.size(); MMDB_lookup_result_s mmdb_lookup_result = MMDB_lookup_sockaddr( maxmind_db.get(), reinterpret_cast<const sockaddr*>(remote_address->sockAddr()), &mmdb_error); if (!mmdb_error) { MMDB_entry_data_list_s* entry_data_list; int status = MMDB_get_entry_data_list(&mmdb_lookup_result.entry, &entry_data_list); if (status == MMDB_SUCCESS) { // populate headers populateGeoLookupResultForDbType(db_type) if (lookup_result.size() > n_prev_hits) { config_->incHit(fmt.format("{}_db", stat_prefix)); } MMDB_free_entry_data_list(entry_data_list); } } else { config_->incLookupError(fmt.format("{}_db", stat_prefix)); } config_->incTotal(fmt.format("{}_db", stat_prefix)); } }

drafted an example PR in #31490, please take a look when you have a chance

JuniorHsu

I've done the first pass modulo nits and questions. Over to owner/maintainer's review @nezdolik @ravenblackx

JuniorHsu · 2023-12-08T18:05:13Z

i'll defer to code owner @nezdolik @ravenblackx

JuniorHsu · 2023-12-08T18:09:47Z

Not sure if we want to dup this warning to city_db_path as well. @nezdolik @ravenblackx thought?

nit: move this field in the end of the proto message, as it is the most recently added.

Is fine to have warning only here, given that there will validation error when user sets both fields (so they should read docs for both fields).

hwyuan · 2023-12-09T19:10:11Z

/retest

Signed-off-by: Haowei Yuan <hyuan@wustl.edu>

nezdolik

Looking at Maxmind comparison of city and country databases (in the bottom of the page), country one contains subset of fields from city, so there will be no case when someone needs both databases to be configured.

nezdolik · 2023-12-12T12:36:19Z

nit: move this field in the end of the proto message, as it is the most recently added.

nezdolik · 2023-12-12T12:42:15Z

Is fine to have warning only here, given that there will validation error when user sets both fields (so they should read docs for both fields).

nezdolik · 2023-12-12T12:42:32Z

@@ -23,7 +23,13 @@ option (xds.annotations.v3.file_status).work_in_progress = true;
 // :ref:`anon_db_path <envoy_v3_api_field_extensions.geoip_providers.maxmind.v3.MaxMindConfig.anon_db_path>` must be configured.


can you also update here?

sure, does it make sense to move the warning of "at most one of country_db_path and city_db_path can be configured" here so that these instructions are together? and remove it from line 30 below

nezdolik · 2023-12-12T12:44:33Z

Given that there is already similar duplicated code prior to this PR and PR adds more duplication, is good opportunity to tackle refactor here.

nezdolik · 2023-12-12T12:46:11Z

  EXPECT_DEATH(initializeProvider(config_yaml), ".*Unable to open Maxmind database file.*");
 }

+TEST_F(GeoipProviderTest, ValidConfigCountryDbSuccessfulLookup) {


can you add tests to geoip_filter_integration_test.cc as well?

sure, will do

nezdolik · 2023-12-12T13:37:03Z

 message MaxMindConfig {
+  // Full file path to the Maxmind country database, e.g. /etc/GeoLite2-Country.mmdb.
+  // Database file is expected to have .mmdb extension.
+  // Note that only one of country_db_path and city_db_path can be set.


Add reference to proto fields, e.g. :ref:city_db_path <envoy_v3_api_field_extensions.geoip_providers.maxmind.v3.MaxMindConfig.city_db_path>

Signed-off-by: Haowei Yuan <hyuan@wustl.edu>

lizan

/lgtm api

github-actions · 2024-01-21T08:01:18Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2024-01-28T08:02:02Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

zetaab · 2024-10-08T18:11:45Z

@nezdolik @lizan @hwyuan could this PR be reopened? At least I see that this is useful feature and actually it was already lgtm by @lizan. So I do not see why it was closed?

nezdolik · 2024-10-09T11:15:26Z

it did require more work on pr to be done

repokitteh-read-only Bot added the api label Dec 2, 2023

repokitteh-read-only Bot assigned lizan Dec 2, 2023

hwyuan force-pushed the add-maxmind-country-db branch 2 times, most recently from ab7875c to f0505e0 Compare December 5, 2023 06:54

JuniorHsu suggested changes Dec 5, 2023

View reviewed changes

hwyuan force-pushed the add-maxmind-country-db branch 2 times, most recently from 8722b63 to 68d7960 Compare December 8, 2023 09:12

hwyuan changed the title ~~WIP: add maxmind country db support~~ add maxmind country db support Dec 8, 2023

hwyuan marked this pull request as ready for review December 8, 2023 17:30

hwyuan requested review from nezdolik and ravenblackx as code owners December 8, 2023 17:30

JuniorHsu reviewed Dec 8, 2023

View reviewed changes

hwyuan force-pushed the add-maxmind-country-db branch 3 times, most recently from 5399e8d to 042ef2b Compare December 9, 2023 07:59

hwyuan force-pushed the add-maxmind-country-db branch from 042ef2b to 006b66d Compare December 9, 2023 19:25

add maxmind country db support

8e12495

Signed-off-by: Haowei Yuan <hyuan@wustl.edu>

hwyuan force-pushed the add-maxmind-country-db branch from 006b66d to 8e12495 Compare December 12, 2023 06:41

nezdolik reviewed Dec 12, 2023

View reviewed changes

update proto and add integration tests

118cb48

Signed-off-by: Haowei Yuan <hyuan@wustl.edu>

lizan reviewed Dec 14, 2023

View reviewed changes

repokitteh-read-only Bot removed the api label Dec 14, 2023

lizan added the waiting label Dec 19, 2023

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 21, 2024

github-actions Bot closed this Jan 28, 2024

		@@ -23,7 +23,13 @@ option (xds.annotations.v3.file_status).work_in_progress = true;
		// :ref:`anon_db_path <envoy_v3_api_field_extensions.geoip_providers.maxmind.v3.MaxMindConfig.anon_db_path>` must be configured.

Conversation

hwyuan commented Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Dec 2, 2023

Uh oh!

repokitteh-read-only Bot commented Dec 2, 2023

Uh oh!

repokitteh-read-only Bot commented Dec 2, 2023

Uh oh!

hwyuan commented Dec 5, 2023

Uh oh!

hwyuan commented Dec 5, 2023

Uh oh!

JuniorHsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhxie Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JuniorHsu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwyuan commented Dec 9, 2023

Uh oh!

nezdolik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hwyuan commented Dec 2, 2023 •

edited

Loading

zhxie Dec 7, 2023 •

edited

Loading