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.
| // PureVPNCountryCodes filters PureVPN servers by deterministic | ||
| // 2-letter country code parsed from the hostname prefix. | ||
| PureVPNCountryCodes []string `json:"purevpn_country_codes"` |
There was a problem hiding this comment.
Use the Countries field and do a lookup country->country-code to maintain retro-compatibility and simplify code.
There was a problem hiding this comment.
Addressed by removing the PureVPNCountryCodes feature from this PR. PureVPN now uses the existing selection fields (including Countries) for compatibility; the extra country-code filter knobs are no longer part of this PR.
| // 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.
| ss.Countries = r.CSV("SERVER_COUNTRIES", reader.RetroKeys(countriesRetroKeys...)) | ||
|
|
||
| ss.Regions = r.CSV("SERVER_REGIONS", reader.RetroKeys("REGION")) | ||
| ss.Regions = nil |
There was a problem hiding this comment.
Addressed. I reverted ss.Regions = nil in read() so SERVER_REGIONS is still parsed normally. PureVPN-specific behavior is now handled later in validation (regions are ignored only when provider is PureVPN).
internal/openvpn/logs_test.go
Outdated
| 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 | ||
| 2. The VPN server crashed 💥, try changing your VPN servers filtering options such as SERVER_CITIES |
There was a problem hiding this comment.
Addressed. Reverted this test change so it matches the restored SERVER_REGIONS log message.
|
|
||
| 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.
|
[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? |
Description
PureVPN provider revamp:
Issue
Assertions