Merge pull request #21909 from madhusudancs/scale-group-apiinstaller

Allow cross-group subresource registrations in the APIInstaller.

pkg/apiserver/api_installer.go

@@ -110,6 +110,63 @@ func (a *APIInstaller) NewWebService() *restful.WebService {
 	return ws
 }
 
+// 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
+// the group version kind supplied in the override.
+func (a *APIInstaller) getResourceKind(path string, storage rest.Storage) (unversioned.GroupVersionKind, error) {
+	if fqKindToRegister, ok := a.group.SubresourceGroupVersionKind[path]; ok {
+		return fqKindToRegister, nil
+	}
+
+	object := storage.New()
+	fqKinds, err := a.group.Typer.ObjectKinds(object)
+	if err != nil {
+		return unversioned.GroupVersionKind{}, err
+	}
+
+	// a given go type can have multiple potential fully qualified kinds.  Find the one that corresponds with the group
+	// we're trying to register here
+	fqKindToRegister := unversioned.GroupVersionKind{}
+	for _, fqKind := range fqKinds {
+		if fqKind.Group == a.group.GroupVersion.Group {
+			fqKindToRegister = a.group.GroupVersion.WithKind(fqKind.Kind)
+			break
+		}
+
+		// TODO This keeps it doing what it was doing before, but it doesn't feel right.
+		if fqKind.Group == extensions.GroupName && fqKind.Kind == "ThirdPartyResourceData" {
+			fqKindToRegister = a.group.GroupVersion.WithKind(fqKind.Kind)
+		}
+	}
+	if fqKindToRegister.IsEmpty() {
+		return unversioned.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 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)
+}
+
 func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storage, ws *restful.WebService, proxyHandler http.Handler) (*unversioned.APIResource, error) {
 	admit := a.group.Admit
 	context := a.group.Context
@@ -119,88 +176,28 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		optionsExternalVersion = *a.group.OptionsExternalVersion
 	}
 
-	var resource, subresource string
+	resource, subresource, err := splitSubresource(path)
-	switch parts := strings.Split(path, "/"); len(parts) {
-	case 2:
-		resource, subresource = parts[0], parts[1]
-	case 1:
-		resource = parts[0]
-	default:
-		// TODO: support deeper paths
-		return nil, fmt.Errorf("api_installer allows only one or two segment paths (resource or resource/subresource)")
-	}
-	hasSubresource := len(subresource) > 0
-
-	object := storage.New()
-	fqKinds, err := a.group.Typer.ObjectKinds(object)
 	if err != nil {
 		return nil, err
 	}
-	// a given go type can have multiple potential fully qualified kinds.  Find the one that corresponds with the group
-	// we're trying to register here
-	fqKindToRegister := unversioned.GroupVersionKind{}
-	for _, fqKind := range fqKinds {
-		if fqKind.Group == a.group.GroupVersion.Group {
-			fqKindToRegister = fqKind
-			break
-		}
 
-		// TODO This keeps it doing what it was doing before, but it doesn't feel right.
+	mapping, err := a.restMapping(resource)
-		if fqKind.Group == extensions.GroupName && fqKind.Kind == "ThirdPartyResourceData" {
+	if err != nil {
-			fqKindToRegister = fqKind
+		return nil, err
-			fqKindToRegister.Group = a.group.GroupVersion.Group
-			fqKindToRegister.Version = a.group.GroupVersion.Version
-		}
 	}
 
-	kind := fqKindToRegister.Kind
+	fqKindToRegister, err := a.getResourceKind(path, storage)
-
+	if err != nil {
-	if fqKindToRegister.IsEmpty() {
+		return nil, err
-		return nil, fmt.Errorf("unable to locate fully qualified kind for %v: found %v when registering for %v", reflect.TypeOf(object), fqKinds, a.group.GroupVersion)
 	}
 
-	versionedPtr, err := a.group.Creater.New(a.group.GroupVersion.WithKind(kind))
+	versionedPtr, err := a.group.Creater.New(fqKindToRegister)
 	if err != nil {
 		return nil, err
 	}
 	versionedObject := indirectArbitraryPointer(versionedPtr)
-
+	kind := fqKindToRegister.Kind
-	mapping, err := a.group.Mapper.RESTMapping(fqKindToRegister.GroupKind(), a.group.GroupVersion.Version)
+	hasSubresource := len(subresource) > 0
-	if err != nil {
-		return nil, err
-	}
-
-	// subresources must have parent resources, and follow the namespacing rules of their parent
-	if hasSubresource {
-		parentStorage, ok := a.group.Storage[resource]
-		if !ok {
-			return nil, fmt.Errorf("subresources can only be declared when the parent is also registered: %s needs %s", path, resource)
-		}
-		parentObject := parentStorage.New()
-
-		parentFQKinds, err := a.group.Typer.ObjectKinds(parentObject)
-		if err != nil {
-			return nil, err
-		}
-		// a given go type can have multiple potential fully qualified kinds.  Find the one that corresponds with the group
-		// we're trying to register here
-		parentFQKindToRegister := unversioned.GroupVersionKind{}
-		for _, fqKind := range parentFQKinds {
-			if fqKind.Group == a.group.GroupVersion.Group {
-				parentFQKindToRegister = fqKind
-				break
-			}
-		}
-		if parentFQKindToRegister.IsEmpty() {
-			return nil, fmt.Errorf("unable to locate fully qualified kind for %v: found %v when registering for %v", reflect.TypeOf(object), fqKinds, a.group.GroupVersion)
-		}
-
-		parentMapping, err := a.group.Mapper.RESTMapping(parentFQKindToRegister.GroupKind(), a.group.GroupVersion.Version)
-		if err != nil {
-			return nil, err
-		}
-		mapping.Scope = parentMapping.Scope
-	}
 
 	// what verbs are supported by the storage, used to know what verbs we support per path
 	creater, isCreater := storage.(rest.Creater)
@@ -329,7 +326,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 	if ok {
 		resourceKind = kindProvider.Kind()
 	} else {
-		resourceKind = fqKindToRegister.Kind
+		resourceKind = kind
 	}
 
 	var apiResource unversioned.APIResource
@@ -451,9 +448,10 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		Creater:        a.group.Creater,
 		Convertor:      a.group.Convertor,
 
+		// TODO: This seems wrong for cross-group subresources. It makes an assumption that a subresource and its parent are in the same group version. Revisit this.
 		Resource:    a.group.GroupVersion.WithResource(resource),
 		Subresource: subresource,
-		Kind:        a.group.GroupVersion.WithKind(kind),
+		Kind:        fqKindToRegister,
 	}
 	for _, action := range actions {
 		reqScope.Namer = action.Namer
@@ -948,3 +946,19 @@ var _ rest.StorageMetadata = defaultStorageMetadata{}
 func (defaultStorageMetadata) ProducesMIMETypes(verb string) []string {
 	return nil
 }
+
+// splitSubresource checks if the given storage path is the path of a subresource and returns
+// the resource and subresource components.
+func splitSubresource(path string) (string, string, error) {
+	var resource, subresource string
+	switch parts := strings.Split(path, "/"); len(parts) {
+	case 2:
+		resource, subresource = parts[0], parts[1]
+	case 1:
+		resource = parts[0]
+	default:
+		// TODO: support deeper paths
+		return "", "", fmt.Errorf("api_installer allows only one or two segment paths (resource or resource/subresource)")
+	}
+	return resource, subresource, nil
+}

pkg/apiserver/apiserver.go

@@ -96,6 +96,12 @@ type APIGroupVersion struct {
 	Context api.RequestContextMapper
 
 	MinRequestTimeout time.Duration
+
+	// SubresourceGroupVersionKind contains the GroupVersionKind overrides for each subresource that is
+	// accessible from this API group version. The GroupVersionKind is that of the external version of
+	// the subresource. The key of this map should be the path of the subresource. The keys here should
+	// match the keys in the Storage map above for subresources.
+	SubresourceGroupVersionKind map[string]unversioned.GroupVersionKind
 }
 
 type ProxyDialerFunc func(network, addr string) (net.Conn, error)

pkg/apiserver/apiserver_test.go

@@ -59,9 +59,12 @@ func convert(obj runtime.Object) (runtime.Object, error) {
 
 // This creates fake API versions, similar to api/latest.go.
 var testAPIGroup = "test.group"
+var testAPIGroup2 = "test.group2"
 var testInternalGroupVersion = unversioned.GroupVersion{Group: testAPIGroup, Version: runtime.APIVersionInternal}
 var testGroupVersion = unversioned.GroupVersion{Group: testAPIGroup, Version: "version"}
 var newGroupVersion = unversioned.GroupVersion{Group: testAPIGroup, Version: "version2"}
+var testGroup2Version = unversioned.GroupVersion{Group: testAPIGroup2, Version: "version"}
+var testInternalGroup2Version = unversioned.GroupVersion{Group: testAPIGroup2, Version: runtime.APIVersionInternal}
 var prefix = "apis"
 
 var grouplessGroupVersion = unversioned.GroupVersion{Group: "", Version: "v1"}
@@ -99,6 +102,11 @@ func interfacesFor(version unversioned.GroupVersion) (*meta.VersionInterfaces, e
 			ObjectConvertor:  api.Scheme,
 			MetadataAccessor: accessor,
 		}, nil
+	case testGroup2Version:
+		return &meta.VersionInterfaces{
+			ObjectConvertor:  api.Scheme,
+			MetadataAccessor: accessor,
+		}, nil
 	default:
 		return nil, fmt.Errorf("unsupported storage version: %s (valid: %v)", version, groupVersions)
 	}
@@ -139,11 +147,18 @@ func addTestTypes() {
 	}
 	api.Scheme.AddKnownTypes(testGroupVersion,
 		&apiservertesting.Simple{}, &apiservertesting.SimpleList{}, &ListOptions{},
-		&api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
+		&api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{},
+		&SimpleXGSubresource{})
 	api.Scheme.AddKnownTypes(testGroupVersion, &api.Pod{})
 	api.Scheme.AddKnownTypes(testInternalGroupVersion,
 		&apiservertesting.Simple{}, &apiservertesting.SimpleList{}, &api.ListOptions{},
-		&apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
+		&apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{},
+		&SimpleXGSubresource{})
+	// Register SimpleXGSubresource in both testGroupVersion and testGroup2Version, and also their
+	// their corresponding internal versions, to verify that the desired group version object is
+	// served in the tests.
+	api.Scheme.AddKnownTypes(testGroup2Version, &SimpleXGSubresource{})
+	api.Scheme.AddKnownTypes(testInternalGroup2Version, &SimpleXGSubresource{})
 }
 
 func addNewTestTypes() {
@@ -3157,6 +3172,124 @@ func TestUpdateChecksAPIVersion(t *testing.T) {
 	}
 }
 
+// SimpleXGSubresource is a cross group subresource, i.e. the subresource does not belong to the
+// same group as its parent resource.
+type SimpleXGSubresource struct {
+	unversioned.TypeMeta `json:",inline"`
+	api.ObjectMeta       `json:"metadata"`
+	SubresourceInfo      string            `json:"subresourceInfo,omitempty"`
+	Labels               map[string]string `json:"labels,omitempty"`
+}
+
+func (obj *SimpleXGSubresource) GetObjectKind() unversioned.ObjectKind { return &obj.TypeMeta }
+
+type SimpleXGSubresourceRESTStorage struct {
+	item SimpleXGSubresource
+}
+
+func (storage *SimpleXGSubresourceRESTStorage) New() runtime.Object {
+	return &SimpleXGSubresource{}
+}
+
+func (storage *SimpleXGSubresourceRESTStorage) Get(ctx api.Context, id string) (runtime.Object, error) {
+	copied, err := api.Scheme.Copy(&storage.item)
+	if err != nil {
+		panic(err)
+	}
+	return copied, nil
+}
+
+func TestXGSubresource(t *testing.T) {
+	container := restful.NewContainer()
+	container.Router(restful.CurlyRouter{})
+	mux := container.ServeMux
+
+	itemID := "theID"
+	subresourceStorage := &SimpleXGSubresourceRESTStorage{
+		item: SimpleXGSubresource{
+			SubresourceInfo: "foo",
+		},
+	}
+	storage := map[string]rest.Storage{
+		"simple":           &SimpleRESTStorage{},
+		"simple/subsimple": subresourceStorage,
+	}
+
+	group := APIGroupVersion{
+		Storage: storage,
+
+		RequestInfoResolver: newTestRequestInfoResolver(),
+
+		Creater:   api.Scheme,
+		Convertor: api.Scheme,
+		Typer:     api.Scheme,
+		Linker:    selfLinker,
+		Mapper:    namespaceMapper,
+
+		ParameterCodec: api.ParameterCodec,
+
+		Admit:   admissionControl,
+		Context: requestContextMapper,
+
+		Root:                   "/" + prefix,
+		GroupVersion:           testGroupVersion,
+		OptionsExternalVersion: &testGroupVersion,
+		Serializer:             api.Codecs,
+
+		SubresourceGroupVersionKind: map[string]unversioned.GroupVersionKind{
+			"simple/subsimple": testGroup2Version.WithKind("SimpleXGSubresource"),
+		},
+	}
+
+	if err := (&group).InstallREST(container); err != nil {
+		panic(fmt.Sprintf("unable to install container %s: %v", group.GroupVersion, err))
+	}
+
+	ws := new(restful.WebService)
+	InstallSupport(mux, ws)
+	container.Add(ws)
+
+	handler := defaultAPIServer{mux, container}
+	server := httptest.NewServer(handler)
+	// TODO: Uncomment when fix #19254
+	// defer server.Close()
+
+	resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/" + itemID + "/subsimple")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("unexpected response: %#v", resp)
+	}
+	var itemOut SimpleXGSubresource
+	body, err := extractBody(resp, &itemOut)
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+
+	// Test if the returned object has the expected group, version and kind
+	// We are directly unmarshaling JSON here because TypeMeta cannot be decoded through the
+	// installed decoders. TypeMeta cannot be decoded because it is added to the ignored
+	// conversion type list in API scheme and hence cannot be converted from input type object
+	// to output type object. So it's values don't appear in the decoded output object.
+	decoder := json.NewDecoder(strings.NewReader(body))
+	var itemFromBody SimpleXGSubresource
+	err = decoder.Decode(&itemFromBody)
+	if err != nil {
+		t.Errorf("unexpected JSON decoding error: %v", err)
+	}
+	if want := fmt.Sprintf("%s/%s", testGroup2Version.Group, testGroup2Version.Version); itemFromBody.APIVersion != want {
+		t.Errorf("unexpected APIVersion got: %+v want: %+v", itemFromBody.APIVersion, want)
+	}
+	if itemFromBody.Kind != "SimpleXGSubresource" {
+		t.Errorf("unexpected Kind got: %+v want: SimpleXGSubresource", itemFromBody.Kind)
+	}
+
+	if itemOut.Name != subresourceStorage.item.Name {
+		t.Errorf("Unexpected data: %#v, expected %#v (%s)", itemOut, subresourceStorage.item, string(body))
+	}
+}
+
 func readBodyOrDie(r io.Reader) []byte {
 	body, err := ioutil.ReadAll(r)
 	if err != nil {

pkg/genericapiserver/genericapiserver.go

@@ -192,6 +192,12 @@ type APIGroupInfo struct {
 	NegotiatedSerializer runtime.NegotiatedSerializer
 	// ParameterCodec performs conversions for query parameters passed to API calls
 	ParameterCodec runtime.ParameterCodec
+
+	// SubresourceGroupVersionKind contains the GroupVersionKind overrides for each subresource that is
+	// accessible from this API group version. The GroupVersionKind is that of the external version of
+	// the subresource. The key of this map should be the path of the subresource. The keys here should
+	// match the keys in the Storage map above for subresources.
+	SubresourceGroupVersionKind map[string]unversioned.GroupVersionKind
 }
 
 // Config is a structure used to configure a GenericAPIServer.
@@ -838,6 +844,7 @@ func (s *GenericAPIServer) getAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV
 	version.Creater = apiGroupInfo.Scheme
 	version.Convertor = apiGroupInfo.Scheme
 	version.Typer = apiGroupInfo.Scheme
+	version.SubresourceGroupVersionKind = apiGroupInfo.SubresourceGroupVersionKind
 	return version, err
 }