Skip to content
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

(Fork) Use new Maxmind download URLs and Basic authentication scheme #48

Closed
wants to merge 11 commits into from

Conversation

sgarner
Copy link
Collaborator

@sgarner sgarner commented Mar 17, 2024

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:

  1. Reading the Maxmind Account ID from env var MAXMIND_ACCOUNT_ID or from the config as account-id
  2. Sending the auth header
  3. Following redirects when downloading

runk
runk previously approved these changes Mar 17, 2024
@runk
Copy link
Owner

runk commented Mar 17, 2024

Thanks for the contribution! Can you please confirm you've tested it locally?

@runk
Copy link
Owner

runk commented Mar 17, 2024

Attempted to run the PR checks, however had to cancel https://github.com/runk/node-geolite2/actions/runs/8318151169/job/22760767237?pr=48 after 45 odd minitues of hanging. Any ideas?

MAXMIND_ACCOUNT_ID wasn't setup


Update:

Looks like adding account_id didn't make much difference - npm i hangs of the self-install

@oschwald
Copy link
Collaborator

The old paths will continue to work with the same parameters. The change is that we will now be redirecting to a presigned R2 URL rather than serving the file directly. If the client is set to follow redirects and the R2 host is not blocked by a firewall rule or proxy, you should be good to go.

@sgarner
Copy link
Collaborator Author

sgarner commented Mar 17, 2024

Thanks for the contribution! Can you please confirm you've tested it locally?

I've been testing locally by running the script directly, and using npm link.

I'm not sure why the script was hanging after successfully downloading. I've added an explicit process.exit() and now it seems to complete okay.

@sgarner
Copy link
Collaborator Author

sgarner commented Mar 17, 2024

The old paths will continue to work with the same parameters. The change is that we will now be redirecting to a presigned R2 URL rather than serving the file directly. If the client is set to follow redirects and the R2 host is not blocked by a firewall rule or proxy, you should be good to go.

Yeah, so following redirects was the critical missing piece.

I guess if we want to avoid a breaking change to require the account ID, we could continue using the old URL without basic auth? But those old URLs don't appear to be supported, from Maxmind's documentation at least, so this might not work long term.

@sgarner
Copy link
Collaborator Author

sgarner commented Mar 18, 2024

I've now made the account ID optional. If not provided, we will continue to use the old URLs without auth, but following redirects. The new URLs will only be used if an account ID is found.

Can you try re-running the tests? I think they're now failing only because the license key secret isn't present when a run is triggered by my push.

@runk
Copy link
Owner

runk commented Mar 18, 2024

Looks like tests fail. I've invited you as a collaborator - you should be able to run tests whenever you like

@sgarner
Copy link
Collaborator Author

sgarner commented Mar 18, 2024

The actions still aren't getting access to the secret. It's probably because this PR is off a fork, can you try hitting the "Re-run jobs" button and see if it works when you do it as owner? Otherwise the policy on this repository may have to change. Or we can re-create the PR on a non-fork branch.

@runk
Copy link
Owner

runk commented Mar 18, 2024 via email

@sgarner
Copy link
Collaborator Author

sgarner commented Mar 18, 2024

Okay, pushed to a non-fork branch and tests are (almost) working in #52, let's continue discussion there.

@sgarner sgarner closed this Mar 18, 2024
@sgarner sgarner changed the title Use new Maxmind download URLs and Basic authentication scheme (Fork) Use new Maxmind download URLs and Basic authentication scheme Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants