PureVPN provider revamp#3165
Conversation
…iner start - prevent leaks for connections made the first ~10 milliseconds when Gluetun starts - seems critical, but in practice this very rarely happen and it very hard to reproduce
|
This PR is more comprehensive but addresses much of the shortcomings of #3149 by finding a better avenue to get an updated server list and parsing accepted protocols, port numbers, and other attributes directly from the same source used by PureVPN's GUI clients. |
|
Test results summary for the PureVPN provider revamp branch. Branch / commit
1) Targeted validation on host (macOS)go build ./cmd/gluetun
go test ./internal/provider/purevpn/... ./internal/provider/purevpn/updater/...Result: ✅ pass 2) Targeted validation in Linux amd64 containerdocker run --rm --platform linux/amd64 \
-v "$PWD":/src -w /src golang:1.25-alpine3.22 \
sh -lc '/usr/local/go/bin/go build ./cmd/gluetun && /usr/local/go/bin/go test ./internal/provider/purevpn/... ./internal/provider/purevpn/updater/...'Result: ✅ pass 3) Full suite on host (informational)go test ./...Result:
PureVPN-related packages are green in all runs above. |
| 1. The VPN server IP address you are trying to connect to is no longer valid 🔌 | ||
| Check out https://github.com/qdm12/gluetun-wiki/blob/main/setup/servers.md#update-the-vpn-servers-list | ||
|
|
||
| 2. The VPN server crashed 💥, try changing your VPN servers filtering options such as SERVER_REGIONS |
There was a problem hiding this comment.
Addressed. I reverted this message change and restored SERVER_REGIONS in internal/openvpn/logs.go (and corresponding test).
| _ = ss | ||
| _ = warner |
There was a problem hiding this comment.
??????
Also we want to maintain retro-compatibility
There was a problem hiding this comment.
Addressed. I removed the placeholder _ = ss / _ = warner edits and restored the Surfshark retro-compatibility branch in getLocationFilterChoices.
| // PureVPNLocationCodes filters PureVPN servers by deterministic | ||
| // location code parsed from the hostname prefix (for example usca, ukm). | ||
| PureVPNLocationCodes []string `json:"purevpn_location_codes"` |
There was a problem hiding this comment.
sounds like we could use the Hostnames field then
There was a problem hiding this comment.
Addressed by removing PureVPNLocationCodes from this PR. Deterministic per-host selection remains available through the existing Hostnames field.
| return fmt.Errorf("%w: %w", ErrCountryNotValid, err) | ||
| } | ||
|
|
||
| err = atLeastOneIsOneOfCaseInsensitive(settings.Regions, filterChoices.Regions, warner) |
There was a problem hiding this comment.
regions is used for nordvpn. I get AI makes it easier to rewrite stuff, but please at the very least read changes made and verify a bit. Otherwise it adds excessive load for me to review these, especially with the increasing amount of AI generated PRs.
There was a problem hiding this comment.
Hey sorry I was a bit moody on the AI-generated PR; if you're not a Go developer, then I 100% appreciate the gesture, and I'll comment on what's to change and the AI can then adapt it. However if you're a Go dev, I would definitely appreciate you reading up on the changes first 😉 !
There was a problem hiding this comment.
No worries - I'm def not a Go dev - my specialty in infra bash/terraform/NodeJS. My understanding was that I stopped using the regions field for PureVPN, but it was still being used for any other provider. Currently doing another scan to doublecheck, but unfortunately I don't have another set of creds with another provider other than PureVPN to confirm.
There was a problem hiding this comment.
I'm trying to figure out a way to populate regions for PureVPN but I think I end up getting ratelimited by the IP geolocation APIs due to the sheer number of servers that need to be looked up.
There was a problem hiding this comment.
Addressed. Region validation remains for non-PureVPN providers (including NordVPN) and the broader region logic has been restored. For PureVPN specifically, I now ignore SERVER_REGIONS at validation time to avoid impacting PureVPN selection.
There was a problem hiding this comment.
Appreciate it. I went through each requested change line-by-line, restored the non-PureVPN region behavior, and split out the extra PureVPN filtering feature work from this PR.
|
|
||
| const url = "https://d11a57lttb2ffq.cloudfront.net/heartbleed/router/Recommended-CA2.zip" | ||
| contents, err := u.unzipper.FetchAndExtract(ctx, url) | ||
| debContent, err := fetchURL(ctx, http.DefaultClient, debURL) |
There was a problem hiding this comment.
use the u.client instead of http.DefaultClient. If not present, inject it into the Updater New function
There was a problem hiding this comment.
Addressed. I injected the HTTP client into the PureVPN updater (Updater now has client *http.Client), defaulting to http.DefaultClient only if nil. .deb URL/content and inventory fetches now use u.client.
There was a problem hiding this comment.
Do not default it, it is never nil.
|
[Generated by Codex] Summary of review feedback addressed:
Validation done locally:
|
|
[Generated by Codex]\n\nI split the selector work into a dedicated stacked PR based on :\n- https://github.com/reedog117/gluetun/pull/1\n\nThis contains the PureVPN selector functionality (, , , , country/location code selectors) separated from the base updater changes. |
|
[Generated by Codex] I split the selector work into a dedicated stacked PR based on This contains the PureVPN selector functionality ( |
This PR #3165 needs to be merged in before I can bring the selector functionality in as a separate PR as it's dependent on the new server inventory selection methods from here. |
2c06921 to
9a5995f
Compare
|
@qdm12 what additional steps are needed at this point to merge this and then i can bring in the newer selector functionality? |
qdm12
left a comment
There was a problem hiding this comment.
Thanks! More comments to address, I'll review one last time after that. It's a bit of a huge PR, so it takes time to review 😉
| if testCase.err == nil { | ||
| require.NoError(t, err) | ||
| return | ||
| } | ||
| require.Error(t, err) | ||
| assert.ErrorIs(t, err, testCase.err) |
There was a problem hiding this comment.
same thing as
| if testCase.err == nil { | |
| require.NoError(t, err) | |
| return | |
| } | |
| require.Error(t, err) | |
| assert.ErrorIs(t, err, testCase.err) | |
| assert.ErrorIs(t, err, testCase.err) |
| // Keep parsing SERVER_REGIONS for retro-compatibility, but | ||
| // do not apply it to PureVPN filtering. | ||
| ss.Regions = nil |
There was a problem hiding this comment.
Why not? It should still be applied somehow, otherwise it breaks compatibility.
| return nil, fmt.Errorf("decompressing %s: %w", dataTarName, err) | ||
| } | ||
|
|
||
| asarContent, err = extractFileFromTar(dataTar, pureVPNAsarPath) |
There was a problem hiding this comment.
inline the constant pureVPNAsarPath right above this line, there is no need to have at global scope if it's used only here.
| parsedCountry, parsedCity, warnings := parseHostname(servers[i].Hostname) | ||
| for _, warning := range warnings { | ||
| u.warner.Warn(warning) | ||
| if !needsGeolocationEnrichment(servers[i]) || len(servers[i].IPs) == 0 { |
There was a problem hiding this comment.
does this trigger often? Asking because ideally I would like to get rid of the ipFetcher entirely, purevpn is the only provider using it for its updater. Hopefully we can figure out enough data without it.
Awesome - will work on this in the next few days |
40f126b to
44d5104
Compare
|
I actually need this rather fast, this is blocking #3301 which I would like to finish soon (because purevpn is the only one using the public ip fetcher and this makes package imports shitty basically) |
|
FYI working on it now |
Description
PureVPN provider revamp:
Issue
Assertions