From 901f495093d7c864c51bfb323bb519c683d5ac67 Mon Sep 17 00:00:00 2001 From: Martin Dobrev Date: Wed, 5 Aug 2026 10:34:03 +0100 Subject: [PATCH] FIX: report the SOCKS frontend's outcomes to the health check Health tracking was wired into the goproxy response hook only, so a proxy serving SOCKS alone never recorded anything: it started healthy and stayed healthy however many connections failed, and /health answered 200 regardless. The endpoint was a liveness check rather than a health one for that deployment. The frontend reports on what it was asked to do, which is reach the target: opening a tunnel is a success and failing to dial one is a failure. A client refused before the proxy tried to reach anything, by its password, its network or the port it asked for, records nothing, since that says something about the client rather than about this proxy's ability to reach the world. That leaves the two frontends counting slightly different things: the HTTP one still treats a 407 as a failure, inherited from the original health check, where the challenge may have come from an upstream proxy with stale credentials or from this proxy challenging its own client. The difference is documented rather than smoothed over, since changing it would change behaviour that is already deployed. --- README.md | 17 +++++++- health.go | 25 +++++++++--- socks.go | 6 +++ socks_test.go | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 91fc765..9d73025 100644 --- a/README.md +++ b/README.md @@ -193,11 +193,24 @@ only thing limiting them. ### Health -`enable_health_check="on"` makes the proxy count the responses it serves: a run -of consecutive failures marks it unhealthy and a single success clears the run. +`enable_health_check="on"` makes the proxy count how its work goes: a run of +consecutive failures marks it unhealthy and a single success clears the run. `/health` on `http_listen` answers 200 while it is healthy and 503 once it is not, which is what the container image's healthcheck probes. +What counts differs slightly between the two frontends: + +* the SOCKS frontend reports on whether it reached the target. Opening a tunnel + is a success and failing to dial one is a failure; a client refused before the + proxy tried to reach anything — wrong password, disallowed network, disallowed + port — records nothing, because it says something about that client rather + than about this proxy; +* the HTTP frontend counts every response it serves, and additionally treats a + 407 as a failure. A `Proxy-Authorization` challenge in the response stream may + have come from an upstream proxy whose credentials have gone stale, which is + the proxy's problem, but it may equally be this proxy challenging its own + client, which is not. + A program embedding the package gets the same thing through the API, and can read the state directly rather than over HTTP: diff --git a/health.go b/health.go index ba38af9..2f897eb 100644 --- a/health.go +++ b/health.go @@ -98,15 +98,30 @@ func (s *Server) Health() *Health { // health check counted, so it is kept: a proxy whose upstream credentials have // gone stale answers nothing but 407. func (s *Server) recordHealth(resp *http.Response) { - if s.health == nil { + if resp == nil || resp.StatusCode == http.StatusProxyAuthRequired { + s.recordHealthFailure() + return } - if resp == nil || resp.StatusCode == http.StatusProxyAuthRequired { - s.health.RecordFailure() + s.recordHealthSuccess() +} - return +// recordHealthSuccess and recordHealthFailure are what the frontends report +// through. They do nothing unless health tracking was asked for. +// +// The SOCKS frontend reports on whether it reached the target, and says nothing +// about the connections it refuses before trying: a client presenting the wrong +// password, or coming from a network that is not allowed, is a statement about +// that client rather than about this proxy's ability to reach the world. +func (s *Server) recordHealthSuccess() { + if s.health != nil { + s.health.RecordSuccess() } +} - s.health.RecordSuccess() +func (s *Server) recordHealthFailure() { + if s.health != nil { + s.health.RecordFailure() + } } diff --git a/socks.go b/socks.go index 6b024d7..0c25c06 100644 --- a/socks.go +++ b/socks.go @@ -394,8 +394,11 @@ func (s *Server) socksDial(client net.Conn, target string) (net.Conn, error) { return nil, fmt.Errorf("%w: %v is not an allowed port", errSOCKSRefused, target) } + // From here the proxy is the one being asked to reach something, so the + // outcome is what its health is made of. route, err := s.route(target) if err != nil { + s.recordHealthFailure() _ = writeSOCKSReply(client, replyGeneralFailure, nil) return nil, fmt.Errorf("couldn't route %v: %w", target, err) @@ -403,11 +406,14 @@ func (s *Server) socksDial(client net.Conn, target string) (net.Conn, error) { upstream, err := s.dialRoute(context.Background(), route, "tcp", target) if err != nil { + s.recordHealthFailure() _ = writeSOCKSReply(client, replyForDialError(err), nil) return nil, fmt.Errorf("couldn't connect to %v: %w", target, err) } + s.recordHealthSuccess() + if err := writeSOCKSReply(client, replySuccess, upstream.LocalAddr()); err != nil { upstream.Close() diff --git a/socks_test.go b/socks_test.go index 5100a0c..067dbd5 100644 --- a/socks_test.go +++ b/socks_test.go @@ -3,6 +3,7 @@ package microproxy import ( "context" "crypto/tls" + "errors" "net" "net/http" "net/http/httptest" @@ -319,3 +320,112 @@ func TestSOCKSShutdown(t *testing.T) { t.Error("expected the listener to be closed") } } + +// unhealthy drives the tracker below its threshold, so that a later success is +// visible as a change rather than as the state it started in. +func unhealthy(t *testing.T, health *Health) { + t.Helper() + + for i := 0; i < DefaultHealthFailureLimit; i++ { + health.RecordFailure() + } + + if health.Healthy() { + t.Fatal("expected the proxy to be unhealthy to begin with") + } +} + +// A tunnel the proxy managed to open says it can reach the world. +func TestSOCKSRecordsHealthOnSuccess(t *testing.T) { + background := httptest.NewServer(constantHandler("Hello, World!")) + defer background.Close() + + server, addr := newTestSOCKS(t, Config{ + AllowedConnectPorts: []int{portOf(t, background.URL)}, + HealthCheckEnabled: "on", + }) + + unhealthy(t, server.Health()) + + resp, err := socksClient(t, addr, "", "").Get(background.URL) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + + if !server.Health().Healthy() { + t.Error("expected an established tunnel to record a success") + } +} + +// A target the proxy could not reach is what the health endpoint exists to +// report. +func TestSOCKSRecordsHealthOnDialFailure(t *testing.T) { + background := httptest.NewServer(constantHandler("Hello, World!")) + defer background.Close() + + server, addr := newTestSOCKS(t, + Config{ + AllowedConnectPorts: []int{portOf(t, background.URL)}, + HealthCheckEnabled: "on", + }, + WithRouter(RouterFunc(func(host string) (Route, error) { + return Route{Dialer: DialerFunc(func(context.Context, string, string) (net.Conn, error) { + return nil, errors.New("the tunnel is down") + })}, nil + }))) + + if _, err := socksClient(t, addr, "", "").Get(background.URL); err == nil { + t.Fatal("expected the connection to fail") + } + + if failures := server.Health().Failures(); failures != 1 { + t.Errorf("expected one failure to be recorded, got %v", failures) + } +} + +// A client refused before the proxy tried to reach anything says nothing about +// the proxy, so it must not count against it. +func TestSOCKSRefusalDoesNotAffectHealth(t *testing.T) { + background := httptest.NewServer(constantHandler("Hello, World!")) + defer background.Close() + + server, addr := newTestSOCKS(t, + Config{ + AllowedConnectPorts: []int{portOf(t, background.URL)}, + HealthCheckEnabled: "on", + }, + WithCredentials(testBasicUsers())) + + if _, err := socksClient(t, addr, user, "wrong").Get(background.URL); err == nil { + t.Fatal("expected the connection to be refused") + } + + if failures := server.Health().Failures(); failures != 0 { + t.Errorf("expected a refused client to record nothing, got %v failures", failures) + } + + if !server.Health().Healthy() { + t.Error("expected the proxy to still be healthy") + } +} + +// A client the network ACLs turn away is refused for the same reason. +func TestSOCKSDeniedNetworkDoesNotAffectHealth(t *testing.T) { + background := httptest.NewServer(constantHandler("Hello, World!")) + defer background.Close() + + server, addr := newTestSOCKS(t, Config{ + AllowedNetworks: []string{"172.16.11.0/24"}, + AllowedConnectPorts: []int{portOf(t, background.URL)}, + HealthCheckEnabled: "on", + }) + + if _, err := socksClient(t, addr, "", "").Get(background.URL); err == nil { + t.Fatal("expected the connection to be refused") + } + + if failures := server.Health().Failures(); failures != 0 { + t.Errorf("expected a denied client to record nothing, got %v failures", failures) + } +}