diff --git a/pkg/registry/core/node/rest/BUILD b/pkg/registry/core/node/rest/BUILD index 5af1784d1d3d..98eb8bf8c8ab 100644 --- a/pkg/registry/core/node/rest/BUILD +++ b/pkg/registry/core/node/rest/BUILD @@ -14,6 +14,7 @@ go_library( "//pkg/kubelet/client:go_default_library", "//pkg/registry/core/node:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/proxy:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//vendor/k8s.io/apiserver/pkg/registry/generic/registry:go_default_library", diff --git a/pkg/registry/core/node/rest/proxy.go b/pkg/registry/core/node/rest/proxy.go index d8d4738b03f1..61356d97b85b 100644 --- a/pkg/registry/core/node/rest/proxy.go +++ b/pkg/registry/core/node/rest/proxy.go @@ -20,9 +20,9 @@ import ( "fmt" "net/http" "net/url" - "path" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/proxy" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" @@ -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 = net.JoinPreservingTrailingSlash(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 } diff --git a/pkg/registry/core/pod/rest/BUILD b/pkg/registry/core/pod/rest/BUILD index 110bbda79d3a..3f9467129d38 100644 --- a/pkg/registry/core/pod/rest/BUILD +++ b/pkg/registry/core/pod/rest/BUILD @@ -20,6 +20,7 @@ go_library( "//pkg/registry/core/pod:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/proxy:go_default_library", "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//vendor/k8s.io/apiserver/pkg/features:go_default_library", diff --git a/pkg/registry/core/pod/rest/subresources.go b/pkg/registry/core/pod/rest/subresources.go index a71f8bbd571b..b152688a101f 100644 --- a/pkg/registry/core/pod/rest/subresources.go +++ b/pkg/registry/core/pod/rest/subresources.go @@ -20,9 +20,9 @@ import ( "fmt" "net/http" "net/url" - "path" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/proxy" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" genericfeatures "k8s.io/apiserver/pkg/features" @@ -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 = net.JoinPreservingTrailingSlash(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 } diff --git a/pkg/registry/core/service/proxy.go b/pkg/registry/core/service/proxy.go index a826892e6a96..03074de98319 100644 --- a/pkg/registry/core/service/proxy.go +++ b/pkg/registry/core/service/proxy.go @@ -20,9 +20,9 @@ import ( "fmt" "net/http" "net/url" - "path" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/proxy" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" @@ -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 = net.JoinPreservingTrailingSlash(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 } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/http.go b/staging/src/k8s.io/apimachinery/pkg/util/net/http.go index 77488388c64a..41be5b2fe259 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/net/http.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/http.go @@ -26,6 +26,7 @@ import ( "net/http" "net/url" "os" + "path" "strconv" "strings" @@ -33,6 +34,26 @@ import ( "golang.org/x/net/http2" ) +// JoinPreservingTrailingSlash does a path.Join of the specified elements, +// preserving any trailing slash on the last non-empty segment +func JoinPreservingTrailingSlash(elem ...string) string { + // do the basic path join + result := path.Join(elem...) + + // find the last non-empty segment + for i := len(elem) - 1; i >= 0; i-- { + if len(elem[i]) > 0 { + // if the last segment ended in a slash, ensure our result does as well + if strings.HasSuffix(elem[i], "/") && !strings.HasSuffix(result, "/") { + result += "/" + } + break + } + } + + return result +} + // IsProbableEOF returns true if the given error resembles a connection termination // scenario that would justify assuming that the watch is empty. // These errors are what the Go http stack returns back to us which are general diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go b/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go index 2d41eda49d72..8f5dd9cdffae 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go @@ -20,6 +20,7 @@ package net import ( "crypto/tls" + "fmt" "net" "net/http" "net/url" @@ -218,3 +219,40 @@ func TestTLSClientConfigHolder(t *testing.T) { t.Errorf("didn't find tls config") } } + +func TestJoinPreservingTrailingSlash(t *testing.T) { + tests := []struct { + a string + b string + want string + }{ + // All empty + {"", "", ""}, + + // Empty a + {"", "/", "/"}, + {"", "foo", "foo"}, + {"", "/foo", "/foo"}, + {"", "/foo/", "/foo/"}, + + // Empty b + {"/", "", "/"}, + {"foo", "", "foo"}, + {"/foo", "", "/foo"}, + {"/foo/", "", "/foo/"}, + + // Both populated + {"/", "/", "/"}, + {"foo", "foo", "foo/foo"}, + {"/foo", "/foo", "/foo/foo"}, + {"/foo/", "/foo/", "/foo/foo/"}, + } + for _, tt := range tests { + name := fmt.Sprintf("%q+%q=%q", tt.a, tt.b, tt.want) + t.Run(name, func(t *testing.T) { + if got := JoinPreservingTrailingSlash(tt.a, tt.b); got != tt.want { + t.Errorf("JoinPreservingTrailingSlash() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index ee5538691478..ded37f7ed1ca 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -2135,15 +2135,25 @@ func TestGetWithOptions(t *testing.T) { requestURL: "/namespaces/default/simple/id?param1=test1¶m2=test2", expectedPath: "", }, + { + name: "with root slash", + requestURL: "/namespaces/default/simple/id/?param1=test1¶m2=test2", + expectedPath: "/", + }, { name: "with path", requestURL: "/namespaces/default/simple/id/a/different/path?param1=test1¶m2=test2", - expectedPath: "a/different/path", + expectedPath: "/a/different/path", + }, + { + name: "with path with trailing slash", + requestURL: "/namespaces/default/simple/id/a/different/path/?param1=test1¶m2=test2", + expectedPath: "/a/different/path/", }, { name: "as subresource", requestURL: "/namespaces/default/simple/id/subresource/another/different/path?param1=test1¶m2=test2", - expectedPath: "another/different/path", + expectedPath: "/another/different/path", }, { name: "cluster-scoped basic", @@ -2155,13 +2165,13 @@ func TestGetWithOptions(t *testing.T) { name: "cluster-scoped basic with path", rootScoped: true, requestURL: "/simple/id/a/cluster/path?param1=test1¶m2=test2", - expectedPath: "a/cluster/path", + expectedPath: "/a/cluster/path", }, { name: "cluster-scoped basic as subresource", rootScoped: true, requestURL: "/simple/id/subresource/another/cluster/path?param1=test1¶m2=test2", - expectedPath: "another/cluster/path", + expectedPath: "/another/cluster/path", }, } @@ -2556,7 +2566,7 @@ func TestConnectWithOptions(t *testing.T) { func TestConnectWithOptionsAndPath(t *testing.T) { responseText := "Hello World" itemID := "theID" - testPath := "a/b/c/def" + testPath := "/a/b/c/def" connectStorage := &ConnecterRESTStorage{ connectHandler: &OutputConnect{ response: responseText, @@ -2572,7 +2582,7 @@ func TestConnectWithOptionsAndPath(t *testing.T) { server := httptest.NewServer(handler) defer server.Close() - resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/" + itemID + "/connect/" + testPath + "?param1=value1¶m2=value2") + resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/" + itemID + "/connect" + testPath + "?param1=value1¶m2=value2") if err != nil { t.Errorf("unexpected error: %v", err) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go index a3c146a35d7d..dabbd85cbeaa 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go @@ -212,7 +212,20 @@ func getRequestOptions(req *http.Request, scope RequestScope, into runtime.Objec if isSubresource { startingIndex = 3 } - newQuery[subpathKey] = []string{strings.Join(requestInfo.Parts[startingIndex:], "/")} + + p := strings.Join(requestInfo.Parts[startingIndex:], "/") + + // ensure non-empty subpaths correctly reflect a leading slash + if len(p) > 0 && !strings.HasPrefix(p, "/") { + p = "/" + p + } + + // ensure subpaths correctly reflect the presence of a trailing slash on the original request + if strings.HasSuffix(requestInfo.Path, "/") && !strings.HasSuffix(p, "/") { + p += "/" + } + + newQuery[subpathKey] = []string{p} query = newQuery } return scope.ParameterCodec.DecodeParameters(query, scope.Kind.GroupVersion(), into)