bryancall commented on code in PR #12948:
URL: https://github.com/apache/trafficserver/pull/12948#discussion_r2920498648
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -51,6 +51,7 @@ MMConditionGeo::initLibrary(const std::string &path)
if (MMDB_SUCCESS != status) {
Dbg(pi_dbg_ctl, "Cannot open %s - %s", path.c_str(),
MMDB_strerror(status));
delete gMaxMindDB;
+ gMaxMindDB = nullptr; // allow retry on next call instead of dangling
pointer
Review Comment:
Fixed in fedfd374 — updated the comment to say "avoid leaving a dangling
global pointer after delete" instead.
##########
plugins/header_rewrite/header_rewrite_test.cc:
##########
@@ -538,12 +545,132 @@ test_tokenizer()
return errors;
}
+#if TS_USE_HRW_MAXMINDDB
+static bool
+file_exists(const char *path)
+{
+ struct stat st;
+ return stat(path, &st) == 0;
Review Comment:
This is test-only code that skips gracefully if no MMDB is found. A
directory named GeoLite2-Country.mmdb at one of these paths would be unusual
enough that I don't think the extra check is warranted.
##########
plugins/header_rewrite/header_rewrite_test.cc:
##########
@@ -538,12 +545,132 @@ test_tokenizer()
return errors;
}
+#if TS_USE_HRW_MAXMINDDB
+static bool
+file_exists(const char *path)
+{
+ struct stat st;
+ return stat(path, &st) == 0;
+}
+
+static const char *
+find_country_mmdb()
+{
+ static const char *paths[] = {
+ "/usr/share/GeoIP/GeoLite2-Country.mmdb",
"/usr/local/share/GeoIP/GeoLite2-Country.mmdb",
+ "/var/lib/GeoIP/GeoLite2-Country.mmdb",
"/opt/geoip/GeoLite2-Country.mmdb",
+ "/usr/share/GeoIP/GeoIP2-Country.mmdb",
"/usr/share/GeoIP/dbip-country-lite.mmdb",
+ };
+ for (auto *p : paths) {
+ if (file_exists(p)) {
+ return p;
+ }
+ }
+ if (const char *env = getenv("MMDB_COUNTRY_PATH")) {
+ if (file_exists(env)) {
+ return env;
+ }
+ }
+ return nullptr;
+}
+
+int
+test_maxmind_geo()
+{
+ const char *db_path = find_country_mmdb();
+ if (db_path == nullptr) {
+ std::cout << "SKIP: No MaxMind country mmdb found (set MMDB_COUNTRY_PATH
to override)" << std::endl;
+ return 0;
Review Comment:
Fair point on the naming, but this is a test helper that skips when no MMDB
exists. The env var is a fallback for unusual install paths, not a strict
override. Keeping it simple.
##########
plugins/header_rewrite/header_rewrite_test.cc:
##########
@@ -538,12 +545,132 @@ test_tokenizer()
return errors;
}
+#if TS_USE_HRW_MAXMINDDB
+static bool
+file_exists(const char *path)
+{
+ struct stat st;
+ return stat(path, &st) == 0;
+}
+
+static const char *
+find_country_mmdb()
+{
+ static const char *paths[] = {
+ "/usr/share/GeoIP/GeoLite2-Country.mmdb",
"/usr/local/share/GeoIP/GeoLite2-Country.mmdb",
+ "/var/lib/GeoIP/GeoLite2-Country.mmdb",
"/opt/geoip/GeoLite2-Country.mmdb",
+ "/usr/share/GeoIP/GeoIP2-Country.mmdb",
"/usr/share/GeoIP/dbip-country-lite.mmdb",
+ };
+ for (auto *p : paths) {
+ if (file_exists(p)) {
+ return p;
+ }
+ }
+ if (const char *env = getenv("MMDB_COUNTRY_PATH")) {
+ if (file_exists(env)) {
+ return env;
+ }
+ }
+ return nullptr;
+}
+
+int
+test_maxmind_geo()
+{
+ const char *db_path = find_country_mmdb();
+ if (db_path == nullptr) {
+ std::cout << "SKIP: No MaxMind country mmdb found (set MMDB_COUNTRY_PATH
to override)" << std::endl;
+ return 0;
+ }
+
+ std::cout << "Testing MaxMind geo lookups with: " << db_path << std::endl;
+
+ int errors = 0;
+ MMDB_s mmdb;
+ int status = MMDB_open(db_path, MMDB_MODE_MMAP, &mmdb);
+ if (MMDB_SUCCESS != status) {
+ std::cerr << "Cannot open " << db_path << ": " << MMDB_strerror(status) <<
std::endl;
+ return 1;
+ }
+
+ // MMDB_lookup_string() returns two independent error codes:
+ // gai_error - getaddrinfo() failure (string-to-IP conversion)
+ // mmdb_error - MMDB lookup failure (database query)
+ // Check both to avoid masking failures.
+ int gai_error, mmdb_error;
+ MMDB_lookup_result_s result = MMDB_lookup_string(&mmdb, "8.8.8.8",
&gai_error, &mmdb_error);
+
+ if (gai_error != 0) {
+ std::cerr << "getaddrinfo failed for 8.8.8.8: " << gai_strerror(gai_error)
<< std::endl;
+ MMDB_close(&mmdb);
+ return 1;
+ }
+ if (MMDB_SUCCESS != mmdb_error || !result.found_entry) {
+ std::cerr << "Cannot look up 8.8.8.8 in " << db_path << ": " <<
MMDB_strerror(mmdb_error) << std::endl;
+ MMDB_close(&mmdb);
+ return 1;
+ }
+
+ MMDB_entry_data_s entry_data;
+
+ // Verify "country" -> "iso_code" path (used by GEO_QUAL_COUNTRY)
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
+ if (MMDB_SUCCESS != status || !entry_data.has_data || entry_data.type !=
MMDB_DATA_TYPE_UTF8_STRING) {
+ std::cerr << "FAIL: country/iso_code lookup failed for 8.8.8.8" <<
std::endl;
+ ++errors;
+ } else {
+ std::string iso(entry_data.utf8_string, entry_data.data_size);
+ if (iso != "US") {
+ std::cerr << "FAIL: expected country iso_code 'US' for 8.8.8.8, got '"
<< iso << "'" << std::endl;
+ ++errors;
+ } else {
Review Comment:
The test only runs when an MMDB file is explicitly installed — it skips on
CI where none exists. Google DNS (8.8.8.8) resolving to US is stable across
every major MMDB vendor. If someone installs a custom database where that's not
true, the test failure would be a useful signal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
