Merge pull request #8886 from csrwng/fix_proxy_url_slash

Fix proxying of URLs that end in "/" in the pod proxy subresource

pkg/registry/generic/rest/proxy.go

@@ -76,12 +76,33 @@ func (h *UpgradeAwareProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Re
 		return
 	}
 
+	loc := *h.Location
+	loc.RawQuery = req.URL.RawQuery
+
+	// If original request URL ended in '/', append a '/' at the end of the
+	// of the proxy URL
+	if !strings.HasSuffix(loc.Path, "/") && strings.HasSuffix(req.URL.Path, "/") {
+		loc.Path += "/"
+	}
+
+	// From pkg/apiserver/proxy.go#ServeHTTP:
+	// Redirect requests with an empty path to a location that ends with a '/'
+	// This is essentially a hack for https://github.com/GoogleCloudPlatform/kubernetes/issues/4958.
+	// Note: Keep this code after tryUpgrade to not break that flow.
+	if len(loc.Path) == 0 {
+		var queryPart string
+		if len(req.URL.RawQuery) > 0 {
+			queryPart = "?" + req.URL.RawQuery
+		}
+		w.Header().Set("Location", req.URL.Path+"/"+queryPart)
+		w.WriteHeader(http.StatusMovedPermanently)
+		return
+	}
+
 	if h.Transport == nil {
 		h.Transport = h.defaultProxyTransport(req.URL)
 	}
 
-	loc := *h.Location
-	loc.RawQuery = req.URL.RawQuery
 	newReq, err := http.NewRequest(req.Method, loc.String(), req.Body)
 	if err != nil {
 		h.err = err
@@ -188,7 +209,11 @@ func (h *UpgradeAwareProxyHandler) dialURL() (net.Conn, error) {
 func (h *UpgradeAwareProxyHandler) defaultProxyTransport(url *url.URL) http.RoundTripper {
 	scheme := url.Scheme
 	host := url.Host
-	pathPrepend := strings.TrimRight(url.Path, h.Location.Path)
+	suffix := h.Location.Path
+	if strings.HasSuffix(url.Path, "/") && !strings.HasSuffix(suffix, "/") {
+		suffix += "/"
+	}
+	pathPrepend := strings.TrimSuffix(url.Path, suffix)
 	return &proxy.Transport{
 		Scheme:      scheme,
 		Host:        host,

pkg/registry/generic/rest/proxy_test.go

@@ -26,6 +26,8 @@ import (
 	"testing"
 
 	"golang.org/x/net/websocket"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/proxy"
 )
 
 type SimpleBackendHandler struct {
@@ -88,24 +90,28 @@ func TestServeHTTP(t *testing.T) {
 		name          string
 		method        string
 		requestPath   string
+		expectedPath  string
 		requestBody   string
 		requestParams map[string]string
 		requestHeader map[string]string
 	}{
 		{
-			name:        "root path, simple get",
-			method:      "GET",
-			requestPath: "/",
+			name:         "root path, simple get",
+			method:       "GET",
+			requestPath:  "/",
+			expectedPath: "/",
 		},
 		{
-			name:        "simple path, get",
-			method:      "GET",
-			requestPath: "/path/to/test",
+			name:         "simple path, get",
+			method:       "GET",
+			requestPath:  "/path/to/test",
+			expectedPath: "/path/to/test",
 		},
 		{
 			name:          "request params",
 			method:        "POST",
-			requestPath:   "/some/path",
+			requestPath:   "/some/path/",
+			expectedPath:  "/some/path/",
 			requestParams: map[string]string{"param1": "value/1", "param2": "value%2"},
 			requestBody:   "test request body",
 		},
@@ -113,8 +119,15 @@ func TestServeHTTP(t *testing.T) {
 			name:          "request headers",
 			method:        "PUT",
 			requestPath:   "/some/path",
+			expectedPath:  "/some/path",
 			requestHeader: map[string]string{"Header1": "value1", "Header2": "value2"},
 		},
+		{
+			name:         "empty path - slash should be added",
+			method:       "GET",
+			requestPath:  "",
+			expectedPath: "/",
+		},
 	}
 
 	for _, test := range tests {
@@ -176,7 +189,7 @@ func TestServeHTTP(t *testing.T) {
 			}
 
 			// Path
-			if backendHandler.requestURL.Path != test.requestPath {
+			if backendHandler.requestURL.Path != test.expectedPath {
 				t.Errorf("Unexpected request path: %s", backendHandler.requestURL.Path)
 			}
 			// Parameters
@@ -240,3 +253,59 @@ func TestProxyUpgrade(t *testing.T) {
 		t.Fatalf("expected '%#v', got '%#v'", e, a)
 	}
 }
+
+func TestDefaultProxyTransport(t *testing.T) {
+	tests := []struct {
+		name,
+		url,
+		location,
+		expectedScheme,
+		expectedHost,
+		expectedPathPrepend string
+	}{
+
+		{
+			name:                "simple path",
+			url:                 "http://test.server:8080/a/test/location",
+			location:            "http://localhost/location",
+			expectedScheme:      "http",
+			expectedHost:        "test.server:8080",
+			expectedPathPrepend: "/a/test",
+		},
+		{
+			name:                "empty path",
+			url:                 "http://test.server:8080/a/test/",
+			location:            "http://localhost",
+			expectedScheme:      "http",
+			expectedHost:        "test.server:8080",
+			expectedPathPrepend: "/a/test",
+		},
+		{
+			name:                "location ending in slash",
+			url:                 "http://test.server:8080/a/test/",
+			location:            "http://localhost/",
+			expectedScheme:      "http",
+			expectedHost:        "test.server:8080",
+			expectedPathPrepend: "/a/test",
+		},
+	}
+
+	for _, test := range tests {
+		locURL, _ := url.Parse(test.location)
+		URL, _ := url.Parse(test.url)
+		h := UpgradeAwareProxyHandler{
+			Location: locURL,
+		}
+		result := h.defaultProxyTransport(URL)
+		transport := result.(*proxy.Transport)
+		if transport.Scheme != test.expectedScheme {
+			t.Errorf("%s: unexpected scheme. Actual: %s, Expected: %s", test.name, transport.Scheme, test.expectedScheme)
+		}
+		if transport.Host != test.expectedHost {
+			t.Errorf("%s: unexpected host. Actual: %s, Expected: %s", test.name, transport.Host, test.expectedHost)
+		}
+		if transport.PathPrepend != test.expectedPathPrepend {
+			t.Errorf("%s: unexpected path prepend. Actual: %s, Expected: %s", test.name, transport.PathPrepend, test.expectedPathPrepend)
+		}
+	}
+}

pkg/util/proxy/transport.go

@@ -77,7 +77,11 @@ type Transport struct {
 // RoundTrip implements the http.RoundTripper interface
 func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
 	// Add reverse proxy headers.
-	req.Header.Set("X-Forwarded-Uri", t.PathPrepend+req.URL.Path)
+	forwardedURI := path.Join(t.PathPrepend, req.URL.Path)
+	if strings.HasSuffix(req.URL.Path, "/") {
+		forwardedURI = forwardedURI + "/"
+	}
+	req.Header.Set("X-Forwarded-Uri", forwardedURI)
 	req.Header.Set("X-Forwarded-Host", t.Host)
 	req.Header.Set("X-Forwarded-Proto", t.Scheme)