New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Geo location support for IPv6 addresses #6690
Conversation
Hi @joostdekeijzer Thank you for following up and testing on 5.3 and 5.6 with and without PECL. I will merge the PR shortly and hopefully it will work now. We have a couple weeks left until the stable release so hopefully enough time to catch any issue 👍 |
Another try to implement IPv6 support
Great! Let me know if there are any issues so I can try to fix those. |
Any documentation on how to use ipv6 maxmind DB? |
I assume you want to use the "new" maxmind databases (.mmdb files) ? The current implementation uses the legacy Maxmind code and only supports the legacy database (.dat) files. The currently used datafile (GeoLiteCity.dat) includes IPv6 information. |
Thank you for clarification @joostdekeijzer |
Hi, I tried this functionality today, but as far as I can tell, I did a simple test using the script at: https://gist.github.com/olavmrk/1cc21f890ffc97998b84 That script gives the following output:
The correct country is Norway, so we can see that using What appears to work is looking up addresses in GeoLiteCityv6 using IPv6 addresses and IPv6-mapped IPv4 addresses. |
Hi, When I test your IP addresses using the Piwik API, But that's not to say that the database is good. I find many IPv6 addresses to be reported in China when I'm quite sure they shouldn't. Eg. a reported IPv6 for google.com using |
Hi, I believe Piwik transforms a IPv6-mapped IPv4 address into a IPv4 address before passing it to the GeoIP module. I only included the lines with the IPv6-mapped IPv4 address since it indicates a possible workaround. To clarify: My problem isn't IPv4 addresses. My problem is that the The For example, looking up Another IPv6-address belonging in Norway is |
Hi, I had an idea about the cause of the strange behavior of IPv6-addresses: It appears that when you try to look up an IPv6 address in E.g.: |
Hi Olav, That's good research! I think I've found the base of the issue in the code. Based on your findings, I guess there will also be the need to periodicly update the |
I'm seeing three options for fixing this:
I guess option 2 is the one requiring the least amount of code changes, but some tricks would probably have to be employed in order to be backwards-compatible with existing installations. I thing it should be possible to look at the database type and switch the method for looking up IPv4 addresses depending on whether it is the Option 3 looks like a better long term solution, but I don't know how tied Piwik is to the existing GeoIp location provider. |
Hi, Loading and maintaining both GeoLiteCity and GeoLiteCityv6 is quite possible, see the modifications here: https://github.com/piwik/piwik/compare/master...joostdekeijzer:geoip-legacy-IPv6-datafiles?expand=1 I'm afraid the code did not get any prettier... Moving to GeoIP2 would be nice but is a lot of work, also because the output is different from GeoIP1(different spelling of names, etc.). I think it needs some review and I haven't run any tests yet. @mattab should this be in a separate ticket or shall I just create a PR with above changes and continue the discussion there? |
To me that looks like a lot of changes -- I think option 2 would have been prettier :) Looking at the code -- isn't the same changes required for the other database types (i.e. isp and org)? Or do they have a different structure combining IPv4 and IPv6 addresses? Another thing I see is that you dropped in |
I did not try option 2 because I was afraid of side-effects with backward-compatibility and the existing visting archives. I just tried your IPv6 address (the one in the testcase https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Fixtures/ManyVisitsWithGeoIP.php#L37 doesn't actually have any GeoIP info). Maybe use the Google IPv6 address instead? |
Just tested: the ISP and ORG databases handle IPv6 addresses the same as GeoLiteCity.dat. Entering the Google IPv6 addres will return:
So this will need to be handled... |
Yes, backwards compatibility can be a challenge. I think it would be best solved by changing the defaults so that new installations would end up using Regarding the IPv6 address that should be tested -- I just don't think the address of a random subscriber on a random ISP is the best approach. |
I'd like some lead developer feedback on this. About your backward compatibility solution: that should indeed work but my feeling is it should be done when migrating to GeoIP2 (I've done some tests with GeoIP2 and the output is really different) Also agree on your IPv6 test address comment. Maybe Piwik has an address themselves? |
GeoIP2 ticket: #4487 |
Hi there,
for sure I would like to help here, but this issue is closed. In general it is helpful for us if we discuss in issues that are still opened, so that we will not forget about them. Would you mind creating an issue to describe the specific problem as above? if there is more than one problem, it is best to create more than one issue 👍 hope it works... |
Hi,
PR #6574 had issues when the GeoIP PECL extension was installed (thus the PR was reverted in #6647).
This is now fixed.
Tested on php53 and php56 with and without the GeoIP PECL extension enabled.
Manually tested with ISP and ORG GeoIP databases.
@tsteur @mattab are there any other test-vector/option to try?