From 2e1506558ac2c82e83cbef469a6e1e845cfc9235 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Mar 2019 20:45:19 -0400 Subject: [PATCH 1/5] Add benchmark for naive endpoint Get so we can measure it Baseline: ``` BenchmarkGet-12 100000 119635 ns/op 20110 B/op 206 allocs/op BenchmarkWatchHTTP-12 100000 65761 ns/op 7296 B/op 139 allocs/op ``` --- .../apiserver/pkg/endpoints/apiserver_test.go | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index bf6c0e69f1b..e65a0566e80 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -1616,6 +1616,41 @@ func TestGet(t *testing.T) { } } +func BenchmarkGet(b *testing.B) { + storage := map[string]rest.Storage{} + simpleStorage := SimpleRESTStorage{ + item: genericapitesting.Simple{ + Other: "foo", + }, + } + selfLinker := &setTestSelfLinker{ + expectedSet: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/id", + name: "id", + namespace: "default", + } + storage["simple"] = &simpleStorage + handler := handleLinker(storage, selfLinker) + server := httptest.NewServer(handler) + defer server.Close() + + u := server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/id" + + b.ResetTimer() + for i := 0; i < b.N; i++ { + resp, err := http.Get(u) + if err != nil { + b.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + b.Fatalf("unexpected response: %#v", resp) + } + if _, err := io.Copy(ioutil.Discard, resp.Body); err != nil { + b.Fatalf("unable to read body") + } + } + b.StopTimer() +} + func TestGetCompression(t *testing.T) { storage := map[string]rest.Storage{} simpleStorage := SimpleRESTStorage{ From 58fb665646aa4c1b63f1322a50e3af7a60e39886 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Mar 2019 20:43:26 -0400 Subject: [PATCH 2/5] IsListType uses reflection and is expensive for hot paths IsListType was causing ~100 allocations for a non list object. It is used in a wide range of code, so perform a more targeted check. The use of `%#v` in a hot return path for `fmt.Errorf()` was the main victim. Replace `%#v` with a typed error and create a cache of types that are lists with a bounded size (probably not necessary, but safer). ``` BenchmarkGet-12 100000 119635 ns/op 20110 B/op 206 allocs/op BenchmarkWatchHTTP-12 100000 65761 ns/op 7296 B/op 139 allocs/op BenchmarkGet-12 100000 109085 ns/op 17831 B/op 152 allocs/op BenchmarkWatchHTTP-12 200000 33966 ns/op 1913 B/op 30 allocs/op ``` --- .../k8s.io/apimachinery/pkg/api/meta/help.go | 66 ++++++++++++++++--- .../apiserver/pkg/endpoints/handlers/rest.go | 5 ++ 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/api/meta/help.go b/staging/src/k8s.io/apimachinery/pkg/api/meta/help.go index 3425055f6ec..f6e2f1a1f58 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/meta/help.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/meta/help.go @@ -17,30 +17,76 @@ limitations under the License. package meta import ( + "errors" "fmt" "reflect" + "sync" "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/runtime" ) -// IsListType returns true if the provided Object has a slice called Items +var ( + // isListCache maintains a cache of types that are checked for lists + // which is used by IsListType. + // TODO: remove and replace with an interface check + isListCache = struct { + lock sync.RWMutex + byType map[reflect.Type]bool + }{ + byType: make(map[reflect.Type]bool, 1024), + } +) + +// IsListType returns true if the provided Object has a slice called Items. +// TODO: Replace the code in this check with an interface comparison by +// creating and enforcing that lists implement a list accessor. func IsListType(obj runtime.Object) bool { - // if we're a runtime.Unstructured, check whether this is a list. - // TODO: refactor GetItemsPtr to use an interface that returns []runtime.Object - if unstructured, ok := obj.(runtime.Unstructured); ok { - return unstructured.IsList() + switch t := obj.(type) { + case runtime.Unstructured: + return t.IsList() + } + t := reflect.TypeOf(obj) + + isListCache.lock.RLock() + ok, exists := isListCache.byType[t] + isListCache.lock.RUnlock() + + if !exists { + _, err := getItemsPtr(obj) + ok = err == nil + + // cache only the first 1024 types + isListCache.lock.Lock() + if len(isListCache.byType) < 1024 { + isListCache.byType[t] = ok + } + isListCache.lock.Unlock() } - _, err := GetItemsPtr(obj) - return err == nil + return ok } +var ( + errExpectFieldItems = errors.New("no Items field in this object") + errExpectSliceItems = errors.New("Items field must be a slice of objects") +) + // GetItemsPtr returns a pointer to the list object's Items member. // If 'list' doesn't have an Items member, it's not really a list type // and an error will be returned. // This function will either return a pointer to a slice, or an error, but not both. +// TODO: this will be replaced with an interface in the future func GetItemsPtr(list runtime.Object) (interface{}, error) { + obj, err := getItemsPtr(list) + if err != nil { + return nil, fmt.Errorf("%T is not a list: %v", err) + } + return obj, nil +} + +// getItemsPtr returns a pointer to the list object's Items member or an error. +func getItemsPtr(list runtime.Object) (interface{}, error) { v, err := conversion.EnforcePtr(list) if err != nil { return nil, err @@ -48,19 +94,19 @@ func GetItemsPtr(list runtime.Object) (interface{}, error) { items := v.FieldByName("Items") if !items.IsValid() { - return nil, fmt.Errorf("no Items field in %#v", list) + return nil, errExpectFieldItems } switch items.Kind() { case reflect.Interface, reflect.Ptr: target := reflect.TypeOf(items.Interface()).Elem() if target.Kind() != reflect.Slice { - return nil, fmt.Errorf("items: Expected slice, got %s", target.Kind()) + return nil, errExpectSliceItems } return items.Interface(), nil case reflect.Slice: return items.Addr().Interface(), nil default: - return nil, fmt.Errorf("items: Expected slice, got %s", items.Kind()) + return nil, errExpectSliceItems } } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go index f482ae317b3..c184ce5f99c 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go @@ -292,7 +292,12 @@ func checkName(obj runtime.Object, name, namespace string, namer ScopeNamer) err } // setObjectSelfLink sets the self link of an object as needed. +// TODO: remove the need for the namer LinkSetters by requiring objects implement either Object or List +// interfaces func setObjectSelfLink(ctx context.Context, obj runtime.Object, req *http.Request, namer ScopeNamer) error { + // We only generate list links on objects that implement ListInterface - historically we duck typed this + // check via reflection, but as we move away from reflection we require that you not only carry Items but + // ListMeta into order to be identified as a list. if !meta.IsListType(obj) { requestInfo, ok := request.RequestInfoFrom(ctx) if !ok { From 83c41eab1d6f55c9ccf61744bfc345c95d3a0664 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Mar 2019 20:48:00 -0400 Subject: [PATCH 3/5] Avoid an allocation on all requests when checking for an old user agent ReplaceAllStrings always allocates, while scanning a relatively short regex twice is slightly more CPU immediately but less later. ``` BenchmarkGet-12 100000 108824 ns/op 17818 B/op 152 allocs/op BenchmarkGet-12 100000 108013 ns/op 17732 B/op 149 allocs/op ``` --- .../src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index cbe632d14ec..ecb5587c075 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -34,7 +34,7 @@ import ( "k8s.io/apiserver/pkg/features" utilfeature "k8s.io/apiserver/pkg/util/feature" - "github.com/emicklei/go-restful" + restful "github.com/emicklei/go-restful" "github.com/prometheus/client_golang/prometheus" ) @@ -346,7 +346,10 @@ func cleanUserAgent(ua string) string { return "Browser" } // If an old "kubectl.exe" has passed us its full path, we discard the path portion. - ua = kubectlExeRegexp.ReplaceAllString(ua, "$1") + if kubectlExeRegexp.MatchString(ua) { + // avoid an allocation + ua = kubectlExeRegexp.ReplaceAllString(ua, "$1") + } return ua } From 59b4f47b225029068bf015117ada1a06d54fbd59 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Mar 2019 20:57:36 -0400 Subject: [PATCH 4/5] Avoid 3 object creations when no dryRun parameter is provided These allocations should only occur rarely. ``` BenchmarkGet-12 100000 108013 ns/op 17732 B/op 149 allocs/op BenchmarkGet-12 100000 109045 ns/op 17608 B/op 146 allocs/op ``` --- .../apiserver/pkg/endpoints/metrics/metrics.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index ecb5587c075..7c5154295ee 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -20,6 +20,7 @@ import ( "bufio" "net" "net/http" + "net/url" "regexp" "strconv" "strings" @@ -234,7 +235,7 @@ func RecordLongRunning(req *http.Request, requestInfo *request.RequestInfo, comp // a request. verb must be uppercase to be backwards compatible with existing monitoring tooling. func MonitorRequest(req *http.Request, verb, group, version, resource, subresource, scope, component, contentType string, httpCode, respSize int, elapsed time.Duration) { reportedVerb := cleanVerb(verb, req) - dryRun := cleanDryRun(req.URL.Query()["dryRun"]) + dryRun := cleanDryRun(req.URL) client := cleanUserAgent(utilnet.GetHTTPClient(req)) elapsedMicroseconds := float64(elapsed / time.Microsecond) elapsedSeconds := elapsed.Seconds() @@ -331,12 +332,19 @@ func cleanVerb(verb string, request *http.Request) string { return reportedVerb } -func cleanDryRun(dryRun []string) string { +func cleanDryRun(u *url.URL) string { + // avoid allocating when we don't see dryRun in the query + if !strings.Contains(u.RawQuery, "dryRun") { + return "" + } + dryRun := u.Query()["dryRun"] if errs := validation.ValidateDryRun(nil, dryRun); len(errs) > 0 { return "invalid" } // Since dryRun could be valid with any arbitrarily long length // we have to dedup and sort the elements before joining them together + // TODO: this is a fairly large allocation for what it does, consider + // a sort and dedup in a single pass return strings.Join(utilsets.NewString(dryRun...).List(), ",") } From 0489d0b1cf139253b82f73b072578073bc5616d6 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Mar 2019 21:00:55 -0400 Subject: [PATCH 5/5] 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),