-
Notifications
You must be signed in to change notification settings - Fork 18
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
Use new Maxmind download URLs and Basic authentication scheme #52
Conversation
I just got this from maxmind.com:
@oschwald is it something that is set in stone or can be tweaked on your side? |
I re-ran the failed tests and they are passing now. |
@runk, is this for the GitHub Action? We have been ratcheting down our free GeoLite download limit due to abuse. The final limit will be 30 downloads per 24-hour period. Do you have an estimate on the maximum number of downloads per day that you might need? |
The test suite runs against 3 versions of Node, each of which downloads 3 database editions, so that's 9 This project itself is not very busy so could go weeks or months without a push, until there is a PR like this one with changes to discuss. But that limit might be problematic for users of this project who are relying on the module to install GeoLite databases in their own build/deploy pipelines. |
I agree that the limit is unfortunate, but it was necessary given the traffic we were seeing. For instance, there were a significant number of misconfigured websites that appeared to be downloading a database on every page load, each downloading hundreds of gigabytes per day. You are correct that HEAD requests do not count towards the download limit. If this project in particular needs an increased limit for the test suite, I think that can be arranged. I would need the account ID associated with it. You can email me at goschwald@maxmind.com. |
@oschwald emailed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! .. and thank you for going through messages and fixing many of them.
It's up to you whether you want to switch to fetch. You can ignore it, do it in this PR or in a follow up PR - totally up to you.
@@ -6,14 +6,16 @@ Maxmind's GeoLite2 Free Databases download helper. | |||
|
|||
### Access Key | |||
|
|||
**IMPORTANT** You must setup `MAXMIND_LICENSE_KEY` environment variable be able to download databases. To do so, go to the https://www.maxmind.com/en/geolite2/signup, create a free account and generate new license key. | |||
**IMPORTANT** You must set up `MAXMIND_ACCOUNT_ID` and `MAXMIND_LICENSE_KEY` environment variables to be able to download databases. To do so, go to the https://www.maxmind.com/en/geolite2/signup, create a free account and generate new license key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's not a must for account id, but happy to keep this wording
scripts/postinstall.js
Outdated
}); | ||
|
||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should switch over to fetch
with nicer promise-based API and simple { redirect: 'follow' }
way of handling redirects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did actually switch to fetch
at one stage just to get it working, haha. But I thought this would be exceeding the scope of what was necessary here.
I'm happy to make another PR for that change
The issue is occurring intermittently in my CI/CD pipeline but not on my local machine. Is this PR related to the same, and why is it not appearing on my local machine?" Earlier it's was not occurring on CI/CD as well.
|
@kundan2403 That was the same error I began seeing a few days ago, which this PR resolves. The error is caused by the postinstall script trying to gunzip a non-successful response body, such as a server error or redirection.
Your local machine may already have the latest database files downloaded, in which case it won't download them again. You could remove the files from |
@sgarner I've attempted the same, but with no luck replicating it locally. |
Use fetch instead of node:https to download files
} | ||
}; | ||
|
||
main() | ||
.then(() => { | ||
// success | ||
process.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it dangling without explicit exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I couldn't figure out why. (Maybe a stream is left open?) Explicit exit fixed it though
Going to cross-reference another report runk/node-maxmind#826 @sgarner where things are at with this PR? |
I'm done with it, leaving it to you to merge when you're ready |
🎉 This PR is included in version 3.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I recently started to see errors indicating
node-geolite2
was unable to download the database files from Maxmind.It appears Maxmind has made some changes in the last few months:
https://dev.maxmind.com/geoip/updating-databases?lang=en#directly-downloading-databases
The permalink URLs to download are different, they now redirect to a presigned URL on
r2.cloudflarestorage.com
, and the initial permalink request requires Basic authentication using the Account ID as username and license key as password, instead of passing the license key as a query parameter.So this PR adds support for:
MAXMIND_ACCOUNT_ID
or from the config asaccount-id