Merge pull request #4778 from lavalamp/fix7

Construct path correctly in proxy

pkg/apiserver/handlers.go

@@ -19,6 +19,7 @@ package apiserver
 import (
 	"fmt"
 	"net/http"
+	"path"
 	"regexp"
 	"runtime/debug"
 	"strings"
@@ -214,8 +215,22 @@ type APIRequestInfo struct {
 	Kind string
 	// Name is empty for some verbs, but if the request directly indicates a name (not in body content) then this field is filled in.
 	Name string
-	// Parts are the path parts for the request relative to /{resource}/{name}
+	// Parts are the path parts for the request, always starting with /{resource}/{name}
 	Parts []string
+	// Raw is the unparsed form of everything other than parts.
+	// Raw + Parts = complete URL path
+	Raw []string
+}
+
+// URLPath returns the URL path for this request, including /{resource}/{name} if present but nothing
+// following that.
+func (info APIRequestInfo) URLPath() string {
+	p := info.Parts
+	if n := len(p); n > 2 {
+		// Only take resource and name
+		p = p[:2]
+	}
+	return path.Join("/", path.Join(info.Raw...), path.Join(p...))
 }
 
 type APIRequestInfoResolver struct {
@@ -247,9 +262,11 @@ type APIRequestInfoResolver struct {
 // /api/{version}/*
 // /api/{version}/*
 func (r *APIRequestInfoResolver) GetAPIRequestInfo(req *http.Request) (APIRequestInfo, error) {
-	requestInfo := APIRequestInfo{}
+	requestInfo := APIRequestInfo{
+		Raw: splitPath(req.URL.Path),
+	}
 
-	currentParts := splitPath(req.URL.Path)
+	currentParts := requestInfo.Raw
 	if len(currentParts) < 1 {
 		return requestInfo, fmt.Errorf("Unable to determine kind and namespace from an empty URL path")
 	}
@@ -322,6 +339,8 @@ func (r *APIRequestInfoResolver) GetAPIRequestInfo(req *http.Request) (APIReques
 
 	// parsing successful, so we now know the proper value for .Parts
 	requestInfo.Parts = currentParts
+	// Raw should have everything not in Parts
+	requestInfo.Raw = requestInfo.Raw[:len(requestInfo.Raw)-len(currentParts)]
 
 	// if there's another part remaining after the kind, then that's the resource name
 	if len(requestInfo.Parts) >= 2 {

pkg/apiserver/handlers_test.go

@@ -20,6 +20,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -142,6 +143,9 @@ func TestGetAPIRequestInfo(t *testing.T) {
 		if !reflect.DeepEqual(successCase.expectedParts, apiRequestInfo.Parts) {
 			t.Errorf("Unexpected parts for url: %s, expected: %v, actual: %v", successCase.url, successCase.expectedParts, apiRequestInfo.Parts)
 		}
+		if e, a := strings.Split(successCase.url, "?")[0], apiRequestInfo.URLPath(); e != a {
+			t.Errorf("Expected %v, got %v", e, a)
+		}
 	}
 
 	errorCases := map[string]string{

pkg/apiserver/proxy.go

@@ -182,64 +182,73 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	// TODO convert this entire proxy to an UpgradeAwareProxy similar to
 	// https://github.com/openshift/origin/blob/master/pkg/util/httpproxy/upgradeawareproxy.go.
 	// That proxy needs to be modified to support multiple backends, not just 1.
-	connectionHeader := strings.ToLower(req.Header.Get(httpstream.HeaderConnection))
-	if strings.Contains(connectionHeader, strings.ToLower(httpstream.HeaderUpgrade)) && len(req.Header.Get(httpstream.HeaderUpgrade)) > 0 {
-		//TODO support TLS? Doesn't look like proxyTransport does anything special ...
-		dialAddr := netutil.CanonicalAddr(destURL)
-		backendConn, err := net.Dial("tcp", dialAddr)
-		if err != nil {
-			status := errToAPIStatus(err)
-			writeJSON(status.Code, r.codec, status, w)
-			return
-		}
-		defer backendConn.Close()
-
-		// TODO should we use _ (a bufio.ReadWriter) instead of requestHijackedConn
-		// when copying between the client and the backend? Docker doesn't when they
-		// hijack, just for reference...
-		requestHijackedConn, _, err := w.(http.Hijacker).Hijack()
-		if err != nil {
-			status := errToAPIStatus(err)
-			writeJSON(status.Code, r.codec, status, w)
-			return
-		}
-		defer requestHijackedConn.Close()
-
-		if err = newReq.Write(backendConn); err != nil {
-			status := errToAPIStatus(err)
-			writeJSON(status.Code, r.codec, status, w)
-			return
-		}
-
-		done := make(chan struct{}, 2)
-
-		go func() {
-			_, err := io.Copy(backendConn, requestHijackedConn)
-			if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
-				glog.Errorf("Error proxying data from client to backend: %v", err)
-			}
-			done <- struct{}{}
-		}()
-
-		go func() {
-			_, err := io.Copy(requestHijackedConn, backendConn)
-			if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
-				glog.Errorf("Error proxying data from backend to client: %v", err)
-			}
-			done <- struct{}{}
-		}()
-
-		<-done
-	} else {
-		proxy := httputil.NewSingleHostReverseProxy(&url.URL{Scheme: "http", Host: destURL.Host})
-		proxy.Transport = &proxyTransport{
-			proxyScheme:      req.URL.Scheme,
-			proxyHost:        req.URL.Host,
-			proxyPathPrepend: path.Join(r.prefix, "ns", namespace, resource, id),
-		}
-		proxy.FlushInterval = 200 * time.Millisecond
-		proxy.ServeHTTP(w, newReq)
+	if r.tryUpgrade(w, req, newReq, destURL) {
+		return
 	}
+
+	proxy := httputil.NewSingleHostReverseProxy(&url.URL{Scheme: "http", Host: destURL.Host})
+	proxy.Transport = &proxyTransport{
+		proxyScheme:      req.URL.Scheme,
+		proxyHost:        req.URL.Host,
+		proxyPathPrepend: requestInfo.URLPath(),
+	}
+	proxy.FlushInterval = 200 * time.Millisecond
+	proxy.ServeHTTP(w, newReq)
+}
+
+// tryUpgrade returns true if the request was handled.
+func (r *ProxyHandler) tryUpgrade(w http.ResponseWriter, req, newReq *http.Request, destURL *url.URL) bool {
+	connectionHeader := strings.ToLower(req.Header.Get(httpstream.HeaderConnection))
+	if !strings.Contains(connectionHeader, strings.ToLower(httpstream.HeaderUpgrade)) || len(req.Header.Get(httpstream.HeaderUpgrade)) == 0 {
+		return false
+	}
+	//TODO support TLS? Doesn't look like proxyTransport does anything special ...
+	dialAddr := netutil.CanonicalAddr(destURL)
+	backendConn, err := net.Dial("tcp", dialAddr)
+	if err != nil {
+		status := errToAPIStatus(err)
+		writeJSON(status.Code, r.codec, status, w)
+		return true
+	}
+	defer backendConn.Close()
+
+	// TODO should we use _ (a bufio.ReadWriter) instead of requestHijackedConn
+	// when copying between the client and the backend? Docker doesn't when they
+	// hijack, just for reference...
+	requestHijackedConn, _, err := w.(http.Hijacker).Hijack()
+	if err != nil {
+		status := errToAPIStatus(err)
+		writeJSON(status.Code, r.codec, status, w)
+		return true
+	}
+	defer requestHijackedConn.Close()
+
+	if err = newReq.Write(backendConn); err != nil {
+		status := errToAPIStatus(err)
+		writeJSON(status.Code, r.codec, status, w)
+		return true
+	}
+
+	done := make(chan struct{}, 2)
+
+	go func() {
+		_, err := io.Copy(backendConn, requestHijackedConn)
+		if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
+			glog.Errorf("Error proxying data from client to backend: %v", err)
+		}
+		done <- struct{}{}
+	}()
+
+	go func() {
+		_, err := io.Copy(requestHijackedConn, backendConn)
+		if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
+			glog.Errorf("Error proxying data from backend to client: %v", err)
+		}
+		done <- struct{}{}
+	}()
+
+	<-done
+	return true
 }
 
 type proxyTransport struct {