reverseproxy: Incorporate latest proxy changes from stdlib (#4266)

I went through the commits that touched stdlib's `reverseproxy.go` file, and copied over all the changes that are to code that was copied into Caddy.

The commits I pulled changes from:

- https://github.com/golang/go/commit/2cc347382f4df3fb40d8d81ec9331f0748b1c394
- https://github.com/golang/go/commit/a5cea062b305c8502bdc959c0eec279dbcd4391f
- https://github.com/golang/go/commit/ecdbffd4ec68b509998792f120868fec319de59b
- https://github.com/golang/go/commit/21898524f66c075d7cfb64a38f17684140e57675
-https://github.com/golang/go/commit/ca3c0df1f8e07337ba4048b191bf905118ebe251
- https://github.com/golang/go/commit/9c017ff30dd21bbdcdb11f39458d3944db530d7e

This may also fix https://github.com/caddyserver/caddy/issues/4247 because of the change to `copyResponse` to set `mlw.flushPending = true` right away.

2 files changed, 37 insertions, 15 deletions

diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 6b217e1ae..633cc657f 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -23,6 +23,7 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"net/textproto"
 	"net/url"
 	"regexp"
 	"strconv"
@@ -80,10 +81,13 @@ type Handler struct {
 	// Upstreams is the list of backends to proxy to.
 	Upstreams UpstreamPool `json:"upstreams,omitempty"`
 
-	// Adjusts how often to flush the response buffer. A
-	// negative value disables response buffering.
-	// TODO: figure out good defaults and write docs for this
-	// (see https://github.com/caddyserver/caddy/issues/1460)
+	// Adjusts how often to flush the response buffer. By default,
+	// no periodic flushing is done. A negative value disables
+	// response buffering, and flushes immediately after each
+	// write to the client. This option is ignored when the upstream's
+	// response is recognized as a streaming response, or if its
+	// content length is -1; for such responses, writes are flushed
+	// to the client immediately.
 	FlushInterval caddy.Duration `json:"flush_interval,omitempty"`
 
 	// Headers manipulates headers between Caddy and the backend.
@@ -528,13 +532,19 @@ func (h Handler) prepareRequest(req *http.Request) error {
 		// If we aren't the first proxy retain prior
 		// X-Forwarded-For information as a comma+space
 		// separated list and fold multiple headers into one.
-		if prior, ok := req.Header["X-Forwarded-For"]; ok {
+		prior, ok := req.Header["X-Forwarded-For"]
+		omit := ok && prior == nil // Issue 38079: nil now means don't populate the header
+		if len(prior) > 0 {
 			clientIP = strings.Join(prior, ", ") + ", " + clientIP
 		}
-		req.Header.Set("X-Forwarded-For", clientIP)
+		if !omit {
+			req.Header.Set("X-Forwarded-For", clientIP)
+		}
 	}
 
-	if req.Header.Get("X-Forwarded-Proto") == "" {
+	prior, ok := req.Header["X-Forwarded-Proto"]
+	omit := ok && prior == nil
+	if len(prior) == 0 && !omit {
 		// set X-Forwarded-Proto; many backend apps expect this too
 		proto := "https"
 		if req.TLS == nil {
@@ -827,10 +837,10 @@ func upgradeType(h http.Header) string {
 // removeConnectionHeaders removes hop-by-hop headers listed in the "Connection" header of h.
 // See RFC 7230, section 6.1
 func removeConnectionHeaders(h http.Header) {
-	if c := h.Get("Connection"); c != "" {
-		for _, f := range strings.Split(c, ",") {
-			if f = strings.TrimSpace(f); f != "" {
-				h.Del(f)
+	for _, f := range h["Connection"] {
+		for _, sf := range strings.Split(f, ",") {
+			if sf = textproto.TrimString(sf); sf != "" {
+				h.Del(sf)
 			}
 		}
 	}
diff --git a/modules/caddyhttp/reverseproxy/streaming.go b/modules/caddyhttp/reverseproxy/streaming.go
index 285c04b78..1db352b70 100644
--- a/modules/caddyhttp/reverseproxy/streaming.go
+++ b/modules/caddyhttp/reverseproxy/streaming.go
@@ -32,6 +32,9 @@ import (
 func (h Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWriter, req *http.Request, res *http.Response) {
 	reqUpType := upgradeType(req.Header)
 	resUpType := upgradeType(res.Header)
+	// TODO: Update to use "net/http/internal/ascii" once we bumped
+	// the minimum Go version to 1.17.
+	// See https://github.com/golang/go/commit/5c489514bc5e61ad9b5b07bd7d8ec65d66a0512a
 	if reqUpType != resUpType {
 		h.logger.Debug("backend tried to switch to unexpected protocol via Upgrade header",
 			zap.String("backend_upgrade", resUpType),
@@ -39,8 +42,6 @@ func (h Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWrite
 		return
 	}
 
-	copyHeader(res.Header, rw.Header())
-
 	hj, ok := rw.(http.Hijacker)
 	if !ok {
 		h.logger.Sugar().Errorf("can't switch protocols using non-Hijacker ResponseWriter type %T", rw)
@@ -78,6 +79,9 @@ func (h Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWrite
 		logger.Debug("connection closed", zap.Duration("duration", time.Since(start)))
 	}()
 
+	copyHeader(rw.Header(), res.Header)
+
+	res.Header = rw.Header()
 	res.Body = nil // so res.Write only writes the headers; we have res.Body in backConn above
 	if err := res.Write(brw); err != nil {
 		h.logger.Debug("response write", zap.Error(err))
@@ -107,13 +111,16 @@ func (h Handler) flushInterval(req *http.Request, res *http.Response) time.Durat
 		return -1 // negative means immediately
 	}
 
+	// We might have the case of streaming for which Content-Length might be unset.
+	if res.ContentLength == -1 {
+		return -1
+	}
+
 	// for h2 and h2c upstream streaming data to client (issues #3556 and #3606)
 	if h.isBidirectionalStream(req, res) {
 		return -1
 	}
 
-	// TODO: more specific cases? e.g. res.ContentLength == -1? (this TODO is from the std lib, but
-	// strangely similar to our isBidirectionalStream function that we implemented ourselves)
 	return time.Duration(h.FlushInterval)
 }
 
@@ -142,6 +149,11 @@ func (h Handler) copyResponse(dst io.Writer, src io.Reader, flushInterval time.D
 				latency: flushInterval,
 			}
 			defer mlw.stop()
+
+			// set up initial timer so headers get flushed even if body writes are delayed
+			mlw.flushPending = true
+			mlw.t = time.AfterFunc(flushInterval, mlw.delayedFlush)
+
 			dst = mlw
 		}
 	}