reverseproxy: refactor HTTP transport layer (#5369)

Co-authored-by: Francis Lavoie <lavofr@gmail.com>
Co-authored-by: Weidi Deng <weidi_deng@icloud.com>

2 files changed, 148 insertions, 140 deletions

diff --git a/caddytest/integration/reverseproxy_test.go b/caddytest/integration/reverseproxy_test.go
index f7b1967..4333d12 100644
--- a/caddytest/integration/reverseproxy_test.go
+++ b/caddytest/integration/reverseproxy_test.go
@@ -22,39 +22,39 @@ func TestSRVReverseProxy(t *testing.T) {
 		},
 		"apps": {
 			"pki": {
-				"certificate_authorities" : {
-					"local" : {
-					"install_trust": false
+				"certificate_authorities": {
+					"local": {
+						"install_trust": false
 					}
 				}
 			},
-		  "http": {
-			"grace_period": 1,
-			"servers": {
-			  "srv0": {
-				"listen": [
-				  ":8080"
-				],
-				"routes": [
-				  {
-					"handle": [
-					  {
-						"handler": "reverse_proxy",
-						"upstreams": [
-						  {
-							"lookup_srv": "srv.host.service.consul"
-						  }
+			"http": {
+				"grace_period": 1,
+				"servers": {
+					"srv0": {
+						"listen": [
+							":18080"
+						],
+						"routes": [
+							{
+								"handle": [
+									{
+										"handler": "reverse_proxy",
+										"upstreams": [
+											{
+												"lookup_srv": "srv.host.service.consul"
+											}
+										]
+									}
+								]
+							}
 						]
-					  }
-					]
-				  }
-				]
-			  }
+					}
+				}
 			}
-		  }
 		}
-	  }
-  `, "json")
+	}
+	`, "json")
 }
 
 func TestSRVWithDial(t *testing.T) {
@@ -62,39 +62,39 @@ func TestSRVWithDial(t *testing.T) {
 	{
 		"apps": {
 			"pki": {
-				"certificate_authorities" : {
-				  "local" : {
-					"install_trust": false
-				  }
+				"certificate_authorities": {
+					"local": {
+						"install_trust": false
+					}
 				}
-			  },
-		  "http": {
-			"grace_period": 1,
-			"servers": {
-			  "srv0": {
-				"listen": [
-				  ":8080"
-				],
-				"routes": [
-				  {
-					"handle": [
-					  {
-						"handler": "reverse_proxy",
-						"upstreams": [
-						  {
-							"dial": "tcp/address.to.upstream:80",
-							"lookup_srv": "srv.host.service.consul"
-						  }
+			},
+			"http": {
+				"grace_period": 1,
+				"servers": {
+					"srv0": {
+						"listen": [
+							":18080"
+						],
+						"routes": [
+							{
+								"handle": [
+									{
+										"handler": "reverse_proxy",
+										"upstreams": [
+											{
+												"dial": "tcp/address.to.upstream:80",
+												"lookup_srv": "srv.host.service.consul"
+											}
+										]
+									}
+								]
+							}
 						]
-					  }
-					]
-				  }
-				]
-			  }
+					}
+				}
 			}
-		  }
 		}
-	  }
+	}
 	`, "json", `upstream: specifying dial address is incompatible with lookup_srv: 0: {\"dial\": \"tcp/address.to.upstream:80\", \"lookup_srv\": \"srv.host.service.consul\"}`)
 }
 
@@ -138,41 +138,41 @@ func TestDialWithPlaceholderUnix(t *testing.T) {
 		},
 		"apps": {
 			"pki": {
-				"certificate_authorities" : {
-				  "local" : {
-					"install_trust": false
-				  }
+				"certificate_authorities": {
+					"local": {
+						"install_trust": false
+					}
 				}
-			  },
-		  "http": {
-			"grace_period": 1,
-			"servers": {
-			  "srv0": {
-				"listen": [
-				  ":8080"
-				],
-				"routes": [
-				  {
-					"handle": [
-					  {
-						"handler": "reverse_proxy",
-						"upstreams": [
-						  {
-							"dial": "unix/{http.request.header.X-Caddy-Upstream-Dial}"
-						  }
+			},
+			"http": {
+				"grace_period": 1,
+				"servers": {
+					"srv0": {
+						"listen": [
+							":18080"
+						],
+						"routes": [
+							{
+								"handle": [
+									{
+										"handler": "reverse_proxy",
+										"upstreams": [
+											{
+												"dial": "unix/{http.request.header.X-Caddy-Upstream-Dial}"
+											}
+										]
+									}
+								]
+							}
 						]
-					  }
-					]
-				  }
-				]
-			  }
-			}
-		  }
+					}
+				}
+		  	}
 		}
-	  }
+	}
 	`, "json")
 
-	req, err := http.NewRequest(http.MethodGet, "http://localhost:8080", nil)
+	req, err := http.NewRequest(http.MethodGet, "http://localhost:18080", nil)
 	if err != nil {
 		t.Fail()
 		return
@@ -190,18 +190,18 @@ func TestReverseProxyWithPlaceholderDialAddress(t *testing.T) {
 		},
 		"apps": {
 			"pki": {
-				"certificate_authorities" : {
-				  "local" : {
-					"install_trust": false
-				  }
+				"certificate_authorities": {
+					"local": {
+						"install_trust": false
+					}
 				}
-			  },
+			},
 			"http": {
 				"grace_period": 1,
 				"servers": {
 					"srv0": {
 						"listen": [
-							":8080"
+							":18080"
 						],
 						"routes": [
 							{
@@ -264,14 +264,14 @@ func TestReverseProxyWithPlaceholderDialAddress(t *testing.T) {
 			}
 		}
 	}
-  	`, "json")
+	`, "json")
 
 	req, err := http.NewRequest(http.MethodGet, "http://localhost:9080", nil)
 	if err != nil {
 		t.Fail()
 		return
 	}
-	req.Header.Set("X-Caddy-Upstream-Dial", "localhost:8080")
+	req.Header.Set("X-Caddy-Upstream-Dial", "localhost:18080")
 	tester.AssertResponse(req, 200, "Hello, World!")
 }
 
@@ -284,18 +284,18 @@ func TestReverseProxyWithPlaceholderTCPDialAddress(t *testing.T) {
 		},
 		"apps": {
 			"pki": {
-				"certificate_authorities" : {
-				  "local" : {
-					"install_trust": false
-				  }
+				"certificate_authorities": {
+					"local": {
+						"install_trust": false
+					}
 				}
-			  },
+			},
 			"http": {
 				"grace_period": 1,
 				"servers": {
 					"srv0": {
 						"listen": [
-							":8080"
+							":18080"
 						],
 						"routes": [
 							{
@@ -340,7 +340,7 @@ func TestReverseProxyWithPlaceholderTCPDialAddress(t *testing.T) {
 										"handler": "reverse_proxy",
 										"upstreams": [
 											{
-												"dial": "tcp/{http.request.header.X-Caddy-Upstream-Dial}:8080"
+												"dial": "tcp/{http.request.header.X-Caddy-Upstream-Dial}:18080"
 											}
 										]
 									}
@@ -358,7 +358,7 @@ func TestReverseProxyWithPlaceholderTCPDialAddress(t *testing.T) {
 			}
 		}
 	}
-  	`, "json")
+	`, "json")
 
 	req, err := http.NewRequest(http.MethodGet, "http://localhost:9080", nil)
 	if err != nil {
@@ -375,42 +375,42 @@ func TestSRVWithActiveHealthcheck(t *testing.T) {
 		"apps": {
 			"pki": {
 				"certificate_authorities" : {
-				  "local" : {
-					"install_trust": false
-				  }
+					"local" : {
+						"install_trust": false
+					}
 				}
-			  },
-		  "http": {
-			"grace_period": 1,
-			"servers": {
-			  "srv0": {
-				"listen": [
-				  ":8080"
-				],
-				"routes": [
-				  {
-					"handle": [
-					  {
-						"handler": "reverse_proxy",
-						"health_checks": {
-							"active": {
-								"path": "/ok"
+			},
+			"http": {
+				"grace_period": 1,
+				"servers": {
+					"srv0": {
+						"listen": [
+							":18080"
+						],
+						"routes": [
+							{
+								"handle": [
+									{
+										"handler": "reverse_proxy",
+										"health_checks": {
+											"active": {
+												"path": "/ok"
+											}
+										},
+										"upstreams": [
+											{
+												"lookup_srv": "srv.host.service.consul"
+											}
+										]
+									}
+								]
 							}
-						},
-						"upstreams": [
-						  {
-							"lookup_srv": "srv.host.service.consul"
-						  }
 						]
-					  }
-					]
-				  }
-				]
-			  }
+					}
+				}
 			}
-		  }
 		}
-	  }
+	}
 	`, "json", `upstream: lookup_srv is incompatible with active health checks: 0: {\"dial\": \"\", \"lookup_srv\": \"srv.host.service.consul\"}`)
 }
 
@@ -440,7 +440,7 @@ func TestReverseProxyHealthCheck(t *testing.T) {
 			health_timeout 100ms
 		}
 	}
-  `, "caddyfile")
+	`, "caddyfile")
 
 	time.Sleep(100 * time.Millisecond) // TODO: for some reason this test seems particularly flaky, getting 503 when it should be 200, unless we wait
 	tester.AssertGetResponse("http://localhost:9080/", 200, "Hello, World!")
diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go
index ec5d2f2..71d06d5 100644
--- a/modules/caddyhttp/reverseproxy/httptransport.go
+++ b/modules/caddyhttp/reverseproxy/httptransport.go
@@ -172,12 +172,19 @@ func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, e
 		}
 	}
 
-	// Set up the dialer to pull the correct information from the context
 	dialContext := func(ctx context.Context, network, address string) (net.Conn, error) {
-		// the proper dialing information should be embedded into the request's context
+		// For unix socket upstreams, we need to recover the dial info from
+		// the request's context, because the Host on the request's URL
+		// will have been modified by directing the request, overwriting
+		// the unix socket filename.
+		// Also, we need to avoid overwriting the address at this point
+		// when not necessary, because http.ProxyFromEnvironment may have
+		// modified the address according to the user's env proxy config.
 		if dialInfo, ok := GetDialInfo(ctx); ok {
-			network = dialInfo.Network
-			address = dialInfo.Address
+			if strings.HasPrefix(dialInfo.Network, "unix") {
+				network = dialInfo.Network
+				address = dialInfo.Address
+			}
 		}
 
 		conn, err := dialer.DialContext(ctx, network, address)
@@ -188,8 +195,8 @@ func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, e
 			return nil, DialError{err}
 		}
 
-		// if read/write timeouts are configured and this is a TCP connection, enforce the timeouts
-		// by wrapping the connection with our own type
+		// if read/write timeouts are configured and this is a TCP connection,
+		// enforce the timeouts by wrapping the connection with our own type
 		if tcpConn, ok := conn.(*net.TCPConn); ok && (h.ReadTimeout > 0 || h.WriteTimeout > 0) {
 			conn = &tcpRWTimeoutConn{
 				TCPConn:      tcpConn,
@@ -203,6 +210,7 @@ func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, e
 	}
 
 	rt := &http.Transport{
+		Proxy:                  http.ProxyFromEnvironment,
 		DialContext:            dialContext,
 		MaxConnsPerHost:        h.MaxConnsPerHost,
 		ResponseHeaderTimeout:  time.Duration(h.ResponseHeaderTimeout),