Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow cross-group subresource registrations in the APIInstaller. #21909

Merged
merged 1 commit into from
Mar 3, 2016
Merged

Allow cross-group subresource registrations in the APIInstaller. #21909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
136 changes: 75 additions & 61 deletions pkg/apiserver/api_installer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,97 +110,94 @@ func (a *APIInstaller) NewWebService() *restful.WebService {
return ws
}

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

optionsExternalVersion := a.group.GroupVersion
if a.group.OptionsExternalVersion != nil {
optionsExternalVersion = *a.group.OptionsExternalVersion
}

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 nil, fmt.Errorf("api_installer allows only one or two segment paths (resource or resource/subresource)")
// 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
}
hasSubresource := len(subresource) > 0

object := storage.New()
fqKinds, err := a.group.Typer.ObjectKinds(object)
if err != nil {
return nil, err
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 = fqKind
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" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this is special-cased? I still don't, but this pull is changing behavior around this bit of hardcoding (unlike the previous comment) so I think it should be re-visited.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's deal with ThirdPartyResource in another PR. The currently implemented behavior isn't what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that should be in another PR.

fqKindToRegister = fqKind
fqKindToRegister.Group = a.group.GroupVersion.Group
fqKindToRegister.Version = a.group.GroupVersion.Version
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
}

kind := fqKindToRegister.Kind
// 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)
}

if fqKindToRegister.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)
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

optionsExternalVersion := a.group.GroupVersion
if a.group.OptionsExternalVersion != nil {
optionsExternalVersion = *a.group.OptionsExternalVersion
}

versionedPtr, err := a.group.Creater.New(a.group.GroupVersion.WithKind(kind))
resource, subresource, err := splitSubresource(path)
if err != nil {
return nil, err
}
versionedObject := indirectArbitraryPointer(versionedPtr)

mapping, err := a.group.Mapper.RESTMapping(fqKindToRegister.GroupKind(), a.group.GroupVersion.Version)
mapping, err := a.restMapping(resource)
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)
}
fqKindToRegister, err := a.getResourceKind(path, storage)
if err != nil {
return nil, err
}

parentMapping, err := a.group.Mapper.RESTMapping(parentFQKindToRegister.GroupKind(), a.group.GroupVersion.Version)
if err != nil {
return nil, err
}
mapping.Scope = parentMapping.Scope
versionedPtr, err := a.group.Creater.New(fqKindToRegister)
if err != nil {
return nil, err
}
versionedObject := indirectArbitraryPointer(versionedPtr)
kind := fqKindToRegister.Kind
hasSubresource := len(subresource) > 0

// what verbs are supported by the storage, used to know what verbs we support per path
creater, isCreater := storage.(rest.Creater)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
6 changes: 6 additions & 0 deletions pkg/apiserver/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
137 changes: 135 additions & 2 deletions pkg/apiserver/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right… we can't decode an API object using a real codec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt We can decode a serialized API object into a destination, not just TypeMeta. In fact we can decode TypeMeta just fine, but the decoded values won't be copied to the destination object (itemOut here) and that is by design. It would be dangerous to overwrite TypeMeta. We could probably implement a converter to allow overwriting TypeMeta in tests, but I don't think it is worth spending time on it.

I think this is a reasonable workaround. Any other alternative suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggett it's that I asked for the version/kind to be checked, and our codec consumes that information and leaves those fields blank.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnstructuredJSONScheme would do this, I think...

_, gvk, err := runtime.UnstructuredJSONScheme.Decode(data, nil, nil)
// assert no err
// assert gvk matches

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's json decoding either way, I just thought there was an existing codepath we could use

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt Do you feel strongly about swapping the current unmarshaling code with this snippet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt Alright then, thanks!

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))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test that the returned item is the correct group/version/kind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

func readBodyOrDie(r io.Reader) []byte {
body, err := ioutil.ReadAll(r)
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions pkg/genericapiserver/genericapiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down