(In reply to comment #0)
> I would say IP::Country::Fast is somewhat obsolete. It hasn't been updated in
> nearly in two years and it doesn't have any easy way of updating it's database.
> 
> Any objections on using Geo::IP as default for 3.4? Probably should have used
> it since few years already. We can leave IP::Country::Fast as backup.
> 
> Are there any licensing issues? From http://www.maxmind.com/app/geoip_country:
> 
> "Under the license agreement, all advertising materials and documentation
> mentioning features or use of this database must display the following
> acknowledgment: "This product includes GeoLite data created by MaxMind,
> available from http://www.maxmind.com/."
> 
> Only other alternative I could find right now is http://www.wipmania.com/, but
> it doesn't seem to have a "standard" Perl module or efficient binary database
> to download for that matter. Using network lookups would be serious overkill
> imho.

if license permits, I'm +1 for the MaxMind path.

Maxmind still requires manual updating.  The procedure for updating IP::Country::Fast is fairly straightforward.  I don't see any real reason to change.

(In reply to comment #2)
> Maxmind still requires manual updating.  The procedure for updating
> IP::Country::Fast is fairly straightforward.  I don't see any real reason to
> change.

Yes Maxmind requires manual updates, it's no surprise. 99% of things like this needs manual updates, have you seen a Perl module or such that updates itself without intervention? But Maxmind provides more accurate and easily available files. It's much more likely that people have already installed geoip libraries than IP::Country::Fast, it's not even packaged for debian.

What part of IP::Country::Fast do you consider fairly straightforward?

- The build process that takes 1-2GB of memory and 15+ minutes on 3GHz AMD
- Downloading hundreds of MBs of ripe data for it
- Assuming that your newly created database isn't overwritten by something (it's in the Perl library path)

And it seems debian actually packages the databases as packages, so it's updated automatically.

(In reply to comment #4)
> And it seems debian actually packages the databases as packages, so it's
> updated automatically.

Um, Geoip that is..

(In reply to comment #5)
> (In reply to comment #4)
> > And it seems debian actually packages the databases as packages, so it's
> > updated automatically.
> 
> Um, Geoip that is..

FTR: Centos provides
GeoIP
GeoIP-data 
in it's "extras" YUM repo

Aside - Comment #3: "... have you seen a Perl module or such that updates itself
without intervention?"

Actually, yes.  The CPAN module itself can do that, if called from cron with:

  "perl -MCPAN -e update"

If it finds an update for itself, it will install it.  It can autoconfigure itself too (and it did when I upgraded to perl 5.14.0 which came out in the past week).

(In reply to comment #7)
> Aside - Comment #3: "... have you seen a Perl module or such that updates
> itself
> without intervention?"
> 
> Actually, yes.  The CPAN module itself can do that, if called from cron with:
> 
>   "perl -MCPAN -e update"
> 
> If it finds an update for itself, it will install it.  It can autoconfigure
> itself too (and it did when I upgraded to perl 5.14.0 which came out in the
> past week).

Well, let me clarify myself, if someone else is confused also..

Of course you can cron script xxx to fetch xxx or update xxx. That still requires user to actually "activate something".

IP::Country::Fast has no updates, other than if the maintainer decides to update the module every few years and expect people to actually upgrade the module also. Many people will not have the resources needed to run the database generation script manually.

Geoip has been "de facto" for years and is in many distributions with automatic package updates. It's also very simple to cron wget the database. Geoip also supports IPv6 when we need it.

If anyone has some legimate concerns, especially on if and where to put the licensing clauses etc, I'm open to hearing that. Other than that, I'll be probably posting proposed patchset when I have time.

I think I just scratched this itch as I noticed the RelayCountry plugin wasn't doing anything for IPv6 addresses.

I've put the plugin here: https://github.com/bodgit/spamassassin-relaycountry-geoip

I've named it RelayCountry2 so it can live alongside the current plugin but will update the same metadata so it's a case of enable one or the other.

I've replicated the behaviour of IP::Country in marking private addresses with "**" and I've added similar rules for IPv6 based on the equivalent RFC.

success in gentoo/funtoo now, it just need dev-perl/Geo-IP unstable to work

> Any objections on using Geo::IP as default for 3.4?
> Probably should have used it since few years already.

No objections from me.
The clause can go into the plugin's POD/manpage, I hope this suffices.


> We can leave IP::Country::Fast as backup.

Matt Dainty wrote:
> I think I just scratched this itch as I noticed the RelayCountry plugin
> wasn't doing anything for IPv6 addresses.
> I've put the plugin here:
>   https://github.com/bodgit/spamassassin-relaycountry-geoip
> I've named it RelayCountry2 so it can live alongside the current plugin but
> will update the same metadata so it's a case of enable one or the other.
> I've replicated the behaviour of IP::Country in marking private addresses
> with "**" and I've added similar rules for IPv6 based on the equivalent RFC.

I wouldn't like to see two variants of functionally equivalent plugins
and letting people scratch their heads, deciding which of them to use.
Either switch entirely to Geo::IP and ditch IP::Country::Fast, or
merge both into a single plugin and let it fallback to IP::Country::Fast
if Geo::IP is not installed.

> The clause can go into the plugin's POD/manpage, I hope this suffices.

(referring to the MaxMind license clause)

I completely agree on having two plugins, I just wrote a new one to keep it separate as personally I'll never run it on a host that doesn't have Geo::IP available. I also didn't want to mess with the SA package $OS provided.

Personally, IP::Country is no good to me going forward as it simply doesn't have IPv6 data so I'd rather just move to Geo::IP entirely, but I wouldn't mind if IP::Country was the fallback option. The data files then also get shared by other things using the GeoIP C library.

If it's useful and could potentially be accepted, I'm happy to whip up a patch to the current plugin that supports both backends, assuming any licensing issue is resolved.

Oh, I forgot to mention, I think you'll need at least Geo::IP 1.39 to get the IPv6 API methods, although I've been using 1.40, and that needs something like 1.4.7 of the C library.

Note:  Some of the IP->ASN databases return more than the two fields that the ASN module uses -- some return up to 5 fields, and a country field is often one of those.  Granted, we only check that for one of the servers in the path (last external or first untrusted), but maybe we should add code to the ASN module to identify the appropriate hop's country when this module fails to produce an answer....

Please review.

IPv6 is supported if Geo::IP 1.39+ is installed (which itself forces API 1.3.7, unless someone skipped cpan make test..).

Sending        INSTALL
Sending        META.yml
Sending        Makefile.PL
Sending        lib/Mail/SpamAssassin/Plugin/RelayCountry.pm
Sending        lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Sending        rules/init.pre
Transmitting file data ......
Committed revision 1308059.

PS. From what I understand, since we aren't actually distributing anything belonging to GeoIP, there is no need for any license clauses. Feel free to correct.

(In reply to comment #16)
> PS. From what I understand, since we aren't actually distributing anything
> belonging to GeoIP, there is no need for any license clauses. Feel free to
> correct.

I am +1 for using Geo::IP for 3.4.  The licensing looks fair with monthly updates free with more fine tuned updates available free.  From my perspective, it seems to be a fair free for some model that the project can support.

And agreed that our code should be fine license-wise.

Regards,
KAM

Henrik Krohns 2012-04-01 10:06:04 UTC 
> Please review. Committed revision 1308059.

+1

This is already folded-in, works fine.
Closing.