admin
ede49b610d
gates / gates (push) Successful in 7s
Every client here pins TLS, so none can use http.DefaultTransport and each hand-rolls its own. A composite literal takes IdleConnTimeout ZERO, which means retain idle connections forever -- not "use a sane default". pbsTargetsFromPVE builds a fresh pbs.Client every cycle and drops the previous one, and an abandoned http.Transport does not close its connections. One stranded socket per cycle, on both sides, forever. Measured: 388 established connections on ep0 over 46 h, 194 per box, zero closed in a 31-minute window. pvestatd and proxmox-backup-client made 162,404 requests in the same window and leaked none. New leaf package internal/httpx owns the default (90s, http.DefaultTransport's own value) and NewTransport, which returns a FRESH transport per call and treats <=0 as "use the default", never "no timeout". pbs.Config gains IdleConnTimeout for tests only. hub and proxmox carried the same missing default and are corrected here for consistency. Neither contributed to the ep0 leak -- both are built once per process and neither talks to ep0:8007. Tests count connections SERVER-side and model the abandonment, so they pin the consequence rather than the field. Two red-proofs, both seen failing: removing the timeout -> "still holds 5 open connection(s), want 0"; DisableKeepAlives -> "3 sequential requests over 3 connection(s), want 1" (the leak test PASSES under that one -- it is the worse-fix guard that catches it). Not released: hand-installed on demo-hp only so demo-felhom stays the control. CHANGELOG heading stays UNRELEASED until the publish is authorised.
75 lines
3.0 KiB
Go
75 lines
3.0 KiB
Go
package httpx
|
|
|
|
import (
|
|
"crypto/tls"
|
|
"net/http"
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
// TestNewTransport_ZeroMeansDefaultNeverForever is the whole point of this package.
|
|
//
|
|
// http.Transport's zero IdleConnTimeout means "retain idle connections FOREVER". Any code path that
|
|
// can reach that zero reintroduces R-344, so an unset, zero or negative value must all land on the
|
|
// default. If someone later "simplifies" NewTransport by passing the argument straight through,
|
|
// this fails.
|
|
func TestNewTransport_ZeroMeansDefaultNeverForever(t *testing.T) {
|
|
for _, tc := range []struct {
|
|
name string
|
|
in time.Duration
|
|
want time.Duration
|
|
}{
|
|
{"zero", 0, DefaultIdleConnTimeout},
|
|
{"negative", -time.Hour, DefaultIdleConnTimeout},
|
|
{"explicit short value (tests)", 50 * time.Millisecond, 50 * time.Millisecond},
|
|
{"explicit long value", time.Hour, time.Hour},
|
|
} {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := NewTransport(&tls.Config{MinVersion: tls.VersionTLS12}, tc.in).IdleConnTimeout
|
|
if got != tc.want {
|
|
t.Fatalf("IdleConnTimeout = %v, want %v", got, tc.want)
|
|
}
|
|
if got == 0 {
|
|
t.Fatal("IdleConnTimeout is 0 — that is 'never expire', which is the R-344 defect itself")
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestDefaultIdleConnTimeout_MatchesTheStandardLibrary pins the number to its justification.
|
|
//
|
|
// 90s is not a tuned value; it is what http.DefaultTransport uses. Reading it off the standard
|
|
// library rather than hardcoding 90 means the constant cannot drift away from the reason given for
|
|
// it in the package doc.
|
|
func TestDefaultIdleConnTimeout_MatchesTheStandardLibrary(t *testing.T) {
|
|
std, ok := http.DefaultTransport.(*http.Transport)
|
|
if !ok {
|
|
t.Skip("http.DefaultTransport is not an *http.Transport in this Go build")
|
|
}
|
|
if DefaultIdleConnTimeout != std.IdleConnTimeout {
|
|
t.Fatalf("DefaultIdleConnTimeout = %v but http.DefaultTransport uses %v — the doc comment's justification no longer holds",
|
|
DefaultIdleConnTimeout, std.IdleConnTimeout)
|
|
}
|
|
}
|
|
|
|
// TestNewTransport_IsFreshEveryCall guards the pooling property the pinning relies on.
|
|
//
|
|
// Each caller pins a DIFFERENT endpoint. A shared transport would pool connections across
|
|
// differently pinned servers, so returning a package-level singleton would be a security change
|
|
// dressed as a tidy-up.
|
|
func TestNewTransport_IsFreshEveryCall(t *testing.T) {
|
|
a := NewTransport(&tls.Config{MinVersion: tls.VersionTLS12}, 0)
|
|
b := NewTransport(&tls.Config{MinVersion: tls.VersionTLS12}, 0)
|
|
if a == b {
|
|
t.Fatal("NewTransport returned the SAME transport twice — connections would be pooled across differently pinned endpoints")
|
|
}
|
|
}
|
|
|
|
// TestNewTransport_KeepsTheTLSConfig — the transport gains a field; it must lose nothing.
|
|
func TestNewTransport_KeepsTheTLSConfig(t *testing.T) {
|
|
cfg := &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} //nolint:gosec // test only
|
|
if got := NewTransport(cfg, 0).TLSClientConfig; got != cfg {
|
|
t.Fatalf("TLSClientConfig = %p, want the config passed in (%p) — the pin would be dropped", got, cfg)
|
|
}
|
|
}
|