From 7aa34a824698facf1f92ea28c3c8ada285b044e5 Mon Sep 17 00:00:00 2001 From: VijayabaskarR-06 Date: Wed, 20 May 2026 23:07:07 +0530 Subject: [PATCH] backend: Reuse a configured HTTP client in /externalproxy The /externalproxy handler created a new http.Client{} on every request, with no timeout and the default transport. A slow or hanging upstream would block the request goroutine indefinitely, leaking goroutines and file descriptors, and the default per-host connection pool meant TCP connections were rebuilt instead of reused under load. Replace the per-request client with a single shared package-level http.Client backed by a tuned http.Transport: ResponseHeaderTimeout cuts off stalling upstreams, dial / TLS / expect-continue timeouts cover handshake failures, and a larger idle-connection pool lets Keep-Alive reuse sockets. The shared client also stops following redirects. The default client follows 30x responses automatically, so an allowed upstream could return a redirect to a disallowed or internal address and the client would fetch it server-side, bypassing the /externalproxy allowlist. CheckRedirect returns http.ErrUseLastResponse so the 30x response is handed back to the handler, which forwards its status and Location header to the caller; the redirect can still be followed client-side but is never chased server-side. Client.Timeout is deliberately not set: it includes body read time and would break legitimate long streaming responses. The transport-level timeouts protect against the slow-headers case without that side effect. Tests cover the behaviours: TestExternalProxyResponseHeaderTimeout drives a slow upstream and confirms the proxy returns 502 fast, TestExternalProxyReusesConnections asserts the upstream sees roughly one new TCP connection, and TestExternalProxyDoesNotFollowRedirects confirms a redirect target outside the allowlist is never fetched and that the Location header is forwarded so the caller can still follow it. --- backend/cmd/headlamp.go | 44 +++++++++- backend/cmd/headlamp_test.go | 166 +++++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 3 deletions(-) diff --git a/backend/cmd/headlamp.go b/backend/cmd/headlamp.go index 6e7c7f70863..dd1b8757179 100644 --- a/backend/cmd/headlamp.go +++ b/backend/cmd/headlamp.go @@ -160,6 +160,40 @@ const ( TokenCacheFileName = "headlamp-token-cache" ) +// externalProxyClient is the shared HTTP client used by the /externalproxy +// handler. It is configured with transport-level timeouts so a slow or hung +// upstream cannot block goroutines and exhaust file descriptors, and with a +// larger per-host idle-connection pool than the net/http default so +// connections can be reused under load. +// +// Redirects are not followed: an allowed upstream could otherwise return a +// 30x pointing at a disallowed or internal address, which the client would +// fetch server-side and bypass the /externalproxy allowlist. Returning +// http.ErrUseLastResponse hands the 30x response back to the handler, which +// forwards its status and Location header to the caller so the redirect can +// still be followed client-side. +// +//nolint:gochecknoglobals // shared client reused across all proxy requests +var externalProxyClient = &http.Client{ + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + DialContext: (&net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + }).DialContext, + ForceAttemptHTTP2: true, + MaxIdleConns: 100, + MaxIdleConnsPerHost: 10, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + ResponseHeaderTimeout: 30 * time.Second, + }, +} + type clientConfig struct { Clusters []Cluster `json:"clusters"` IsDynamicClusterEnabled bool `json:"isDynamicClusterEnabled"` @@ -742,9 +776,7 @@ func createHeadlampHandler(ctx context.Context, config *HeadlampConfig) http.Han w.Header().Set("Pragma", "no-cache") w.Header().Set("X-Accel-Expires", "0") - client := http.Client{} - - resp, err := client.Do(proxyReq) //nolint:gosec + resp, err := externalProxyClient.Do(proxyReq) //nolint:gosec if err != nil { logger.Log(logger.LevelError, nil, err, "making request") http.Error(w, err.Error(), http.StatusBadGateway) @@ -796,6 +828,12 @@ func createHeadlampHandler(ctx context.Context, config *HeadlampConfig) http.Han return } + // Forward Location so the caller can act on redirect (30x) responses, + // which are passed through unfollowed rather than chased server-side. + if location := resp.Header.Get("Location"); location != "" { + w.Header().Set("Location", location) + } + w.WriteHeader(resp.StatusCode) // Stream the body, putting the already-read first chunk back in front of diff --git a/backend/cmd/headlamp_test.go b/backend/cmd/headlamp_test.go index 6ce3a6c3beb..62de436cf12 100644 --- a/backend/cmd/headlamp_test.go +++ b/backend/cmd/headlamp_test.go @@ -36,6 +36,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "testing" "time" @@ -749,6 +750,171 @@ func TestExternalProxyStreamsLargeBody(t *testing.T) { } } +func TestExternalProxyDoesNotFollowRedirects(t *testing.T) { + var secretHits int32 + + // A server that must never be reached server-side: it stands in for a + // disallowed or internal address a redirect could try to point at. + secret := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&secretHits, 1) + w.WriteHeader(http.StatusOK) + })) + defer secret.Close() + + // The allowed upstream redirects to the secret server. + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, secret.URL, http.StatusFound) + })) + defer upstream.Close() + + handler := newExternalProxyHandler(t, upstream.URL) + + req, err := http.NewRequestWithContext(context.Background(), "GET", "/externalproxy", nil) + if err != nil { + t.Fatal(err) + } + + req.Header.Set("proxy-to", upstream.URL) + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusFound { + t.Errorf("status code = %d, want %d (the redirect must be forwarded, not followed)", + rr.Code, http.StatusFound) + } + + if hits := atomic.LoadInt32(&secretHits); hits != 0 { + t.Errorf("redirect target was fetched %d time(s); the allowlist must not be bypassable via redirects", + hits) + } + + if location := rr.Header().Get("Location"); location != secret.URL { + t.Errorf("Location header = %q, want %q (the redirect must stay usable client-side)", + location, secret.URL) + } +} + +func TestExternalProxyResponseHeaderTimeout(t *testing.T) { + originalTransport := externalProxyClient.Transport + + baseTransport, ok := originalTransport.(*http.Transport) + if !ok { + t.Fatalf("externalProxyClient.Transport = %T, want *http.Transport", originalTransport) + } + + clonedTransport := baseTransport.Clone() + clonedTransport.ResponseHeaderTimeout = 200 * time.Millisecond + externalProxyClient.Transport = clonedTransport + + defer func() { + externalProxyClient.CloseIdleConnections() + externalProxyClient.Transport = originalTransport + externalProxyClient.CloseIdleConnections() + }() + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(2 * time.Second) + w.WriteHeader(http.StatusOK) + })) + defer upstream.Close() + + handler := newExternalProxyHandler(t, upstream.URL) + + srv := httptest.NewServer(handler) + defer srv.Close() + + // A per-request deadline keeps the suite fail-fast: if the handler ever + // regresses and blocks, the request errors out instead of hanging until + // the overall go test timeout. + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, "GET", srv.URL+"/externalproxy", nil) + if err != nil { + t.Fatal(err) + } + + req.Header.Set("proxy-to", upstream.URL) + + start := time.Now() + + resp, err := http.DefaultClient.Do(req) + elapsed := time.Since(start) + + if err != nil { + t.Fatal(err) + } + + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusBadGateway { + t.Errorf("status code = %d, want %d (timeout should produce 502)", + resp.StatusCode, http.StatusBadGateway) + } + + if elapsed > 1*time.Second { + t.Errorf("request took %v, expected to fail fast (well under upstream's 2s sleep)", elapsed) + } +} + +func TestExternalProxyReusesConnections(t *testing.T) { + var connCount int32 + + upstream := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + + _, _ = w.Write([]byte("OK")) + })) + + upstream.Config.ConnState = func(c net.Conn, s http.ConnState) { + if s == http.StateNew { + atomic.AddInt32(&connCount, 1) + } + } + + upstream.Start() + defer upstream.Close() + + handler := newExternalProxyHandler(t, upstream.URL) + + srv := httptest.NewServer(handler) + defer srv.Close() + + for i := 0; i < 5; i++ { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + + req, err := http.NewRequestWithContext(ctx, "GET", srv.URL+"/externalproxy", nil) + if err != nil { + cancel() + t.Fatal(err) + } + + req.Header.Set("proxy-to", upstream.URL) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + cancel() + t.Fatal(err) + } + + if resp.StatusCode != http.StatusOK { + t.Errorf("request %d: status code = %d, want %d", i, resp.StatusCode, http.StatusOK) + } + + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + + cancel() + } + + got := atomic.LoadInt32(&connCount) + if got > 2 { + t.Errorf("upstream saw %d new connections across 5 sequential requests, "+ + "expected ~1 with Keep-Alive enabled", got) + } +} + func TestExternalProxyTimeout(t *testing.T) { originalLimit := externalProxyTimeout externalProxyTimeout = 50 * time.Millisecond