egress: make the exporter-failure branch reachable, and pin what the tests claim

Round four on #417.

The failure test added last round stopped covering its branch the moment
I added endpoint pre-validation: "bad host" fails redactEndpoint, so
enableOTELLogs exits at the earlier warning and the sanitizing path is
never reached. Removing the sanitization would have left it green. That
is the third test in this PR to quietly stop testing what it claims, so
this one is structural rather than input-dependent: newLogExporter is
indirected — the same shape as initMetricsFn — and the test stubs it to
return an error quoting the endpoint. There is no environment value that
reaches this branch, because anything malformed enough to break
otlploghttp is now refused earlier and otlploghttp prefers logging and
falling back to returning an error.

redactEndpoint now accepts only http and https. This exporter is OTLP
over HTTP, so "ftp://collector/v1/logs" parses fine and would have been
announced as enabled while being unsendable.

peerAttrs' country test only checked that donor_country existed, so it
would have stayed green if the lookup reverted to geolocating
transportAddr — which behind Caddy is loopback, reporting every donor as
unknown while still populating the attribute. Now stubs the lookup and
asserts the forwarded address wins.

Author: Adam Fisk

egress/otellogs.go

@@ -82,7 +82,7 @@ func enableOTELLogs(ctx context.Context) func(context.Context) error {
 		return func(context.Context) error { return nil }
 	}
 
-	exp, err := otlploghttp.New(ctx)
+	exp, err := newLogExporter(ctx)
 	if err != nil {
 		// The exporter reports what it could not parse, which for an endpoint
 		// problem is the endpoint — credentials included. Substituted rather
@@ -144,6 +144,14 @@ func enableOTELLogs(ctx context.Context) func(context.Context) error {
 // hold up process exit.
 const logShutdownTimeout = 5 * time.Second
 
+// newLogExporter is indirected so the failure branch below is reachable from a
+// test. otlploghttp.New declines to fail for most bad input — it logs through
+// the SDK's error handler and falls back — so there is no environment value
+// that exercises the sanitizing path. Same shape as initMetricsFn in metrics.go.
+var newLogExporter = func(ctx context.Context) (sdklog.Exporter, error) {
+	return otlploghttp.New(ctx)
+}
+
 // otlpLogsEndpointVars are the variables that can supply a logs endpoint, in
 // OTEL's own precedence order: signal-specific first, then shared.
 //
@@ -214,16 +222,21 @@ func (h *teeHandler) WithGroup(name string) slog.Handler {
 // is read by more people than the config is. Scheme, host and path are what make
 // the line useful.
 //
-// Anything that is not an absolute URL is refused outright rather than
-// returned. url.Parse accepts opaque strings ("secret", "http:token") and
+// Anything that is not an absolute http/https URL is refused outright rather
+// than returned. url.Parse accepts opaque strings ("secret", "http:token") and
 // network-path references ("//s3cr3t", which parses with that as the Host and
 // no scheme) without error, and clearing User does nothing to any of them, so
 // returning the parsed form would echo the whole value. Requiring both a scheme
 // and a host is what makes the redaction meaningful; with no structure to rely
 // on there is no way to tell which part was secret.
 func redactEndpoint(raw string) (string, bool) {
 	u, err := url.Parse(raw)
-	if err != nil || u.Scheme == "" || u.Host == "" {
+	if err != nil || u.Host == "" {
+		return "", false
+	}
+	// http/https only: this exporter is OTLP over HTTP, so "ftp://collector"
+	// parses fine and would be announced as enabled while being unsendable.
+	if u.Scheme != "http" && u.Scheme != "https" {
 		return "", false
 	}
 	u.User = nil

egress/otellogs_test.go

@@ -3,13 +3,16 @@ package egress
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
 	"testing"
 	"time"
+
+	sdklog "go.opentelemetry.io/otel/sdk/log"
 )
 
 // recordingHandler captures what a leg of the tee actually received.
@@ -419,6 +422,7 @@ func TestRedactEndpoint(t *testing.T) {
 		{"hostless path is refused", "/v1/logs?api-key=SECRET", "(unparseable)"},
 		{"empty is refused", "", "(unparseable)"},
 		{"network-path reference is refused", "//s3cr3t", "(unparseable)"},
+		{"non-http scheme is refused", "ftp://collector/v1/logs", "(unparseable)"},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			got, ok := redactEndpoint(tc.in)
@@ -437,15 +441,25 @@ func TestRedactEndpoint(t *testing.T) {
 
 // The exporter-construction failure path, which is where a credential is most
 // likely to appear: a malformed-endpoint error is exactly the one that quotes
-// the endpoint back. Untested, a regression to passing err straight to slog
-// would leave the suite green while writing the credential to the journal.
+// the endpoint back.
+//
+// The constructor is stubbed because no environment value reaches this branch —
+// anything malformed enough to break otlploghttp is refused earlier by
+// redactEndpoint, and otlploghttp itself prefers logging and falling back to
+// returning an error. An earlier version of this test used a malformed endpoint
+// and silently stopped covering the branch once that validation was added.
 func TestEnableOTELLogs_FailureDoesNotEchoTheEndpoint(t *testing.T) {
-	// Parses well enough to get past the configured-endpoint guard, and badly
-	// enough that the exporter refuses it.
-	const bad = "http://user:s3cr3t@bad host:99999/v1/logs"
-	t.Setenv("OTEL_EXPORTER_OTLP_LOGS_ENDPOINT", bad)
+	const endpoint = "https://user:s3cr3t@collector.example/v1/logs"
+	t.Setenv("OTEL_EXPORTER_OTLP_LOGS_ENDPOINT", endpoint)
 	t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "")
 
+	prevCtor := newLogExporter
+	t.Cleanup(func() { newLogExporter = prevCtor })
+	// Quotes the endpoint back, exactly as an OTLP endpoint error does.
+	newLogExporter = func(context.Context) (sdklog.Exporter, error) {
+		return nil, fmt.Errorf("invalid endpoint %q: nope", endpoint)
+	}
+
 	var stderr bytes.Buffer
 	prev := slog.Default()
 	slog.SetDefault(slog.New(slog.NewTextHandler(&stderr, &slog.HandlerOptions{Level: slog.LevelDebug})))
@@ -458,12 +472,15 @@ func TestEnableOTELLogs_FailureDoesNotEchoTheEndpoint(t *testing.T) {
 	if strings.Contains(out, "s3cr3t") {
 		t.Errorf("the endpoint's credential reached the journal: %q", out)
 	}
-	if strings.Contains(out, bad) {
+	if strings.Contains(out, endpoint) {
 		t.Errorf("the raw endpoint reached the journal: %q", out)
 	}
-	// Whatever happened, it has to be explained — silently doing nothing is the
-	// failure mode this whole file exists to avoid.
-	if !strings.Contains(out, "Log export") {
-		t.Errorf("nothing explained the outcome: %q", out)
+	// The diagnostic has to survive the sanitizing, or the operator learns
+	// nothing about why export is off.
+	if !strings.Contains(out, "could not build the OTLP log exporter") {
+		t.Errorf("the failure was not explained: %q", out)
+	}
+	if !strings.Contains(out, "collector.example/v1/logs") {
+		t.Errorf("the redacted endpoint is missing, so the line is not actionable: %q", out)
 	}
 }

egress/refusals_test.go

@@ -2,6 +2,7 @@ package egress
 
 import (
 	"fmt"
+	"net"
 	"net/http/httptest"
 	"runtime"
 	"strings"
@@ -592,3 +593,34 @@ func TestShouldLogRefusal_Concurrent(t *testing.T) {
 		t.Errorf("suppressed = %d, want %d — a concurrent increment was lost", suppressed, want)
 	}
 }
+
+// The forwarded address is what gets geolocated, not the socket's. Behind Caddy
+// RemoteAddr is always loopback, so a regression to geolocating it would report
+// every donor as unknown while still populating the attribute — which the
+// existence check above would not notice.
+func TestPeerAttrs_GeolocatesTheForwardedDonorNotTheProxy(t *testing.T) {
+	orig := lookupDonorGeo()
+	t.Cleanup(func() { setDonorGeo(orig) })
+	setDonorGeo(fakeCountryLookup{"203.0.113.7": "SE", "127.0.0.1": "ZZ"})
+
+	r := httptest.NewRequest("GET", "/ws", nil)
+	r.RemoteAddr = "127.0.0.1:54321"
+	r.Header.Set("X-Forwarded-For", "203.0.113.7")
+
+	kv := map[string]any{}
+	attrs := peerAttrs(r)
+	for i := 0; i < len(attrs); i += 2 {
+		kv[attrs[i].(string)] = attrs[i+1]
+	}
+	if got := kv["donor_country"]; got != "SE" {
+		t.Errorf("donor_country = %v, want SE — the proxy's address was geolocated instead of the donor's", got)
+	}
+}
+
+// fakeCountryLookup resolves the addresses a test names and nothing else.
+type fakeCountryLookup map[string]string
+
+func (f fakeCountryLookup) CountryCode(ip net.IP) string { return f[ip.String()] }
+func (f fakeCountryLookup) ISP(ip net.IP) string         { return "" }
+func (f fakeCountryLookup) ASN(ip net.IP) string         { return "" }
+func (f fakeCountryLookup) ASName(ip net.IP) string      { return "" }