From 0225d76b6af3308065c83ee79984eab9f7f11612 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sat, 11 Apr 2015 11:13:10 -0400 Subject: [PATCH] Support subpath on GET for GetterWithOptions Allows REST consumers to build paths like: /api/v1beta3/namespaces/foo/webhookresource// Also fixes parameter exposure for subresources (was only fixed for v1beta3). --- pkg/api/rest/rest.go | 11 +++-- pkg/apiserver/api_installer.go | 76 ++++++++++++++++++++------------- pkg/apiserver/apiserver_test.go | 54 ++++++++++++++++++++++- pkg/apiserver/resthandler.go | 13 +++++- pkg/registry/pod/etcd/etcd.go | 9 +++- 5 files changed, 124 insertions(+), 39 deletions(-) diff --git a/pkg/api/rest/rest.go b/pkg/api/rest/rest.go index 65ee1c81b25..069ed3c2f8c 100644 --- a/pkg/api/rest/rest.go +++ b/pkg/api/rest/rest.go @@ -55,7 +55,8 @@ type Getter interface { } // GetterWithOptions is an object that retrieve a named RESTful resource and takes -// additional options on the get request +// additional options on the get request. It allows a caller to also receive the +// subpath of the GET request. type GetterWithOptions interface { // Get finds a resource in the storage by name and returns it. // Although it can return an arbitrary error value, IsNotFound(err) is true for the @@ -65,8 +66,12 @@ type GetterWithOptions interface { Get(ctx api.Context, name string, options runtime.Object) (runtime.Object, error) // NewGetOptions returns an empty options object that will be used to pass - // options to the Get method. - NewGetOptions() runtime.Object + // options to the Get method. It may return a bool and a string, if true, the + // value of the request path below the object will be included as the named + // string in the serialization of the runtime object. E.g., returning "path" + // will convert the trailing request scheme value to "path" in the map[string][]string + // passed to the convertor. + NewGetOptions() (runtime.Object, bool, string) } // Deleter is an object that can delete a named RESTful resource. diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index c9d88e399d5..10ff4c90c5f 100644 --- a/pkg/apiserver/api_installer.go +++ b/pkg/apiserver/api_installer.go @@ -110,6 +110,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag // TODO: support deeper paths return fmt.Errorf("api_installer allows only one or two segment paths (resource or resource/subresource)") } + hasSubresource := len(subresource) > 0 object := storage.New() _, kind, err := a.group.Typer.ObjectVersionAndKind(object) @@ -177,10 +178,14 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag return err } versionedStatus := indirectArbitraryPointer(versionedStatusPtr) - var getOptions runtime.Object - var getOptionsKind string + var ( + getOptions runtime.Object + getOptionsKind string + getSubpath bool + getSubpathKey string + ) if isGetterWithOptions { - getOptions = getterWithOptions.NewGetOptions() + getOptions, getSubpath, getSubpathKey = getterWithOptions.NewGetOptions() _, getOptionsKind, err = a.group.Typer.ObjectVersionAndKind(getOptions) if err != nil { return err @@ -206,21 +211,26 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag // Get the list of actions for the given scope. if scope.Name() != meta.RESTScopeNameNamespace { resourcePath := resource + resourceParams := params itemPath := resourcePath + "/{name}" - if len(subresource) > 0 { - itemPath = itemPath + "/" + subresource - resourcePath = itemPath - } nameParams := append(params, nameParam) proxyParams := append(nameParams, pathParam) + if hasSubresource { + itemPath = itemPath + "/" + subresource + resourcePath = itemPath + resourceParams = nameParams + } namer := rootScopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, itemPath)} // Handler for standard REST verbs (GET, PUT, POST and DELETE). - actions = appendIf(actions, action{"LIST", resourcePath, params, namer}, isLister) - actions = appendIf(actions, action{"POST", resourcePath, params, namer}, isCreater) - actions = appendIf(actions, action{"WATCHLIST", "watch/" + resourcePath, params, namer}, allowWatchList) + actions = appendIf(actions, action{"LIST", resourcePath, resourceParams, namer}, isLister) + actions = appendIf(actions, action{"POST", resourcePath, resourceParams, namer}, isCreater) + actions = appendIf(actions, action{"WATCHLIST", "watch/" + resourcePath, resourceParams, namer}, allowWatchList) actions = appendIf(actions, action{"GET", itemPath, nameParams, namer}, isGetter) + if getSubpath { + actions = appendIf(actions, action{"GET", itemPath + "/{path:*}", proxyParams, namer}, isGetter) + } actions = appendIf(actions, action{"PUT", itemPath, nameParams, namer}, isUpdater) actions = appendIf(actions, action{"PATCH", itemPath, nameParams, namer}, isPatcher) actions = appendIf(actions, action{"DELETE", itemPath, nameParams, namer}, isDeleter) @@ -238,25 +248,26 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag namespaceParams := []*restful.Parameter{namespaceParam} resourcePath := namespacedPath + resourceParams := namespaceParams itemPath := namespacedPath + "/{name}" - if len(subresource) > 0 { - itemPath = itemPath + "/" + subresource - resourcePath = itemPath - } nameParams := append(namespaceParams, nameParam) proxyParams := append(nameParams, pathParam) + if hasSubresource { + itemPath = itemPath + "/" + subresource + resourcePath = itemPath + resourceParams = nameParams + } namer := scopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, itemPath), false} - actions = appendIf(actions, action{"LIST", resourcePath, namespaceParams, namer}, isLister) - // Some paths ("/pods/{name}/binding", I'm looking at you) contain an embedded '{name}') - if strings.Contains(resourcePath, "{name}") { - actions = appendIf(actions, action{"POST", resourcePath, nameParams, namer}, isCreater) - } else { - actions = appendIf(actions, action{"POST", resourcePath, namespaceParams, namer}, isCreater) - } - actions = appendIf(actions, action{"WATCHLIST", "watch/" + resourcePath, namespaceParams, namer}, allowWatchList) + actions = appendIf(actions, action{"LIST", resourcePath, resourceParams, namer}, isLister) + actions = appendIf(actions, action{"POST", resourcePath, resourceParams, namer}, isCreater) + // DEPRECATED + actions = appendIf(actions, action{"WATCHLIST", "watch/" + resourcePath, resourceParams, namer}, allowWatchList) actions = appendIf(actions, action{"GET", itemPath, nameParams, namer}, isGetter) + if getSubpath { + actions = appendIf(actions, action{"GET", itemPath + "/{path:*}", proxyParams, namer}, isGetter) + } actions = appendIf(actions, action{"PUT", itemPath, nameParams, namer}, isUpdater) actions = appendIf(actions, action{"PATCH", itemPath, nameParams, namer}, isPatcher) actions = appendIf(actions, action{"DELETE", itemPath, nameParams, namer}, isDeleter) @@ -278,20 +289,25 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag basePath := resource resourcePath := basePath + resourceParams := namespaceParams itemPath := resourcePath + "/{name}" - if len(subresource) > 0 { - itemPath = itemPath + "/" + subresource - resourcePath = itemPath - } nameParams := append(namespaceParams, nameParam) proxyParams := append(nameParams, pathParam) + if hasSubresource { + itemPath = itemPath + "/" + subresource + resourcePath = itemPath + resourceParams = nameParams + } namer := legacyScopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, itemPath)} - actions = appendIf(actions, action{"LIST", resourcePath, namespaceParams, namer}, isLister) - actions = appendIf(actions, action{"POST", resourcePath, namespaceParams, namer}, isCreater) - actions = appendIf(actions, action{"WATCHLIST", "watch/" + resourcePath, namespaceParams, namer}, allowWatchList) + actions = appendIf(actions, action{"LIST", resourcePath, resourceParams, namer}, isLister) + actions = appendIf(actions, action{"POST", resourcePath, resourceParams, namer}, isCreater) + actions = appendIf(actions, action{"WATCHLIST", "watch/" + resourcePath, resourceParams, namer}, allowWatchList) actions = appendIf(actions, action{"GET", itemPath, nameParams, namer}, isGetter) + if getSubpath { + actions = appendIf(actions, action{"GET", itemPath + "/{path:*}", proxyParams, namer}, isGetter) + } actions = appendIf(actions, action{"PUT", itemPath, nameParams, namer}, isUpdater) actions = appendIf(actions, action{"PATCH", itemPath, nameParams, namer}, isPatcher) actions = appendIf(actions, action{"DELETE", itemPath, nameParams, namer}, isDeleter) @@ -336,7 +352,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag case "GET": // Get a resource. var handler restful.RouteFunction if isGetterWithOptions { - handler = GetResourceWithOptions(getterWithOptions, reqScope, getOptionsKind) + handler = GetResourceWithOptions(getterWithOptions, reqScope, getOptionsKind, getSubpath, getSubpathKey) } else { handler = GetResource(getter, reqScope) } diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index ecc3a6af0b5..a15a4dbc709 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -239,6 +239,7 @@ type SimpleGetOptions struct { api.TypeMeta `json:",inline"` Param1 string `json:"param1"` Param2 string `json:"param2"` + Path string `json:"atAPath"` } func (*SimpleGetOptions) IsAnAPIObject() {} @@ -459,9 +460,12 @@ func (m *MetadataRESTStorage) ProducesMIMETypes(method string) []string { return m.types } +var _ rest.StorageMetadata = &MetadataRESTStorage{} + type GetWithOptionsRESTStorage struct { *SimpleRESTStorage optionsReceived runtime.Object + takesPath string } func (r *GetWithOptionsRESTStorage) Get(ctx api.Context, name string, options runtime.Object) (runtime.Object, error) { @@ -472,10 +476,15 @@ func (r *GetWithOptionsRESTStorage) Get(ctx api.Context, name string, options ru return r.SimpleRESTStorage.Get(ctx, name) } -func (r *GetWithOptionsRESTStorage) NewGetOptions() runtime.Object { - return &SimpleGetOptions{} +func (r *GetWithOptionsRESTStorage) NewGetOptions() (runtime.Object, bool, string) { + if len(r.takesPath) > 0 { + return &SimpleGetOptions{}, true, r.takesPath + } + return &SimpleGetOptions{}, false, "" } +var _ rest.GetterWithOptions = &GetWithOptionsRESTStorage{} + func extractBody(response *http.Response, object runtime.Object) (string, error) { defer response.Body.Close() body, err := ioutil.ReadAll(response.Body) @@ -963,6 +972,47 @@ func TestGetWithOptions(t *testing.T) { } } +func TestGetWithOptionsAndPath(t *testing.T) { + storage := map[string]rest.Storage{} + simpleStorage := GetWithOptionsRESTStorage{ + SimpleRESTStorage: &SimpleRESTStorage{ + item: Simple{ + Other: "foo", + }, + }, + takesPath: "atAPath", + } + storage["simple"] = &simpleStorage + handler := handle(storage) + server := httptest.NewServer(handler) + defer server.Close() + + resp, err := http.Get(server.URL + "/api/version/simple/id/a/different/path?param1=test1¶m2=test2&atAPath=not") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("unexpected response: %#v", resp) + } + var itemOut Simple + body, err := extractBody(resp, &itemOut) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if itemOut.Name != simpleStorage.item.Name { + t.Errorf("Unexpected data: %#v, expected %#v (%s)", itemOut, simpleStorage.item, string(body)) + } + + opts, ok := simpleStorage.optionsReceived.(*SimpleGetOptions) + if !ok { + t.Errorf("Unexpected options object received: %#v", simpleStorage.optionsReceived) + return + } + if opts.Param1 != "test1" || opts.Param2 != "test2" || opts.Path != "a/different/path" { + t.Errorf("Did not receive expected options: %#v", opts) + } +} func TestGetAlternateSelfLink(t *testing.T) { storage := map[string]rest.Storage{} simpleStorage := SimpleRESTStorage{ diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index bd3d351c6f6..c5f98807c21 100644 --- a/pkg/apiserver/resthandler.go +++ b/pkg/apiserver/resthandler.go @@ -113,10 +113,19 @@ func GetResource(r rest.Getter, scope RequestScope) restful.RouteFunction { } // GetResourceWithOptions returns a function that handles retrieving a single resource from a rest.Storage object. -func GetResourceWithOptions(r rest.GetterWithOptions, scope RequestScope, getOptionsKind string) restful.RouteFunction { +func GetResourceWithOptions(r rest.GetterWithOptions, scope RequestScope, getOptionsKind string, subpath bool, subpathKey string) restful.RouteFunction { return getResourceHandler(scope, func(ctx api.Context, name string, req *restful.Request) (runtime.Object, error) { - opts, err := queryToObject(req.Request.URL.Query(), scope, getOptionsKind) + query := req.Request.URL.Query() + if subpath { + newQuery := make(url.Values) + for k, v := range query { + newQuery[k] = v + } + newQuery[subpathKey] = []string{req.PathParameter("path")} + query = newQuery + } + opts, err := queryToObject(query, scope, getOptionsKind) if err != nil { return nil, err } diff --git a/pkg/registry/pod/etcd/etcd.go b/pkg/registry/pod/etcd/etcd.go index 008d27a8b9e..990265464f4 100644 --- a/pkg/registry/pod/etcd/etcd.go +++ b/pkg/registry/pod/etcd/etcd.go @@ -110,6 +110,8 @@ func (r *BindingREST) New() runtime.Object { return &api.Binding{} } +var _ = rest.Creater(&BindingREST{}) + // Create ensures a pod is bound to a specific host. func (r *BindingREST) Create(ctx api.Context, obj runtime.Object) (out runtime.Object, err error) { binding := obj.(*api.Binding) @@ -197,6 +199,9 @@ type LogREST struct { kubeletConn client.ConnectionInfoGetter } +// LogREST implements GetterWithOptions +var _ = rest.GetterWithOptions(&LogREST{}) + // New creates a new Pod log options object func (r *LogREST) New() runtime.Object { return &api.PodLogOptions{} @@ -221,6 +226,6 @@ func (r *LogREST) Get(ctx api.Context, name string, opts runtime.Object) (runtim } // NewGetOptions creates a new options object -func (r *LogREST) NewGetOptions() runtime.Object { - return &api.PodLogOptions{} +func (r *LogREST) NewGetOptions() (runtime.Object, bool, string) { + return &api.PodLogOptions{}, false, "" }