From 0489d0b1cf139253b82f73b072578073bc5616d6 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Mar 2019 21:00:55 -0400 Subject: [PATCH] Unify runtime.SerializerInfo with negotiate.AcceptedMediaTypes There was no reason to have two types and this avoids ~10% of allocations on the GET code path. ``` BenchmarkGet-12 100000 109045 ns/op 17608 B/op 146 allocs/op BenchmarkGet-12 100000 108850 ns/op 15942 B/op 132 allocs/op ``` --- .../pkg/apiserver/customresource_handler.go | 10 ++- .../unstructured/unstructuredscheme/scheme.go | 10 ++- .../apimachinery/pkg/runtime/interfaces.go | 4 ++ .../pkg/runtime/serializer/codec_factory.go | 12 ++++ .../handlers/negotiation/negotiate.go | 71 ++++++------------- .../handlers/negotiation/negotiate_test.go | 20 +++++- .../apiserver/pkg/endpoints/installer.go | 5 ++ .../server/storage/storage_factory_test.go | 21 +++++- .../src/k8s.io/client-go/dynamic/scheme.go | 4 ++ 9 files changed, 100 insertions(+), 57 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index c5c9a73228c..db9b43b2694 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -644,6 +644,8 @@ func (s unstructuredNegotiatedSerializer) SupportedMediaTypes() []runtime.Serial return []runtime.SerializerInfo{ { MediaType: "application/json", + MediaTypeType: "application", + MediaTypeSubType: "json", EncodesAsText: true, Serializer: json.NewSerializer(json.DefaultMetaFactory, s.creator, s.typer, false), PrettySerializer: json.NewSerializer(json.DefaultMetaFactory, s.creator, s.typer, true), @@ -654,9 +656,11 @@ func (s unstructuredNegotiatedSerializer) SupportedMediaTypes() []runtime.Serial }, }, { - MediaType: "application/yaml", - EncodesAsText: true, - Serializer: json.NewYAMLSerializer(json.DefaultMetaFactory, s.creator, s.typer), + MediaType: "application/yaml", + MediaTypeType: "application", + MediaTypeSubType: "yaml", + EncodesAsText: true, + Serializer: json.NewYAMLSerializer(json.DefaultMetaFactory, s.creator, s.typer), }, } } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructuredscheme/scheme.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructuredscheme/scheme.go index ab2574e8287..3d7b6f05b6f 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructuredscheme/scheme.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructuredscheme/scheme.go @@ -51,6 +51,8 @@ func (s unstructuredNegotiatedSerializer) SupportedMediaTypes() []runtime.Serial return []runtime.SerializerInfo{ { MediaType: "application/json", + MediaTypeType: "application", + MediaTypeSubType: "json", EncodesAsText: true, Serializer: json.NewSerializer(json.DefaultMetaFactory, s.creator, s.typer, false), PrettySerializer: json.NewSerializer(json.DefaultMetaFactory, s.creator, s.typer, true), @@ -61,9 +63,11 @@ func (s unstructuredNegotiatedSerializer) SupportedMediaTypes() []runtime.Serial }, }, { - MediaType: "application/yaml", - EncodesAsText: true, - Serializer: json.NewYAMLSerializer(json.DefaultMetaFactory, s.creator, s.typer), + MediaType: "application/yaml", + MediaTypeType: "application", + MediaTypeSubType: "yaml", + EncodesAsText: true, + Serializer: json.NewYAMLSerializer(json.DefaultMetaFactory, s.creator, s.typer), }, } } diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go b/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go index 699ff13e04f..00bffe3081c 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go @@ -91,6 +91,10 @@ type Framer interface { type SerializerInfo struct { // MediaType is the value that represents this serializer over the wire. MediaType string + // MediaTypeType is the first part of the MediaType ("application" in "application/json"). + MediaTypeType string + // MediaTypeSubType is the second part of the MediaType ("json" in "application/json"). + MediaTypeSubType string // EncodesAsText indicates this serializer can be encoded to UTF-8 safely. EncodesAsText bool // Serializer is the individual object serializer for this media type. diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go index 65f451124d5..e0c7d60e81f 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go @@ -17,6 +17,9 @@ limitations under the License. package serializer import ( + "mime" + "strings" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer/json" @@ -120,6 +123,15 @@ func newCodecFactory(scheme *runtime.Scheme, serializers []serializerType) Codec Serializer: d.Serializer, PrettySerializer: d.PrettySerializer, } + + mediaType, _, err := mime.ParseMediaType(info.MediaType) + if err != nil { + panic(err) + } + parts := strings.SplitN(mediaType, "/", 2) + info.MediaTypeType = parts[0] + info.MediaTypeSubType = parts[1] + if d.StreamSerializer != nil { info.StreamSerializer = &runtime.StreamSerializerInfo{ Serializer: d.StreamSerializer, diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go index b513eb5edc8..8ac1e8f3568 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go @@ -43,13 +43,13 @@ func MediaTypesForSerializer(ns runtime.NegotiatedSerializer) (mediaTypes, strea // NegotiateOutputMediaType negotiates the output structured media type and a serializer, or // returns an error. func NegotiateOutputMediaType(req *http.Request, ns runtime.NegotiatedSerializer, restrictions EndpointRestrictions) (MediaTypeOptions, runtime.SerializerInfo, error) { - mediaType, ok := NegotiateMediaTypeOptions(req.Header.Get("Accept"), AcceptedMediaTypesForEndpoint(ns), restrictions) + mediaType, ok := NegotiateMediaTypeOptions(req.Header.Get("Accept"), ns.SupportedMediaTypes(), restrictions) if !ok { supported, _ := MediaTypesForSerializer(ns) return mediaType, runtime.SerializerInfo{}, NewNotAcceptableError(supported) } // TODO: move into resthandler - info := mediaType.Accepted.Serializer + info := mediaType.Accepted if (mediaType.Pretty || isPrettyPrint(req)) && info.PrettySerializer != nil { info.Serializer = info.PrettySerializer } @@ -58,12 +58,12 @@ func NegotiateOutputMediaType(req *http.Request, ns runtime.NegotiatedSerializer // NegotiateOutputMediaTypeStream returns a stream serializer for the given request. func NegotiateOutputMediaTypeStream(req *http.Request, ns runtime.NegotiatedSerializer, restrictions EndpointRestrictions) (runtime.SerializerInfo, error) { - mediaType, ok := NegotiateMediaTypeOptions(req.Header.Get("Accept"), AcceptedMediaTypesForEndpoint(ns), restrictions) - if !ok || mediaType.Accepted.Serializer.StreamSerializer == nil { + mediaType, ok := NegotiateMediaTypeOptions(req.Header.Get("Accept"), ns.SupportedMediaTypes(), restrictions) + if !ok || mediaType.Accepted.StreamSerializer == nil { _, supported := MediaTypesForSerializer(ns) return runtime.SerializerInfo{}, NewNotAcceptableError(supported) } - return mediaType.Accepted.Serializer, nil + return mediaType.Accepted, nil } // NegotiateInputSerializer returns the input serializer for the provided request. @@ -99,10 +99,13 @@ func NegotiateInputSerializerForMediaType(mediaType string, streaming bool, ns r func isPrettyPrint(req *http.Request) bool { // DEPRECATED: should be part of the content type if req.URL != nil { - pp := req.URL.Query().Get("pretty") - if len(pp) > 0 { - pretty, _ := strconv.ParseBool(pp) - return pretty + // avoid an allocation caused by parsing the URL query + if strings.Contains(req.URL.RawQuery, "pretty") { + pp := req.URL.Query().Get("pretty") + if len(pp) > 0 { + pretty, _ := strconv.ParseBool(pp) + return pretty + } } } userAgent := req.UserAgent() @@ -139,17 +142,6 @@ func (emptyEndpointRestrictions) AllowsConversion(schema.GroupVersionKind, strin func (emptyEndpointRestrictions) AllowsServerVersion(string) bool { return false } func (emptyEndpointRestrictions) AllowsStreamSchema(s string) bool { return s == "watch" } -// AcceptedMediaType contains information about a valid media type that the -// server can serialize. -type AcceptedMediaType struct { - // Type is the first part of the media type ("application") - Type string - // SubType is the second part of the media type ("json") - SubType string - // Serializer is the serialization info this object accepts - Serializer runtime.SerializerInfo -} - // MediaTypeOptions describes information for a given media type that may alter // the server response type MediaTypeOptions struct { @@ -176,13 +168,13 @@ type MediaTypeOptions struct { Unrecognized []string // the accepted media type from the client - Accepted *AcceptedMediaType + Accepted runtime.SerializerInfo } // acceptMediaTypeOptions returns an options object that matches the provided media type params. If // it returns false, the provided options are not allowed and the media type must be skipped. These // parameters are unversioned and may not be changed. -func acceptMediaTypeOptions(params map[string]string, accepts *AcceptedMediaType, endpoint EndpointRestrictions) (MediaTypeOptions, bool) { +func acceptMediaTypeOptions(params map[string]string, accepts *runtime.SerializerInfo, endpoint EndpointRestrictions) (MediaTypeOptions, bool) { var options MediaTypeOptions // extract all known parameters @@ -208,7 +200,7 @@ func acceptMediaTypeOptions(params map[string]string, accepts *AcceptedMediaType // controls the streaming schema case "stream": - if len(v) > 0 && (accepts.Serializer.StreamSerializer == nil || !endpoint.AllowsStreamSchema(v)) { + if len(v) > 0 && (accepts.StreamSerializer == nil || !endpoint.AllowsStreamSchema(v)) { return MediaTypeOptions{}, false } options.Stream = v @@ -236,16 +228,16 @@ func acceptMediaTypeOptions(params map[string]string, accepts *AcceptedMediaType } } - if options.Convert != nil && !endpoint.AllowsConversion(*options.Convert, accepts.Type, accepts.SubType) { + if options.Convert != nil && !endpoint.AllowsConversion(*options.Convert, accepts.MediaTypeType, accepts.MediaTypeSubType) { return MediaTypeOptions{}, false } - options.Accepted = accepts + options.Accepted = *accepts return options, true } type candidateMediaType struct { - accepted *AcceptedMediaType + accepted *runtime.SerializerInfo clauses goautoneg.Accept } @@ -253,10 +245,10 @@ type candidateMediaTypeSlice []candidateMediaType // NegotiateMediaTypeOptions returns the most appropriate content type given the accept header and // a list of alternatives along with the accepted media type parameters. -func NegotiateMediaTypeOptions(header string, accepted []AcceptedMediaType, endpoint EndpointRestrictions) (MediaTypeOptions, bool) { +func NegotiateMediaTypeOptions(header string, accepted []runtime.SerializerInfo, endpoint EndpointRestrictions) (MediaTypeOptions, bool) { if len(header) == 0 && len(accepted) > 0 { return MediaTypeOptions{ - Accepted: &accepted[0], + Accepted: accepted[0], }, true } @@ -266,8 +258,8 @@ func NegotiateMediaTypeOptions(header string, accepted []AcceptedMediaType, endp for i := range accepted { accepts := &accepted[i] switch { - case clause.Type == accepts.Type && clause.SubType == accepts.SubType, - clause.Type == accepts.Type && clause.SubType == "*", + case clause.Type == accepts.MediaTypeType && clause.SubType == accepts.MediaTypeSubType, + clause.Type == accepts.MediaTypeType && clause.SubType == "*", clause.Type == "*" && clause.SubType == "*": candidates = append(candidates, candidateMediaType{accepted: accepts, clauses: clause}) } @@ -282,22 +274,3 @@ func NegotiateMediaTypeOptions(header string, accepted []AcceptedMediaType, endp return MediaTypeOptions{}, false } - -// AcceptedMediaTypesForEndpoint returns an array of structs that are used to efficiently check which -// allowed media types the server exposes. -func AcceptedMediaTypesForEndpoint(ns runtime.NegotiatedSerializer) []AcceptedMediaType { - var acceptedMediaTypes []AcceptedMediaType - for _, info := range ns.SupportedMediaTypes() { - segments := strings.SplitN(info.MediaType, "/", 2) - if len(segments) == 1 { - segments = append(segments, "*") - } - t := AcceptedMediaType{ - Type: segments[0], - SubType: segments[1], - Serializer: info, - } - acceptedMediaTypes = append(acceptedMediaTypes, t) - } - return acceptedMediaTypes -} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate_test.go index 6d9bcff7bd2..72b75f34b69 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate_test.go @@ -17,8 +17,10 @@ limitations under the License. package negotiation import ( + "mime" "net/http" "net/url" + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -39,7 +41,23 @@ type fakeNegotiater struct { func (n *fakeNegotiater) SupportedMediaTypes() []runtime.SerializerInfo { var out []runtime.SerializerInfo for _, s := range n.types { - info := runtime.SerializerInfo{Serializer: n.serializer, MediaType: s, EncodesAsText: true} + mediaType, _, err := mime.ParseMediaType(s) + if err != nil { + panic(err) + } + parts := strings.SplitN(mediaType, "/", 2) + if len(parts) == 1 { + // this is an error on the server side + parts = append(parts, "") + } + + info := runtime.SerializerInfo{ + Serializer: n.serializer, + MediaType: s, + MediaTypeType: parts[0], + MediaTypeSubType: parts[1], + EncodesAsText: true, + } for _, t := range n.streamTypes { if t == s { info.StreamSerializer = &runtime.StreamSerializerInfo{ diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go index c48c2d61f63..21cd3c87733 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go @@ -512,6 +512,11 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag // // test/integration/auth_test.go is currently the most comprehensive status code test + for _, s := range a.group.Serializer.SupportedMediaTypes() { + if len(s.MediaTypeSubType) == 0 || len(s.MediaTypeType) == 0 { + return nil, fmt.Errorf("all serializers in the group Serializer must have MediaTypeType and MediaTypeSubType set: %s", s.MediaType) + } + } mediaTypes, streamMediaTypes := negotiation.MediaTypesForSerializer(a.group.Serializer) allMediaTypes := append(mediaTypes, streamMediaTypes...) ws.Produces(allMediaTypes...) diff --git a/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go b/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go index 3c8c1c2d3c6..8bbbbb9ff34 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go @@ -17,7 +17,9 @@ limitations under the License. package storage import ( + "mime" "reflect" + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -60,7 +62,24 @@ type fakeNegotiater struct { func (n *fakeNegotiater) SupportedMediaTypes() []runtime.SerializerInfo { var out []runtime.SerializerInfo for _, s := range n.types { - info := runtime.SerializerInfo{Serializer: n.serializer, MediaType: s, EncodesAsText: true} + mediaType, _, err := mime.ParseMediaType(s) + if err != nil { + panic(err) + } + parts := strings.SplitN(mediaType, "/", 2) + if len(parts) == 1 { + // this is an error on the server side + parts = append(parts, "") + } + + info := runtime.SerializerInfo{ + Serializer: n.serializer, + MediaType: s, + MediaTypeType: parts[0], + MediaTypeSubType: parts[1], + EncodesAsText: true, + } + for _, t := range n.streamTypes { if t == s { info.StreamSerializer = &runtime.StreamSerializerInfo{ diff --git a/staging/src/k8s.io/client-go/dynamic/scheme.go b/staging/src/k8s.io/client-go/dynamic/scheme.go index c4aa081f91f..4596104d8a6 100644 --- a/staging/src/k8s.io/client-go/dynamic/scheme.go +++ b/staging/src/k8s.io/client-go/dynamic/scheme.go @@ -43,6 +43,8 @@ func init() { var watchJsonSerializerInfo = runtime.SerializerInfo{ MediaType: "application/json", + MediaTypeType: "application", + MediaTypeSubType: "json", EncodesAsText: true, Serializer: json.NewSerializer(json.DefaultMetaFactory, watchScheme, watchScheme, false), PrettySerializer: json.NewSerializer(json.DefaultMetaFactory, watchScheme, watchScheme, true), @@ -77,6 +79,8 @@ func (s basicNegotiatedSerializer) SupportedMediaTypes() []runtime.SerializerInf return []runtime.SerializerInfo{ { MediaType: "application/json", + MediaTypeType: "application", + MediaTypeSubType: "json", EncodesAsText: true, Serializer: json.NewSerializer(json.DefaultMetaFactory, basicScheme, basicScheme, false), PrettySerializer: json.NewSerializer(json.DefaultMetaFactory, basicScheme, basicScheme, true),