Merge pull request #52065 from liggitt/proxy-request-uri

Automatic merge from submit-queue

Fix proxied request-uri to be valid HTTP requests

Fixes #52022, introduced in 1.7. Stringifying/re-parsing the URL masked that the path was not constructed with a leading `/` in the first place.

This makes upgrade requests proxied to pods/services via the API server proxy subresources be valid HTTP requests

```release-note
Fixes an issue with upgrade requests made via pod/service/node proxy subresources sending a non-absolute HTTP request-uri to backends
```

pkg/registry/core/node/rest/proxy.go

@@ -70,7 +70,7 @@ func (r *ProxyREST) Connect(ctx genericapirequest.Context, id string, opts runti
 	if err != nil {
 		return nil, err
 	}
-	location.Path = path.Join(location.Path, proxyOpts.Path)
+	location.Path = path.Join("/", location.Path, proxyOpts.Path)
 	// Return a proxy handler that uses the desired transport, wrapped with additional proxy handling (to get URL rewriting, X-Forwarded-* headers, etc)
 	return newThrottledUpgradeAwareProxyHandler(location, transport, true, false, responder), nil
 }

pkg/registry/core/pod/rest/subresources.go

@@ -71,7 +71,7 @@ func (r *ProxyREST) Connect(ctx genericapirequest.Context, id string, opts runti
 	if err != nil {
 		return nil, err
 	}
-	location.Path = path.Join(location.Path, proxyOpts.Path)
+	location.Path = path.Join("/", location.Path, proxyOpts.Path)
 	// Return a proxy handler that uses the desired transport, wrapped with additional proxy handling (to get URL rewriting, X-Forwarded-* headers, etc)
 	return newThrottledUpgradeAwareProxyHandler(location, transport, true, false, false, responder), nil
 }

pkg/registry/core/service/proxy.go

@@ -66,7 +66,7 @@ func (r *ProxyREST) Connect(ctx genericapirequest.Context, id string, opts runti
 	if err != nil {
 		return nil, err
 	}
-	location.Path = path.Join(location.Path, proxyOpts.Path)
+	location.Path = path.Join("/", location.Path, proxyOpts.Path)
 	// Return a proxy handler that uses the desired transport, wrapped with additional proxy handling (to get URL rewriting, X-Forwarded-* headers, etc)
 	return newThrottledUpgradeAwareProxyHandler(location, transport, true, false, responder), nil
 }

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

@@ -159,11 +159,20 @@ func NewUpgradeRequestRoundTripper(connection, request http.RoundTripper) Upgrad
 	}
 }
 
+// normalizeLocation returns the result of parsing the full URL, with scheme set to http if missing
+func normalizeLocation(location *url.URL) *url.URL {
+	normalized, _ := url.Parse(location.String())
+	if len(normalized.Scheme) == 0 {
+		normalized.Scheme = "http"
+	}
+	return normalized
+}
+
 // NewUpgradeAwareHandler creates a new proxy handler with a default flush interval. Responder is required for returning
 // errors to the caller.
 func NewUpgradeAwareHandler(location *url.URL, transport http.RoundTripper, wrapTransport, upgradeRequired bool, responder ErrorResponder) *UpgradeAwareHandler {
 	return &UpgradeAwareHandler{
-		Location:        location,
+		Location:        normalizeLocation(location),
 		Transport:       transport,
 		WrapTransport:   wrapTransport,
 		UpgradeRequired: upgradeRequired,
@@ -174,9 +183,6 @@ func NewUpgradeAwareHandler(location *url.URL, transport http.RoundTripper, wrap
 
 // ServeHTTP handles the proxy request
 func (h *UpgradeAwareHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
-	if len(h.Location.Scheme) == 0 {
-		h.Location.Scheme = "http"
-	}
 	if h.tryUpgrade(w, req) {
 		return
 	}

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware_test.go

@@ -241,11 +241,7 @@ func TestServeHTTP(t *testing.T) {
 			responder := &fakeResponder{t: t}
 			backendURL, _ := url.Parse(backendServer.URL)
 			backendURL.Path = test.requestPath
-			proxyHandler := &UpgradeAwareHandler{
-				Location:        backendURL,
-				Responder:       responder,
-				UpgradeRequired: test.upgradeRequired,
-			}
+			proxyHandler := NewUpgradeAwareHandler(backendURL, nil, false, test.upgradeRequired, responder)
 			proxyServer := httptest.NewServer(proxyHandler)
 			defer proxyServer.Close()
 			proxyURL, _ := url.Parse(proxyServer.URL)
@@ -457,13 +453,9 @@ func TestProxyUpgrade(t *testing.T) {
 
 				serverURL, _ := url.Parse(backendServer.URL)
 				serverURL.Path = backendPath
-				proxyHandler := &UpgradeAwareHandler{
-					Location:           serverURL,
-					Transport:          tc.ProxyTransport,
-					UpgradeTransport:   tc.UpgradeTransport,
-					InterceptRedirects: redirect,
-					Responder:          &noErrorsAllowed{t: t},
-				}
+				proxyHandler := NewUpgradeAwareHandler(serverURL, tc.ProxyTransport, false, false, &noErrorsAllowed{t: t})
+				proxyHandler.UpgradeTransport = tc.UpgradeTransport
+				proxyHandler.InterceptRedirects = redirect
 				proxy := httptest.NewServer(proxyHandler)
 				defer proxy.Close()
 
@@ -509,14 +501,15 @@ func TestProxyUpgradeErrorResponse(t *testing.T) {
 			return &fakeConn{err: expectedErr}, nil
 		}
 		responder = &fakeResponder{t: t, w: w}
-		proxyHandler := &UpgradeAwareHandler{
-			Location: &url.URL{
+		proxyHandler := NewUpgradeAwareHandler(
+			&url.URL{
 				Host: "fake-backend",
 			},
-			UpgradeRequired: true,
-			Responder:       responder,
-			Transport:       transport,
-		}
+			transport,
+			false,
+			true,
+			responder,
+		)
 		proxyHandler.ServeHTTP(w, r)
 	}))
 	defer proxy.Close()
@@ -575,9 +568,7 @@ func TestDefaultProxyTransport(t *testing.T) {
 	for _, test := range tests {
 		locURL, _ := url.Parse(test.location)
 		URL, _ := url.Parse(test.url)
-		h := UpgradeAwareHandler{
-			Location: locURL,
-		}
+		h := NewUpgradeAwareHandler(locURL, nil, false, false, nil)
 		result := h.defaultProxyTransport(URL, nil)
 		transport := result.(*corsRemovingTransport).RoundTripper.(*Transport)
 		if transport.Scheme != test.expectedScheme {
@@ -751,11 +742,7 @@ func TestProxyRequestContentLengthAndTransferEncoding(t *testing.T) {
 
 		responder := &fakeResponder{t: t}
 		backendURL, _ := url.Parse(downstreamServer.URL)
-		proxyHandler := &UpgradeAwareHandler{
-			Location:        backendURL,
-			Responder:       responder,
-			UpgradeRequired: false,
-		}
+		proxyHandler := NewUpgradeAwareHandler(backendURL, nil, false, false, responder)
 		proxyServer := httptest.NewServer(proxyHandler)
 		defer proxyServer.Close()