ipn/ipnlocal: maintain a proxy handler per backend (#6804)

By default, `http.Transport` keeps idle connections open hoping to re-use them in the future. Combined with a separate transport per request in HTTP proxy this results in idle connection leak.

Fixes #6773

ipn/ipnlocal/local.go

@@ -13,6 +13,7 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"net/http/httputil"
 	"net/netip"
 	"net/url"
 	"os"
@@ -206,7 +207,8 @@ type LocalBackend struct {
 	lastServeConfJSON mem.RO              // last JSON that was parsed into serveConfig
 	serveConfig       ipn.ServeConfigView // or !Valid if none
 
-	serveListeners map[netip.AddrPort]*serveListener // addrPort => serveListener
+	serveListeners     map[netip.AddrPort]*serveListener // addrPort => serveListener
+	serveProxyHandlers sync.Map                          // string (HTTPHandler.Proxy) => *httputil.ReverseProxy
 
 	// statusLock must be held before calling statusChanged.Wait() or
 	// statusChanged.Broadcast().
@@ -3773,6 +3775,9 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn.
 			return true
 		})
 		handlePorts = append(handlePorts, servePorts...)
+
+		b.setServeProxyHandlersLocked()
+
 		// don't listen on netmap addresses if we're in userspace mode
 		if !wgengine.IsNetstack(b.e) {
 			b.updateServeTCPPortNetMapAddrListenersLocked(servePorts)
@@ -3788,6 +3793,49 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn.
 	b.setTCPPortsIntercepted(handlePorts)
 }
 
+// setServeProxyHandlersLocked ensures there is an http proxy handler for each
+// backend specified in serveConfig. It expects serveConfig to be valid and
+// up-to-date, so should be called after reloadServeConfigLocked.
+func (b *LocalBackend) setServeProxyHandlersLocked() {
+	if !b.serveConfig.Valid() {
+		return
+	}
+	var backends map[string]bool
+	b.serveConfig.Web().Range(func(_ ipn.HostPort, conf ipn.WebServerConfigView) (cont bool) {
+		conf.Handlers().Range(func(_ string, h ipn.HTTPHandlerView) (cont bool) {
+			backend := h.Proxy()
+			mak.Set(&backends, backend, true)
+			if _, ok := b.serveProxyHandlers.Load(backend); ok {
+				return true
+			}
+
+			b.logf("serve: creating a new proxy handler for %s", backend)
+			p, err := b.proxyHandlerForBackend(backend)
+			if err != nil {
+				// The backend endpoint (h.Proxy) should have been validated by expandProxyTarget
+				// in the CLI, so just log the error here.
+				b.logf("[unexpected] could not create proxy for %v: %s", backend, err)
+				return true
+			}
+			b.serveProxyHandlers.Store(backend, p)
+			return true
+		})
+		return true
+	})
+
+	// Clean up handlers for proxy backends that are no longer present
+	// in configuration.
+	b.serveProxyHandlers.Range(func(key, value any) bool {
+		backend := key.(string)
+		if !backends[backend] {
+			b.logf("serve: closing idle connections to %s", backend)
+			value.(*httputil.ReverseProxy).Transport.(*http.Transport).CloseIdleConnections()
+			b.serveProxyHandlers.Delete(backend)
+		}
+		return true
+	})
+}
+
 // operatorUserName returns the current pref's OperatorUser's name, or the
 // empty string if none.
 func (b *LocalBackend) operatorUserName() string {

ipn/ipnlocal/serve.go

@@ -211,7 +211,6 @@ func (b *LocalBackend) SetServeConfig(config *ipn.ServeConfig) error {
 	}
 
 	b.setTCPPortsInterceptedFromNetmapAndPrefsLocked(b.pm.CurrentPrefs())
-
 	return nil
 }
 
@@ -389,6 +388,30 @@ func (b *LocalBackend) getServeHandler(r *http.Request) (_ ipn.HTTPHandlerView,
 	}
 }
 
+// proxyHandlerForBackend creates a new HTTP reverse proxy for a particular backend that
+// we serve requests for. `backend` is a HTTPHandler.Proxy string (url, hostport or just port).
+func (b *LocalBackend) proxyHandlerForBackend(backend string) (*httputil.ReverseProxy, error) {
+	targetURL, insecure := expandProxyArg(backend)
+	u, err := url.Parse(targetURL)
+	if err != nil {
+		return nil, fmt.Errorf("invalid url %s: %w", targetURL, err)
+	}
+	rp := httputil.NewSingleHostReverseProxy(u)
+	rp.Transport = &http.Transport{
+		DialContext: b.dialer.SystemDial,
+		TLSClientConfig: &tls.Config{
+			InsecureSkipVerify: insecure,
+		},
+		// Values for the following parameters have been copied from http.DefaultTransport.
+		ForceAttemptHTTP2:     true,
+		MaxIdleConns:          100,
+		IdleConnTimeout:       90 * time.Second,
+		TLSHandshakeTimeout:   10 * time.Second,
+		ExpectContinueTimeout: 1 * time.Second,
+	}
+	return rp, nil
+}
+
 func (b *LocalBackend) serveWebHandler(w http.ResponseWriter, r *http.Request) {
 	h, mountPoint, ok := b.getServeHandler(r)
 	if !ok {
@@ -405,23 +428,12 @@ func (b *LocalBackend) serveWebHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if v := h.Proxy(); v != "" {
-		// TODO(bradfitz): this is a lot of setup per HTTP request. We should
-		// build the whole http.Handler with all the muxing and child handlers
-		// only on start/config change. But this works for now (2022-11-09).
-		targetURL, insecure := expandProxyArg(v)
-		u, err := url.Parse(targetURL)
-		if err != nil {
-			http.Error(w, "bad proxy config", http.StatusInternalServerError)
+		p, ok := b.serveProxyHandlers.Load(v)
+		if !ok {
+			http.Error(w, "unknown proxy destination", http.StatusInternalServerError)
 			return
 		}
-		rp := httputil.NewSingleHostReverseProxy(u)
-		rp.Transport = &http.Transport{
-			DialContext: b.dialer.SystemDial,
-			TLSClientConfig: &tls.Config{
-				InsecureSkipVerify: insecure,
-			},
-		}
-		rp.ServeHTTP(w, r)
+		p.(http.Handler).ServeHTTP(w, r)
 		return
 	}