From 6627cda96c16dbc9a5945192064a262065116b02 Mon Sep 17 00:00:00 2001 From: "Quentin McGaw (desktop)" Date: Sat, 11 Sep 2021 22:22:55 +0000 Subject: [PATCH] Feat: `HEALTH_ADDRESS_TO_PING` variable - Defaults to `1.1.1.1` - Add more Ping integration tests with different addresses - Add unit test pinging 127.0.0.1 - Add comment explaining why we need to use ICMP instead of UDP --- Dockerfile | 1 + internal/configuration/health.go | 8 ++++ internal/configuration/health_test.go | 53 +++++++++++++++++++++++- internal/configuration/settings_test.go | 1 + internal/healthcheck/health_ping_test.go | 32 ++++++++++++-- internal/healthcheck/health_test.go | 17 +++++++- internal/healthcheck/ping.go | 5 +-- internal/healthcheck/server.go | 2 +- 8 files changed, 109 insertions(+), 10 deletions(-) diff --git a/Dockerfile b/Dockerfile index 581130e2..181a38ac 100644 --- a/Dockerfile +++ b/Dockerfile @@ -125,6 +125,7 @@ ENV VPNSP=pia \ LOG_LEVEL=info \ # Health HEALTH_SERVER_ADDRESS=127.0.0.1:9999 \ + HEALTH_ADDRESS_TO_PING=1.1.1.1 \ HEALTH_OPENVPN_DURATION_INITIAL=6s \ HEALTH_OPENVPN_DURATION_ADDITION=5s \ # DNS over TLS diff --git a/internal/configuration/health.go b/internal/configuration/health.go index b6c22bf9..10a96517 100644 --- a/internal/configuration/health.go +++ b/internal/configuration/health.go @@ -12,6 +12,7 @@ import ( // Health contains settings for the healthcheck and health server. type Health struct { ServerAddress string + AddressToPing string VPN HealthyWait } @@ -24,6 +25,8 @@ func (settings *Health) lines() (lines []string) { lines = append(lines, indent+lastIndent+"Server address: "+settings.ServerAddress) + lines = append(lines, indent+lastIndent+"Address to ping: "+settings.AddressToPing) + lines = append(lines, indent+lastIndent+"VPN:") for _, line := range settings.VPN.lines() { lines = append(lines, indent+indent+line) @@ -49,6 +52,11 @@ func (settings *Health) read(r reader) (err error) { return fmt.Errorf("environment variable HEALTH_SERVER_ADDRESS: %w", err) } + settings.AddressToPing, err = r.env.Get("HEALTH_ADDRESS_TO_PING", params.Default("1.1.1.1")) + if err != nil { + return fmt.Errorf("environment variable HEALTH_ADDRESS_TO_PING: %w", err) + } + retroKeyOption := params.RetroKeys([]string{"HEALTH_OPENVPN_DURATION_INITIAL"}, r.onRetroActive) settings.VPN.Initial, err = r.env.Duration("HEALTH_VPN_DURATION_INITIAL", params.Default("6s"), retroKeyOption) if err != nil { diff --git a/internal/configuration/health_test.go b/internal/configuration/health_test.go index 2d327e70..fb52e7a3 100644 --- a/internal/configuration/health_test.go +++ b/internal/configuration/health_test.go @@ -15,8 +15,15 @@ import ( func Test_Health_String(t *testing.T) { t.Parallel() - var health Health - const expected = "|--Health:\n |--Server address: \n |--VPN:\n |--Initial duration: 0s" + health := Health{ + ServerAddress: "a", + AddressToPing: "b", + } + const expected = `|--Health: + |--Server address: a + |--Address to ping: b + |--VPN: + |--Initial duration: 0s` s := health.String() @@ -34,6 +41,7 @@ func Test_Health_lines(t *testing.T) { lines: []string{ "|--Health:", " |--Server address: ", + " |--Address to ping: ", " |--VPN:", " |--Initial duration: 0s", }, @@ -41,6 +49,7 @@ func Test_Health_lines(t *testing.T) { "filled settings": { settings: Health{ ServerAddress: "address:9999", + AddressToPing: "1.1.1.1", VPN: HealthyWait{ Initial: time.Second, Addition: time.Minute, @@ -49,6 +58,7 @@ func Test_Health_lines(t *testing.T) { lines: []string{ "|--Health:", " |--Server address: address:9999", + " |--Address to ping: 1.1.1.1", " |--VPN:", " |--Initial duration: 1s", " |--Addition duration: 1m0s", @@ -73,6 +83,12 @@ func Test_Health_read(t *testing.T) { errDummy := errors.New("dummy") + type stringCall struct { + call bool + s string + err error + } + type stringCallWithWarning struct { call bool s string @@ -88,6 +104,7 @@ func Test_Health_read(t *testing.T) { testCases := map[string]struct { serverAddress stringCallWithWarning + addressToPing stringCall vpnInitial durationCall vpnAddition durationCall expected Health @@ -98,6 +115,10 @@ func Test_Health_read(t *testing.T) { call: true, s: "127.0.0.1:9999", }, + addressToPing: stringCall{ + call: true, + s: "1.2.3.4", + }, vpnInitial: durationCall{ call: true, duration: time.Second, @@ -108,6 +129,7 @@ func Test_Health_read(t *testing.T) { }, expected: Health{ ServerAddress: "127.0.0.1:9999", + AddressToPing: "1.2.3.4", VPN: HealthyWait{ Initial: time.Second, Addition: time.Minute, @@ -126,10 +148,27 @@ func Test_Health_read(t *testing.T) { }, err: errors.New("environment variable HEALTH_SERVER_ADDRESS: dummy"), }, + "address to ping error": { + serverAddress: stringCallWithWarning{ + call: true, + }, + addressToPing: stringCall{ + call: true, + s: "address", + err: errDummy, + }, + expected: Health{ + AddressToPing: "address", + }, + err: errors.New("environment variable HEALTH_ADDRESS_TO_PING: dummy"), + }, "initial error": { serverAddress: stringCallWithWarning{ call: true, }, + addressToPing: stringCall{ + call: true, + }, vpnInitial: durationCall{ call: true, duration: time.Second, @@ -146,6 +185,9 @@ func Test_Health_read(t *testing.T) { serverAddress: stringCallWithWarning{ call: true, }, + addressToPing: stringCall{ + call: true, + }, vpnInitial: durationCall{ call: true, duration: time.Second, @@ -186,6 +228,13 @@ func Test_Health_read(t *testing.T) { } } + if testCase.addressToPing.call { + value := testCase.addressToPing.s + err := testCase.addressToPing.err + env.EXPECT().Get("HEALTH_ADDRESS_TO_PING", gomock.Any()). + Return(value, err) + } + if testCase.vpnInitial.call { value := testCase.vpnInitial.duration err := testCase.vpnInitial.err diff --git a/internal/configuration/settings_test.go b/internal/configuration/settings_test.go index 358736a8..7ad59490 100644 --- a/internal/configuration/settings_test.go +++ b/internal/configuration/settings_test.go @@ -51,6 +51,7 @@ func Test_Settings_lines(t *testing.T) { " |--Timezone: NOT SET ⚠️ - it can cause time related issues", "|--Health:", " |--Server address: ", + " |--Address to ping: ", " |--VPN:", " |--Initial duration: 0s", "|--HTTP control server:", diff --git a/internal/healthcheck/health_ping_test.go b/internal/healthcheck/health_ping_test.go index 2d3b7f5b..48d98428 100644 --- a/internal/healthcheck/health_ping_test.go +++ b/internal/healthcheck/health_ping_test.go @@ -5,6 +5,7 @@ package healthcheck import ( "context" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -12,9 +13,34 @@ import ( func Test_healthCheck_ping(t *testing.T) { t.Parallel() - pinger := newPinger() + const timeout = time.Second - err := healthCheck(context.Background(), pinger) + testCases := map[string]struct { + address string + err error + }{ + "1.1.1.1": { + address: "1.1.1.1", + }, + "99.99.99.99": { + address: "99.99.99.99", + err: context.DeadlineExceeded, + }, + } - assert.NoError(t, err) + 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 8e46f99e..b07a35bc 100644 --- a/internal/healthcheck/health_test.go +++ b/internal/healthcheck/health_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "testing" + "time" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -69,7 +70,7 @@ func Test_healthCheck(t *testing.T) { t.Run("canceled real pinger", func(t *testing.T) { t.Parallel() - pinger := newPinger() + pinger := newPinger("1.1.1.1") canceledCtx, cancel := context.WithCancel(context.Background()) cancel() @@ -78,4 +79,18 @@ func Test_healthCheck(t *testing.T) { 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 index ac5f4706..a0c566a8 100644 --- a/internal/healthcheck/ping.go +++ b/internal/healthcheck/ping.go @@ -9,11 +9,10 @@ type Pinger interface { Stop() } -func newPinger() (pinger *ping.Pinger) { - const addrToPing = "1.1.1.1" +func newPinger(addrToPing string) (pinger *ping.Pinger) { const count = 1 pinger = ping.New(addrToPing) pinger.Count = count - pinger.SetPrivileged(true) + pinger.SetPrivileged(true) // see https://github.com/go-ping/ping#linux return pinger } diff --git a/internal/healthcheck/server.go b/internal/healthcheck/server.go index abc9586c..425c4c3f 100644 --- a/internal/healthcheck/server.go +++ b/internal/healthcheck/server.go @@ -27,7 +27,7 @@ func NewServer(config configuration.Health, return &Server{ logger: logger, handler: newHandler(logger), - pinger: newPinger(), + pinger: newPinger(config.AddressToPing), config: config, vpn: vpnHealth{ looper: vpnLooper,