From 10cbaf7ce064ff34c724d205f724cbb9c7f6308a Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Mon, 26 Sep 2016 17:18:19 +0200 Subject: [PATCH] Store RequestInfo in Context ... in order to replace the manual RequestInfoResolver dependency injection through out the code. --- pkg/apiserver/api_installer.go | 10 ++- pkg/apiserver/apiserver.go | 5 -- pkg/apiserver/apiserver_test.go | 64 +++++++++++-------- pkg/apiserver/errors.go | 7 ++ pkg/apiserver/filters/audit.go | 6 +- pkg/apiserver/filters/audit_test.go | 44 ++++++++----- pkg/apiserver/filters/authorization.go | 38 +++++++---- pkg/apiserver/filters/authorization_test.go | 22 +++++-- pkg/apiserver/filters/requestinfo.go | 46 +++++++++++++ pkg/apiserver/filters/requestinfo_test.go | 29 +++++++++ pkg/apiserver/proxy.go | 34 ++++++---- pkg/apiserver/proxy_test.go | 8 +-- pkg/apiserver/request/doc.go | 20 ++++++ pkg/apiserver/{ => request}/requestinfo.go | 38 ++++++++--- .../{ => request}/requestinfo_test.go | 9 ++- pkg/genericapiserver/config.go | 10 ++- pkg/genericapiserver/filters/panics.go | 25 ++++++-- pkg/genericapiserver/genericapiserver.go | 7 +- pkg/master/master.go | 5 +- pkg/master/master_test.go | 6 +- 20 files changed, 310 insertions(+), 123 deletions(-) create mode 100644 pkg/apiserver/filters/requestinfo.go create mode 100644 pkg/apiserver/filters/requestinfo_test.go create mode 100644 pkg/apiserver/request/doc.go rename pkg/apiserver/{ => request}/requestinfo.go (86%) rename pkg/apiserver/{ => request}/requestinfo_test.go (98%) diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index 317081b6f8c..85e536cfeef 100644 --- a/pkg/apiserver/api_installer.go +++ b/pkg/apiserver/api_installer.go @@ -40,7 +40,6 @@ import ( type APIInstaller struct { group *APIGroupVersion - info *RequestInfoResolver prefix string // Path prefix where API resources are to be registered. minRequestTimeout time.Duration } @@ -67,11 +66,10 @@ func (a *APIInstaller) Install(ws *restful.WebService) (apiResources []unversion errors = make([]error, 0) proxyHandler := (&ProxyHandler{ - prefix: a.prefix + "/proxy/", - storage: a.group.Storage, - serializer: a.group.Serializer, - context: a.group.Context, - requestInfoResolver: a.info, + prefix: a.prefix + "/proxy/", + storage: a.group.Storage, + serializer: a.group.Serializer, + mapper: a.group.Context, }) // Register the paths in a deterministic (sorted) order to get a deterministic swagger spec. diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index bc4bf419561..ef1b3f200df 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -67,10 +67,6 @@ type APIGroupVersion struct { // GroupVersion is the external group version GroupVersion unversioned.GroupVersion - // RequestInfoResolver is used to parse URLs for the legacy proxy handler. Don't use this for anything else - // TODO: refactor proxy handler to use sub resources - RequestInfoResolver *RequestInfoResolver - // OptionsExternalVersion controls the Kubernetes APIVersion used for common objects in the apiserver // schema like api.Status, api.DeleteOptions, and api.ListOptions. Other implementors may // define a version "v1beta1" but want to use the Kubernetes "v1" internal objects. If @@ -175,7 +171,6 @@ func (g *APIGroupVersion) newInstaller() *APIInstaller { prefix := path.Join(g.Root, g.GroupVersion.Group, g.GroupVersion.Version) installer := &APIInstaller{ group: g, - info: g.RequestInfoResolver, prefix: prefix, minRequestTimeout: g.MinRequestTimeout, } diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index 545336b23f9..9b331590fb1 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -39,11 +39,14 @@ import ( "k8s.io/kubernetes/pkg/api/rest" "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/apiserver/filters" + "k8s.io/kubernetes/pkg/apiserver/request" apiservertesting "k8s.io/kubernetes/pkg/apiserver/testing" "k8s.io/kubernetes/pkg/fields" "k8s.io/kubernetes/pkg/labels" "k8s.io/kubernetes/pkg/runtime" "k8s.io/kubernetes/pkg/util/diff" + "k8s.io/kubernetes/pkg/util/sets" "k8s.io/kubernetes/pkg/watch" "k8s.io/kubernetes/pkg/watch/versioned" "k8s.io/kubernetes/plugin/pkg/admission/admit" @@ -259,8 +262,6 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission. template := APIGroupVersion{ Storage: storage, - RequestInfoResolver: newTestRequestInfoResolver(), - Creater: api.Scheme, Convertor: api.Scheme, Copier: api.Scheme, @@ -2386,14 +2387,13 @@ func TestCreateChecksDecode(t *testing.T) { func TestUpdateREST(t *testing.T) { makeGroup := func(storage map[string]rest.Storage) *APIGroupVersion { return &APIGroupVersion{ - Storage: storage, - Root: "/" + prefix, - RequestInfoResolver: newTestRequestInfoResolver(), - Creater: api.Scheme, - Convertor: api.Scheme, - Copier: api.Scheme, - Typer: api.Scheme, - Linker: selfLinker, + Storage: storage, + Root: "/" + prefix, + Creater: api.Scheme, + Convertor: api.Scheme, + Copier: api.Scheme, + Typer: api.Scheme, + Linker: selfLinker, Admit: admissionControl, Context: requestContextMapper, @@ -2472,13 +2472,12 @@ func TestParentResourceIsRequired(t *testing.T) { Storage: map[string]rest.Storage{ "simple/sub": storage, }, - Root: "/" + prefix, - RequestInfoResolver: newTestRequestInfoResolver(), - Creater: api.Scheme, - Convertor: api.Scheme, - Copier: api.Scheme, - Typer: api.Scheme, - Linker: selfLinker, + Root: "/" + prefix, + Creater: api.Scheme, + Convertor: api.Scheme, + Copier: api.Scheme, + Typer: api.Scheme, + Linker: selfLinker, Admit: admissionControl, Context: requestContextMapper, @@ -2504,13 +2503,12 @@ func TestParentResourceIsRequired(t *testing.T) { "simple": &SimpleRESTStorage{}, "simple/sub": storage, }, - Root: "/" + prefix, - RequestInfoResolver: newTestRequestInfoResolver(), - Creater: api.Scheme, - Convertor: api.Scheme, - Copier: api.Scheme, - Typer: api.Scheme, - Linker: selfLinker, + Root: "/" + prefix, + Creater: api.Scheme, + Convertor: api.Scheme, + Copier: api.Scheme, + Typer: api.Scheme, + Linker: selfLinker, Admit: admissionControl, Context: requestContextMapper, @@ -3131,8 +3129,6 @@ func TestXGSubresource(t *testing.T) { group := APIGroupVersion{ Storage: storage, - RequestInfoResolver: newTestRequestInfoResolver(), - Creater: api.Scheme, Convertor: api.Scheme, Copier: api.Scheme, @@ -3159,8 +3155,7 @@ func TestXGSubresource(t *testing.T) { panic(fmt.Sprintf("unable to install container %s: %v", group.GroupVersion, err)) } - handler := defaultAPIServer{mux, container} - server := httptest.NewServer(handler) + server := newTestServer(defaultAPIServer{mux, container}) defer server.Close() resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/" + itemID + "/subsimple") @@ -3249,3 +3244,16 @@ func BenchmarkUpdateProtobuf(b *testing.B) { } b.StopTimer() } + +func newTestServer(handler http.Handler) *httptest.Server { + handler = filters.WithRequestInfo(handler, newTestRequestInfoResolver(), requestContextMapper) + handler = api.WithRequestContext(handler, requestContextMapper) + return httptest.NewServer(handler) +} + +func newTestRequestInfoResolver() *request.RequestInfoResolver { + return &request.RequestInfoResolver{ + APIPrefixes: sets.NewString("api", "apis"), + GrouplessAPIPrefixes: sets.NewString("api"), + } +} diff --git a/pkg/apiserver/errors.go b/pkg/apiserver/errors.go index a1413a4c31b..689652311e2 100755 --- a/pkg/apiserver/errors.go +++ b/pkg/apiserver/errors.go @@ -76,6 +76,13 @@ func notFound(w http.ResponseWriter, req *http.Request) { fmt.Fprintf(w, "Not Found: %#v", req.RequestURI) } +// internalError renders a simple internal error +func internalError(w http.ResponseWriter, req *http.Request, err error) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "Internal Server Error: %#v", req.RequestURI) + runtime.HandleError(err) +} + // errAPIPrefixNotFound indicates that a RequestInfo resolution failed because the request isn't under // any known API prefixes type errAPIPrefixNotFound struct { diff --git a/pkg/apiserver/filters/audit.go b/pkg/apiserver/filters/audit.go index b4c3c42487a..5e745cddda4 100644 --- a/pkg/apiserver/filters/audit.go +++ b/pkg/apiserver/filters/audit.go @@ -89,7 +89,11 @@ func WithAudit(handler http.Handler, attributeGetter RequestAttributeGetter, out return handler } return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - attribs := attributeGetter.GetAttribs(req) + attribs, err := attributeGetter.GetAttribs(req) + if err != nil { + internalError(w, req, err) + return + } asuser := req.Header.Get("Impersonate-User") if len(asuser) == 0 { asuser = "" diff --git a/pkg/apiserver/filters/audit_test.go b/pkg/apiserver/filters/audit_test.go index 1ac8599cf23..1b125fe6095 100644 --- a/pkg/apiserver/filters/audit_test.go +++ b/pkg/apiserver/filters/audit_test.go @@ -29,9 +29,8 @@ import ( "testing" "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/apiserver" + "k8s.io/kubernetes/pkg/apiserver/request" "k8s.io/kubernetes/pkg/auth/user" - "k8s.io/kubernetes/pkg/util/sets" ) type simpleResponseWriter struct { @@ -72,22 +71,14 @@ func (*fakeHTTPHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { w.WriteHeader(200) } -type fakeRequestContextMapper struct{} - -func (*fakeRequestContextMapper) Get(req *http.Request) (api.Context, bool) { - return api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "admin"}), true - -} - -func (*fakeRequestContextMapper) Update(req *http.Request, context api.Context) error { - return nil -} - func TestAudit(t *testing.T) { var buf bytes.Buffer - attributeGetter := NewRequestAttributeGetter(&fakeRequestContextMapper{}, - &apiserver.RequestInfoResolver{APIPrefixes: sets.NewString("api", "apis"), GrouplessAPIPrefixes: sets.NewString("api")}) + + attributeGetter := NewRequestAttributeGetter(&fakeRequestContextMapper{ + user: &user.DefaultInfo{Name: "admin"}, + }) handler := WithAudit(&fakeHTTPHandler{}, attributeGetter, &buf) + req, _ := http.NewRequest("GET", "/api/v1/namespaces/default/pods", nil) req.RemoteAddr = "127.0.0.1" handler.ServeHTTP(httptest.NewRecorder(), req) @@ -110,3 +101,26 @@ func TestAudit(t *testing.T) { t.Errorf("Unexpected second line of audit: %s", line[1]) } } + +type fakeRequestContextMapper struct { + user *user.DefaultInfo +} + +func (m *fakeRequestContextMapper) Get(req *http.Request) (api.Context, bool) { + ctx := api.NewContext() + if m.user != nil { + ctx = api.WithUser(ctx, m.user) + } + + resolver := newTestRequestInfoResolver() + info, err := resolver.GetRequestInfo(req) + if err == nil { + ctx = request.WithRequestInfo(ctx, info) + } + + return ctx, true +} + +func (*fakeRequestContextMapper) Update(req *http.Request, context api.Context) error { + return nil +} diff --git a/pkg/apiserver/filters/authorization.go b/pkg/apiserver/filters/authorization.go index 9395f811a0a..4a20c1db2cd 100644 --- a/pkg/apiserver/filters/authorization.go +++ b/pkg/apiserver/filters/authorization.go @@ -17,12 +17,13 @@ limitations under the License. package filters import ( + "errors" "net/http" "github.com/golang/glog" "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/apiserver" + "k8s.io/kubernetes/pkg/apiserver/request" "k8s.io/kubernetes/pkg/auth/authorizer" ) @@ -33,7 +34,12 @@ func WithAuthorization(handler http.Handler, getAttribs RequestAttributeGetter, return handler } return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - authorized, reason, err := a.Authorize(getAttribs.GetAttribs(req)) + attrs, err := getAttribs.GetAttribs(req) + if err != nil { + internalError(w, req, err) + return + } + authorized, reason, err := a.Authorize(attrs) if err != nil { internalError(w, req, err) return @@ -49,31 +55,35 @@ func WithAuthorization(handler http.Handler, getAttribs RequestAttributeGetter, // RequestAttributeGetter is a function that extracts authorizer.Attributes from an http.Request type RequestAttributeGetter interface { - GetAttribs(req *http.Request) (attribs authorizer.Attributes) + GetAttribs(req *http.Request) (authorizer.Attributes, error) } type requestAttributeGetter struct { requestContextMapper api.RequestContextMapper - requestInfoResolver *apiserver.RequestInfoResolver } // NewAttributeGetter returns an object which implements the RequestAttributeGetter interface. -func NewRequestAttributeGetter(requestContextMapper api.RequestContextMapper, requestInfoResolver *apiserver.RequestInfoResolver) RequestAttributeGetter { - return &requestAttributeGetter{requestContextMapper, requestInfoResolver} +func NewRequestAttributeGetter(requestContextMapper api.RequestContextMapper) RequestAttributeGetter { + return &requestAttributeGetter{requestContextMapper} } -func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attributes { +func (r *requestAttributeGetter) GetAttribs(req *http.Request) (authorizer.Attributes, error) { attribs := authorizer.AttributesRecord{} ctx, ok := r.requestContextMapper.Get(req) - if ok { - user, ok := api.UserFrom(ctx) - if ok { - attribs.User = user - } + if !ok { + return nil, errors.New("no context found for request") } - requestInfo, _ := r.requestInfoResolver.GetRequestInfo(req) + user, ok := api.UserFrom(ctx) + if ok { + attribs.User = user + } + + requestInfo, found := request.RequestInfoFrom(ctx) + if !found { + return nil, errors.New("no RequestInfo found in the context") + } // Start with common attributes that apply to resource and non-resource requests attribs.ResourceRequest = requestInfo.IsResourceRequest @@ -87,5 +97,5 @@ func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attrib attribs.Namespace = requestInfo.Namespace attribs.Name = requestInfo.Name - return &attribs + return &attribs, nil } diff --git a/pkg/apiserver/filters/authorization_test.go b/pkg/apiserver/filters/authorization_test.go index ad234554e4a..64181a7dac1 100644 --- a/pkg/apiserver/filters/authorization_test.go +++ b/pkg/apiserver/filters/authorization_test.go @@ -18,18 +18,18 @@ package filters import ( "net/http" + "net/http/httptest" "reflect" "testing" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/apis/extensions" - "k8s.io/kubernetes/pkg/apiserver" "k8s.io/kubernetes/pkg/auth/authorizer" - "k8s.io/kubernetes/pkg/util/sets" ) func TestGetAttribs(t *testing.T) { - r := &requestAttributeGetter{api.NewRequestContextMapper(), &apiserver.RequestInfoResolver{APIPrefixes: sets.NewString("api", "apis"), GrouplessAPIPrefixes: sets.NewString("api")}} + mapper := api.NewRequestContextMapper() + attributeGetter := NewRequestAttributeGetter(mapper) testcases := map[string]struct { Verb string @@ -103,8 +103,20 @@ func TestGetAttribs(t *testing.T) { for k, tc := range testcases { req, _ := http.NewRequest(tc.Verb, tc.Path, nil) - attribs := r.GetAttribs(req) - if !reflect.DeepEqual(attribs, tc.ExpectedAttributes) { + req.RemoteAddr = "127.0.0.1" + + var attribs authorizer.Attributes + var err error + var handler http.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + attribs, err = attributeGetter.GetAttribs(req) + }) + handler = WithRequestInfo(handler, newTestRequestInfoResolver(), mapper) + handler = api.WithRequestContext(handler, mapper) + handler.ServeHTTP(httptest.NewRecorder(), req) + + if err != nil { + t.Errorf("%s: unexpected error: %v", k, err) + } else if !reflect.DeepEqual(attribs, tc.ExpectedAttributes) { t.Errorf("%s: expected\n\t%#v\ngot\n\t%#v", k, tc.ExpectedAttributes, attribs) } } diff --git a/pkg/apiserver/filters/requestinfo.go b/pkg/apiserver/filters/requestinfo.go new file mode 100644 index 00000000000..0da847bbf27 --- /dev/null +++ b/pkg/apiserver/filters/requestinfo.go @@ -0,0 +1,46 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package filters + +import ( + "errors" + "fmt" + "net/http" + + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/apiserver/request" +) + +// WithRequestInfo attaches a RequestInfo to the context. +func WithRequestInfo(handler http.Handler, resolver *request.RequestInfoResolver, requestContextMapper api.RequestContextMapper) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + ctx, ok := requestContextMapper.Get(req) + if !ok { + internalError(w, req, errors.New("no context found for request")) + return + } + + info, err := resolver.GetRequestInfo(req) + if err != nil { + internalError(w, req, fmt.Errorf("failed to create RequestInfo: %v", err)) + } + + requestContextMapper.Update(req, request.WithRequestInfo(ctx, info)) + + handler.ServeHTTP(w, req) + }) +} diff --git a/pkg/apiserver/filters/requestinfo_test.go b/pkg/apiserver/filters/requestinfo_test.go new file mode 100644 index 00000000000..b7e2341f60e --- /dev/null +++ b/pkg/apiserver/filters/requestinfo_test.go @@ -0,0 +1,29 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package filters + +import ( + "k8s.io/kubernetes/pkg/apiserver/request" + "k8s.io/kubernetes/pkg/util/sets" +) + +func newTestRequestInfoResolver() *request.RequestInfoResolver { + return &request.RequestInfoResolver{ + APIPrefixes: sets.NewString("api", "apis"), + GrouplessAPIPrefixes: sets.NewString("api"), + } +} diff --git a/pkg/apiserver/proxy.go b/pkg/apiserver/proxy.go index 403d219ee93..0ed1fab196e 100644 --- a/pkg/apiserver/proxy.go +++ b/pkg/apiserver/proxy.go @@ -17,6 +17,7 @@ limitations under the License. package apiserver import ( + "errors" "io" "math/rand" "net/http" @@ -27,7 +28,7 @@ import ( "time" "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/api/errors" + apierrors "k8s.io/kubernetes/pkg/api/errors" "k8s.io/kubernetes/pkg/api/rest" "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/apiserver/metrics" @@ -38,16 +39,16 @@ import ( proxyutil "k8s.io/kubernetes/pkg/util/proxy" "github.com/golang/glog" + "k8s.io/kubernetes/pkg/apiserver/request" ) // ProxyHandler provides a http.Handler which will proxy traffic to locations // specified by items implementing Redirector. type ProxyHandler struct { - prefix string - storage map[string]rest.Storage - serializer runtime.NegotiatedSerializer - context api.RequestContextMapper - requestInfoResolver *RequestInfoResolver + prefix string + storage map[string]rest.Storage + serializer runtime.NegotiatedSerializer + mapper api.RequestContextMapper } func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { @@ -59,8 +60,19 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { reqStart := time.Now() defer metrics.Monitor(&verb, &apiResource, net.GetHTTPClient(req), w.Header().Get("Content-Type"), httpCode, reqStart) - requestInfo, err := r.requestInfoResolver.GetRequestInfo(req) - if err != nil || !requestInfo.IsResourceRequest { + ctx, ok := r.mapper.Get(req) + if !ok { + internalError(w, req, errors.New("Error getting request context")) + return + } + + requestInfo, ok := request.RequestInfoFrom(ctx) + if !ok { + internalError(w, req, errors.New("Error getting RequestInfo from context")) + httpCode = http.StatusInternalServerError + return + } + if !requestInfo.IsResourceRequest { notFound(w, req) httpCode = http.StatusNotFound return @@ -68,10 +80,6 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { verb = requestInfo.Verb namespace, resource, parts := requestInfo.Namespace, requestInfo.Resource, requestInfo.Parts - ctx, ok := r.context.Get(req) - if !ok { - ctx = api.NewContext() - } ctx = api.WithNamespace(ctx, namespace) if len(parts) < 2 { notFound(w, req) @@ -104,7 +112,7 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { redirector, ok := storage.(rest.Redirector) if !ok { httplog.LogOf(req, w).Addf("'%v' is not a redirector", resource) - httpCode = errorNegotiated(errors.NewMethodNotSupported(api.Resource(resource), "proxy"), r.serializer, gv, w, req) + httpCode = errorNegotiated(apierrors.NewMethodNotSupported(api.Resource(resource), "proxy"), r.serializer, gv, w, req) return } diff --git a/pkg/apiserver/proxy_test.go b/pkg/apiserver/proxy_test.go index 157025320c4..75601d637ee 100644 --- a/pkg/apiserver/proxy_test.go +++ b/pkg/apiserver/proxy_test.go @@ -204,7 +204,7 @@ func TestProxyRequestContentLengthAndTransferEncoding(t *testing.T) { expectedResourceNamespace: "default", } namespaceHandler := handleNamespaced(map[string]rest.Storage{"foo": simpleStorage}) - server := httptest.NewServer(namespaceHandler) + server := newTestServer(namespaceHandler) defer server.Close() // Dial the proxy server @@ -310,7 +310,7 @@ func TestProxy(t *testing.T) { } namespaceHandler := handleNamespaced(map[string]rest.Storage{"foo": simpleStorage}) - namespaceServer := httptest.NewServer(namespaceHandler) + namespaceServer := newTestServer(namespaceHandler) defer namespaceServer.Close() // test each supported URL pattern for finding the redirection resource in the proxy in a particular namespace @@ -432,7 +432,7 @@ func TestProxyUpgrade(t *testing.T) { namespaceHandler := handleNamespaced(map[string]rest.Storage{"foo": simpleStorage}) - server := httptest.NewServer(namespaceHandler) + server := newTestServer(namespaceHandler) defer server.Close() ws, err := websocket.Dial("ws://"+server.Listener.Addr().String()+"/"+prefix+"/"+newGroupVersion.Group+"/"+newGroupVersion.Version+"/proxy/namespaces/myns/foo/123", "", "http://127.0.0.1/") @@ -496,7 +496,7 @@ func TestRedirectOnMissingTrailingSlash(t *testing.T) { } handler := handleNamespaced(map[string]rest.Storage{"foo": simpleStorage}) - server := httptest.NewServer(handler) + server := newTestServer(handler) defer server.Close() proxyTestPattern := "/" + prefix + "/" + newGroupVersion.Group + "/" + newGroupVersion.Version + "/proxy/namespaces/ns/foo/id" + item.path diff --git a/pkg/apiserver/request/doc.go b/pkg/apiserver/request/doc.go new file mode 100644 index 00000000000..52a0c1e1428 --- /dev/null +++ b/pkg/apiserver/request/doc.go @@ -0,0 +1,20 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package request contains everything around extracting info from +// a http request object. +// TODO: this package is temporary. Handlers must move into pkg/apiserver/handlers to avoid dependency cycle +package request // import "k8s.io/kubernetes/pkg/apiserver/request" diff --git a/pkg/apiserver/requestinfo.go b/pkg/apiserver/request/requestinfo.go similarity index 86% rename from pkg/apiserver/requestinfo.go rename to pkg/apiserver/request/requestinfo.go index 8fb3bfeabf7..04bdb55cddd 100644 --- a/pkg/apiserver/requestinfo.go +++ b/pkg/apiserver/request/requestinfo.go @@ -1,5 +1,5 @@ /* -Copyright 2015 The Kubernetes Authors. +Copyright 2016 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package apiserver +package request import ( "fmt" @@ -68,8 +68,8 @@ var namespaceSubresources = sets.NewString("status", "finalize") var NamespaceSubResourcesForTest = sets.NewString(namespaceSubresources.List()...) type RequestInfoResolver struct { - APIPrefixes sets.String - GrouplessAPIPrefixes sets.String + APIPrefixes sets.String // without leading and trailing slashes + GrouplessAPIPrefixes sets.String // without leading and trailing slashes } // TODO write an integration test against the swagger doc to test the RequestInfo and match up behavior to responses @@ -103,7 +103,7 @@ type RequestInfoResolver struct { // /api // /healthz // / -func (r *RequestInfoResolver) GetRequestInfo(req *http.Request) (RequestInfo, error) { +func (r *RequestInfoResolver) GetRequestInfo(req *http.Request) (*RequestInfo, error) { // start with a non-resource request until proven otherwise requestInfo := RequestInfo{ IsResourceRequest: false, @@ -114,12 +114,12 @@ func (r *RequestInfoResolver) GetRequestInfo(req *http.Request) (RequestInfo, er currentParts := splitPath(req.URL.Path) if len(currentParts) < 3 { // return a non-resource request - return requestInfo, nil + return &requestInfo, nil } if !r.APIPrefixes.Has(currentParts[0]) { // return a non-resource request - return requestInfo, nil + return &requestInfo, nil } requestInfo.APIPrefix = currentParts[0] currentParts = currentParts[1:] @@ -128,7 +128,7 @@ func (r *RequestInfoResolver) GetRequestInfo(req *http.Request) (RequestInfo, er // one part (APIPrefix) has already been consumed, so this is actually "do we have four parts?" if len(currentParts) < 3 { // return a non-resource request - return requestInfo, nil + return &requestInfo, nil } requestInfo.APIGroup = currentParts[0] @@ -142,7 +142,7 @@ func (r *RequestInfoResolver) GetRequestInfo(req *http.Request) (RequestInfo, er // handle input of form /{specialVerb}/* if specialVerbs.Has(currentParts[0]) { if len(currentParts) < 2 { - return requestInfo, fmt.Errorf("unable to determine kind and namespace from url, %v", req.URL) + return &requestInfo, fmt.Errorf("unable to determine kind and namespace from url, %v", req.URL) } requestInfo.Verb = currentParts[0] @@ -204,7 +204,25 @@ func (r *RequestInfoResolver) GetRequestInfo(req *http.Request) (RequestInfo, er requestInfo.Verb = "deletecollection" } - return requestInfo, nil + return &requestInfo, nil +} + +type requestInfoKeyType int + +// requestInfoKey is the RequestInfo key for the context. It's of private type here. Because +// keys are interfaces and interfaces are equal when the type and the value is equal, this +// does not conflict with the keys defined in pkg/api. +const requestInfoKey requestInfoKeyType = iota + +// WithRequestInfo returns a copy of parent in which the request info value is set +func WithRequestInfo(parent api.Context, info *RequestInfo) api.Context { + return api.WithValue(parent, requestInfoKey, info) +} + +// RequestInfoFrom returns the value of the RequestInfo key on the ctx +func RequestInfoFrom(ctx api.Context) (*RequestInfo, bool) { + info, ok := ctx.Value(requestInfoKey).(*RequestInfo) + return info, ok } // splitPath returns the segments for a URL path. diff --git a/pkg/apiserver/requestinfo_test.go b/pkg/apiserver/request/requestinfo_test.go similarity index 98% rename from pkg/apiserver/requestinfo_test.go rename to pkg/apiserver/request/requestinfo_test.go index efb8432ccb3..13c59a5511e 100644 --- a/pkg/apiserver/requestinfo_test.go +++ b/pkg/apiserver/request/requestinfo_test.go @@ -1,5 +1,5 @@ /* -Copyright 2014 The Kubernetes Authors. +Copyright 2016 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package apiserver +package request import ( "net/http" @@ -194,5 +194,8 @@ func TestGetNonAPIRequestInfo(t *testing.T) { } func newTestRequestInfoResolver() *RequestInfoResolver { - return &RequestInfoResolver{sets.NewString("api", "apis"), sets.NewString("api")} + return &RequestInfoResolver{ + APIPrefixes: sets.NewString("api", "apis"), + GrouplessAPIPrefixes: sets.NewString("api"), + } } diff --git a/pkg/genericapiserver/config.go b/pkg/genericapiserver/config.go index 64cd48a8ac9..24cdd6573f3 100644 --- a/pkg/genericapiserver/config.go +++ b/pkg/genericapiserver/config.go @@ -354,17 +354,21 @@ func (s *GenericAPIServer) buildHandlerChains(c *Config, handler http.Handler) ( // insecure filters insecure = handler - insecure = genericfilters.WithPanicRecovery(insecure, s.NewRequestInfoResolver()) + insecure = genericfilters.WithPanicRecovery(insecure, c.RequestContextMapper) + insecure = apiserverfilters.WithRequestInfo(insecure, s.NewRequestInfoResolver(), c.RequestContextMapper) + insecure = api.WithRequestContext(insecure, c.RequestContextMapper) insecure = genericfilters.WithTimeoutForNonLongRunningRequests(insecure, c.LongRunningFunc) // secure filters - attributeGetter := apiserverfilters.NewRequestAttributeGetter(c.RequestContextMapper, s.NewRequestInfoResolver()) + attributeGetter := apiserverfilters.NewRequestAttributeGetter(c.RequestContextMapper) secure = handler secure = apiserverfilters.WithAuthorization(secure, attributeGetter, c.Authorizer) secure = apiserverfilters.WithImpersonation(secure, c.RequestContextMapper, c.Authorizer) secure = apiserverfilters.WithAudit(secure, attributeGetter, c.AuditWriter) // before impersonation to read original user secure = authhandlers.WithAuthentication(secure, c.RequestContextMapper, c.Authenticator, authhandlers.Unauthorized(c.SupportsBasicAuth)) - secure = genericfilters.WithPanicRecovery(secure, s.NewRequestInfoResolver()) + secure = genericfilters.WithPanicRecovery(secure, c.RequestContextMapper) + secure = apiserverfilters.WithRequestInfo(secure, s.NewRequestInfoResolver(), c.RequestContextMapper) + secure = api.WithRequestContext(secure, c.RequestContextMapper) secure = genericfilters.WithTimeoutForNonLongRunningRequests(secure, c.LongRunningFunc) secure = genericfilters.WithMaxInFlightLimit(secure, c.MaxRequestsInFlight, c.LongRunningFunc) diff --git a/pkg/genericapiserver/filters/panics.go b/pkg/genericapiserver/filters/panics.go index 91470b6bf7f..f46f399dcb6 100644 --- a/pkg/genericapiserver/filters/panics.go +++ b/pkg/genericapiserver/filters/panics.go @@ -22,14 +22,15 @@ import ( "github.com/golang/glog" - "k8s.io/kubernetes/pkg/api/errors" - "k8s.io/kubernetes/pkg/apiserver" + "k8s.io/kubernetes/pkg/api" + apierrors "k8s.io/kubernetes/pkg/api/errors" + "k8s.io/kubernetes/pkg/apiserver/request" "k8s.io/kubernetes/pkg/httplog" "k8s.io/kubernetes/pkg/util/runtime" ) // WithPanicRecovery wraps an http Handler to recover and log panics. -func WithPanicRecovery(handler http.Handler, resolver *apiserver.RequestInfoResolver) http.Handler { +func WithPanicRecovery(handler http.Handler, requestContextMapper api.RequestContextMapper) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { defer runtime.HandleCrash(func(err interface{}) { http.Error(w, "This request caused apisever to panic. Look in log for details.", http.StatusInternalServerError) @@ -37,8 +38,19 @@ func WithPanicRecovery(handler http.Handler, resolver *apiserver.RequestInfoReso }) logger := httplog.NewLogged(req, &w) - requestInfo, err := resolver.GetRequestInfo(req) - if err != nil || requestInfo.Verb != "proxy" { + + var requestInfo *request.RequestInfo + ctx, ok := requestContextMapper.Get(req) + if !ok { + glog.Errorf("no context found for request, handler chain must be wrong") + } else { + requestInfo, ok = request.RequestInfoFrom(ctx) + if !ok { + glog.Errorf("no RequestInfo found in context, handler chain must be wrong") + } + } + + if !ok || requestInfo.Verb != "proxy" { logger.StacktraceWhen( httplog.StatusIsNot( http.StatusOK, @@ -52,12 +64,13 @@ func WithPanicRecovery(handler http.Handler, resolver *apiserver.RequestInfoReso http.StatusUnauthorized, http.StatusForbidden, http.StatusNotModified, - errors.StatusUnprocessableEntity, + apierrors.StatusUnprocessableEntity, http.StatusSwitchingProtocols, ), ) } defer logger.Log() + // Dispatch to the internal handler handler.ServeHTTP(w, req) }) diff --git a/pkg/genericapiserver/genericapiserver.go b/pkg/genericapiserver/genericapiserver.go index 39e48ab6079..0eb30ce3c03 100644 --- a/pkg/genericapiserver/genericapiserver.go +++ b/pkg/genericapiserver/genericapiserver.go @@ -42,6 +42,7 @@ import ( "k8s.io/kubernetes/pkg/apimachinery" "k8s.io/kubernetes/pkg/apimachinery/registered" "k8s.io/kubernetes/pkg/apiserver" + "k8s.io/kubernetes/pkg/apiserver/request" "k8s.io/kubernetes/pkg/genericapiserver/openapi" "k8s.io/kubernetes/pkg/genericapiserver/openapi/common" "k8s.io/kubernetes/pkg/genericapiserver/options" @@ -187,8 +188,8 @@ func (s *GenericAPIServer) MinRequestTimeout() time.Duration { return s.minRequestTimeout } -func (s *GenericAPIServer) NewRequestInfoResolver() *apiserver.RequestInfoResolver { - return &apiserver.RequestInfoResolver{ +func (s *GenericAPIServer) NewRequestInfoResolver() *request.RequestInfoResolver { + return &request.RequestInfoResolver{ APIPrefixes: sets.NewString(strings.Trim(s.legacyAPIPrefix, "/"), strings.Trim(s.apiPrefix, "/")), // all possible API prefixes GrouplessAPIPrefixes: sets.NewString(strings.Trim(s.legacyAPIPrefix, "/")), // APIPrefixes that won't have groups (legacy) } @@ -452,8 +453,6 @@ func (s *GenericAPIServer) getAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV func (s *GenericAPIServer) newAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupVersion unversioned.GroupVersion) (*apiserver.APIGroupVersion, error) { return &apiserver.APIGroupVersion{ - RequestInfoResolver: s.NewRequestInfoResolver(), - GroupVersion: groupVersion, ParameterCodec: apiGroupInfo.ParameterCodec, diff --git a/pkg/master/master.go b/pkg/master/master.go index f68bdfd3434..42463a30154 100644 --- a/pkg/master/master.go +++ b/pkg/master/master.go @@ -785,9 +785,8 @@ func (m *Master) thirdpartyapi(group, kind, version, pluralResource string) *api apiRoot := extensionsrest.MakeThirdPartyPath("") return &apiserver.APIGroupVersion{ - Root: apiRoot, - GroupVersion: externalVersion, - RequestInfoResolver: m.NewRequestInfoResolver(), + Root: apiRoot, + GroupVersion: externalVersion, Creater: thirdpartyresourcedata.NewObjectCreator(group, version, api.Scheme), Convertor: api.Scheme, diff --git a/pkg/master/master_test.go b/pkg/master/master_test.go index 931ef81c02e..30ad0a7b5cb 100644 --- a/pkg/master/master_test.go +++ b/pkg/master/master_test.go @@ -47,7 +47,8 @@ import ( "k8s.io/kubernetes/pkg/apis/extensions" extensionsapiv1beta1 "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" "k8s.io/kubernetes/pkg/apis/rbac" - "k8s.io/kubernetes/pkg/apiserver" + "k8s.io/kubernetes/pkg/apiserver/request" + "k8s.io/kubernetes/pkg/generated/openapi" "k8s.io/kubernetes/pkg/genericapiserver" "k8s.io/kubernetes/pkg/kubelet/client" "k8s.io/kubernetes/pkg/registry/core/endpoint" @@ -72,7 +73,6 @@ import ( "github.com/go-openapi/validate" "github.com/stretchr/testify/assert" "golang.org/x/net/context" - "k8s.io/kubernetes/pkg/generated/openapi" ) // setUp is a convience function for setting up for (most) tests. @@ -192,7 +192,7 @@ func TestNamespaceSubresources(t *testing.T) { master, etcdserver, _, _ := newMaster(t) defer etcdserver.Terminate(t) - expectedSubresources := apiserver.NamespaceSubResourcesForTest + expectedSubresources := request.NamespaceSubResourcesForTest foundSubresources := sets.NewString() for k := range master.v1ResourcesStorage {