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

update requestScope to fully qualify kind and resource #17956

Merged
merged 1 commit into from
Dec 2, 2015
Merged

update requestScope to fully qualify kind and resource #17956

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
27 changes: 24 additions & 3 deletions pkg/api/unversioned/group_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@ import (
"strings"
)

// GroupVersionResource unambiguously identifies a resource. It doesn't anonymously include GroupVersion
// to avoid automatic coersion. It doesn't use a GroupVersion to avoid custom marshalling
type GroupVersionResource struct {
Group string
Version string
Resource string
}

func (gvr GroupVersionResource) GroupVersion() GroupVersion {
return GroupVersion{Group: gvr.Group, Version: gvr.Version}
}

func (gvr *GroupVersionResource) String() string {
return gvr.Group + "/" + gvr.Version + ", Resource=" + gvr.Resource
}

// GroupKind specifies a Group and a Kind, but does not force a version. This is useful for identifying
// concepts during lookup stages without having partially valid types
type GroupKind struct {
Expand All @@ -45,9 +61,9 @@ type GroupVersionKind struct {
Kind string
}

// TODO remove this
func NewGroupVersionKind(gv GroupVersion, kind string) GroupVersionKind {
return GroupVersionKind{Group: gv.Group, Version: gv.Version, Kind: kind}
// IsEmpty returns true if group, version, and kind are empty
func (gvk GroupVersionKind) IsEmpty() bool {
return len(gvk.Group) == 0 && len(gvk.Version) == 0 && len(gvk.Kind) == 0
}

func (gvk GroupVersionKind) GroupKind() GroupKind {
Expand Down Expand Up @@ -125,6 +141,11 @@ func (gv GroupVersion) WithKind(kind string) GroupVersionKind {
return GroupVersionKind{Group: gv.Group, Version: gv.Version, Kind: kind}
}

// WithResource creates a GroupVersionResource based on the method receiver's GroupVersion and the passed Resource.
func (gv GroupVersion) WithResource(resource string) GroupVersionResource {
return GroupVersionResource{Group: gv.Group, Version: gv.Version, Resource: resource}
}

// MarshalJSON implements the json.Marshaller interface.
func (gv GroupVersion) MarshalJSON() ([]byte, error) {
s := gv.String()
Expand Down
58 changes: 36 additions & 22 deletions pkg/apiserver/api_installer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,18 +216,27 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
}
versionedStatus := indirectArbitraryPointer(versionedStatusPtr)
var (
getOptions runtime.Object
versionedGetOptions runtime.Object
getOptionsKind string
getSubpath bool
getSubpathKey string
getOptions runtime.Object
versionedGetOptions runtime.Object
getOptionsInternalKind unversioned.GroupVersionKind
getOptionsExternalKind unversioned.GroupVersionKind
getSubpath bool
getSubpathKey string
)
if isGetterWithOptions {
getOptions, getSubpath, getSubpathKey = getterWithOptions.NewGetOptions()
_, getOptionsKind, err = a.group.Typer.ObjectVersionAndKind(getOptions)
getOptionsGVString, getOptionsKind, err := a.group.Typer.ObjectVersionAndKind(getOptions)
Copy link
Member

Choose a reason for hiding this comment

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

What if several groups share the same getOptions in the future? Then the getOptionsGVString may not equal to reqScope.InternalVersion (which is the old behavior).

I prefer the old way, using the reqScope.InternalVersion, which must be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the old way, using the reqScope.InternalVersion, which must be correct.

I think you may have gotten the fields backwards. The reason we had to have the hack (different version) before was precisely because the reqScope.InternalVersion was never correct unless you were in the legacy kube group. For every other group, your reqScope.InternalVersion is for your group, but your getOptions were for /v1 (or unversioned).

This code is done in the order that always correct: "Give me an instance of the getOptions type you expect and I'll determine the exact internal group."

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the continuous questions here, but let me ask one more. It seems the only getOptions in our code base is the PodLogOptions, it's handled at path /pods/log, and the reqScope.InternalVersion is "", which is correct. So my question is:

your reqScope.InternalVersion is for your group, but your getOptions were for /v1` (or unversioned)

when will this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the continuous questions here, but let me ask one more. It seems the only getOptions in our code base is the PodLogOptions, it's handled at path /pods/log, and the reqScope.InternalVersion is "", which is correct. So my question is:

your reqScope.InternalVersion is for your group, but your getOptions were for /v1` (or unversioned)
when will this happen?

This ends up mattering downstream when you're trying to have compatibility with other APIs. The origin /logs endpoints as a for instance. The final method destination is also shared with ConnectResource (the odd refactoring was done as part of the ListOptions work of all things).

I think explicitly locating exactly what's needed gives us flexibility where we need it, since query parameters are not currently self-identifying.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed explanation. It makes sense.

One last comment, can we get rid of the reqScope.InternalVersion? It seems no one uses it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One last comment, can we get rid of the reqScope.InternalVersion? It seems no one uses it anymore.

Good call. Removed.

if err != nil {
return nil, err
}
gv, err := unversioned.ParseGroupVersion(getOptionsGVString)
if err != nil {
return nil, err
}
getOptionsInternalKind = gv.WithKind(getOptionsKind)
// TODO this should be a list of all the different external versions we can coerce into the internalKind
getOptionsExternalKind = serverGroupVersion.WithKind(getOptionsKind)

versionedGetOptions, err = a.group.Creater.New(serverGroupVersion.String(), getOptionsKind)
if err != nil {
return nil, err
Expand All @@ -236,19 +245,28 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
}

var (
connectOptions runtime.Object
versionedConnectOptions runtime.Object
connectOptionsKind string
connectSubpath bool
connectSubpathKey string
connectOptions runtime.Object
versionedConnectOptions runtime.Object
connectOptionsInternalKind unversioned.GroupVersionKind
connectOptionsExternalKind unversioned.GroupVersionKind
connectSubpath bool
connectSubpathKey string
)
if isConnecter {
connectOptions, connectSubpath, connectSubpathKey = connecter.NewConnectOptions()
if connectOptions != nil {
_, connectOptionsKind, err = a.group.Typer.ObjectVersionAndKind(connectOptions)
connectOptionsGVString, connectOptionsKind, err := a.group.Typer.ObjectVersionAndKind(connectOptions)
if err != nil {
return nil, err
}
gv, err := unversioned.ParseGroupVersion(connectOptionsGVString)
if err != nil {
return nil, err
}
connectOptionsInternalKind = gv.WithKind(connectOptionsKind)
// TODO this should be a list of all the different external versions we can coerce into the internalKind
connectOptionsExternalKind = serverGroupVersion.WithKind(connectOptionsKind)

versionedConnectOptions, err = a.group.Creater.New(serverGroupVersion.String(), connectOptionsKind)
}
}
Expand Down Expand Up @@ -383,14 +401,10 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
Creater: a.group.Creater,
Convertor: a.group.Convertor,
Codec: mapping.Codec,
APIVersion: a.group.GroupVersion.String(),
// TODO, this internal version needs to plumbed through from the caller
// this works in all the cases we have now
InternalVersion: unversioned.GroupVersion{Group: a.group.GroupVersion.Group},
ServerAPIVersion: serverGroupVersion.String(),
Resource: resource,
Subresource: subresource,
Kind: kind,

Resource: a.group.GroupVersion.WithResource(resource),
Subresource: subresource,
Kind: a.group.GroupVersion.WithKind(kind),
}
for _, action := range actions {
reqScope.Namer = action.Namer
Expand All @@ -403,7 +417,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, getSubpath, getSubpathKey)
handler = GetResourceWithOptions(getterWithOptions, reqScope, getOptionsInternalKind, getOptionsExternalKind, getSubpath, getSubpathKey)
} else {
handler = GetResource(getter, reqScope)
}
Expand Down Expand Up @@ -584,7 +598,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
doc = "connect " + method + " requests to " + subresource + " of " + kind
}
route := ws.Method(method).Path(action.Path).
To(ConnectResource(connecter, reqScope, admit, connectOptionsKind, path, connectSubpath, connectSubpathKey)).
To(ConnectResource(connecter, reqScope, admit, connectOptionsInternalKind, connectOptionsExternalKind, path, connectSubpath, connectSubpathKey)).
Filter(m).
Doc(doc).
Operation("connect" + strings.Title(strings.ToLower(method)) + namespaced + kind + strings.Title(subresource)).
Expand Down
4 changes: 2 additions & 2 deletions pkg/apiserver/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,9 @@ func SupportedResourcesHandler(groupVersion unversioned.GroupVersion, apiResourc
// response. The Accept header and current API version will be passed in, and the output will be copied
// directly to the response body. If content type is returned it is used, otherwise the content type will
// be "application/octet-stream". All other objects are sent to standard JSON serialization.
func write(statusCode int, apiVersion string, codec runtime.Codec, object runtime.Object, w http.ResponseWriter, req *http.Request) {
func write(statusCode int, groupVersion unversioned.GroupVersion, codec runtime.Codec, object runtime.Object, w http.ResponseWriter, req *http.Request) {
if stream, ok := object.(rest.ResourceStreamer); ok {
out, flush, contentType, err := stream.InputStream(apiVersion, req.Header.Get("Accept"))
out, flush, contentType, err := stream.InputStream(groupVersion.String(), req.Header.Get("Accept"))
if err != nil {
errorJSONFatal(err, codec, w)
return
Expand Down