FIX: report the SOCKS frontend's outcomes to the health check

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:
 

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()
+	}
 }

socks.go

@@ -394,20 +394,26 @@ 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)
 	}
 
 	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()
 

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)
+	}
+}