From 42c5a5da3166830c80085bb43ef5d8e49467d344 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Fri, 22 May 2026 00:29:35 +0000 Subject: [PATCH] PR feedback --- README.md | 5 +-- internal/provider/ovpn/openvpnconf.go | 3 -- internal/provider/ovpn/updater/api.go | 60 ++++++--------------------- internal/storage/copy.go | 16 ++----- internal/storage/copy_test.go | 44 +++----------------- internal/storage/filter.go | 6 ++- internal/storage/hardcoded.go | 2 +- 7 files changed, 30 insertions(+), 106 deletions(-) diff --git a/README.md b/README.md index 78945f36..36cee842 100644 --- a/README.md +++ b/README.md @@ -65,9 +65,8 @@ Lightweight swiss-army-knife-like VPN client to multiple VPN service providers - Supports Wireguard both kernelspace and userspace - For **AirVPN**, **FastestVPN**, **Ivpn**, **Mullvad**, **NordVPN**, **Ovpn**, **Perfect privacy**, **ProtonVPN**, **Surfshark** and **Windscribe** - For **Cyberghost**, **Private Internet Access**, **PrivateVPN**, **PureVPN**, **Torguard**, **VPN Unlimited** and **VyprVPN** using [the custom provider](https://github.com/qdm12/gluetun-wiki/blob/main/setup/providers/custom.md) - -- For custom Wireguard configurations using [the custom provider](https://github.com/qdm12/gluetun-wiki/blob/main/setup/providers/custom.md) -- More in progress, see [#134](https://github.com/passteque/gluetun/issues/134) + - For custom Wireguard configurations using [the custom provider](https://github.com/qdm12/gluetun-wiki/blob/main/setup/providers/custom.md) + - More in progress, see [#134](https://github.com/passteque/gluetun/issues/134) - Supports AmneziaWG only with the custom provider for now - DNS over TLS baked in with service provider(s) of your choice - DNS fine blocking of malicious/ads/surveillance hostnames and IP addresses, with live update every 24 hours diff --git a/internal/provider/ovpn/openvpnconf.go b/internal/provider/ovpn/openvpnconf.go index e095e5ac..2ce92977 100644 --- a/internal/provider/ovpn/openvpnconf.go +++ b/internal/provider/ovpn/openvpnconf.go @@ -26,9 +26,6 @@ func (p *Provider) OpenVPNConfig(connection models.Connection, }, TLSAuth: "81782767e4d59c4464cc5d1896f1cf6015017d53ac62e2e3b94b889e00b2c69ddc01944fe1c6d895b4d80540502eb71910b8d785c9efa9e3182343532adffe1cfbb7bb6eae39c502da2748edf0fb89b8a20b0a1085cc1f06135037881bc0c4ad8f2c0f4f72d2ab466fb54af3d8264c5fddeb0f21aa0ca41863678f5fc4c44de4ca0926b36dfddc42c6f2fabd1694bdc8215b2d223b9c21dc6734c2c778093187afb8c33403b228b9af68b540c284f6d183bcc88bd41d47bd717996e499ce1cbbfa768a9723c19c58314c4d19cfed82e543ee92e73d38ad26d4fbec231c0f9f3b30773a5c87792e9bc7c34e8d7611002ebedd044e48a0f1f96527bfdcc940aa09", //nolint:lll KeyDirection: "1", - ExtraLines: []string{ - "replay-window 256", - }, } if strings.HasSuffix(connection.Hostname, "singapore.ovpn.com") { diff --git a/internal/provider/ovpn/updater/api.go b/internal/provider/ovpn/updater/api.go index d838d2e0..9a00287e 100644 --- a/internal/provider/ovpn/updater/api.go +++ b/internal/provider/ovpn/updater/api.go @@ -67,17 +67,11 @@ func fetchAPI(ctx context.Context, client *http.Client) ( return data, nil } -var ( - ErrCityNotSet = errors.New("city is not set") - ErrCountryNameNotSet = errors.New("country name is not set") - ErrServersNotSet = errors.New("servers array is not set") -) - func (a *apiDataCenter) validate() (err error) { conditionalErrors := []conditionalError{ - {err: ErrCityNotSet, condition: a.City == ""}, - {err: ErrCountryNameNotSet, condition: a.CountryName == ""}, - {err: ErrServersNotSet, condition: len(a.Servers) == 0}, + {err: "city is not set", condition: a.City == ""}, + {err: "country name is not set", condition: a.CountryName == ""}, + {err: "servers array is not set", condition: len(a.Servers) == 0}, } err = collectErrors(conditionalErrors) if err != nil { @@ -106,29 +100,19 @@ func (a *apiDataCenter) validate() (err error) { return nil } -var ( - ErrIPFieldNotValid = errors.New("ip address is not set") - ErrHostnameFieldNotSet = errors.New("hostname field is not set") - ErrPublicKeyFieldNotSet = errors.New("public key field is not set") - ErrWireguardPortsNotSet = errors.New("wireguard ports array is not set") - ErrWireguardPortNotDefault = errors.New("wireguard port is not the default 9929") - ErrMultiHopOpenVPNPortNotSet = errors.New("multihop OpenVPN port is not set") - ErrMultiHopWireguardPortNotSet = errors.New("multihop WireGuard port is not set") -) - func (a *apiServer) validate() (err error) { const defaultWireguardPort = 9929 conditionalErrors := []conditionalError{ - {err: ErrIPFieldNotValid, condition: !a.IP.IsValid()}, - {err: ErrHostnameFieldNotSet, condition: a.Ptr == ""}, - {err: ErrPublicKeyFieldNotSet, condition: a.PublicKey == ""}, - {err: ErrWireguardPortsNotSet, condition: len(a.WireguardPorts) == 0}, + {err: "ip address is not set", condition: !a.IP.IsValid()}, + {err: "hostname field is not set", condition: a.Ptr == ""}, + {err: "public key field is not set", condition: a.PublicKey == ""}, + {err: "wireguard ports array is not set", condition: len(a.WireguardPorts) == 0}, { - err: ErrWireguardPortNotDefault, + err: "wireguard port is not the default 9929", condition: len(a.WireguardPorts) != 1 || a.WireguardPorts[0] != defaultWireguardPort, }, - {err: ErrMultiHopOpenVPNPortNotSet, condition: a.MultiHopOpenvpnPort == 0}, - {err: ErrMultiHopWireguardPortNotSet, condition: a.MultiHopWireguardPort == 0}, + {err: "multihop OpenVPN port is not set", condition: a.MultiHopOpenvpnPort == 0}, + {err: "multihop WireGuard port is not set", condition: a.MultiHopWireguardPort == 0}, } err = collectErrors(conditionalErrors) switch { @@ -144,28 +128,12 @@ func (a *apiServer) validate() (err error) { } type conditionalError struct { - err error + err string condition bool } -type joinedError struct { - errs []error -} - -func (e *joinedError) Unwrap() []error { - return e.errs -} - -func (e *joinedError) Error() string { - errStrings := make([]string, len(e.errs)) - for i, err := range e.errs { - errStrings[i] = err.Error() - } - return strings.Join(errStrings, "; ") -} - func collectErrors(conditionalErrors []conditionalError) (err error) { - errs := make([]error, 0, len(conditionalErrors)) + errs := make([]string, 0, len(conditionalErrors)) for _, conditionalError := range conditionalErrors { if !conditionalError.condition { continue @@ -177,7 +145,5 @@ func collectErrors(conditionalErrors []conditionalError) (err error) { return nil } - return &joinedError{ - errs: errs, - } + return errors.New(strings.Join(errs, "; ")) } diff --git a/internal/storage/copy.go b/internal/storage/copy.go index 113478f9..3bc53ebd 100644 --- a/internal/storage/copy.go +++ b/internal/storage/copy.go @@ -1,23 +1,15 @@ package storage import ( - "net/netip" + "slices" "github.com/qdm12/gluetun/internal/models" ) func copyServer(server models.Server) (serverCopy models.Server) { serverCopy = server - serverCopy.IPs = copyIPs(server.IPs) + serverCopy.IPs = slices.Clone(server.IPs) + serverCopy.PortsTCP = slices.Clone(server.PortsTCP) + serverCopy.PortsUDP = slices.Clone(server.PortsUDP) return serverCopy } - -func copyIPs(toCopy []netip.Addr) (copied []netip.Addr) { - if toCopy == nil { - return nil - } - - copied = make([]netip.Addr, len(toCopy)) - copy(copied, toCopy) - return copied -} diff --git a/internal/storage/copy_test.go b/internal/storage/copy_test.go index bea568d2..ea16b53c 100644 --- a/internal/storage/copy_test.go +++ b/internal/storage/copy_test.go @@ -21,43 +21,9 @@ func Test_copyServer(t *testing.T) { assert.Equal(t, server, serverCopy) // Check for mutation serverCopy.IPs[0] = netip.AddrFrom4([4]byte{9, 9, 9, 9}) - assert.NotEqual(t, server, serverCopy) -} - -func Test_copyIPs(t *testing.T) { - t.Parallel() - - testCases := map[string]struct { - toCopy []netip.Addr - copied []netip.Addr - }{ - "nil": {}, - "empty": { - toCopy: []netip.Addr{}, - copied: []netip.Addr{}, - }, - "single IP": { - toCopy: []netip.Addr{netip.AddrFrom4([4]byte{1, 1, 1, 1})}, - copied: []netip.Addr{netip.AddrFrom4([4]byte{1, 1, 1, 1})}, - }, - "two IPs": { - toCopy: []netip.Addr{netip.AddrFrom4([4]byte{1, 1, 1, 1}), netip.AddrFrom4([4]byte{2, 2, 2, 2})}, - copied: []netip.Addr{netip.AddrFrom4([4]byte{1, 1, 1, 1}), netip.AddrFrom4([4]byte{2, 2, 2, 2})}, - }, - } - - for name, testCase := range testCases { - t.Run(name, func(t *testing.T) { - t.Parallel() - - copied := copyIPs(testCase.toCopy) - - assert.Equal(t, testCase.copied, copied) - - if len(copied) > 0 { - testCase.toCopy[0] = netip.AddrFrom4([4]byte{9, 9, 9, 9}) - assert.NotEqual(t, testCase.toCopy[0], testCase.copied[0]) - } - }) - } + serverCopy.PortsTCP = []uint16{80} + serverCopy.PortsUDP = []uint16{53} + assert.NotEqual(t, server.IPs, serverCopy.IPs) + assert.NotEqual(t, server.PortsTCP, serverCopy.PortsTCP) + assert.NotEqual(t, server.PortsUDP, serverCopy.PortsUDP) } diff --git a/internal/storage/filter.go b/internal/storage/filter.go index 4ff4d609..c56d0ece 100644 --- a/internal/storage/filter.go +++ b/internal/storage/filter.go @@ -123,7 +123,11 @@ func filterServer(server models.Server, return true } - if filterByPorts(selection, server.PortsTCP) { + serverPorts := server.PortsUDP + if server.VPN == vpn.OpenVPN && server.TCP { + serverPorts = server.PortsTCP + } + if filterByPorts(selection, serverPorts) { return true } diff --git a/internal/storage/hardcoded.go b/internal/storage/hardcoded.go index 89798bcf..5fd6f82c 100644 --- a/internal/storage/hardcoded.go +++ b/internal/storage/hardcoded.go @@ -33,7 +33,7 @@ func parseHardcodedServers() (allServers models.AllServers) { providerFile, err := serversmodule.Files.Open(filename) if err != nil { const rootURL = "https://github.com/qdm12/gluetun-servers/blob/main/pkg/servers" - panic(fmt.Sprintf("reading embedded provider file defined at %s/%s.json: %s", rootURL, filename, err)) + panic(fmt.Sprintf("reading embedded provider file defined at %s/%s: %s", rootURL, filename, err)) } defer providerFile.Close() // no-op