From c6f68a64e61ebb8cb372ac2279ac6f772d023a8c Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Mon, 21 Mar 2022 19:57:35 +0000 Subject: [PATCH] fix(health): use TCP dialing instead of ping - `HEALTH_TARGET_ADDRESS` to replace `HEALTH_ADDRESS_TO_PING` - Remove `github.com/go-ping/ping` dependency - Dial TCP the target address, appending `:443` if port is not set --- Dockerfile | 2 +- go.mod | 3 - go.sum | 6 - internal/configuration/settings/health.go | 16 +- .../configuration/settings/settings_test.go | 2 +- internal/configuration/sources/env/health.go | 2 +- internal/healthcheck/health.go | 57 +++++-- internal/healthcheck/health_ping_test.go | 46 ------ internal/healthcheck/health_test.go | 141 ++++++++++-------- internal/healthcheck/ping.go | 18 --- internal/healthcheck/pinger_mock_test.go | 60 -------- internal/healthcheck/server.go | 5 +- 12 files changed, 131 insertions(+), 227 deletions(-) delete mode 100644 internal/healthcheck/health_ping_test.go delete mode 100644 internal/healthcheck/ping.go delete mode 100644 internal/healthcheck/pinger_mock_test.go diff --git a/Dockerfile b/Dockerfile index 4c13995b..d2db8f04 100644 --- a/Dockerfile +++ b/Dockerfile @@ -124,7 +124,7 @@ ENV VPN_SERVICE_PROVIDER=pia \ LOG_LEVEL=info \ # Health HEALTH_SERVER_ADDRESS=127.0.0.1:9999 \ - HEALTH_ADDRESS_TO_PING=github.com \ + HEALTH_TARGET_ADDRESS=github.com:443 \ HEALTH_VPN_DURATION_INITIAL=6s \ HEALTH_VPN_DURATION_ADDITION=5s \ # DNS over TLS diff --git a/go.mod b/go.mod index 3bbd89d6..903132ab 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,6 @@ go 1.17 require ( github.com/breml/rootcerts v0.2.2 github.com/fatih/color v1.13.0 - github.com/go-ping/ping v0.0.0-20210911151512-381826476871 github.com/golang/mock v1.6.0 github.com/qdm12/dns v1.11.0 github.com/qdm12/golibs v0.0.0-20210822203818-5c568b0777b6 @@ -26,7 +25,6 @@ require ( require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/google/go-cmp v0.5.5 // indirect - github.com/google/uuid v1.2.0 // indirect github.com/josharian/native v0.0.0-20200817173448-b6b71def0850 // indirect github.com/mattn/go-colorable v0.1.9 // indirect github.com/mattn/go-isatty v0.0.14 // indirect @@ -41,6 +39,5 @@ require ( go4.org/unsafe/assume-no-moving-gc v0.0.0-20201222180813-1025295fd063 // indirect golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97 // indirect golang.org/x/net v0.0.0-20210504132125-bbd867fde50d // indirect - golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect ) diff --git a/go.sum b/go.sum index e3b37b09..654125b0 100644 --- a/go.sum +++ b/go.sum @@ -32,8 +32,6 @@ github.com/go-openapi/spec v0.17.0/go.mod h1:XkF/MOi14NmjsfZ8VtAKf8pIlbZzyoTvZsd github.com/go-openapi/strfmt v0.17.0/go.mod h1:P82hnJI0CXkErkXi8IKjPbNBM6lV6+5pLP5l494TcyU= github.com/go-openapi/swag v0.17.0/go.mod h1:AByQ+nYG6gQg71GINrmuDXCPWdL640yX49/kXLo40Tg= github.com/go-openapi/validate v0.17.0/go.mod h1:Uh4HdOzKt19xGIGm1qHf/ofbX1YQ4Y+MYsct2VUrAJ4= -github.com/go-ping/ping v0.0.0-20210911151512-381826476871 h1:wtjTfjwAR/BYYMJ+QOLI/3J/qGEI0fgrkZvgsEWK2/Q= -github.com/go-ping/ping v0.0.0-20210911151512-381826476871/go.mod h1:xIFjORFzTxqIV/tDVGO4eDy/bLuSyawEeojSm3GfRGk= github.com/golang/mock v1.5.0/go.mod h1:CWnOUgYIOo4TcNZ0wHX3YZCqsaM1I1Jvs6v3mP3KVu8= github.com/golang/mock v1.6.0 h1:ErTB+efbowRARo13NNdxyJji2egdxLGQhRaY+DUumQc= github.com/golang/mock v1.6.0/go.mod h1:p6yTPP+5HYm5mzsMV8JkE6ZKdX+/wYM6Hr+LicevLPs= @@ -47,8 +45,6 @@ github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/google/uuid v1.2.0 h1:qJYtXnJRWmpe7m/3XlyhrsLrEURqHRM2kxzoxXqyUDs= -github.com/google/uuid v1.2.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/gotify/go-api-client/v2 v2.0.4/go.mod h1:VKiah/UK20bXsr0JObE1eBVLW44zbBouzjuri9iwjFU= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= @@ -176,7 +172,6 @@ golang.org/x/net v0.0.0-20201216054612-986b41b23924/go.mod h1:m0MpNAwzfU5UDzcl9v golang.org/x/net v0.0.0-20201224014010-6772e930b67b/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210119194325-5f4716e94777/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= -golang.org/x/net v0.0.0-20210316092652-d523dce5a7f4/go.mod h1:RBQZq4jEuRlivfhVLdyRGr576XBO4/greRjx4P4O3yc= golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM= golang.org/x/net v0.0.0-20210504132125-bbd867fde50d h1:nTDGCTeAu2LhcsHTRzjyIUbZHCJ4QePArsm27Hka0UM= golang.org/x/net v0.0.0-20210504132125-bbd867fde50d/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= @@ -210,7 +205,6 @@ golang.org/x/sys v0.0.0-20210123111255-9b0068b26619/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210216163648-f7da38b97c65/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210309040221-94ec62e08169/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210315160823-c6e025ad8005/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210403161142-5e06dd20ab57/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/internal/configuration/settings/health.go b/internal/configuration/settings/health.go index 2f4358b9..c333a9fc 100644 --- a/internal/configuration/settings/health.go +++ b/internal/configuration/settings/health.go @@ -15,10 +15,10 @@ type Health struct { // for the health check server. // It cannot be the empty string in the internal state. ServerAddress string - // AddressToPing is the IP address or domain name to - // ping periodically for the health check. + // TargetAddress is the address (host or host:port) + // to TCP dial to periodically for the health check. // It cannot be the empty string in the internal state. - AddressToPing string + TargetAddress string VPN HealthyWait } @@ -41,7 +41,7 @@ func (h Health) Validate() (err error) { func (h *Health) copy() (copied Health) { return Health{ ServerAddress: h.ServerAddress, - AddressToPing: h.AddressToPing, + TargetAddress: h.TargetAddress, VPN: h.VPN.copy(), } } @@ -50,7 +50,7 @@ func (h *Health) copy() (copied Health) { // unset field of the receiver settings object. func (h *Health) MergeWith(other Health) { h.ServerAddress = helpers.MergeWithString(h.ServerAddress, other.ServerAddress) - h.AddressToPing = helpers.MergeWithString(h.AddressToPing, other.AddressToPing) + h.TargetAddress = helpers.MergeWithString(h.TargetAddress, other.TargetAddress) h.VPN.mergeWith(other.VPN) } @@ -59,13 +59,13 @@ func (h *Health) MergeWith(other Health) { // settings. func (h *Health) OverrideWith(other Health) { h.ServerAddress = helpers.OverrideWithString(h.ServerAddress, other.ServerAddress) - h.AddressToPing = helpers.OverrideWithString(h.AddressToPing, other.AddressToPing) + h.TargetAddress = helpers.OverrideWithString(h.TargetAddress, other.TargetAddress) h.VPN.overrideWith(other.VPN) } func (h *Health) SetDefaults() { h.ServerAddress = helpers.DefaultString(h.ServerAddress, "127.0.0.1:9999") - h.AddressToPing = helpers.DefaultString(h.AddressToPing, "github.com") + h.TargetAddress = helpers.DefaultString(h.TargetAddress, "github.com:443") h.VPN.setDefaults() } @@ -76,7 +76,7 @@ func (h Health) String() string { func (h Health) toLinesNode() (node *gotree.Node) { node = gotree.New("Health settings:") node.Appendf("Server listening address: %s", h.ServerAddress) - node.Appendf("Address to ping: %s", h.AddressToPing) + node.Appendf("Target address: %s", h.TargetAddress) node.AppendNode(h.VPN.toLinesNode("VPN")) return node } diff --git a/internal/configuration/settings/settings_test.go b/internal/configuration/settings/settings_test.go index d64a2f0a..89f1ba16 100644 --- a/internal/configuration/settings/settings_test.go +++ b/internal/configuration/settings/settings_test.go @@ -66,7 +66,7 @@ func Test_Settings_String(t *testing.T) { | └── Log level: INFO ├── Health settings: | ├── Server listening address: 127.0.0.1:9999 -| ├── Address to ping: github.com +| ├── Target address: github.com:443 | └── VPN wait durations: | ├── Initial duration: 6s | └── Additional duration: 5s diff --git a/internal/configuration/sources/env/health.go b/internal/configuration/sources/env/health.go index 15dee44c..699470d4 100644 --- a/internal/configuration/sources/env/health.go +++ b/internal/configuration/sources/env/health.go @@ -10,7 +10,7 @@ import ( func (r *Reader) ReadHealth() (health settings.Health, err error) { health.ServerAddress = os.Getenv("HEALTH_SERVER_ADDRESS") - health.AddressToPing = os.Getenv("HEALTH_ADDRESS_TO_PING") + _, health.TargetAddress = r.getEnvWithRetro("HEALTH_TARGET_ADDRESS", "HEALTH_ADDRESS_TO_PING") health.VPN.Initial, err = r.readDurationWithRetro( "HEALTH_VPN_DURATION_INITIAL", diff --git a/internal/healthcheck/health.go b/internal/healthcheck/health.go index 084c25a9..ca836d78 100644 --- a/internal/healthcheck/health.go +++ b/internal/healthcheck/health.go @@ -3,6 +3,9 @@ package healthcheck import ( "context" + "errors" + "fmt" + "net" "time" ) @@ -14,7 +17,12 @@ func (s *Server) runHealthcheckLoop(ctx context.Context, done chan<- struct{}) { for { previousErr := s.handler.getErr() - err := healthCheck(ctx, s.pinger) + const healthcheckTimeout = 3 * time.Second + healthcheckCtx, healthcheckCancel := context.WithTimeout( + ctx, healthcheckTimeout) + err := s.healthCheck(healthcheckCtx) + healthcheckCancel() + s.handler.setErr(err) if previousErr != nil && err == nil { @@ -56,23 +64,40 @@ func (s *Server) runHealthcheckLoop(ctx context.Context, done chan<- struct{}) { } } -func healthCheck(ctx context.Context, pinger Pinger) (err error) { +func (s *Server) healthCheck(ctx context.Context) (err error) { // TODO use mullvad API if current provider is Mullvad - // If we run without root, you need to run this on the gluetun binary: - // setcap cap_net_raw=+ep /path/to/your/compiled/binary - // Alternatively, we could have a separate binary just for healthcheck to - // reduce attack surface. - errCh := make(chan error) - go func() { - errCh <- pinger.Run() - }() - select { - case <-ctx.Done(): - pinger.Stop() - <-errCh - return ctx.Err() - case err = <-errCh: + address, err := makeAddressToDial(s.config.TargetAddress) + if err != nil { return err } + + const dialNetwork = "tcp4" + connection, err := s.dialer.DialContext(ctx, dialNetwork, address) + if err != nil { + return fmt.Errorf("cannot dial: %w", err) + } + + err = connection.Close() + if err != nil { + return fmt.Errorf("cannot close connection: %w", err) + } + + return nil +} + +func makeAddressToDial(address string) (addressToDial string, err error) { + host, port, err := net.SplitHostPort(address) + if err != nil { + addrErr := new(net.AddrError) + ok := errors.As(err, &addrErr) + if !ok || addrErr.Err != "missing port in address" { + return "", fmt.Errorf("cannot split host and port from address: %w", err) + } + host = address + const defaultPort = "443" + port = defaultPort + } + address = net.JoinHostPort(host, port) + return address, nil } diff --git a/internal/healthcheck/health_ping_test.go b/internal/healthcheck/health_ping_test.go deleted file mode 100644 index 1a350b1b..00000000 --- a/internal/healthcheck/health_ping_test.go +++ /dev/null @@ -1,46 +0,0 @@ -//go:build integration - -package healthcheck - -import ( - "context" - "testing" - "time" - - "github.com/stretchr/testify/assert" -) - -func Test_healthCheck_ping(t *testing.T) { - t.Parallel() - - const timeout = time.Second - - testCases := map[string]struct { - address string - err error - }{ - "github.com": { - address: "github.com", - }, - "99.99.99.99": { - address: "99.99.99.99", - err: context.DeadlineExceeded, - }, - } - - for name, testCase := range testCases { - testCase := testCase - t.Run(name, func(t *testing.T) { - t.Parallel() - - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - pinger := newPinger(testCase.address) - - err := healthCheck(ctx, pinger) - - assert.ErrorIs(t, testCase.err, err) - }) - } -} diff --git a/internal/healthcheck/health_test.go b/internal/healthcheck/health_test.go index 0aa307b3..b480c879 100644 --- a/internal/healthcheck/health_test.go +++ b/internal/healthcheck/health_test.go @@ -2,40 +2,90 @@ package healthcheck import ( "context" - "errors" + "fmt" + "net" "testing" "time" - "github.com/golang/mock/gomock" + "github.com/qdm12/gluetun/internal/configuration/settings" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func Test_healthCheck(t *testing.T) { +func Test_Server_healthCheck(t *testing.T) { t.Parallel() - canceledCtx, cancel := context.WithCancel(context.Background()) - cancel() + t.Run("canceled real dialer", func(t *testing.T) { + t.Parallel() - someErr := errors.New("error") + dialer := &net.Dialer{} + const address = "github.com:443" + + server := &Server{ + dialer: dialer, + config: settings.Health{ + TargetAddress: address, + }, + } + + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + + err := server.healthCheck(canceledCtx) + + require.Error(t, err) + assert.Contains(t, err.Error(), "operation was canceled") + }) + + t.Run("dial localhost:0", func(t *testing.T) { + t.Parallel() + + listener, err := net.Listen("tcp4", "localhost:0") + require.NoError(t, err) + t.Cleanup(func() { + err = listener.Close() + assert.NoError(t, err) + }) + + listeningAddress := listener.Addr() + + dialer := &net.Dialer{} + server := &Server{ + dialer: dialer, + config: settings.Health{ + TargetAddress: listeningAddress.String(), + }, + } + + const timeout = 100 * time.Millisecond + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + err = server.healthCheck(ctx) + + assert.NoError(t, err) + }) +} + +func Test_makeAddressToDial(t *testing.T) { + t.Parallel() testCases := map[string]struct { - ctx context.Context - runErr error - stopCall bool - err error + address string + addressToDial string + err error }{ - "success": { - ctx: context.Background(), + "host without port": { + address: "test.com", + addressToDial: "test.com:443", }, - "error": { - ctx: context.Background(), - runErr: someErr, - err: someErr, + "host with port": { + address: "test.com:80", + addressToDial: "test.com:80", }, - "context canceled": { - ctx: canceledCtx, - stopCall: true, - err: context.Canceled, + "bad address": { + address: "test.com::", + err: fmt.Errorf("cannot split host and port from address: address test.com::: too many colons in address"), //nolint:lll }, } @@ -43,54 +93,15 @@ func Test_healthCheck(t *testing.T) { testCase := testCase t.Run(name, func(t *testing.T) { t.Parallel() - ctrl := gomock.NewController(t) - stopped := make(chan struct{}) + addressToDial, err := makeAddressToDial(testCase.address) - pinger := NewMockPinger(ctrl) - pinger.EXPECT().Run().DoAndReturn(func() error { - if testCase.stopCall { - <-stopped - } - return testCase.runErr - }) - - if testCase.stopCall { - pinger.EXPECT().Stop().DoAndReturn(func() { - close(stopped) - }) + assert.Equal(t, testCase.addressToDial, addressToDial) + if testCase.err != nil { + assert.EqualError(t, err, testCase.err.Error()) + } else { + assert.NoError(t, err) } - - err := healthCheck(testCase.ctx, pinger) - - assert.ErrorIs(t, testCase.err, err) }) } - - t.Run("canceled real pinger", func(t *testing.T) { - t.Parallel() - - pinger := newPinger("github.com") - - canceledCtx, cancel := context.WithCancel(context.Background()) - cancel() - - err := healthCheck(canceledCtx, pinger) - - assert.ErrorIs(t, context.Canceled, err) - }) - - t.Run("ping 127.0.0.1", func(t *testing.T) { - t.Parallel() - - pinger := newPinger("127.0.0.1") - - const timeout = 100 * time.Millisecond - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - err := healthCheck(ctx, pinger) - - assert.NoError(t, err) - }) } diff --git a/internal/healthcheck/ping.go b/internal/healthcheck/ping.go deleted file mode 100644 index a0c566a8..00000000 --- a/internal/healthcheck/ping.go +++ /dev/null @@ -1,18 +0,0 @@ -package healthcheck - -import "github.com/go-ping/ping" - -//go:generate mockgen -destination=pinger_mock_test.go -package healthcheck . Pinger - -type Pinger interface { - Run() error - Stop() -} - -func newPinger(addrToPing string) (pinger *ping.Pinger) { - const count = 1 - pinger = ping.New(addrToPing) - pinger.Count = count - pinger.SetPrivileged(true) // see https://github.com/go-ping/ping#linux - return pinger -} diff --git a/internal/healthcheck/pinger_mock_test.go b/internal/healthcheck/pinger_mock_test.go deleted file mode 100644 index 63375cd4..00000000 --- a/internal/healthcheck/pinger_mock_test.go +++ /dev/null @@ -1,60 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/qdm12/gluetun/internal/healthcheck (interfaces: Pinger) - -// Package healthcheck is a generated GoMock package. -package healthcheck - -import ( - reflect "reflect" - - gomock "github.com/golang/mock/gomock" -) - -// MockPinger is a mock of Pinger interface. -type MockPinger struct { - ctrl *gomock.Controller - recorder *MockPingerMockRecorder -} - -// MockPingerMockRecorder is the mock recorder for MockPinger. -type MockPingerMockRecorder struct { - mock *MockPinger -} - -// NewMockPinger creates a new mock instance. -func NewMockPinger(ctrl *gomock.Controller) *MockPinger { - mock := &MockPinger{ctrl: ctrl} - mock.recorder = &MockPingerMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockPinger) EXPECT() *MockPingerMockRecorder { - return m.recorder -} - -// Run mocks base method. -func (m *MockPinger) Run() error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Run") - ret0, _ := ret[0].(error) - return ret0 -} - -// Run indicates an expected call of Run. -func (mr *MockPingerMockRecorder) Run() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Run", reflect.TypeOf((*MockPinger)(nil).Run)) -} - -// Stop mocks base method. -func (m *MockPinger) Stop() { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Stop") -} - -// Stop indicates an expected call of Stop. -func (mr *MockPingerMockRecorder) Stop() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Stop", reflect.TypeOf((*MockPinger)(nil).Stop)) -} diff --git a/internal/healthcheck/server.go b/internal/healthcheck/server.go index 27bdc651..1c45f705 100644 --- a/internal/healthcheck/server.go +++ b/internal/healthcheck/server.go @@ -2,6 +2,7 @@ package healthcheck import ( "context" + "net" "github.com/qdm12/gluetun/internal/configuration/settings" "github.com/qdm12/gluetun/internal/vpn" @@ -16,7 +17,7 @@ type ServerRunner interface { type Server struct { logger Logger handler *handler - pinger Pinger + dialer *net.Dialer config settings.Health vpn vpnHealth } @@ -26,7 +27,7 @@ func NewServer(config settings.Health, return &Server{ logger: logger, handler: newHandler(), - pinger: newPinger(config.AddressToPing), + dialer: &net.Dialer{}, config: config, vpn: vpnHealth{ looper: vpnLooper,