fix(proxy): isolate and apply 30s timeout to upgrade proxy dialer (#4125)

This Pull Request resolves a critical P0 availability defect regarding
unbounded goroutine accumulation within the WebSocket and HTTP upgrade
proxy pathways under stalled backend conditions.

By replacing the blocking, context-less net.Dial and tls.Dial calls
inside dialBackend() with context-aware DialContext routines, the proxy
now correctly propagates req.Context() cancellation and enforces a
strict dial timeout.

The timeout has been fully wired into the proxy configuration factory,
defaulting to 30 seconds while allowing direct overrides via
Params.Timeout. Comprehensive regression and E2E timeout verification
tests have been integrated into proxy/upgrade_test.go.

---------

Signed-off-by: glatinone <93207632+glatinone@users.noreply.github.com>

Author: glatinone

config/config.go

@@ -300,6 +300,7 @@ type Config struct {
 	MaxHeaderBytes               int           `yaml:"max-header-bytes"`
 	EnableConnMetricsServer      bool          `yaml:"enable-connection-metrics"`
 	TimeoutBackend               time.Duration `yaml:"timeout-backend"`
+	UpgradeDialTimeout           time.Duration `yaml:"upgrade-dial-timeout"`
 	KeepaliveBackend             time.Duration `yaml:"keepalive-backend"`
 	EnableDualstackBackend       bool          `yaml:"enable-dualstack-backend"`
 	TlsHandshakeTimeoutBackend   time.Duration `yaml:"tls-timeout-backend"`
@@ -689,6 +690,7 @@ func NewConfig() *Config {
 	flag.IntVar(&cfg.MaxHeaderBytes, "max-header-bytes", http.DefaultMaxHeaderBytes, "set MaxHeaderBytes for http server connections")
 	flag.BoolVar(&cfg.EnableConnMetricsServer, "enable-connection-metrics", false, "enables connection metrics for http server connections")
 	flag.DurationVar(&cfg.TimeoutBackend, "timeout-backend", 60*time.Second, "sets the TCP client connection timeout for backend connections")
+	flag.DurationVar(&cfg.UpgradeDialTimeout, "upgrade-dial-timeout", 0, "sets the explicit connect-time ceiling for websocket/spdy upgrade backend connections. Zero falls back to the built-in 30s default; negative disables the ceiling entirely")
 	flag.DurationVar(&cfg.KeepaliveBackend, "keepalive-backend", 30*time.Second, "sets the keepalive for backend connections")
 	flag.BoolVar(&cfg.EnableDualstackBackend, "enable-dualstack-backend", true, "enables DualStack for backend connections")
 	flag.DurationVar(&cfg.TlsHandshakeTimeoutBackend, "tls-timeout-backend", 60*time.Second, "sets the TLS handshake timeout for backend connections")
@@ -1139,6 +1141,7 @@ func (c *Config) ToOptions() skipper.Options {
 		MaxHeaderBytes:               c.MaxHeaderBytes,
 		EnableConnMetricsServer:      c.EnableConnMetricsServer,
 		TimeoutBackend:               c.TimeoutBackend,
+		UpgradeDialTimeout:           c.UpgradeDialTimeout,
 		KeepAliveBackend:             c.KeepaliveBackend,
 		DualStackBackend:             c.EnableDualstackBackend,
 		TLSHandshakeTimeoutBackend:   c.TlsHandshakeTimeoutBackend,

proxy/proxy.go

@@ -342,6 +342,11 @@ type Params struct {
 	// Timeout sets the TCP client connection timeout for proxy http connections to the backend
 	Timeout time.Duration
 
+	// UpgradeDialTimeout sets the explicit connect-time ceiling for
+	// WebSocket/SPDY upgrade connections to the backend. Zero falls back
+	// to the built-in default; negative disables the ceiling entirely.
+	UpgradeDialTimeout time.Duration
+
 	// ResponseHeaderTimeout sets the HTTP response timeout for
 	// proxy http connections to the backend.
 	ResponseHeaderTimeout time.Duration
@@ -491,6 +496,7 @@ type Proxy struct {
 	upgradeAuditLogOut       io.Writer
 	upgradeAuditLogErr       io.Writer
 	auditLogHook             chan struct{}
+	upgradeDialTimeout       time.Duration
 	clientTLS                *tls.Config
 	hostname                 string
 	onPanicSometimes         rate.Sometimes
@@ -926,6 +932,7 @@ func WithParams(p Params) *Proxy {
 		flushInterval:            p.FlushInterval,
 		experimentalUpgrade:      p.ExperimentalUpgrade,
 		experimentalUpgradeAudit: p.ExperimentalUpgradeAudit,
+		upgradeDialTimeout:       p.UpgradeDialTimeout,
 		maxLoops:                 p.MaxLoopbacks,
 		breakers:                 p.CircuitBreakers,
 		limiters:                 p.RateLimiters,
@@ -1037,6 +1044,7 @@ func (p *Proxy) makeUpgradeRequest(ctx *context, req *http.Request) {
 		auditLogOut:     p.upgradeAuditLogOut,
 		auditLogErr:     p.upgradeAuditLogErr,
 		auditLogHook:    p.auditLogHook,
+		dialTimeout:     p.upgradeDialTimeout,
 	}
 
 	upgradeProxy.serveHTTP(ctx.responseWriter, req)

proxy/upgrade.go

@@ -13,10 +13,13 @@ import (
 	"net/http/httputil"
 	"net/url"
 	"strings"
+	"time"
 
 	log "github.com/sirupsen/logrus"
 )
 
+const defaultUpgradeDialTimeout = 30 * time.Second
+
 // isUpgradeRequest returns true if and only if there is a "Connection"
 // key with the value "Upgrade" in Headers of the given request.
 func isUpgradeRequest(req *http.Request) bool {
@@ -38,7 +41,6 @@ func getUpgradeRequest(req *http.Request) string {
 	return ""
 }
 
-// UpgradeProxy stores everything needed to make the connection upgrade.
 type upgradeProxy struct {
 	backendAddr     *url.URL
 	reverseProxy    *httputil.ReverseProxy
@@ -48,6 +50,24 @@ type upgradeProxy struct {
 	auditLogOut     io.Writer
 	auditLogErr     io.Writer
 	auditLogHook    chan struct{}
+	dialTimeout     time.Duration
+}
+
+// resolvedDialTimeout applies a strict 3-state backward-compatibility
+// contract for the configured dial timeout:
+//   - negative: the ceiling is disabled entirely (0), matching
+//     net.Dialer.Timeout == 0 semantics.
+//   - zero (unset): falls back to defaultUpgradeDialTimeout.
+//   - positive: used as configured.
+func (p *upgradeProxy) resolvedDialTimeout() time.Duration {
+	switch {
+	case p.dialTimeout < 0:
+		return 0
+	case p.dialTimeout == 0:
+		return defaultUpgradeDialTimeout
+	default:
+		return p.dialTimeout
+	}
 }
 
 // TODO: add user here
@@ -190,26 +210,32 @@ func (p *upgradeProxy) dialBackend(req *http.Request) (net.Conn, error) {
 
 	switch p.backendAddr.Scheme {
 	case "http":
-		return net.Dial("tcp", dialAddr)
+		return (&net.Dialer{Timeout: p.resolvedDialTimeout()}).DialContext(req.Context(), "tcp", dialAddr)
+
 	case "https":
-		tlsConn, err := tls.Dial("tcp", dialAddr, p.tlsClientConfig)
+		tlsDialer := &tls.Dialer{
+			NetDialer: &net.Dialer{Timeout: p.resolvedDialTimeout()},
+			Config:    p.tlsClientConfig,
+		}
+		conn, err := tlsDialer.DialContext(req.Context(), "tcp", dialAddr)
 		if err != nil {
 			return nil, err
 		}
 
 		if !p.insecure {
 			hostToVerify, _, err := net.SplitHostPort(dialAddr)
 			if err != nil {
+				conn.Close()
 				return nil, err
 			}
-			err = tlsConn.VerifyHostname(hostToVerify)
-			if err != nil {
-				tlsConn.Close()
+			if err = conn.(*tls.Conn).VerifyHostname(hostToVerify); err != nil {
+				conn.Close()
 				return nil, err
 			}
 		}
 
-		return tlsConn, nil
+		return conn, nil
+
 	default:
 		return nil, fmt.Errorf("unknown scheme: %s", p.backendAddr.Scheme)
 	}

proxy/upgrade_test.go

@@ -3,6 +3,8 @@ package proxy
 import (
 	"bufio"
 	"bytes"
+	stdlibcontext "context"
+	"crypto/tls"
 	"fmt"
 	"io"
 	"math/rand"
@@ -479,3 +481,213 @@ func TestAuditLogging(t *testing.T) {
 		}
 	}))
 }
+
+func TestResolvedDialTimeoutDefault(t *testing.T) {
+	p := getUpgradeProxy() // dialTimeout is zero-value
+	got := p.resolvedDialTimeout()
+	if got != defaultUpgradeDialTimeout {
+		t.Errorf("resolvedDialTimeout() = %v; want defaultUpgradeDialTimeout (%v)",
+			got, defaultUpgradeDialTimeout)
+	}
+}
+
+func TestResolvedDialTimeoutConfigured(t *testing.T) {
+	const want = 5 * time.Second
+	u, _ := url.ParseRequestURI("http://127.0.0.1:8080/foo")
+	p := &upgradeProxy{
+		backendAddr: u,
+		dialTimeout: want,
+	}
+	if got := p.resolvedDialTimeout(); got != want {
+		t.Errorf("resolvedDialTimeout() = %v; want %v", got, want)
+	}
+}
+
+func TestResolvedDialTimeoutNegativeDisablesTimeout(t *testing.T) {
+	u, _ := url.ParseRequestURI("http://127.0.0.1:8080/foo")
+	p := &upgradeProxy{
+		backendAddr: u,
+		dialTimeout: -1 * time.Second,
+	}
+	if got := p.resolvedDialTimeout(); got != 0 {
+		t.Errorf("resolvedDialTimeout() = %v; want 0 (ceiling disabled)", got)
+	}
+}
+
+func TestDialBackendHTTPSTimesOutOnStalledHandshake(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	defer ln.Close()
+
+	done := make(chan struct{})
+	t.Cleanup(func() { close(done) })
+
+	accepted := make(chan struct{})
+	go func() {
+		conn, err := ln.Accept()
+		if err != nil {
+			return
+		}
+		close(accepted)
+		defer conn.Close()
+		// Stall: hold the TCP connection open but never write TLS data,
+		// bounded by the test's own lifetime via done.
+		select {
+		case <-done:
+			return
+		case <-time.After(30 * time.Second):
+		}
+	}()
+
+	const dialTimeout = 250 * time.Millisecond
+
+	backendURL, _ := url.Parse("https://" + ln.Addr().String())
+	p := &upgradeProxy{
+		backendAddr: backendURL,
+		/* #nosec G402 – test-only, no production TLS */
+		tlsClientConfig: &tls.Config{InsecureSkipVerify: true},
+		insecure:        true,
+		dialTimeout:     dialTimeout,
+	}
+
+	req, err := http.NewRequestWithContext(
+		stdlibcontext.Background(),
+		http.MethodGet,
+		"https://"+ln.Addr().String()+"/ws",
+		nil,
+	)
+	require.NoError(t, err)
+	req.URL = backendURL
+
+	start := time.Now()
+	conn, err := p.dialBackend(req)
+	elapsed := time.Since(start)
+
+	select {
+	case <-accepted:
+	case <-time.After(2 * time.Second):
+		t.Fatal("backend never accepted TCP connection — test precondition failed")
+	}
+
+	require.Error(t, err, "dialBackend to a stalled TLS backend must return an error")
+	assert.Nil(t, conn, "conn must be nil on dial timeout")
+
+	assert.GreaterOrEqual(t, elapsed, dialTimeout,
+		"dial returned before dialTimeout (%v); got %v — timeout may be wired to zero", dialTimeout, elapsed)
+	assert.Less(t, elapsed, 3*dialTimeout,
+		"dial took %v, expected < 3×dialTimeout (%v) — default 30 s may have been used instead", elapsed, 3*dialTimeout)
+}
+
+func TestDialBackendRespectsContextCancellation(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	defer ln.Close()
+
+	done := make(chan struct{})
+	t.Cleanup(func() { close(done) })
+
+	go func() {
+		conn, err := ln.Accept()
+		if err != nil {
+			return
+		}
+		defer conn.Close()
+		// stall TLS handshake, bounded by the test's own lifetime via done.
+		select {
+		case <-done:
+			return
+		case <-time.After(30 * time.Second):
+		}
+	}()
+
+	backendURL, _ := url.Parse("https://" + ln.Addr().String())
+	p := &upgradeProxy{
+		backendAddr: backendURL,
+		/* #nosec G402 – test-only */
+		tlsClientConfig: &tls.Config{InsecureSkipVerify: true},
+		insecure:        true,
+		dialTimeout:     10 * time.Second, // long hard ceiling — context fires first
+	}
+
+	const ctxTimeout = 150 * time.Millisecond
+	ctx, cancel := stdlibcontext.WithTimeout(stdlibcontext.Background(), ctxTimeout)
+	defer cancel()
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet,
+		"https://"+ln.Addr().String()+"/ws", nil)
+	require.NoError(t, err)
+	req.URL = backendURL
+
+	start := time.Now()
+	_, err = p.dialBackend(req)
+	elapsed := time.Since(start)
+
+	require.Error(t, err, "dialBackend must fail when context is cancelled")
+	assert.Less(t, elapsed, 3*ctxTimeout,
+		"dial must abort close to context deadline (%v), not wait for dialTimeout (10 s); took %v",
+		ctxTimeout, elapsed)
+}
+
+func TestDialBackendTimeoutViaParams(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	defer ln.Close()
+
+	done := make(chan struct{})
+	t.Cleanup(func() { close(done) })
+
+	go func() {
+		conn, err := ln.Accept()
+		if err != nil {
+			return
+		}
+		defer conn.Close()
+		select {
+		case <-done:
+			return
+		case <-time.After(30 * time.Second):
+		}
+	}()
+
+	const customTimeout = 300 * time.Millisecond
+	backendAddr := "https://" + ln.Addr().String()
+	routes := fmt.Sprintf(`route: Path("/ws") -> "%s";`, backendAddr)
+
+	tp, err := newTestProxyWithParams(routes, Params{
+		ExperimentalUpgrade: true,
+		UpgradeDialTimeout:  customTimeout,
+	})
+	require.NoError(t, err)
+	defer tp.close()
+
+	skipper := httptest.NewServer(tp.proxy)
+	defer skipper.Close()
+
+	skipperURL, _ := url.Parse(skipper.URL)
+	clientConn, err := net.Dial("tcp", skipperURL.Host)
+	require.NoError(t, err)
+	defer clientConn.Close()
+
+	u, _ := url.ParseRequestURI("wss://www.example.org/ws")
+	r := &http.Request{
+		URL:    u,
+		Method: http.MethodGet,
+		Header: http.Header{
+			"Connection": []string{"Upgrade"},
+			"Upgrade":    []string{"websocket"},
+		},
+	}
+	require.NoError(t, r.Write(clientConn))
+
+	start := time.Now()
+	reader := bufio.NewReader(clientConn)
+	resp, err := http.ReadResponse(reader, r)
+	elapsed := time.Since(start)
+	require.NoError(t, err)
+
+	assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode,
+		"stalled backend must yield 503 ServiceUnavailable")
+	assert.Less(t, elapsed, 3*customTimeout,
+		"proxy must fail within 3×Params.Timeout (%v); took %v — wiring may be broken",
+		customTimeout, elapsed)
+}

skipper.go

@@ -450,6 +450,11 @@ type Options struct {
 	// proxy http connections to the backend.
 	TimeoutBackend time.Duration
 
+	// UpgradeDialTimeout sets the explicit connect-time ceiling for
+	// WebSocket/SPDY upgrade connections to the backend. Zero falls back
+	// to the built-in default; negative disables the ceiling entirely.
+	UpgradeDialTimeout time.Duration
+
 	// ResponseHeaderTimeout sets the HTTP response timeout for
 	// proxy http connections to the backend.
 	ResponseHeaderTimeoutBackend time.Duration
@@ -2513,6 +2518,7 @@ func run(o Options, sig chan os.Signal, idleConnsCH chan struct{}) error {
 		MaxLoopbacks:                     o.MaxLoopbacks,
 		DefaultHTTPStatus:                o.DefaultHTTPStatus,
 		Timeout:                          o.TimeoutBackend,
+		UpgradeDialTimeout:               o.UpgradeDialTimeout,
 		ResponseHeaderTimeout:            o.ResponseHeaderTimeoutBackend,
 		ExpectContinueTimeout:            o.ExpectContinueTimeoutBackend,
 		KeepAlive:                        o.KeepAliveBackend,