launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:drop-geoip-legacy into launchpad:master.

Commit message:
Drop GeoIP/GeoLite Legacy support

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/391491

Production has been using GeoIP2 data provided by MaxMind via IS for a while, so this was only for convenience on systems that don't have access to that data.  I considered packaging the GeoLite2 Country database in ppa:launchpad/ubuntu/ppa, but it comes with the licence condition that redistributors must keep it up to date, which is too onerous for a test package.  Instead, allow running with no GeoIP database at all, and set the testrunner configuration to use a test database derived only from the IP addresses used in the Launchpad test suite and public WHOIS data.

The build script for the test database is in Perl, since I couldn't find a Python writer library for the MaxMind database format; but I've also committed the resulting database, since it's tiny.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:drop-geoip-legacy into launchpad:master.

diff --git a/configs/testrunner/launchpad-lazr.conf b/configs/testrunner/launchpad-lazr.conf
index e6f9994..e5a0ad8 100644
--- a/configs/testrunner/launchpad-lazr.conf
+++ b/configs/testrunner/launchpad-lazr.conf
@@ -103,6 +103,7 @@ max_scaling: 2
 [launchpad]
 basic_auth_password: test
 max_attachment_size: 1024
+geoip_database: lib/lp/services/geoip/tests/data/test.mmdb
 logparser_max_parsed_lines: 100000
 homepage_recent_posts_feed: http://launchpad.test:8093/blog-feed
 openid_canonical_root: http://testopenid.test/
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index bb8afcd..c16270a 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -1072,8 +1072,9 @@ max_bug_feed_cache_minutes: 60
 # datatype: integer
 code_homepage_product_cloud_size: 120
 
-# The database used by our GeoIP library.
-geoip_database: /usr/share/GeoIP/GeoIP.dat
+# The database used by our GeoIP library.  Only GeoIP2/GeoLite2 Country
+# databases are supported.
+geoip_database: none
 
 # The maximum number of lines that should be parsed by the launchpad
 # log parser. The default value of None means there is no maximum.
diff --git a/lib/lp/services/geoip/model.py b/lib/lp/services/geoip/model.py
index 7987324..0ba0489 100644
--- a/lib/lp/services/geoip/model.py
+++ b/lib/lp/services/geoip/model.py
@@ -9,7 +9,6 @@ __all__ = [
 
 import os
 
-import GeoIP as libGeoIP
 from geoip2.database import Reader
 from geoip2.errors import AddressNotFoundError
 from zope.component import getUtility
@@ -31,38 +30,34 @@ from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.services.worlddata.interfaces.language import ILanguageSet
 
 
+class NonexistentGeoIPDatabase(Exception):
+    """Configured GeoIP database does not exist."""
+
+
 @implementer(IGeoIP)
 class GeoIP:
     """See `IGeoIP`."""
 
     @cachedproperty
     def _gi(self):
+        if config.launchpad.geoip_database is None:
+            return None
         if not os.path.exists(config.launchpad.geoip_database):
-            raise NoGeoIPDatabaseFound(
-                "No GeoIP DB found. Please install launchpad-dependencies.")
-        if config.launchpad.geoip_database.endswith('.mmdb'):
-            return Reader(config.launchpad.geoip_database)
-        else:
-            return libGeoIP.open(
-                config.launchpad.geoip_database, libGeoIP.GEOIP_MEMORY_CACHE)
+            raise NonexistentGeoIPDatabase(
+                "The configured GeoIP DB (%s) does not exist." %
+                config.launchpad.geoip_database)
+        return Reader(config.launchpad.geoip_database)
 
     def getCountryCodeByAddr(self, ip_address):
         """See `IGeoIP`."""
         if not ipaddress_is_global(ip_address):
             return None
-        if isinstance(self._gi, Reader):
-            try:
-                return self._gi.country(ip_address).country.iso_code
-            except AddressNotFoundError:
-                return None
-        else:
-            try:
-                return self._gi.country_code_by_addr(ip_address)
-            except SystemError:
-                # libGeoIP may raise a SystemError if it doesn't find a
-                # record for some IP addresses (e.g. 255.255.255.255), so we
-                # need to catch that and return None here.
-                return None
+        if self._gi is None:
+            return None
+        try:
+            return self._gi.country(ip_address).country.iso_code
+        except AddressNotFoundError:
+            return None
 
     def getCountryByAddr(self, ip_address):
         """See `IGeoIP`."""
@@ -141,7 +136,3 @@ class RequestPreferredLanguages(object):
 
         languages = [language for language in languages if language.visible]
         return sorted(languages, key=lambda x: x.englishname)
-
-
-class NoGeoIPDatabaseFound(Exception):
-    """No GeoIP database was found."""
diff --git a/lib/lp/services/geoip/tests/data/make-db.pl b/lib/lp/services/geoip/tests/data/make-db.pl
new file mode 100755
index 0000000..74f01f1
--- /dev/null
+++ b/lib/lp/services/geoip/tests/data/make-db.pl
@@ -0,0 +1,34 @@
+#! /usr/bin/perl -w
+
+# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# Create a tiny test database in the style of GeoLite2-Country, for use by
+# the Launchpad test suite.  Requires libmaxmind-db-writer-perl.
+
+use MaxMind::DB::Writer::Tree;
+
+my %types = (
+    country => 'map',
+    iso_code => 'utf8_string',
+);
+
+my $tree = MaxMind::DB::Writer::Tree->new(
+    ip_version => 4,
+    record_size => 24,
+    database_type => 'Launchpad-Test-Country',
+    languages => ['en'],
+    description => { en => 'Launchpad test data' },
+    map_key_type_callback => sub { $types{$_[0]} },
+);
+
+$tree->insert_network('69.232.0.0/15', { country => { iso_code => 'US' } });
+$tree->insert_network('82.211.80.0/20', { country => { iso_code => 'GB' } });
+$tree->insert_network('121.44.0.0/15', { country => { iso_code => 'AU' } });
+$tree->insert_network('157.92.0.0/16', { country => { iso_code => 'AR' } });
+$tree->insert_network('201.13.0.0/16', { country => { iso_code => 'BR' } });
+$tree->insert_network('202.214.0.0/16', { country => { iso_code => 'JP' } });
+
+open my $fh, '>:raw', 'test.mmdb'
+    or die "Can't open test.mmdb for writing: $!";
+$tree->write_tree($fh);
diff --git a/lib/lp/services/geoip/tests/data/test.mmdb b/lib/lp/services/geoip/tests/data/test.mmdb
new file mode 100644
index 0000000..ddeaded
Binary files /dev/null and b/lib/lp/services/geoip/tests/data/test.mmdb differ
diff --git a/system-packages.txt b/system-packages.txt
index 0ae5029..6812444 100644
--- a/system-packages.txt
+++ b/system-packages.txt
@@ -22,9 +22,6 @@ apt_pkg
 # utilities/js-deps
 convoy
 
-# lp.services.geoip.model
-GeoIP
-
 # lp.testing.html5browser
 [optional] gi