From 14e43f49d6abc70392ab4ceac439ee9afdb77b1a Mon Sep 17 00:00:00 2001 From: David Eads Date: Tue, 24 Apr 2018 18:31:41 -0400 Subject: [PATCH] rest mappings cannot logically be object converters Kubernetes-commit: 6900f8856f8cd9a6c94a156b9e4a9fee0c16f807 --- pkg/endpoints/apiserver_test.go | 61 +++++++++++------------------- pkg/endpoints/groupversion.go | 5 ++- pkg/endpoints/installer.go | 67 +++++++++++---------------------- pkg/registry/rest/rest.go | 1 + pkg/server/genericapiserver.go | 2 +- 5 files changed, 47 insertions(+), 89 deletions(-) diff --git a/pkg/endpoints/apiserver_test.go b/pkg/endpoints/apiserver_test.go index 869a8338b..78ef76447 100644 --- a/pkg/endpoints/apiserver_test.go +++ b/pkg/endpoints/apiserver_test.go @@ -117,7 +117,6 @@ var parameterCodec = runtime.NewParameterCodec(scheme) var accessor = meta.NewAccessor() var selfLinker runtime.SelfLinker = accessor -var mapper, namespaceMapper meta.RESTMapper // The mappers with namespace and with legacy namespace scopes. var admissionControl admission.Interface func init() { @@ -203,25 +202,6 @@ func init() { addTestTypes() addNewTestTypes() - nsMapper := newMapper() - - // enumerate all supported versions, get the kinds, and register with - // the mapper how to address our resources - for _, gv := range groupVersions { - for kind := range scheme.KnownTypes(gv) { - gvk := gv.WithKind(kind) - root := bool(kind == "SimpleRoot") - if root { - nsMapper.Add(gvk, meta.RESTScopeRoot) - } else { - nsMapper.Add(gvk, meta.RESTScopeNamespace) - } - } - } - - mapper = nsMapper - namespaceMapper = nsMapper - scheme.AddFieldLabelConversionFunc(grouplessGroupVersion.String(), "Simple", func(label, value string) (string, string, error) { return label, value, nil @@ -263,12 +243,12 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission. template := APIGroupVersion{ Storage: storage, - Creater: scheme, - Convertor: scheme, - Defaulter: scheme, - Typer: scheme, - Linker: selfLinker, - Mapper: namespaceMapper, + Creater: scheme, + Convertor: scheme, + Defaulter: scheme, + Typer: scheme, + Linker: selfLinker, + RootScopedKinds: sets.NewString("SimpleRoot"), ParameterCodec: parameterCodec, @@ -841,7 +821,6 @@ func TestNotFound(t *testing.T) { if response.StatusCode != v.Status { t.Errorf("Expected %d for %s (%s), Got %#v", v.Status, v.Method, k, response) - t.Errorf("MAPPER: %v", mapper) } } } @@ -3188,15 +3167,15 @@ func TestParentResourceIsRequired(t *testing.T) { Storage: map[string]rest.Storage{ "simple/sub": storage, }, - Root: "/" + prefix, - Creater: scheme, - Convertor: scheme, - Defaulter: scheme, - Typer: scheme, - Linker: selfLinker, + Root: "/" + prefix, + Creater: scheme, + Convertor: scheme, + Defaulter: scheme, + Typer: scheme, + Linker: selfLinker, + RootScopedKinds: sets.NewString("SimpleRoot"), - Admit: admissionControl, - Mapper: namespaceMapper, + Admit: admissionControl, GroupVersion: newGroupVersion, OptionsExternalVersion: &newGroupVersion, @@ -3225,8 +3204,7 @@ func TestParentResourceIsRequired(t *testing.T) { Typer: scheme, Linker: selfLinker, - Admit: admissionControl, - Mapper: namespaceMapper, + Admit: admissionControl, GroupVersion: newGroupVersion, OptionsExternalVersion: &newGroupVersion, @@ -3806,6 +3784,8 @@ type SimpleXGSubresourceRESTStorage struct { itemGVK schema.GroupVersionKind } +var _ = rest.GroupVersionKindProvider(&SimpleXGSubresourceRESTStorage{}) + func (storage *SimpleXGSubresourceRESTStorage) New() runtime.Object { return &genericapitesting.SimpleXGSubresource{} } @@ -3814,12 +3794,14 @@ func (storage *SimpleXGSubresourceRESTStorage) Get(ctx context.Context, id strin return storage.item.DeepCopyObject(), nil } -var _ = rest.GroupVersionKindProvider(&SimpleXGSubresourceRESTStorage{}) - func (storage *SimpleXGSubresourceRESTStorage) GroupVersionKind(containingGV schema.GroupVersion) schema.GroupVersionKind { return storage.itemGVK } +func (*SimpleXGSubresourceRESTStorage) ClusterScoped() bool { + return false +} + func TestXGSubresource(t *testing.T) { container := restful.NewContainer() container.Router(restful.CurlyRouter{}) @@ -3845,7 +3827,6 @@ func TestXGSubresource(t *testing.T) { Defaulter: scheme, Typer: scheme, Linker: selfLinker, - Mapper: namespaceMapper, ParameterCodec: parameterCodec, diff --git a/pkg/endpoints/groupversion.go b/pkg/endpoints/groupversion.go index a8c62dd5b..7060eb738 100644 --- a/pkg/endpoints/groupversion.go +++ b/pkg/endpoints/groupversion.go @@ -22,11 +22,11 @@ import ( "github.com/emicklei/go-restful" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/endpoints/discovery" "k8s.io/apiserver/pkg/registry/rest" @@ -55,7 +55,8 @@ type APIGroupVersion struct { // version (for when the inevitable meta/v2 group emerges). MetaGroupVersion *schema.GroupVersion - Mapper meta.RESTMapper + // RootScopedKinds are the root scoped kinds for the primary GroupVersion + RootScopedKinds sets.String // Serializer is used to determine how to convert responses from API methods into bytes to send over // the wire. diff --git a/pkg/endpoints/installer.go b/pkg/endpoints/installer.go index 70921e781..2cddaa9d8 100644 --- a/pkg/endpoints/installer.go +++ b/pkg/endpoints/installer.go @@ -28,7 +28,6 @@ import ( restful "github.com/emicklei/go-restful" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/runtime" @@ -133,18 +132,18 @@ func (a *APIInstaller) newWebService() *restful.WebService { } // getResourceKind returns the external group version kind registered for the given storage -// object. If the storage object is a subresource and has an override supplied for it, it returns +// object and whether or not the kind is cluster scoped. If the storage object is a subresource and has an override supplied for it, it returns // the group version kind supplied in the override. -func (a *APIInstaller) getResourceKind(path string, storage rest.Storage) (schema.GroupVersionKind, error) { +func (a *APIInstaller) getResourceKind(path string, storage rest.Storage) (schema.GroupVersionKind, bool, error) { // Let the storage tell us exactly what GVK it has if gvkProvider, ok := storage.(rest.GroupVersionKindProvider); ok { - return gvkProvider.GroupVersionKind(a.group.GroupVersion), nil + return gvkProvider.GroupVersionKind(a.group.GroupVersion), gvkProvider.ClusterScoped(), nil } object := storage.New() fqKinds, _, err := a.group.Typer.ObjectKinds(object) if err != nil { - return schema.GroupVersionKind{}, err + return schema.GroupVersionKind{}, false, err } // a given go type can have multiple potential fully qualified kinds. Find the one that corresponds with the group @@ -157,32 +156,11 @@ func (a *APIInstaller) getResourceKind(path string, storage rest.Storage) (schem } } if fqKindToRegister.Empty() { - return schema.GroupVersionKind{}, fmt.Errorf("unable to locate fully qualified kind for %v: found %v when registering for %v", reflect.TypeOf(object), fqKinds, a.group.GroupVersion) + return schema.GroupVersionKind{}, false, fmt.Errorf("unable to locate fully qualified kind for %v: found %v when registering for %v", reflect.TypeOf(object), fqKinds, a.group.GroupVersion) } - return fqKindToRegister, nil -} -// restMapping returns rest mapper for the resource. -// Example REST paths that this mapper maps. -// 1. Resource only, no subresource: -// Resource Type: batch/v1.Job (input args: resource = "jobs") -// REST path: /apis/batch/v1/namespaces/{namespace}/job/{name} -// 2. Subresource and its parent belong to different API groups and/or versions: -// Resource Type: extensions/v1beta1.ReplicaSet (input args: resource = "replicasets") -// Subresource Type: autoscaling/v1.Scale -// REST path: /apis/extensions/v1beta1/namespaces/{namespace}/replicaset/{name}/scale -func (a *APIInstaller) restMapping(resource string) (*meta.RESTMapping, error) { - // subresources must have parent resources, and follow the namespacing rules of their parent. - // So get the storage of the resource (which is the parent resource in case of subresources) - storage, ok := a.group.Storage[resource] - if !ok { - return nil, fmt.Errorf("unable to locate the storage object for resource: %s", resource) - } - fqKindToRegister, err := a.getResourceKind(resource, storage) - if err != nil { - return nil, fmt.Errorf("unable to locate fully qualified kind for mapper resource %s: %v", resource, err) - } - return a.group.Mapper.RESTMapping(fqKindToRegister.GroupKind(), fqKindToRegister.Version) + // group is guaranteed to match based on the check above + return fqKindToRegister, a.group.RootScopedKinds.Has(fqKindToRegister.Kind), nil } func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storage, ws *restful.WebService) (*metav1.APIResource, error) { @@ -198,12 +176,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag return nil, err } - mapping, err := a.restMapping(resource) - if err != nil { - return nil, err - } - - fqKindToRegister, err := a.getResourceKind(path, storage) + fqKindToRegister, clusterScoped, err := a.getResourceKind(path, storage) if err != nil { return nil, err } @@ -338,7 +311,6 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag } allowWatchList := isWatcher && isLister // watching on lists is allowed only for kinds that support both watch and list. - scope := mapping.Scope nameParam := ws.PathParameter("name", "name of the "+kind).DataType("string") pathParam := ws.PathParameter("path", "path to the resource").DataType("string") @@ -357,8 +329,8 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag var apiResource metav1.APIResource // Get the list of actions for the given scope. - switch scope.Name() { - case meta.RESTScopeNameRoot: + switch { + case clusterScoped: // Handle non-namespace scoped resources like nodes. resourcePath := resource resourceParams := params @@ -402,10 +374,11 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag actions = appendIf(actions, action{"CONNECT", itemPath, nameParams, namer, false}, isConnecter) actions = appendIf(actions, action{"CONNECT", itemPath + "/{path:*}", proxyParams, namer, false}, isConnecter && connectSubpath) break - case meta.RESTScopeNameNamespace: + default: + namespaceParamName := "namespaces" // Handler for standard REST verbs (GET, PUT, POST and DELETE). - namespaceParam := ws.PathParameter(scope.ArgumentName(), scope.ParamDescription()).DataType("string") - namespacedPath := scope.ParamName() + "/{" + scope.ArgumentName() + "}/" + resource + namespaceParam := ws.PathParameter("namespace", "object name and auth scope, such as for teams and projects").DataType("string") + namespacedPath := namespaceParamName + "/{" + "namespace" + "}/" + resource namespaceParams := []*restful.Parameter{namespaceParam} resourcePath := namespacedPath @@ -426,7 +399,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag namer := handlers.ContextBasedNaming{ SelfLinker: a.group.Linker, ClusterScoped: false, - SelfLinkPathPrefix: gpath.Join(a.prefix, scope.ParamName()) + "/", + SelfLinkPathPrefix: gpath.Join(a.prefix, namespaceParamName) + "/", SelfLinkPathSuffix: itemPathSuffix, } @@ -455,8 +428,6 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag actions = appendIf(actions, action{"WATCHLIST", "watch/" + resource, params, namer, true}, allowWatchList) } break - default: - return nil, fmt.Errorf("unsupported restscope: %s", scope.Name()) } // Create Routes for the actions. @@ -539,7 +510,11 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag // If there is a subresource, kind should be the parent's kind. if hasSubresource { - fqParentKind, err := a.getResourceKind(resource, a.group.Storage[resource]) + parentStorage, ok := a.group.Storage[resource] + if !ok { + return nil, fmt.Errorf("missing parent storage: %q", resource) + } + fqParentKind, _, err := a.getResourceKind(resource, parentStorage) if err != nil { return nil, err } @@ -654,7 +629,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag string(types.MergePatchType), string(types.StrategicMergePatchType), } - handler := metrics.InstrumentRouteFunc(action.Verb, resource, subresource, requestScope, restfulPatchResource(patcher, reqScope, admit, mapping.ObjectConvertor, supportedTypes)) + handler := metrics.InstrumentRouteFunc(action.Verb, resource, subresource, requestScope, restfulPatchResource(patcher, reqScope, admit, a.group.Convertor, supportedTypes)) route := ws.PATCH(action.Path).To(handler). Doc(doc). Param(ws.QueryParameter("pretty", "If 'true', then the output is pretty printed.")). diff --git a/pkg/registry/rest/rest.go b/pkg/registry/rest/rest.go index 19564b181..1e440bf64 100644 --- a/pkg/registry/rest/rest.go +++ b/pkg/registry/rest/rest.go @@ -83,6 +83,7 @@ type CategoriesProvider interface { // TODO KindProvider (only used by federation) should be removed and replaced with this, but that presents greater risk late in 1.8. type GroupVersionKindProvider interface { GroupVersionKind(containingGV schema.GroupVersion) schema.GroupVersionKind + ClusterScoped() bool } // Lister is an object that can retrieve resources that match the provided field and label criteria. diff --git a/pkg/server/genericapiserver.go b/pkg/server/genericapiserver.go index 6e25a9391..aac1569af 100644 --- a/pkg/server/genericapiserver.go +++ b/pkg/server/genericapiserver.go @@ -423,7 +423,7 @@ func (s *GenericAPIServer) newAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV Defaulter: apiGroupInfo.Scheme, Typer: apiGroupInfo.Scheme, Linker: apiGroupInfo.GroupMeta.SelfLinker, - Mapper: apiGroupInfo.GroupMeta.RESTMapper, + RootScopedKinds: apiGroupInfo.GroupMeta.RootScopedKinds, Admit: s.admissionControl, MinRequestTimeout: s.minRequestTimeout,