Fix header_rewrite MaxMind geo lookups for GeoIP2/GeoLite2 mmdb databases by bryancall · Pull Request #12948 · apache/trafficserver

bryancall · 2026-03-06T23:32:48Z

Problem

The header_rewrite plugin's MaxMind geo lookup code used incorrect MMDB
field paths that don't match the structure of standard MaxMind GeoIP2
databases (GeoLite2, GeoIP2, DBIP). All geo lookups (%{GEO:COUNTRY},
%{GEO:ASN}, %{GEO:ASN-NAME}) returned (unknown) or -1 even
when the mmdb file loaded successfully. The code queried flat field names
like "country_code" that don't exist -- these databases use nested
structures like country -> iso_code.

Changes

Fix country code lookup -- change GEO_QUAL_COUNTRY from flat
"country_code" to nested "country", "iso_code" path, returning
the ISO country code (e.g. "KR", "US") consistent with the old GeoIP
backend's GeoIP_country_code_by_ipnum() behavior
Fix null-after-delete bug in initLibrary -- set
gMaxMindDB = nullptr after delete on MMDB_open failure to
avoid leaving a dangling global pointer
Add data validation -- check entry_data.has_data and
entry_data.type before accessing entry values to avoid reading
uninitialized data
Remove unnecessary MMDB_get_entry_data_list allocation -- the
MMDB_get_value() API works directly from result.entry, so the
expensive list allocation/traversal was unnecessary overhead on every
geo lookup
Add MMDB path validation unit test -- verifies the nested lookup
paths against a real MaxMind/DBIP database with gai_error checking;
skips gracefully when no MMDB file is available

Testing

Standalone MMDB test program verified field paths against DBIP
Country Lite and ASN Lite databases on eris (ASAN build)
Korean IP (1.201.194.27): COUNTRY: KR
Google DNS (8.8.8.8): COUNTRY: US
ASN db (1.201.194.27): ASN-NAME: KINX, ASN: 9286
Loopback (127.0.0.1): correctly returns (unknown) / -1
mmdblookup confirms paths match database structure

Fixes: #11812

The MaxMind geo lookup code used incorrect field paths that don't match the structure of standard GeoLite2 and DBIP mmdb databases. Country lookups used a flat "country_code" field that doesn't exist; the correct path is "country" -> "names" -> "en" for country name and "country" -> "iso_code" for the ISO code. Changes: - Fix GEO_QUAL_COUNTRY to use nested path "country/names/en" - Add missing GEO_QUAL_COUNTRY_ISO support using "country/iso_code" - Add has_data and type checks before accessing entry data - Fix memory leak: set gMaxMindDB to nullptr after delete on open failure - Ensure entry_data_list is freed on all code paths Verified with DBIP Country Lite and ASN Lite databases using the same MMDB_get_value paths. Matches the field paths used by maxmind_acl plugin. Fixes: apache#11812

bryancall · 2026-03-06T23:33:04Z

[approve ci autest 1]

Copilot

Pull request overview

This PR updates the header_rewrite plugin’s MaxMind (MMDB) geo lookup implementation to better match the field layout used by standard GeoLite2 / DBIP databases, and improves cleanup/error-handling around lookups and initialization.

Changes:

Adjust MMDB lookup paths for geo string fields and add handling for GEO_QUAL_COUNTRY_ISO in the string lookup function.
Improve robustness by validating entry_data before reading values and ensuring MMDB entry data lists are freed on more return paths.
Fix a retry reliability issue by nulling gMaxMindDB after delete on MMDB_open() failure.

You can also share your feedback on Copilot code review. Take the survey.

plugins/header_rewrite/conditions_geo_maxmind.cc

- GEO_QUAL_COUNTRY now returns iso_code ("KR") instead of English name ("South Korea"), matching the GeoIP backend behavior - Remove dead GEO_QUAL_COUNTRY_ISO case from get_geo_string() since the dispatcher routes it to get_geo_int() - Remove unnecessary MMDB_get_entry_data_list() allocation -- the MMDB_get_value() API works directly from result.entry

Adds a test that verifies the MMDB lookup paths used by the geo conditions (country/iso_code, country/names/en) work correctly against a real MaxMind or DBIP database. The test skips gracefully if no MMDB file is available, and can be pointed at a specific file via MMDB_COUNTRY_PATH environment variable.

plugins/header_rewrite/header_rewrite_test.cc

zwoop

Looks good. I made a comment, but it's no big deal.

Check gai_error from MMDB_lookup_string() in the unit test instead of silently ignoring it (per zwoop review comment). Add comments explaining the nested MMDB field paths, nullptr-after-delete fix, data type validation guards, and the dual error code API.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

You can also share your feedback on Copilot code review. Take the survey.

plugins/header_rewrite/header_rewrite_test.cc

plugins/header_rewrite/conditions_geo_maxmind.cc

bryancall · 2026-03-11T19:07:29Z

[approve ci autest 0]

Some vendor MMDB databases use flat top-level fields (country_code), while standard GeoLite2/GeoIP2/DBIP databases use nested paths (country/iso_code). Detect the schema on the first successful lookup by probing for the nested path first, then falling back to flat. Cache the result in an atomic for the process lifetime. Add a Python script to generate test MMDB files with both schemas, and rewrite the unit test to verify lookups work against both.

Probe the database at load time with well-known IPs (8.8.8.8, 1.1.1.1, 128.0.0.1) instead of detecting lazily on first lookup. Removes the need for std::atomic and simplifies the hot path.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

You can also share your feedback on Copilot code review. Take the survey.

plugins/header_rewrite/conditions_geo_maxmind.cc

plugins/header_rewrite/CMakeLists.txt

plugins/header_rewrite/generate_test_mmdb.py

plugins/header_rewrite/header_rewrite_test.cc

plugins/header_rewrite/generate_test_mmdb.py

- Check for mmdb-writer/netaddr Python modules at CMake configure time before adding the MMDB generation target (prevents Ninja failures when declared outputs are not produced) - Exit non-zero from generate_test_mmdb.py on missing dependencies - Fix skip message to mention both required packages (mmdb-writer netaddr) - Add missing <cstdlib> include for getenv() - Remove unused <arpa/inet.h> and <netdb.h> includes from test file

bryancall · 2026-03-12T23:46:01Z

[approve ci autest 2]

bryancall added this to the 11.0.0 milestone Mar 6, 2026

bryancall self-assigned this Mar 6, 2026

bryancall added Bug header_rewrite header_rewrite plugin labels Mar 6, 2026

bryancall requested a review from Copilot March 6, 2026 23:33

Copilot started reviewing on behalf of bryancall March 6, 2026 23:34 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

bryancall changed the title ~~Fix header_rewrite MaxMind geo lookups for standard mmdb databases~~ Fix header_rewrite MaxMind geo lookups for GeoIP2/GeoLite2 mmdb databases Mar 6, 2026

bryancall mentioned this pull request Mar 6, 2026

ATS 10.0.0, header_rewrite plugin does not work with geoip file (maxmind mmdb) #11812

Closed

zwoop self-requested a review March 9, 2026 18:51

zwoop reviewed Mar 9, 2026

View reviewed changes

plugins/header_rewrite/header_rewrite_test.cc Outdated Show resolved Hide resolved

zwoop previously approved these changes Mar 9, 2026

View reviewed changes

bryancall requested a review from cmcfarlen March 9, 2026 22:16

bryancall dismissed zwoop’s stale review via dc1ab5d March 11, 2026 18:52

bryancall requested a review from Copilot March 11, 2026 18:52

Copilot started reviewing on behalf of bryancall March 11, 2026 18:53 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Fix misleading comment: initLibrary runs under std::call_once, no retry

fedfd37

bryancall requested a review from zwoop March 11, 2026 19:56

zwoop previously approved these changes Mar 11, 2026

View reviewed changes

bryancall dismissed zwoop’s stale review via c24e446 March 12, 2026 18:44

Move schema detection from runtime to initLibrary

64344c8

Probe the database at load time with well-known IPs (8.8.8.8, 1.1.1.1, 128.0.0.1) instead of detecting lazily on first lookup. Removes the need for std::atomic and simplifies the hot path.

bryancall requested a review from zwoop March 12, 2026 22:26

bryancall requested a review from Copilot March 12, 2026 22:27

Copilot started reviewing on behalf of bryancall March 12, 2026 22:27 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Conversation

bryancall commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Testing

Uh oh!

bryancall commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zwoop left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryancall commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryancall commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bryancall commented Mar 6, 2026 •

edited

Loading