From 7c68809f4e13a7ac7c9e216a69410e7c1966c1df Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Tue, 15 Jun 2021 14:54:48 -0600
Subject: [PATCH] reverseproxy: Fix overwriting of max_idle_conns_per_host
 (closes #4201)

Also split the Caddyfile subdirective keepalive_idle_conns into two properties so the conns and conns_per_host can be set separately.

This is technically a breaking change, but probably anyone who this breaks already had a broken config anyway, and silently fixing it won't help them fix their configs.
---
 modules/caddyhttp/reverseproxy/caddyfile.go   | 22 ++++++++++---------
 .../caddyhttp/reverseproxy/httptransport.go   | 10 +++------
 .../caddyhttp/reverseproxy/reverseproxy.go    |  2 +-
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go
index 61eac7ed..8bbdf5e8 100644
--- a/modules/caddyhttp/reverseproxy/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/caddyfile.go
@@ -978,6 +978,18 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 					h.KeepAlive = new(KeepAlive)
 				}
 				h.KeepAlive.MaxIdleConns = num
+
+			case "keepalive_idle_conns_per_host":
+				if !d.NextArg() {
+					return d.ArgErr()
+				}
+				num, err := strconv.Atoi(d.Val())
+				if err != nil {
+					return d.Errf("bad integer value '%s': %v", d.Val(), err)
+				}
+				if h.KeepAlive == nil {
+					h.KeepAlive = new(KeepAlive)
+				}
 				h.KeepAlive.MaxIdleConnsPerHost = num
 
 			case "versions":
@@ -1004,16 +1016,6 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				}
 				h.MaxConnsPerHost = num
 
-			case "max_idle_conns_per_host":
-				if !d.NextArg() {
-					return d.ArgErr()
-				}
-				num, err := strconv.Atoi(d.Val())
-				if err != nil {
-					return d.Errf("bad integer value '%s': %v", d.Val(), err)
-				}
-				h.MaxIdleConnsPerHost = num
-
 			default:
 				return d.Errf("unrecognized subdirective %s", d.Val())
 			}
diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go
index fdaf56e1..f217a4ba 100644
--- a/modules/caddyhttp/reverseproxy/httptransport.go
+++ b/modules/caddyhttp/reverseproxy/httptransport.go
@@ -62,9 +62,6 @@ type HTTPTransport struct {
 	// Maximum number of connections per host. Default: 0 (no limit)
 	MaxConnsPerHost int `json:"max_conns_per_host,omitempty"`
 
-	// Maximum number of idle connections per host. Default: 0 (uses Go's default of 2)
-	MaxIdleConnsPerHost int `json:"max_idle_conns_per_host,omitempty"`
-
 	// How long to wait before timing out trying to connect to
 	// an upstream.
 	DialTimeout caddy.Duration `json:"dial_timeout,omitempty"`
@@ -197,7 +194,6 @@ func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error)
 			return conn, nil
 		},
 		MaxConnsPerHost:        h.MaxConnsPerHost,
-		MaxIdleConnsPerHost:    h.MaxIdleConnsPerHost,
 		ResponseHeaderTimeout:  time.Duration(h.ResponseHeaderTimeout),
 		ExpectContinueTimeout:  time.Duration(h.ExpectContinueTimeout),
 		MaxResponseHeaderBytes: h.MaxResponseHeaderSize,
@@ -412,13 +408,13 @@ type KeepAlive struct {
 	// How often to probe for liveness.
 	ProbeInterval caddy.Duration `json:"probe_interval,omitempty"`
 
-	// Maximum number of idle connections.
+	// Maximum number of idle connections. Default: 0, which means no limit.
 	MaxIdleConns int `json:"max_idle_conns,omitempty"`
 
-	// Maximum number of idle connections per upstream host.
+	// Maximum number of idle connections per host. Default: 0, which uses Go's default of 2.
 	MaxIdleConnsPerHost int `json:"max_idle_conns_per_host,omitempty"`
 
-	// How long connections should be kept alive when idle.
+	// How long connections should be kept alive when idle. Default: 0, which means no timeout.
 	IdleConnTimeout caddy.Duration `json:"idle_timeout,omitempty"`
 }
 
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index dbbf6471..6b217e1a 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -204,7 +204,7 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 			KeepAlive: &KeepAlive{
 				ProbeInterval:       caddy.Duration(30 * time.Second),
 				IdleConnTimeout:     caddy.Duration(2 * time.Minute),
-				MaxIdleConnsPerHost: 32,
+				MaxIdleConnsPerHost: 32, // seems about optimal, see #2805
 			},
 			DialTimeout: caddy.Duration(10 * time.Second),
 		}