From 9f39d471505a2ef2f9b04b3620d996404f53ee3c Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Wed, 19 Nov 2025 16:00:20 +0000 Subject: [PATCH] feat(healthcheck): `HEALTH_ICMP_TARGET_IP` -> `HEALTH_ICMP_TARGET_IPS` - Specify fallback ICMP IP addresses - Defaults changed from 1.1.1.1 to 1.1.1.1,8.8.8.8 - Small periodic check cycles through addresses as it fails and moves to retry --- Dockerfile | 2 +- internal/configuration/settings/health.go | 49 ++++++++++++++----- .../configuration/settings/settings_test.go | 4 +- internal/healthcheck/checker.go | 28 ++++++----- internal/vpn/interfaces.go | 2 +- internal/vpn/tunnelup.go | 8 +-- 6 files changed, 60 insertions(+), 33 deletions(-) diff --git a/Dockerfile b/Dockerfile index 056322eb..d6e6c07c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -164,7 +164,7 @@ ENV VPN_SERVICE_PROVIDER=pia \ # Health HEALTH_SERVER_ADDRESS=127.0.0.1:9999 \ HEALTH_TARGET_ADDRESSES=cloudflare.com:443,github.com:443 \ - HEALTH_ICMP_TARGET_IP=1.1.1.1 \ + HEALTH_ICMP_TARGET_IPS=1.1.1.1,8.8.8.8 \ HEALTH_RESTART_VPN=on \ # DNS DNS_SERVER=on \ diff --git a/internal/configuration/settings/health.go b/internal/configuration/settings/health.go index e22adf61..e2ac010e 100644 --- a/internal/configuration/settings/health.go +++ b/internal/configuration/settings/health.go @@ -1,6 +1,7 @@ package settings import ( + "errors" "fmt" "net/netip" "os" @@ -22,22 +23,38 @@ type Health struct { // Addresses after the first one are used as fallbacks for retries. // It cannot be empty in the internal state. TargetAddresses []string - // ICMPTargetIP is the IP address to use for ICMP echo requests - // in the health checker. It can be set to an unspecified address (0.0.0.0) - // such that the VPN server IP is used, although this can be less reliable. - // It defaults to 1.1.1.1, and cannot be left empty (invalid) in the internal state. - ICMPTargetIP netip.Addr + // ICMPTargetIPs are the IP addresses to use for ICMP echo requests + // in the health checker. The slice can be set to a single + // unspecified address (0.0.0.0) such that the VPN server IP is used, + // although this can be less reliable. It defaults to [1.1.1.1,8.8.8.8], + // and cannot be left empty in the internal state. + ICMPTargetIPs []netip.Addr // RestartVPN indicates whether to restart the VPN connection // when the healthcheck fails. RestartVPN *bool } +var ( + ErrICMPTargetIPNotValid = errors.New("ICMP target IP address is not valid") + ErrICMPTargetIPsNotCompatible = errors.New("ICMP target IP addresses are not compatible") +) + func (h Health) Validate() (err error) { err = validate.ListeningAddress(h.ServerAddress, os.Getuid()) if err != nil { return fmt.Errorf("server listening address is not valid: %w", err) } + for _, ip := range h.ICMPTargetIPs { + switch { + case !ip.IsValid(): + return fmt.Errorf("%w: %s", ErrICMPTargetIPNotValid, ip) + case ip.IsUnspecified() && len(h.ICMPTargetIPs) > 1: + return fmt.Errorf("%w: only a single IP address must be set if it is to be unspecified", + ErrICMPTargetIPsNotCompatible) + } + } + return nil } @@ -45,7 +62,7 @@ func (h *Health) copy() (copied Health) { return Health{ ServerAddress: h.ServerAddress, TargetAddresses: h.TargetAddresses, - ICMPTargetIP: h.ICMPTargetIP, + ICMPTargetIPs: gosettings.CopySlice(h.ICMPTargetIPs), RestartVPN: gosettings.CopyPointer(h.RestartVPN), } } @@ -56,14 +73,17 @@ func (h *Health) copy() (copied Health) { func (h *Health) OverrideWith(other Health) { h.ServerAddress = gosettings.OverrideWithComparable(h.ServerAddress, other.ServerAddress) h.TargetAddresses = gosettings.OverrideWithSlice(h.TargetAddresses, other.TargetAddresses) - h.ICMPTargetIP = gosettings.OverrideWithComparable(h.ICMPTargetIP, other.ICMPTargetIP) + h.ICMPTargetIPs = gosettings.OverrideWithSlice(h.ICMPTargetIPs, other.ICMPTargetIPs) h.RestartVPN = gosettings.OverrideWithPointer(h.RestartVPN, other.RestartVPN) } func (h *Health) SetDefaults() { h.ServerAddress = gosettings.DefaultComparable(h.ServerAddress, "127.0.0.1:9999") h.TargetAddresses = gosettings.DefaultSlice(h.TargetAddresses, []string{"cloudflare.com:443", "github.com:443"}) - h.ICMPTargetIP = gosettings.DefaultComparable(h.ICMPTargetIP, netip.AddrFrom4([4]byte{1, 1, 1, 1})) + h.ICMPTargetIPs = gosettings.DefaultSlice(h.ICMPTargetIPs, []netip.Addr{ + netip.AddrFrom4([4]byte{1, 1, 1, 1}), + netip.AddrFrom4([4]byte{8, 8, 8, 8}), + }) h.RestartVPN = gosettings.DefaultPointer(h.RestartVPN, true) } @@ -78,11 +98,14 @@ func (h Health) toLinesNode() (node *gotree.Node) { for _, targetAddr := range h.TargetAddresses { targetAddrs.Append(targetAddr) } - icmpTarget := "VPN server IP" - if !h.ICMPTargetIP.IsUnspecified() { - icmpTarget = h.ICMPTargetIP.String() + if len(h.ICMPTargetIPs) == 1 && h.ICMPTargetIPs[0].IsUnspecified() { + node.Appendf("ICMP target IP: VPN server IP address") + } else { + icmpIPs := node.Appendf("ICMP target IPs:") + for _, ip := range h.ICMPTargetIPs { + icmpIPs.Append(ip.String()) + } } - node.Appendf("ICMP target IP: %s", icmpTarget) node.Appendf("Restart VPN on healthcheck failure: %s", gosettings.BoolToYesNo(h.RestartVPN)) return node } @@ -91,7 +114,7 @@ func (h *Health) Read(r *reader.Reader) (err error) { h.ServerAddress = r.String("HEALTH_SERVER_ADDRESS") h.TargetAddresses = r.CSV("HEALTH_TARGET_ADDRESSES", reader.RetroKeys("HEALTH_ADDRESS_TO_PING", "HEALTH_TARGET_ADDRESS")) - h.ICMPTargetIP, err = r.NetipAddr("HEALTH_ICMP_TARGET_IP") + h.ICMPTargetIPs, err = r.CSVNetipAddresses("HEALTH_ICMP_TARGET_IPS", reader.RetroKeys("HEALTH_ICMP_TARGET_IP")) if err != nil { return err } diff --git a/internal/configuration/settings/settings_test.go b/internal/configuration/settings/settings_test.go index 8c4dbc2c..454da841 100644 --- a/internal/configuration/settings/settings_test.go +++ b/internal/configuration/settings/settings_test.go @@ -60,7 +60,9 @@ func Test_Settings_String(t *testing.T) { | ├── Target addresses: | | ├── cloudflare.com:443 | | └── github.com:443 -| ├── ICMP target IP: 1.1.1.1 +| ├── ICMP target IPs: +| | ├── 1.1.1.1 +| | └── 8.8.8.8 | └── Restart VPN on healthcheck failure: yes ├── Shadowsocks server settings: | └── Enabled: no diff --git a/internal/healthcheck/checker.go b/internal/healthcheck/checker.go index 0d6622d8..9404360b 100644 --- a/internal/healthcheck/checker.go +++ b/internal/healthcheck/checker.go @@ -16,13 +16,13 @@ import ( ) type Checker struct { - tlsDialAddrs []string - dialer *net.Dialer - echoer *icmp.Echoer - dnsClient *dns.Client - logger Logger - icmpTarget netip.Addr - configMutex sync.Mutex + tlsDialAddrs []string + dialer *net.Dialer + echoer *icmp.Echoer + dnsClient *dns.Client + logger Logger + icmpTargetIPs []netip.Addr + configMutex sync.Mutex icmpNotPermitted bool smallCheckName string @@ -48,11 +48,11 @@ func NewChecker(logger Logger) *Checker { // SetConfig sets the TCP+TLS dial addresses and the ICMP echo IP address // to target by the [Checker]. // This function MUST be called before calling [Checker.Start]. -func (c *Checker) SetConfig(tlsDialAddrs []string, icmpTarget netip.Addr) { +func (c *Checker) SetConfig(tlsDialAddrs []string, icmpTargets []netip.Addr) { c.configMutex.Lock() defer c.configMutex.Unlock() c.tlsDialAddrs = tlsDialAddrs - c.icmpTarget = icmpTarget + c.icmpTargetIPs = icmpTargets } // Start starts the checker by first running a blocking 6s-timed TCP+TLS check, @@ -63,7 +63,7 @@ func (c *Checker) SetConfig(tlsDialAddrs []string, icmpTarget netip.Addr) { // It returns an error if the initial TCP+TLS check fails. // The Checker has to be ultimately stopped by calling [Checker.Stop]. func (c *Checker) Start(ctx context.Context) (runError <-chan error, err error) { - if len(c.tlsDialAddrs) == 0 || c.icmpTarget.IsUnspecified() { + if len(c.tlsDialAddrs) == 0 || len(c.icmpTargetIPs) == 0 { panic("call Checker.SetConfig with non empty values before Checker.Start") } @@ -118,13 +118,14 @@ func (c *Checker) Stop() error { c.stop() <-c.done c.tlsDialAddrs = nil - c.icmpTarget = netip.Addr{} + c.icmpTargetIPs = nil return nil } func (c *Checker) smallPeriodicCheck(ctx context.Context) error { c.configMutex.Lock() - ip := c.icmpTarget + icmpTargetIPs := make([]netip.Addr, len(c.icmpTargetIPs)) + copy(icmpTargetIPs, c.icmpTargetIPs) c.configMutex.Unlock() tryTimeouts := []time.Duration{ 5 * time.Second, @@ -138,10 +139,11 @@ func (c *Checker) smallPeriodicCheck(ctx context.Context) error { 15 * time.Second, 30 * time.Second, } - check := func(ctx context.Context, _ int) error { + check := func(ctx context.Context, try int) error { if c.icmpNotPermitted { return c.dnsClient.Check(ctx) } + ip := icmpTargetIPs[try%len(icmpTargetIPs)] err := c.echoer.Echo(ctx, ip) if errors.Is(err, icmp.ErrNotPermitted) { c.icmpNotPermitted = true diff --git a/internal/vpn/interfaces.go b/internal/vpn/interfaces.go index ea346e05..d783e5d8 100644 --- a/internal/vpn/interfaces.go +++ b/internal/vpn/interfaces.go @@ -101,7 +101,7 @@ type CmdStarter interface { } type HealthChecker interface { - SetConfig(tlsDialAddrs []string, icmpTarget netip.Addr) + SetConfig(tlsDialAddrs []string, icmpTargetIPs []netip.Addr) Start(ctx context.Context) (runError <-chan error, err error) Stop() error } diff --git a/internal/vpn/tunnelup.go b/internal/vpn/tunnelup.go index 8b5da91a..16225fc0 100644 --- a/internal/vpn/tunnelup.go +++ b/internal/vpn/tunnelup.go @@ -31,11 +31,11 @@ func (l *Loop) onTunnelUp(ctx, loopCtx context.Context, data tunnelUpData) { } } - icmpTarget := l.healthSettings.ICMPTargetIP - if icmpTarget.IsUnspecified() { - icmpTarget = data.serverIP + icmpTargetIPs := l.healthSettings.ICMPTargetIPs + if len(icmpTargetIPs) == 1 && icmpTargetIPs[0].IsUnspecified() { + icmpTargetIPs = []netip.Addr{data.serverIP} } - l.healthChecker.SetConfig(l.healthSettings.TargetAddresses, icmpTarget) + l.healthChecker.SetConfig(l.healthSettings.TargetAddresses, icmpTargetIPs) healthErrCh, err := l.healthChecker.Start(ctx) l.healthServer.SetError(err)