From dc8d0de70bd113881be107dbff35d82d48c8f8ea Mon Sep 17 00:00:00 2001 From: deads2k Date: Tue, 22 Sep 2015 15:43:29 -0400 Subject: [PATCH 1/3] update APIRequestInfo for APIGroup --- pkg/apiserver/apiserver.go | 40 +++++++++++++++-- pkg/apiserver/errors.go | 19 ++++++++ pkg/apiserver/handlers.go | 82 ++++++++++++++++++---------------- pkg/apiserver/handlers_test.go | 55 ++++++++++++----------- pkg/master/master.go | 11 +++-- 5 files changed, 134 insertions(+), 73 deletions(-) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 7ef887e1dda..50ab1d9ae2b 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -78,6 +78,10 @@ type Mux interface { type APIGroupVersion struct { Storage map[string]rest.Storage + // Root is the APIPrefix under which this is being served. It can also be the APIPrefix/APIGroup that is being served + // Since the APIGroup may not contain a '/', you can get the APIGroup by parsing from the last '/' + // TODO Currently, an APIPrefix with a '/' is not supported in conjunction with an empty APIGroup. This struct should + // be refactored to keep separate information separate to avoid this sort of problem in the future. Root string Version string @@ -112,6 +116,38 @@ const ( MaxTimeoutSecs = 600 ) +func (g *APIGroupVersion) GetAPIPrefix() string { + slashlessRoot := strings.Trim(g.Root, "/") + if lastSlashIndex := strings.LastIndex(slashlessRoot, "/"); lastSlashIndex != -1 { + return slashlessRoot[:lastSlashIndex] + } + + return slashlessRoot +} + +func (g *APIGroupVersion) GetAPIGroup() string { + slashlessRoot := strings.Trim(g.Root, "/") + if lastSlashIndex := strings.LastIndex(slashlessRoot, "/"); lastSlashIndex != -1 { + return slashlessRoot[lastSlashIndex:] + } + + return "" +} + +func (g *APIGroupVersion) GetAPIVersion() string { + return g.Version +} + +func (g *APIGroupVersion) GetAPIRequestInfoResolver() *APIRequestInfoResolver { + apiPrefix := g.GetAPIPrefix() + info := &APIRequestInfoResolver{sets.NewString(apiPrefix), sets.String{}, g.Mapper} + if len(g.GetAPIGroup()) == 0 { + info.GrouplessAPIPrefixes.Insert(apiPrefix) + } + + return info +} + // InstallREST registers the REST handlers (storage, watch, proxy and redirect) into a restful Container. // It is expected that the provided path root prefix will serve all operations. Root MUST NOT end // in a slash. @@ -151,12 +187,10 @@ func (g *APIGroupVersion) UpdateREST(container *restful.Container) error { // newInstaller is a helper to create the installer. Used by InstallREST and UpdateREST. func (g *APIGroupVersion) newInstaller() *APIInstaller { - info := &APIRequestInfoResolver{sets.NewString(strings.TrimPrefix(g.Root, "/")), g.Mapper} - prefix := path.Join(g.Root, g.Version) installer := &APIInstaller{ group: g, - info: info, + info: g.GetAPIRequestInfoResolver(), prefix: prefix, minRequestTimeout: g.MinRequestTimeout, proxyDialerFn: g.ProxyDialerFn, diff --git a/pkg/apiserver/errors.go b/pkg/apiserver/errors.go index 16cc8b1f173..a7c1fbdf328 100644 --- a/pkg/apiserver/errors.go +++ b/pkg/apiserver/errors.go @@ -86,3 +86,22 @@ func forbidden(w http.ResponseWriter, req *http.Request) { w.WriteHeader(http.StatusForbidden) fmt.Fprintf(w, "Forbidden: %#v", req.RequestURI) } + +// errAPIPrefixNotFound indicates that a APIRequestInfo resolution failed because the request isn't under +// any known API prefixes +type errAPIPrefixNotFound struct { + SpecifiedPrefix string +} + +func (e *errAPIPrefixNotFound) Error() string { + return fmt.Sprintf("no valid API prefix found matching %v", e.SpecifiedPrefix) +} + +func IsAPIPrefixNotFound(err error) bool { + if err == nil { + return false + } + + _, ok := err.(*errAPIPrefixNotFound) + return ok +} diff --git a/pkg/apiserver/handlers.go b/pkg/apiserver/handlers.go index 8e4cefb124d..788795d2212 100644 --- a/pkg/apiserver/handlers.go +++ b/pkg/apiserver/handlers.go @@ -348,8 +348,8 @@ type requestAttributeGetter struct { } // NewAttributeGetter returns an object which implements the RequestAttributeGetter interface. -func NewRequestAttributeGetter(requestContextMapper api.RequestContextMapper, restMapper meta.RESTMapper, apiRoots ...string) RequestAttributeGetter { - return &requestAttributeGetter{requestContextMapper, &APIRequestInfoResolver{sets.NewString(apiRoots...), restMapper}} +func NewRequestAttributeGetter(requestContextMapper api.RequestContextMapper, restMapper meta.RESTMapper, apiRoots []string, grouplessAPIRoots []string) RequestAttributeGetter { + return &requestAttributeGetter{requestContextMapper, &APIRequestInfoResolver{sets.NewString(apiRoots...), sets.NewString(grouplessAPIRoots...), restMapper}} } func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attributes { @@ -395,6 +395,8 @@ func WithAuthorizationCheck(handler http.Handler, getAttribs RequestAttributeGet type APIRequestInfo struct { // Verb is the kube verb associated with the request, not the http verb. This includes things like list and watch. Verb string + APIPrefix string + APIGroup string APIVersion string Namespace string // Resource is the name of the resource being requested. This is not the kind. For example: pods @@ -415,66 +417,68 @@ type APIRequestInfo struct { } type APIRequestInfoResolver struct { - APIPrefixes sets.String - RestMapper meta.RESTMapper + APIPrefixes sets.String + GrouplessAPIPrefixes sets.String + RestMapper meta.RESTMapper } // TODO write an integration test against the swagger doc to test the APIRequestInfo and match up behavior to responses // GetAPIRequestInfo returns the information from the http request. If error is not nil, APIRequestInfo holds the information as best it is known before the failure // Valid Inputs: // Storage paths -// /namespaces -// /namespaces/{namespace} -// /namespaces/{namespace}/{resource} -// /namespaces/{namespace}/{resource}/{resourceName} -// /{resource} -// /{resource}/{resourceName} +// /apis/{api-group}/{version}/namespaces +// /api/{version}/namespaces +// /api/{version}/namespaces/{namespace} +// /api/{version}/namespaces/{namespace}/{resource} +// /api/{version}/namespaces/{namespace}/{resource}/{resourceName} +// /api/{version}/{resource} +// /api/{version}/{resource}/{resourceName} // // Special verbs: -// /proxy/{resource}/{resourceName} -// /proxy/namespaces/{namespace}/{resource}/{resourceName} -// /redirect/namespaces/{namespace}/{resource}/{resourceName} -// /redirect/{resource}/{resourceName} -// /watch/{resource} -// /watch/namespaces/{namespace}/{resource} -// -// Fully qualified paths for above: -// /api/{version}/* -// /api/{version}/* +// /api/{version}/proxy/{resource}/{resourceName} +// /api/{version}/proxy/namespaces/{namespace}/{resource}/{resourceName} +// /api/{version}/redirect/namespaces/{namespace}/{resource}/{resourceName} +// /api/{version}/redirect/{resource}/{resourceName} +// /api/{version}/watch/{resource} +// /api/{version}/watch/namespaces/{namespace}/{resource} func (r *APIRequestInfoResolver) GetAPIRequestInfo(req *http.Request) (APIRequestInfo, error) { requestInfo := APIRequestInfo{ Raw: 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") + if len(currentParts) < 3 { + return requestInfo, fmt.Errorf("a resource request must have a url with at least three parts, not %v", req.URL) } - for _, currPrefix := range r.APIPrefixes.List() { - // handle input of form /api/{version}/* by adjusting special paths - if currentParts[0] == currPrefix { - if len(currentParts) > 1 { - requestInfo.APIVersion = currentParts[1] - } + if !r.APIPrefixes.Has(currentParts[0]) { + return requestInfo, &errAPIPrefixNotFound{currentParts[0]} + } + requestInfo.APIPrefix = currentParts[0] + currentParts = currentParts[1:] - if len(currentParts) > 2 { - currentParts = currentParts[2:] - } else { - return requestInfo, fmt.Errorf("Unable to determine kind and namespace from url, %v", req.URL) - } + if !r.GrouplessAPIPrefixes.Has(requestInfo.APIPrefix) { + // one part (APIPrefix) has already been consumed, so this is actually "do we have four parts?" + if len(currentParts) < 3 { + return requestInfo, fmt.Errorf("a resource request with an API group must have a url with at least four parts, not %v", req.URL) } + + requestInfo.APIGroup = currentParts[0] + currentParts = currentParts[1:] } + requestInfo.APIVersion = currentParts[0] + currentParts = currentParts[1:] + // handle input of form /{specialVerb}/* if _, ok := specialVerbs[currentParts[0]]; ok { - requestInfo.Verb = currentParts[0] - - if len(currentParts) > 1 { - currentParts = currentParts[1:] - } else { - return requestInfo, fmt.Errorf("Unable to determine kind and namespace from url, %v", req.URL) + if len(currentParts) < 2 { + return requestInfo, fmt.Errorf("unable to determine kind and namespace from url, %v", req.URL) } + + requestInfo.Verb = currentParts[0] + currentParts = currentParts[1:] + } else { switch req.Method { case "POST": diff --git a/pkg/apiserver/handlers_test.go b/pkg/apiserver/handlers_test.go index b54e3cbc8c0..a0927757dd8 100644 --- a/pkg/apiserver/handlers_test.go +++ b/pkg/apiserver/handlers_test.go @@ -199,6 +199,8 @@ func TestGetAPIRequestInfo(t *testing.T) { method string url string expectedVerb string + expectedAPIPrefix string + expectedAPIGroup string expectedAPIVersion string expectedNamespace string expectedResource string @@ -209,42 +211,38 @@ func TestGetAPIRequestInfo(t *testing.T) { }{ // resource paths - {"GET", "/namespaces", "list", "", "", "namespaces", "", "Namespace", "", []string{"namespaces"}}, - {"GET", "/namespaces/other", "get", "", "other", "namespaces", "", "Namespace", "other", []string{"namespaces", "other"}}, + {"GET", "/api/v1/namespaces", "list", "api", "", "v1", "", "namespaces", "", "Namespace", "", []string{"namespaces"}}, + {"GET", "/api/v1/namespaces/other", "get", "api", "", "v1", "other", "namespaces", "", "Namespace", "other", []string{"namespaces", "other"}}, - {"GET", "/namespaces/other/pods", "list", "", "other", "pods", "", "Pod", "", []string{"pods"}}, - {"GET", "/namespaces/other/pods/foo", "get", "", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/pods", "list", "", api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", "/namespaces/other/pods/foo", "get", "", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/namespaces/other/pods", "list", "", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/namespaces/other/pods", "list", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/namespaces/other/pods/foo", "get", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/pods", "list", "api", "", "v1", api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/namespaces/other/pods/foo", "get", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/namespaces/other/pods", "list", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, // special verbs - {"GET", "/proxy/namespaces/other/pods/foo", "proxy", "", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/redirect/namespaces/other/pods/foo", "redirect", "", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/watch/pods", "watch", "", api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", "/watch/namespaces/other/pods", "watch", "", "other", "pods", "", "Pod", "", []string{"pods"}}, - - // fully-qualified paths - {"GET", getPath("pods", "other", ""), "list", testapi.Default.Version(), "other", "pods", "", "Pod", "", []string{"pods"}}, - {"GET", getPath("pods", "other", "foo"), "get", testapi.Default.Version(), "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", getPath("pods", "", ""), "list", testapi.Default.Version(), api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"POST", getPath("pods", "", ""), "create", testapi.Default.Version(), api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", getPath("pods", "", "foo"), "get", testapi.Default.Version(), api.NamespaceAll, "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", pathWithPrefix("proxy", "pods", "", "foo"), "proxy", testapi.Default.Version(), api.NamespaceAll, "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", pathWithPrefix("watch", "pods", "", ""), "watch", testapi.Default.Version(), api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", pathWithPrefix("redirect", "pods", "", ""), "redirect", testapi.Default.Version(), api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", pathWithPrefix("watch", "pods", "other", ""), "watch", testapi.Default.Version(), "other", "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/proxy/namespaces/other/pods/foo", "proxy", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/redirect/namespaces/other/pods/foo", "redirect", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/watch/pods", "watch", "api", "", "v1", api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/watch/namespaces/other/pods", "watch", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, // subresource identification - {"GET", "/namespaces/other/pods/foo/status", "get", "", "other", "pods", "status", "Pod", "foo", []string{"pods", "foo", "status"}}, - {"PUT", "/namespaces/other/finalize", "update", "", "other", "finalize", "", "", "", []string{"finalize"}}, + {"GET", "/api/v1/namespaces/other/pods/foo/status", "get", "api", "", "v1", "other", "pods", "status", "Pod", "foo", []string{"pods", "foo", "status"}}, + {"PUT", "/api/v1/namespaces/other/finalize", "update", "api", "", "v1", "other", "finalize", "", "", "", []string{"finalize"}}, // verb identification - {"PATCH", "/namespaces/other/pods/foo", "patch", "", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"DELETE", "/namespaces/other/pods/foo", "delete", "", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"PATCH", "/api/v1/namespaces/other/pods/foo", "patch", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"DELETE", "/api/v1/namespaces/other/pods/foo", "delete", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, + {"POST", "/api/v1/namespaces/other/pods", "create", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + + // api group identification + {"POST", "/apis/experimental/v1/namespaces/other/pods", "create", "api", "experimental", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + + // api version identification + {"POST", "/apis/experimental/v1beta3/namespaces/other/pods", "create", "api", "experimental", "v1beta3", "other", "pods", "", "Pod", "", []string{"pods"}}, } - apiRequestInfoResolver := &APIRequestInfoResolver{sets.NewString("api"), testapi.Default.RESTMapper()} + apiRequestInfoResolver := &APIRequestInfoResolver{sets.NewString("api", "apis"), sets.NewString("api"), testapi.Default.RESTMapper()} for _, successCase := range successCases { req, _ := http.NewRequest(successCase.method, successCase.url, nil) @@ -282,7 +280,10 @@ func TestGetAPIRequestInfo(t *testing.T) { errorCases := map[string]string{ "no resource path": "/", "just apiversion": "/api/version/", + "just prefix, group, version": "/apis/group/version/", "apiversion with no resource": "/api/version/", + "bad prefix": "/badprefix/version/resource", + "missing api group": "/apis/version/resource", } for k, v := range errorCases { req, err := http.NewRequest("GET", v, nil) diff --git a/pkg/master/master.go b/pkg/master/master.go index 71d4afbf808..01e9088bf61 100644 --- a/pkg/master/master.go +++ b/pkg/master/master.go @@ -573,7 +573,7 @@ func (m *Master) init(c *Config) { apiserver.InstallSupport(m.muxHelper, m.rootWebService, c.EnableProfiling, healthzChecks...) apiserver.AddApiWebService(m.handlerContainer, c.APIPrefix, apiVersions) defaultVersion := m.defaultAPIGroupVersion() - requestInfoResolver := &apiserver.APIRequestInfoResolver{APIPrefixes: sets.NewString(strings.TrimPrefix(defaultVersion.Root, "/")), RestMapper: defaultVersion.Mapper} + requestInfoResolver := defaultVersion.GetAPIRequestInfoResolver() apiserver.InstallServiceErrorHandler(m.handlerContainer, requestInfoResolver, apiVersions) // allGroups records all supported groups at /apis @@ -608,7 +608,7 @@ func (m *Master) init(c *Config) { } apiserver.AddGroupWebService(m.handlerContainer, c.APIGroupPrefix+"/"+latest.GroupOrDie("experimental").Group+"/", group) allGroups = append(allGroups, group) - expRequestInfoResolver := &apiserver.APIRequestInfoResolver{APIPrefixes: sets.NewString(strings.TrimPrefix(expVersion.Root, "/")), RestMapper: expVersion.Mapper} + expRequestInfoResolver := expVersion.GetAPIRequestInfoResolver() apiserver.InstallServiceErrorHandler(m.handlerContainer, expRequestInfoResolver, []string{expVersion.Version}) } @@ -652,7 +652,10 @@ func (m *Master) init(c *Config) { m.InsecureHandler = handler - attributeGetter := apiserver.NewRequestAttributeGetter(m.requestContextMapper, latest.GroupOrDie("").RESTMapper, "api") + attributeGetter := apiserver.NewRequestAttributeGetter(m.requestContextMapper, latest.GroupOrDie("").RESTMapper, + []string{strings.Trim(c.APIPrefix, "/"), strings.Trim(thirdpartyprefix, "/")}, // all possible API prefixes + []string{strings.Trim(c.APIPrefix, "/")}, // APIPrefixes that won't have groups (legacy) + ) handler = apiserver.WithAuthorizationCheck(handler, attributeGetter, m.authorizer) // Install Authenticator @@ -918,7 +921,7 @@ func (m *Master) InstallThirdPartyResource(rsrc *expapi.ThirdPartyResource) erro } apiserver.AddGroupWebService(m.handlerContainer, path, apiGroup) m.addThirdPartyResourceStorage(path, thirdparty.Storage[strings.ToLower(kind)+"s"].(*thirdpartyresourcedataetcd.REST)) - thirdPartyRequestInfoResolver := &apiserver.APIRequestInfoResolver{APIPrefixes: sets.NewString(strings.TrimPrefix(group, "/")), RestMapper: thirdparty.Mapper} + thirdPartyRequestInfoResolver := &apiserver.APIRequestInfoResolver{APIPrefixes: sets.NewString(strings.Trim(thirdpartyprefix, "/")), RestMapper: thirdparty.Mapper} apiserver.InstallServiceErrorHandler(m.handlerContainer, thirdPartyRequestInfoResolver, []string{thirdparty.Version}) return nil } From 8db054651c90accff95f890461c456b209e7424e Mon Sep 17 00:00:00 2001 From: deads2k Date: Wed, 23 Sep 2015 11:23:49 -0400 Subject: [PATCH 2/3] plumb APIGroup to authorization attributes and test --- pkg/apiserver/handlers.go | 2 + pkg/auth/authorizer/interfaces.go | 8 +++ test/integration/auth_test.go | 88 +++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+) diff --git a/pkg/apiserver/handlers.go b/pkg/apiserver/handlers.go index 788795d2212..296ea9aa32b 100644 --- a/pkg/apiserver/handlers.go +++ b/pkg/apiserver/handlers.go @@ -367,6 +367,8 @@ func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attrib apiRequestInfo, _ := r.apiRequestInfoResolver.GetAPIRequestInfo(req) + attribs.APIGroup = apiRequestInfo.APIGroup + // If a path follows the conventions of the REST object store, then // we can extract the resource. Otherwise, not. attribs.Resource = apiRequestInfo.Resource diff --git a/pkg/auth/authorizer/interfaces.go b/pkg/auth/authorizer/interfaces.go index b93da19ab14..d64576764d7 100644 --- a/pkg/auth/authorizer/interfaces.go +++ b/pkg/auth/authorizer/interfaces.go @@ -41,6 +41,9 @@ type Attributes interface { // The kind of object, if a request is for a REST object. GetResource() string + + // The group of the resource, if a request is for a REST object. + GetAPIGroup() string } // Authorizer makes an authorization decision based on information gained by making @@ -61,6 +64,7 @@ type AttributesRecord struct { User user.Info ReadOnly bool Namespace string + APIGroup string Resource string } @@ -83,3 +87,7 @@ func (a AttributesRecord) GetNamespace() string { func (a AttributesRecord) GetResource() string { return a.Resource } + +func (a AttributesRecord) GetAPIGroup() string { + return a.APIGroup +} diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go index c6cc30b3ebf..54b9f3d64ba 100644 --- a/test/integration/auth_test.go +++ b/test/integration/auth_test.go @@ -792,6 +792,94 @@ func newAuthorizerWithContents(t *testing.T, contents string) authorizer.Authori return pl } +type trackingAuthorizer struct { + requestAttributes []authorizer.Attributes +} + +func (a *trackingAuthorizer) Authorize(attributes authorizer.Attributes) error { + a.requestAttributes = append(a.requestAttributes, attributes) + return nil +} + +// TestAuthorizationAttributeDetermination tests that authorization attributes are built correctly +func TestAuthorizationAttributeDetermination(t *testing.T) { + framework.DeleteAllEtcdKeys() + + etcdStorage, err := framework.NewEtcdStorage() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + trackingAuthorizer := &trackingAuthorizer{} + + var m *master.Master + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + m.Handler.ServeHTTP(w, req) + })) + defer s.Close() + + m = master.New(&master.Config{ + DatabaseStorage: etcdStorage, + KubeletClient: client.FakeKubeletClient{}, + EnableCoreControllers: true, + EnableLogsSupport: false, + EnableUISupport: false, + EnableIndex: true, + APIPrefix: "/api", + Authenticator: getTestTokenAuth(), + Authorizer: trackingAuthorizer, + AdmissionControl: admit.NewAlwaysAdmit(), + StorageVersions: map[string]string{"": testapi.Default.Version()}, + }) + + transport := http.DefaultTransport + + requests := map[string]struct { + verb string + URL string + expectedAttributes authorizer.Attributes + }{ + "prefix/version/resource": {"GET", "/api/v1/pods", authorizer.AttributesRecord{APIGroup: "", Resource: "pods"}}, + "prefix/group/version/resource": {"GET", "/apis/experimental/v1/pods", authorizer.AttributesRecord{APIGroup: "experimental", Resource: "pods"}}, + } + + currentAuthorizationAttributesIndex := 0 + + for testName, r := range requests { + token := BobToken + req, err := http.NewRequest(r.verb, s.URL+r.URL, nil) + if err != nil { + t.Logf("case %v", testName) + t.Fatalf("unexpected error: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + func() { + resp, err := transport.RoundTrip(req) + defer resp.Body.Close() + if err != nil { + t.Logf("case %v", r) + t.Fatalf("unexpected error: %v", err) + } + + found := false + for i := currentAuthorizationAttributesIndex; i < len(trackingAuthorizer.requestAttributes); i++ { + if trackingAuthorizer.requestAttributes[i].GetAPIGroup() == r.expectedAttributes.GetAPIGroup() && + trackingAuthorizer.requestAttributes[i].GetResource() == r.expectedAttributes.GetResource() { + found = true + break + } + + t.Logf("%#v did not match %#v", r.expectedAttributes, trackingAuthorizer.requestAttributes[i].(*authorizer.AttributesRecord)) + } + if !found { + t.Errorf("did not find %#v in %#v", r.expectedAttributes, trackingAuthorizer.requestAttributes[currentAuthorizationAttributesIndex:]) + } + + currentAuthorizationAttributesIndex = len(trackingAuthorizer.requestAttributes) + }() + } +} + // TestNamespaceAuthorization tests that authorization can be controlled // by namespace. func TestNamespaceAuthorization(t *testing.T) { From df870cf36a742ad232894112e5670bd15f56c435 Mon Sep 17 00:00:00 2001 From: deads2k Date: Fri, 25 Sep 2015 14:57:10 -0400 Subject: [PATCH 3/3] remove Kind from APIRequestInfo --- pkg/apiserver/apiserver.go | 43 ++++----------------------------- pkg/apiserver/apiserver_test.go | 41 +++++++++++++++++++------------ pkg/apiserver/handlers.go | 13 ++-------- pkg/apiserver/handlers_test.go | 43 +++++++++++++++------------------ pkg/master/master.go | 29 ++++++++++++---------- 5 files changed, 67 insertions(+), 102 deletions(-) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 50ab1d9ae2b..ec19f11e705 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -42,7 +42,6 @@ import ( "k8s.io/kubernetes/pkg/util" "k8s.io/kubernetes/pkg/util/errors" "k8s.io/kubernetes/pkg/util/flushwriter" - "k8s.io/kubernetes/pkg/util/sets" "k8s.io/kubernetes/pkg/version" "github.com/emicklei/go-restful" @@ -78,13 +77,13 @@ type Mux interface { type APIGroupVersion struct { Storage map[string]rest.Storage - // Root is the APIPrefix under which this is being served. It can also be the APIPrefix/APIGroup that is being served - // Since the APIGroup may not contain a '/', you can get the APIGroup by parsing from the last '/' - // TODO Currently, an APIPrefix with a '/' is not supported in conjunction with an empty APIGroup. This struct should - // be refactored to keep separate information separate to avoid this sort of problem in the future. Root string Version string + // APIRequestInfoResolver 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 + APIRequestInfoResolver *APIRequestInfoResolver + // ServerVersion 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 @@ -116,38 +115,6 @@ const ( MaxTimeoutSecs = 600 ) -func (g *APIGroupVersion) GetAPIPrefix() string { - slashlessRoot := strings.Trim(g.Root, "/") - if lastSlashIndex := strings.LastIndex(slashlessRoot, "/"); lastSlashIndex != -1 { - return slashlessRoot[:lastSlashIndex] - } - - return slashlessRoot -} - -func (g *APIGroupVersion) GetAPIGroup() string { - slashlessRoot := strings.Trim(g.Root, "/") - if lastSlashIndex := strings.LastIndex(slashlessRoot, "/"); lastSlashIndex != -1 { - return slashlessRoot[lastSlashIndex:] - } - - return "" -} - -func (g *APIGroupVersion) GetAPIVersion() string { - return g.Version -} - -func (g *APIGroupVersion) GetAPIRequestInfoResolver() *APIRequestInfoResolver { - apiPrefix := g.GetAPIPrefix() - info := &APIRequestInfoResolver{sets.NewString(apiPrefix), sets.String{}, g.Mapper} - if len(g.GetAPIGroup()) == 0 { - info.GrouplessAPIPrefixes.Insert(apiPrefix) - } - - return info -} - // InstallREST registers the REST handlers (storage, watch, proxy and redirect) into a restful Container. // It is expected that the provided path root prefix will serve all operations. Root MUST NOT end // in a slash. @@ -190,7 +157,7 @@ func (g *APIGroupVersion) newInstaller() *APIInstaller { prefix := path.Join(g.Root, g.Version) installer := &APIInstaller{ group: g, - info: g.GetAPIRequestInfoResolver(), + info: g.APIRequestInfoResolver, prefix: prefix, minRequestTimeout: g.MinRequestTimeout, proxyDialerFn: g.ProxyDialerFn, diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index 82a2c292175..e2cae1467cd 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -42,6 +42,7 @@ import ( "k8s.io/kubernetes/pkg/labels" "k8s.io/kubernetes/pkg/runtime" "k8s.io/kubernetes/pkg/util" + "k8s.io/kubernetes/pkg/util/sets" "k8s.io/kubernetes/pkg/version" "k8s.io/kubernetes/pkg/watch" "k8s.io/kubernetes/plugin/pkg/admission/admit" @@ -190,11 +191,16 @@ func handleLinker(storage map[string]rest.Storage, selfLinker runtime.SelfLinker return handleInternal(true, storage, admissionControl, selfLinker) } +func newTestAPIRequestInfoResolver() *APIRequestInfoResolver { + return &APIRequestInfoResolver{sets.NewString("api", "apis"), sets.NewString("api")} +} + func handleInternal(legacy bool, storage map[string]rest.Storage, admissionControl admission.Interface, selfLinker runtime.SelfLinker) http.Handler { group := &APIGroupVersion{ Storage: storage, Root: "/api", + APIRequestInfoResolver: newTestAPIRequestInfoResolver(), Creater: api.Scheme, Convertor: api.Scheme, @@ -2014,12 +2020,13 @@ func TestCreateChecksDecode(t *testing.T) { func TestUpdateREST(t *testing.T) { makeGroup := func(storage map[string]rest.Storage) *APIGroupVersion { return &APIGroupVersion{ - Storage: storage, - Root: "/api", - Creater: api.Scheme, - Convertor: api.Scheme, - Typer: api.Scheme, - Linker: selfLinker, + Storage: storage, + Root: "/api", + APIRequestInfoResolver: newTestAPIRequestInfoResolver(), + Creater: api.Scheme, + Convertor: api.Scheme, + Typer: api.Scheme, + Linker: selfLinker, Admit: admissionControl, Context: requestContextMapper, @@ -2096,11 +2103,12 @@ func TestParentResourceIsRequired(t *testing.T) { Storage: map[string]rest.Storage{ "simple/sub": storage, }, - Root: "/api", - Creater: api.Scheme, - Convertor: api.Scheme, - Typer: api.Scheme, - Linker: selfLinker, + Root: "/api", + APIRequestInfoResolver: newTestAPIRequestInfoResolver(), + Creater: api.Scheme, + Convertor: api.Scheme, + Typer: api.Scheme, + Linker: selfLinker, Admit: admissionControl, Context: requestContextMapper, @@ -2124,11 +2132,12 @@ func TestParentResourceIsRequired(t *testing.T) { "simple": &SimpleRESTStorage{}, "simple/sub": storage, }, - Root: "/api", - Creater: api.Scheme, - Convertor: api.Scheme, - Typer: api.Scheme, - Linker: selfLinker, + Root: "/api", + APIRequestInfoResolver: newTestAPIRequestInfoResolver(), + Creater: api.Scheme, + Convertor: api.Scheme, + Typer: api.Scheme, + Linker: selfLinker, Admit: admissionControl, Context: requestContextMapper, diff --git a/pkg/apiserver/handlers.go b/pkg/apiserver/handlers.go index 296ea9aa32b..8661b735101 100644 --- a/pkg/apiserver/handlers.go +++ b/pkg/apiserver/handlers.go @@ -31,7 +31,6 @@ import ( "github.com/golang/glog" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/errors" - "k8s.io/kubernetes/pkg/api/meta" "k8s.io/kubernetes/pkg/auth/authorizer" "k8s.io/kubernetes/pkg/httplog" "k8s.io/kubernetes/pkg/util/sets" @@ -348,8 +347,8 @@ type requestAttributeGetter struct { } // NewAttributeGetter returns an object which implements the RequestAttributeGetter interface. -func NewRequestAttributeGetter(requestContextMapper api.RequestContextMapper, restMapper meta.RESTMapper, apiRoots []string, grouplessAPIRoots []string) RequestAttributeGetter { - return &requestAttributeGetter{requestContextMapper, &APIRequestInfoResolver{sets.NewString(apiRoots...), sets.NewString(grouplessAPIRoots...), restMapper}} +func NewRequestAttributeGetter(requestContextMapper api.RequestContextMapper, apiRequestInfoResolver *APIRequestInfoResolver) RequestAttributeGetter { + return &requestAttributeGetter{requestContextMapper, apiRequestInfoResolver} } func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attributes { @@ -407,8 +406,6 @@ type APIRequestInfo struct { // For instance, /pods has the resource "pods" and the kind "Pod", while /pods/foo/status has the resource "pods", the sub resource "status", and the kind "Pod" // (because status operates on pods). The binding resource for a pod though may be /pods/foo/binding, which has resource "pods", subresource "binding", and kind "Binding". Subresource string - // Kind is the type of object being manipulated. For example: Pod - 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, always starting with /{resource}/{name} @@ -421,7 +418,6 @@ type APIRequestInfo struct { type APIRequestInfoResolver struct { APIPrefixes sets.String GrouplessAPIPrefixes sets.String - RestMapper meta.RESTMapper } // TODO write an integration test against the swagger doc to test the APIRequestInfo and match up behavior to responses @@ -534,10 +530,5 @@ func (r *APIRequestInfoResolver) GetAPIRequestInfo(req *http.Request) (APIReques requestInfo.Verb = "list" } - // if we have a resource, we have a good shot at being able to determine kind - if len(requestInfo.Resource) > 0 { - _, requestInfo.Kind, _ = r.RestMapper.VersionAndKindForResource(requestInfo.Resource) - } - return requestInfo, nil } diff --git a/pkg/apiserver/handlers_test.go b/pkg/apiserver/handlers_test.go index a0927757dd8..cf9e79f425f 100644 --- a/pkg/apiserver/handlers_test.go +++ b/pkg/apiserver/handlers_test.go @@ -30,7 +30,6 @@ import ( "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/errors" "k8s.io/kubernetes/pkg/api/testapi" - "k8s.io/kubernetes/pkg/util/sets" ) type fakeRL bool @@ -205,44 +204,43 @@ func TestGetAPIRequestInfo(t *testing.T) { expectedNamespace string expectedResource string expectedSubresource string - expectedKind string expectedName string expectedParts []string }{ // resource paths - {"GET", "/api/v1/namespaces", "list", "api", "", "v1", "", "namespaces", "", "Namespace", "", []string{"namespaces"}}, - {"GET", "/api/v1/namespaces/other", "get", "api", "", "v1", "other", "namespaces", "", "Namespace", "other", []string{"namespaces", "other"}}, + {"GET", "/api/v1/namespaces", "list", "api", "", "v1", "", "namespaces", "", "", []string{"namespaces"}}, + {"GET", "/api/v1/namespaces/other", "get", "api", "", "v1", "other", "namespaces", "", "other", []string{"namespaces", "other"}}, - {"GET", "/api/v1/namespaces/other/pods", "list", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, - {"GET", "/api/v1/namespaces/other/pods/foo", "get", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/api/v1/pods", "list", "api", "", "v1", api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", "/api/v1/namespaces/other/pods/foo", "get", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/api/v1/namespaces/other/pods", "list", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/namespaces/other/pods", "list", "api", "", "v1", "other", "pods", "", "", []string{"pods"}}, + {"GET", "/api/v1/namespaces/other/pods/foo", "get", "api", "", "v1", "other", "pods", "", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/pods", "list", "api", "", "v1", api.NamespaceAll, "pods", "", "", []string{"pods"}}, + {"GET", "/api/v1/namespaces/other/pods/foo", "get", "api", "", "v1", "other", "pods", "", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/namespaces/other/pods", "list", "api", "", "v1", "other", "pods", "", "", []string{"pods"}}, // special verbs - {"GET", "/api/v1/proxy/namespaces/other/pods/foo", "proxy", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/api/v1/redirect/namespaces/other/pods/foo", "redirect", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"GET", "/api/v1/watch/pods", "watch", "api", "", "v1", api.NamespaceAll, "pods", "", "Pod", "", []string{"pods"}}, - {"GET", "/api/v1/watch/namespaces/other/pods", "watch", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"GET", "/api/v1/proxy/namespaces/other/pods/foo", "proxy", "api", "", "v1", "other", "pods", "", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/redirect/namespaces/other/pods/foo", "redirect", "api", "", "v1", "other", "pods", "", "foo", []string{"pods", "foo"}}, + {"GET", "/api/v1/watch/pods", "watch", "api", "", "v1", api.NamespaceAll, "pods", "", "", []string{"pods"}}, + {"GET", "/api/v1/watch/namespaces/other/pods", "watch", "api", "", "v1", "other", "pods", "", "", []string{"pods"}}, // subresource identification - {"GET", "/api/v1/namespaces/other/pods/foo/status", "get", "api", "", "v1", "other", "pods", "status", "Pod", "foo", []string{"pods", "foo", "status"}}, - {"PUT", "/api/v1/namespaces/other/finalize", "update", "api", "", "v1", "other", "finalize", "", "", "", []string{"finalize"}}, + {"GET", "/api/v1/namespaces/other/pods/foo/status", "get", "api", "", "v1", "other", "pods", "status", "foo", []string{"pods", "foo", "status"}}, + {"PUT", "/api/v1/namespaces/other/finalize", "update", "api", "", "v1", "other", "finalize", "", "", []string{"finalize"}}, // verb identification - {"PATCH", "/api/v1/namespaces/other/pods/foo", "patch", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"DELETE", "/api/v1/namespaces/other/pods/foo", "delete", "api", "", "v1", "other", "pods", "", "Pod", "foo", []string{"pods", "foo"}}, - {"POST", "/api/v1/namespaces/other/pods", "create", "api", "", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"PATCH", "/api/v1/namespaces/other/pods/foo", "patch", "api", "", "v1", "other", "pods", "", "foo", []string{"pods", "foo"}}, + {"DELETE", "/api/v1/namespaces/other/pods/foo", "delete", "api", "", "v1", "other", "pods", "", "foo", []string{"pods", "foo"}}, + {"POST", "/api/v1/namespaces/other/pods", "create", "api", "", "v1", "other", "pods", "", "", []string{"pods"}}, // api group identification - {"POST", "/apis/experimental/v1/namespaces/other/pods", "create", "api", "experimental", "v1", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"POST", "/apis/experimental/v1/namespaces/other/pods", "create", "api", "experimental", "v1", "other", "pods", "", "", []string{"pods"}}, // api version identification - {"POST", "/apis/experimental/v1beta3/namespaces/other/pods", "create", "api", "experimental", "v1beta3", "other", "pods", "", "Pod", "", []string{"pods"}}, + {"POST", "/apis/experimental/v1beta3/namespaces/other/pods", "create", "api", "experimental", "v1beta3", "other", "pods", "", "", []string{"pods"}}, } - apiRequestInfoResolver := &APIRequestInfoResolver{sets.NewString("api", "apis"), sets.NewString("api"), testapi.Default.RESTMapper()} + apiRequestInfoResolver := newTestAPIRequestInfoResolver() for _, successCase := range successCases { req, _ := http.NewRequest(successCase.method, successCase.url, nil) @@ -260,9 +258,6 @@ func TestGetAPIRequestInfo(t *testing.T) { if successCase.expectedNamespace != apiRequestInfo.Namespace { t.Errorf("Unexpected namespace for url: %s, expected: %s, actual: %s", successCase.url, successCase.expectedNamespace, apiRequestInfo.Namespace) } - if successCase.expectedKind != apiRequestInfo.Kind { - t.Errorf("Unexpected kind for url: %s, expected: %s, actual: %s", successCase.url, successCase.expectedKind, apiRequestInfo.Kind) - } if successCase.expectedResource != apiRequestInfo.Resource { t.Errorf("Unexpected resource for url: %s, expected: %s, actual: %s", successCase.url, successCase.expectedResource, apiRequestInfo.Resource) } diff --git a/pkg/master/master.go b/pkg/master/master.go index 01e9088bf61..697a2da8eda 100644 --- a/pkg/master/master.go +++ b/pkg/master/master.go @@ -572,9 +572,7 @@ func (m *Master) init(c *Config) { apiserver.InstallSupport(m.muxHelper, m.rootWebService, c.EnableProfiling, healthzChecks...) apiserver.AddApiWebService(m.handlerContainer, c.APIPrefix, apiVersions) - defaultVersion := m.defaultAPIGroupVersion() - requestInfoResolver := defaultVersion.GetAPIRequestInfoResolver() - apiserver.InstallServiceErrorHandler(m.handlerContainer, requestInfoResolver, apiVersions) + apiserver.InstallServiceErrorHandler(m.handlerContainer, m.newAPIRequestInfoResolver(), apiVersions) // allGroups records all supported groups at /apis allGroups := []api.APIGroup{} @@ -608,8 +606,7 @@ func (m *Master) init(c *Config) { } apiserver.AddGroupWebService(m.handlerContainer, c.APIGroupPrefix+"/"+latest.GroupOrDie("experimental").Group+"/", group) allGroups = append(allGroups, group) - expRequestInfoResolver := expVersion.GetAPIRequestInfoResolver() - apiserver.InstallServiceErrorHandler(m.handlerContainer, expRequestInfoResolver, []string{expVersion.Version}) + apiserver.InstallServiceErrorHandler(m.handlerContainer, m.newAPIRequestInfoResolver(), []string{expVersion.Version}) } // This should be done after all groups are registered @@ -652,10 +649,7 @@ func (m *Master) init(c *Config) { m.InsecureHandler = handler - attributeGetter := apiserver.NewRequestAttributeGetter(m.requestContextMapper, latest.GroupOrDie("").RESTMapper, - []string{strings.Trim(c.APIPrefix, "/"), strings.Trim(thirdpartyprefix, "/")}, // all possible API prefixes - []string{strings.Trim(c.APIPrefix, "/")}, // APIPrefixes that won't have groups (legacy) - ) + attributeGetter := apiserver.NewRequestAttributeGetter(m.requestContextMapper, m.newAPIRequestInfoResolver()) handler = apiserver.WithAuthorizationCheck(handler, attributeGetter, m.authorizer) // Install Authenticator @@ -781,9 +775,17 @@ func (m *Master) getServersToValidate(c *Config) map[string]apiserver.Server { return serversToValidate } +func (m *Master) newAPIRequestInfoResolver() *apiserver.APIRequestInfoResolver { + return &apiserver.APIRequestInfoResolver{ + sets.NewString(strings.Trim(m.apiPrefix, "/"), strings.Trim(thirdpartyprefix, "/")), // all possible API prefixes + sets.NewString(strings.Trim(m.apiPrefix, "/")), // APIPrefixes that won't have groups (legacy) + } +} + func (m *Master) defaultAPIGroupVersion() *apiserver.APIGroupVersion { return &apiserver.APIGroupVersion{ Root: m.apiPrefix, + APIRequestInfoResolver: m.newAPIRequestInfoResolver(), Mapper: latest.GroupOrDie("").RESTMapper, @@ -921,8 +923,7 @@ func (m *Master) InstallThirdPartyResource(rsrc *expapi.ThirdPartyResource) erro } apiserver.AddGroupWebService(m.handlerContainer, path, apiGroup) m.addThirdPartyResourceStorage(path, thirdparty.Storage[strings.ToLower(kind)+"s"].(*thirdpartyresourcedataetcd.REST)) - thirdPartyRequestInfoResolver := &apiserver.APIRequestInfoResolver{APIPrefixes: sets.NewString(strings.Trim(thirdpartyprefix, "/")), RestMapper: thirdparty.Mapper} - apiserver.InstallServiceErrorHandler(m.handlerContainer, thirdPartyRequestInfoResolver, []string{thirdparty.Version}) + apiserver.InstallServiceErrorHandler(m.handlerContainer, m.newAPIRequestInfoResolver(), []string{thirdparty.Version}) return nil } @@ -936,7 +937,9 @@ func (m *Master) thirdpartyapi(group, kind, version string) *apiserver.APIGroupV } return &apiserver.APIGroupVersion{ - Root: apiRoot, + Root: apiRoot, + Version: version, + APIRequestInfoResolver: m.newAPIRequestInfoResolver(), Creater: thirdpartyresourcedata.NewObjectCreator(version, api.Scheme), Convertor: api.Scheme, @@ -946,7 +949,6 @@ func (m *Master) thirdpartyapi(group, kind, version string) *apiserver.APIGroupV Codec: thirdpartyresourcedata.NewCodec(latest.GroupOrDie("experimental").Codec, kind), Linker: latest.GroupOrDie("experimental").SelfLinker, Storage: storage, - Version: version, ServerVersion: latest.GroupOrDie("").GroupVersion, Context: m.requestContextMapper, @@ -993,6 +995,7 @@ func (m *Master) experimental(c *Config) *apiserver.APIGroupVersion { return &apiserver.APIGroupVersion{ Root: m.apiGroupPrefix, + APIRequestInfoResolver: m.newAPIRequestInfoResolver(), Creater: api.Scheme, Convertor: api.Scheme,