From 66a14268c591a30090de4f89185b8d880ddbdf18 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Wed, 16 Oct 2024 17:27:56 -0400 Subject: [PATCH 1/7] Use runtime.SerializerInfo in place of internal "serializerType". CodecFactory construction uses an unexported struct type named "serializerType" to hold serializer definitions. There are few differences between it and runtime.SerializerInfo, and they do not appear to be used anymore. For example, serializerType includes an unused FileExtensions field, and has distinct ContentType (singular) and AcceptContentTypes (plural) fields instead of runtime.SerializeInfo's singular MediaType. All remaining uses of serializerType set AcceptContentTypes to a single-entry slice whose element is equal to its ContentType field. During construction of a CodecFactory, all serializerType values were already being mechanically translated into runtime.SerializerInfo values. Moving to an exported type for serializer definitions makes it easier to expose an option to allow callers to register their own serializer definitions, which in turn makes it possible to conditionally include new serializers at runtime (especially behind feature gates). --- .../pkg/runtime/serializer/codec_factory.go | 134 ++++++------------ 1 file changed, 44 insertions(+), 90 deletions(-) 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 ff982084204..52aac57ac86 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,9 +17,6 @@ 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" @@ -28,41 +25,26 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer/versioning" ) -// serializerExtensions are for serializers that are conditionally compiled in -var serializerExtensions = []func(*runtime.Scheme) (serializerType, bool){} - -type serializerType struct { - AcceptContentTypes []string - ContentType string - FileExtensions []string - // EncodesAsText should be true if this content type can be represented safely in UTF-8 - EncodesAsText bool - - Serializer runtime.Serializer - PrettySerializer runtime.Serializer - StrictSerializer runtime.Serializer - - AcceptStreamContentTypes []string - StreamContentType string - - Framer runtime.Framer - StreamSerializer runtime.Serializer -} - -func newSerializersForScheme(scheme *runtime.Scheme, mf json.MetaFactory, options CodecFactoryOptions) []serializerType { +func newSerializersForScheme(scheme *runtime.Scheme, mf json.MetaFactory, options CodecFactoryOptions) []runtime.SerializerInfo { jsonSerializer := json.NewSerializerWithOptions( mf, scheme, scheme, json.SerializerOptions{Yaml: false, Pretty: false, Strict: options.Strict}, ) - jsonSerializerType := serializerType{ - AcceptContentTypes: []string{runtime.ContentTypeJSON}, - ContentType: runtime.ContentTypeJSON, - FileExtensions: []string{"json"}, - EncodesAsText: true, - Serializer: jsonSerializer, - - Framer: json.Framer, - StreamSerializer: jsonSerializer, + jsonSerializerType := runtime.SerializerInfo{ + MediaType: runtime.ContentTypeJSON, + MediaTypeType: "application", + MediaTypeSubType: "json", + EncodesAsText: true, + Serializer: jsonSerializer, + StrictSerializer: json.NewSerializerWithOptions( + mf, scheme, scheme, + json.SerializerOptions{Yaml: false, Pretty: false, Strict: true}, + ), + StreamSerializer: &runtime.StreamSerializerInfo{ + EncodesAsText: true, + Serializer: jsonSerializer, + Framer: json.Framer, + }, } if options.Pretty { jsonSerializerType.PrettySerializer = json.NewSerializerWithOptions( @@ -71,12 +53,6 @@ func newSerializersForScheme(scheme *runtime.Scheme, mf json.MetaFactory, option ) } - strictJSONSerializer := json.NewSerializerWithOptions( - mf, scheme, scheme, - json.SerializerOptions{Yaml: false, Pretty: false, Strict: true}, - ) - jsonSerializerType.StrictSerializer = strictJSONSerializer - yamlSerializer := json.NewSerializerWithOptions( mf, scheme, scheme, json.SerializerOptions{Yaml: true, Pretty: false, Strict: options.Strict}, @@ -88,35 +64,31 @@ func newSerializersForScheme(scheme *runtime.Scheme, mf json.MetaFactory, option protoSerializer := protobuf.NewSerializer(scheme, scheme) protoRawSerializer := protobuf.NewRawSerializer(scheme, scheme) - serializers := []serializerType{ + serializers := []runtime.SerializerInfo{ jsonSerializerType, { - AcceptContentTypes: []string{runtime.ContentTypeYAML}, - ContentType: runtime.ContentTypeYAML, - FileExtensions: []string{"yaml"}, - EncodesAsText: true, - Serializer: yamlSerializer, - StrictSerializer: strictYAMLSerializer, + MediaType: runtime.ContentTypeYAML, + MediaTypeType: "application", + MediaTypeSubType: "yaml", + EncodesAsText: true, + Serializer: yamlSerializer, + StrictSerializer: strictYAMLSerializer, }, { - AcceptContentTypes: []string{runtime.ContentTypeProtobuf}, - ContentType: runtime.ContentTypeProtobuf, - FileExtensions: []string{"pb"}, - Serializer: protoSerializer, + MediaType: runtime.ContentTypeProtobuf, + MediaTypeType: "application", + MediaTypeSubType: "vnd.kubernetes.protobuf", + Serializer: protoSerializer, // note, strict decoding is unsupported for protobuf, // fall back to regular serializing StrictSerializer: protoSerializer, - - Framer: protobuf.LengthDelimitedFramer, - StreamSerializer: protoRawSerializer, + StreamSerializer: &runtime.StreamSerializerInfo{ + Serializer: protoRawSerializer, + Framer: protobuf.LengthDelimitedFramer, + }, }, } - for _, fn := range serializerExtensions { - if serializer, ok := fn(scheme); ok { - serializers = append(serializers, serializer) - } - } return serializers } @@ -184,7 +156,7 @@ func NewCodecFactory(scheme *runtime.Scheme, mutators ...CodecFactoryOptionsMuta } // newCodecFactory is a helper for testing that allows a different metafactory to be specified. -func newCodecFactory(scheme *runtime.Scheme, serializers []serializerType) CodecFactory { +func newCodecFactory(scheme *runtime.Scheme, serializers []runtime.SerializerInfo) CodecFactory { decoders := make([]runtime.Decoder, 0, len(serializers)) var accepts []runtime.SerializerInfo alreadyAccepted := make(map[string]struct{}) @@ -192,38 +164,20 @@ func newCodecFactory(scheme *runtime.Scheme, serializers []serializerType) Codec var legacySerializer runtime.Serializer for _, d := range serializers { decoders = append(decoders, d.Serializer) - for _, mediaType := range d.AcceptContentTypes { - if _, ok := alreadyAccepted[mediaType]; ok { - continue - } - alreadyAccepted[mediaType] = struct{}{} - info := runtime.SerializerInfo{ - MediaType: d.ContentType, - EncodesAsText: d.EncodesAsText, - Serializer: d.Serializer, - PrettySerializer: d.PrettySerializer, - StrictSerializer: d.StrictSerializer, - } + if _, ok := alreadyAccepted[d.MediaType]; ok { + continue + } + alreadyAccepted[d.MediaType] = struct{}{} - mediaType, _, err := mime.ParseMediaType(info.MediaType) - if err != nil { - panic(err) - } - parts := strings.SplitN(mediaType, "/", 2) - info.MediaTypeType = parts[0] - info.MediaTypeSubType = parts[1] + acceptedSerializerShallowCopy := d + if d.StreamSerializer != nil { + cloned := *d.StreamSerializer + acceptedSerializerShallowCopy.StreamSerializer = &cloned + } + accepts = append(accepts, acceptedSerializerShallowCopy) - if d.StreamSerializer != nil { - info.StreamSerializer = &runtime.StreamSerializerInfo{ - Serializer: d.StreamSerializer, - EncodesAsText: d.EncodesAsText, - Framer: d.Framer, - } - } - accepts = append(accepts, info) - if mediaType == runtime.ContentTypeJSON { - legacySerializer = d.Serializer - } + if d.MediaType == runtime.ContentTypeJSON { + legacySerializer = d.Serializer } } if legacySerializer == nil { From db1239d354f90454a158ff8d0a27e6b8d6e927b0 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Wed, 16 Oct 2024 17:41:00 -0400 Subject: [PATCH 2/7] Add WithSerializer option to add serializers to CodecFactory. --- .../pkg/runtime/serializer/codec_factory.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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 52aac57ac86..77bb3074525 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 @@ -89,6 +89,10 @@ func newSerializersForScheme(scheme *runtime.Scheme, mf json.MetaFactory, option }, } + for _, f := range options.serializers { + serializers = append(serializers, f(scheme, scheme)) + } + return serializers } @@ -108,6 +112,8 @@ type CodecFactoryOptions struct { Strict bool // Pretty includes a pretty serializer along with the non-pretty one Pretty bool + + serializers []func(runtime.ObjectCreater, runtime.ObjectTyper) runtime.SerializerInfo } // CodecFactoryOptionsMutator takes a pointer to an options struct and then modifies it. @@ -134,6 +140,13 @@ func DisableStrict(options *CodecFactoryOptions) { options.Strict = false } +// WithSerializer configures a serializer to be supported in addition to the default serializers. +func WithSerializer(f func(runtime.ObjectCreater, runtime.ObjectTyper) runtime.SerializerInfo) CodecFactoryOptionsMutator { + return func(options *CodecFactoryOptions) { + options.serializers = append(options.serializers, f) + } +} + // NewCodecFactory provides methods for retrieving serializers for the supported wire formats // and conversion wrappers to define preferred internal and external versions. In the future, // as the internal version is used less, callers may instead use a defaulting serializer and From d638d64572e1e0086c4e762aee95c6e7a5db1a9e Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Wed, 16 Oct 2024 17:42:06 -0400 Subject: [PATCH 3/7] Add CBOR serializer option to disable JSON transcoding of raw types. --- .../pkg/runtime/serializer/cbor/cbor.go | 25 ++++++++++++++--- .../pkg/runtime/serializer/cbor/cbor_test.go | 27 +++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor.go b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor.go index 46c1b009491..20fa7eb04cf 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor.go @@ -68,17 +68,30 @@ type Serializer interface { var _ Serializer = &serializer{} type options struct { - strict bool + strict bool + transcode bool } type Option func(*options) +// Strict configures a serializer to return a strict decoding error when it encounters map keys that +// do not correspond to a field in the target object of a decode operation. This option is disabled +// by default. func Strict(s bool) Option { return func(opts *options) { opts.strict = s } } +// Transcode configures a serializer to transcode the "raw" bytes of a decoded runtime.RawExtension +// or metav1.FieldsV1 object to JSON. This is enabled by default to support existing programs that +// depend on the assumption that objects of either type contain valid JSON. +func Transcode(s bool) Option { + return func(opts *options) { + opts.transcode = s + } +} + type serializer struct { metaFactory metaFactory creater runtime.ObjectCreater @@ -88,6 +101,8 @@ type serializer struct { func (serializer) private() {} +// NewSerializer creates and returns a serializer configured with the provided options. The default +// options are equivalent to explicitly passing Strict(false) and Transcode(true). func NewSerializer(creater runtime.ObjectCreater, typer runtime.ObjectTyper, options ...Option) Serializer { return newSerializer(&defaultMetaFactory{}, creater, typer, options...) } @@ -98,6 +113,7 @@ func newSerializer(metaFactory metaFactory, creater runtime.ObjectCreater, typer creater: creater, typer: typer, } + s.options.transcode = true for _, o := range options { o(&s.options) } @@ -337,9 +353,10 @@ func (s *serializer) Decode(data []byte, gvk *schema.GroupVersionKind, into runt return nil, actual, err } - // TODO: Make possible to disable this behavior. - if err := transcodeRawTypes(obj); err != nil { - return nil, actual, err + if s.options.transcode { + if err := transcodeRawTypes(obj); err != nil { + return nil, actual, err + } } return obj, actual, strict diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor_test.go b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor_test.go index a47858c6ad9..23894a06258 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor_test.go @@ -345,6 +345,33 @@ func TestDecode(t *testing.T) { } }, }, + { + name: "raw types not transcoded", + options: []Option{Transcode(false)}, + data: []byte{0xa4, 0x41, 'f', 0xa1, 0x41, 'a', 0x01, 0x42, 'f', 'p', 0xa1, 0x41, 'z', 0x02, 0x41, 'r', 0xa1, 0x41, 'b', 0x03, 0x42, 'r', 'p', 0xa1, 0x41, 'y', 0x04}, + gvk: &schema.GroupVersionKind{}, + metaFactory: stubMetaFactory{gvk: &schema.GroupVersionKind{}}, + typer: stubTyper{gvks: []schema.GroupVersionKind{{Group: "x", Version: "y", Kind: "z"}}}, + into: &structWithRawFields{}, + expectedObj: &structWithRawFields{ + FieldsV1: metav1.FieldsV1{Raw: []byte{0xa1, 0x41, 'a', 0x01}}, + FieldsV1Pointer: &metav1.FieldsV1{Raw: []byte{0xa1, 0x41, 'z', 0x02}}, + // RawExtension's UnmarshalCBOR ensures the self-described CBOR tag + // is present in the result so that there is never any ambiguity in + // distinguishing CBOR from JSON or Protobuf. It is unnecessary for + // FieldsV1 to do the same because the initial byte is always + // sufficient to distinguish a valid JSON-encoded FieldsV1 from a + // valid CBOR-encoded FieldsV1. + RawExtension: runtime.RawExtension{Raw: []byte{0xd9, 0xd9, 0xf7, 0xa1, 0x41, 'b', 0x03}}, + RawExtensionPointer: &runtime.RawExtension{Raw: []byte{0xd9, 0xd9, 0xf7, 0xa1, 0x41, 'y', 0x04}}, + }, + expectedGVK: &schema.GroupVersionKind{Group: "x", Version: "y", Kind: "z"}, + assertOnError: func(t *testing.T, err error) { + if err != nil { + t.Errorf("expected nil error, got: %v", err) + } + }, + }, { name: "object with embedded typemeta and objectmeta", data: []byte("\xa2\x48metadata\xa1\x44name\x43foo\x44spec\xa0"), // {"metadata": {"name": "foo"}} From 0cad1a89b6721308746cc1a12f12de31a259a0d3 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Wed, 23 Oct 2024 16:36:25 -0400 Subject: [PATCH 4/7] Wire test-only feature gate for CBOR serving. To mitigate the risk of introducing a new protocol, integration tests for CBOR will be written using a test-only feature gate instance that is not wired to runtime options. On alpha graduation, the test-only feature gate instance will be replaced by a normal feature gate in the existing apiserver feature gate instance. --- .../pkg/apiserver/customresource_handler.go | 28 ++- .../test/integration/cbor_test.go | 236 ++++++++++++++++++ .../apiserver/pkg/features/kube_features.go | 19 ++ .../src/k8s.io/apiserver/pkg/server/config.go | 6 +- .../apiserver/pkg/server/config_test.go | 22 ++ .../pkg/util/feature/feature_gate.go | 12 + .../test_data/versioned_feature_list.yaml | 6 + test/integration/framework/cbor.go | 70 ++++++ 8 files changed, 397 insertions(+), 2 deletions(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/test/integration/cbor_test.go create mode 100644 test/integration/framework/cbor.go 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 1c27ebf730d..be1076d7912 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 @@ -54,6 +54,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/runtime/serializer/cbor" "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apimachinery/pkg/runtime/serializer/protobuf" "k8s.io/apimachinery/pkg/runtime/serializer/versioning" @@ -69,8 +70,10 @@ import ( "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/metrics" apirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/generic" genericfilters "k8s.io/apiserver/pkg/server/filters" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/apiserver/pkg/util/webhook" "k8s.io/apiserver/pkg/warning" "k8s.io/client-go/scale" @@ -600,6 +603,20 @@ func (r *crdHandler) GetCustomResourceListerCollectionDeleter(crd *apiextensions return info.storages[info.storageVersion].CustomResource, nil } +func newCBORSerializerInfo(creater runtime.ObjectCreater, typer runtime.ObjectTyper) runtime.SerializerInfo { + return runtime.SerializerInfo{ + MediaType: "application/cbor", + MediaTypeType: "application", + MediaTypeSubType: "cbor", + Serializer: cbor.NewSerializer(creater, typer), + StrictSerializer: cbor.NewSerializer(creater, typer, cbor.Strict(true)), + StreamSerializer: &runtime.StreamSerializerInfo{ + Framer: cbor.NewFramer(), + Serializer: cbor.NewSerializer(creater, typer, cbor.Transcode(false)), + }, + } +} + // getOrCreateServingInfoFor gets the CRD serving info for the given CRD UID if the key exists in the storage map. // Otherwise the function fetches the up-to-date CRD using the given CRD name and creates CRD serving info. func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crdInfo, error) { @@ -892,6 +909,11 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd }, }, } + + if utilfeature.TestOnlyFeatureGate.Enabled(features.TestOnlyCBORServingAndStorage) { + negotiatedSerializer.supportedMediaTypes = append(negotiatedSerializer.supportedMediaTypes, newCBORSerializerInfo(creator, typer)) + } + var standardSerializers []runtime.SerializerInfo for _, s := range negotiatedSerializer.SupportedMediaTypes() { if s.MediaType == runtime.ContentTypeProtobuf { @@ -955,7 +977,11 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd scaleScope := *requestScopes[v.Name] scaleConverter := scale.NewScaleConverter() scaleScope.Subresource = "scale" - scaleScope.Serializer = serializer.NewCodecFactory(scaleConverter.Scheme()) + var opts []serializer.CodecFactoryOptionsMutator + if utilfeature.TestOnlyFeatureGate.Enabled(features.TestOnlyCBORServingAndStorage) { + opts = append(opts, serializer.WithSerializer(newCBORSerializerInfo)) + } + scaleScope.Serializer = serializer.NewCodecFactory(scaleConverter.Scheme(), opts...) scaleScope.Kind = autoscalingv1.SchemeGroupVersion.WithKind("Scale") scaleScope.Namer = handlers.ContextBasedNaming{ Namer: meta.NewAccessor(), diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/cbor_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/cbor_test.go new file mode 100644 index 00000000000..fe6dc695098 --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/cbor_test.go @@ -0,0 +1,236 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package integration + +import ( + "context" + "fmt" + "testing" + + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + "k8s.io/apiextensions-apiserver/test/integration/fixtures" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" + cbor "k8s.io/apimachinery/pkg/runtime/serializer/cbor/direct" + "k8s.io/apiserver/pkg/features" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/rest" + "k8s.io/client-go/util/retry" + featuregatetesting "k8s.io/component-base/featuregate/testing" +) + +func TestCBORServingEnablement(t *testing.T) { + for _, tc := range []struct { + name string + enabled bool + }{ + {name: "enabled", enabled: true}, + {name: "disabled", enabled: false}, + } { + t.Run(tc.name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.TestOnlyFeatureGate, features.TestOnlyCBORServingAndStorage, tc.enabled) + + tearDown, config, _, err := fixtures.StartDefaultServer(t) + if err != nil { + t.Fatal(err) + } + defer tearDown() + + apiExtensionsClientset, err := apiextensionsclientset.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + dynamicClient, err := dynamic.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + + crd := &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.mygroup.example.com"}, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "mygroup.example.com", + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{{ + Name: "v1beta1", + Served: true, + Storage: true, + Schema: fixtures.AllowAllSchema(), + Subresources: &apiextensionsv1.CustomResourceSubresources{ + Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + Scale: &apiextensionsv1.CustomResourceSubresourceScale{ + SpecReplicasPath: ".spec.replicas", + StatusReplicasPath: ".status.replicas", + }, + }, + }}, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "foos", + Singular: "foo", + Kind: "Foo", + ListKind: "FooList", + }, + Scope: apiextensionsv1.ClusterScoped, + }, + } + if _, err = fixtures.CreateNewV1CustomResourceDefinition(crd, apiExtensionsClientset, dynamicClient); err != nil { + t.Fatal(err) + } + cr, err := dynamicClient.Resource(schema.GroupVersionResource{Group: "mygroup.example.com", Version: "v1beta1", Resource: "foos"}).Create( + context.TODO(), + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "mygroup.example.com/v1beta1", + "kind": "Foo", + "metadata": map[string]interface{}{ + "name": fmt.Sprintf("test-cbor-%s", tc.name), + }, + "spec": map[string]interface{}{ + "replicas": int64(0), + }, + "status": map[string]interface{}{ + "replicas": int64(0), + }, + }}, + metav1.CreateOptions{}, + ) + if err != nil { + t.Fatal(err) + } + + config = rest.CopyConfig(config) + config.NegotiatedSerializer = serializer.NewCodecFactory(runtime.NewScheme()).WithoutConversion() + config.APIPath = "/apis" + config.GroupVersion = &schema.GroupVersion{Group: "mygroup.example.com", Version: "v1beta1"} + restClient, err := rest.RESTClientFor(config) + if err != nil { + t.Fatal(err) + } + + for _, subresource := range []string{"", "status", "scale"} { + err = restClient.Get(). + Resource(crd.Spec.Names.Plural). + SubResource(subresource). + Name(cr.GetName()). + SetHeader("Accept", "application/cbor"). + Do(context.TODO()).Error() + switch { + case tc.enabled && err == nil: + // ok + case !tc.enabled && errors.IsNotAcceptable(err): + // ok + default: + t.Errorf("unexpected error on read (subresource %q): %v", subresource, err) + } + } + + createBody, err := cbor.Marshal(map[string]interface{}{ + "apiVersion": "mygroup.example.com/v1beta1", + "kind": "Foo", + "metadata": map[string]interface{}{ + "name": fmt.Sprintf("test-cbor-%s-2", tc.name), + }, + "spec": map[string]interface{}{ + "replicas": int64(0), + }, + "status": map[string]interface{}{ + "replicas": int64(0), + }, + }) + if err != nil { + t.Fatal(err) + } + err = restClient.Post(). + Resource(crd.Spec.Names.Plural). + SetHeader("Content-Type", "application/cbor"). + Body(createBody). + Do(context.TODO()).Error() + switch { + case tc.enabled && err == nil: + // ok + case !tc.enabled && errors.IsUnsupportedMediaType(err): + // ok + default: + t.Errorf("unexpected error on write: %v", err) + } + + scaleBody, err := cbor.Marshal(map[string]interface{}{ + "apiVersion": "autoscaling/v1", + "kind": "Scale", + "metadata": map[string]interface{}{ + "name": cr.GetName(), + }, + "spec": map[string]interface{}{ + "replicas": int64(0), + }, + "status": map[string]interface{}{ + "replicas": int64(0), + }, + }) + if err != nil { + t.Fatal(err) + } + err = restClient.Put(). + Resource(crd.Spec.Names.Plural). + SubResource("scale"). + Name(cr.GetName()). + SetHeader("Content-Type", "application/cbor"). + Body(scaleBody). + Do(context.TODO()).Error() + switch { + case tc.enabled && err == nil: + // ok + case !tc.enabled && errors.IsUnsupportedMediaType(err): + // ok + default: + t.Errorf("unexpected error on scale write: %v", err) + } + + err = retry.RetryOnConflict(retry.DefaultBackoff, func() error { + latest, err := dynamicClient.Resource(schema.GroupVersionResource{Group: "mygroup.example.com", Version: "v1beta1", Resource: "foos"}).Get(context.TODO(), cr.GetName(), metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + + statusBody, err := cbor.Marshal(latest.Object) + if err != nil { + t.Fatal(err) + } + + return restClient.Put(). + Resource(crd.Spec.Names.Plural). + SubResource("status"). + Name(cr.GetName()). + SetHeader("Content-Type", "application/cbor"). + Body(statusBody). + Do(context.TODO()).Error() + }) + switch { + case tc.enabled && err == nil: + // ok + case !tc.enabled && errors.IsUnsupportedMediaType(err): + // ok + default: + t.Fatalf("unexpected error on status write: %v", err) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/features/kube_features.go b/staging/src/k8s.io/apiserver/pkg/features/kube_features.go index f4c6fb3a756..1c5d1cc6d49 100644 --- a/staging/src/k8s.io/apiserver/pkg/features/kube_features.go +++ b/staging/src/k8s.io/apiserver/pkg/features/kube_features.go @@ -87,6 +87,15 @@ const ( // Allows authorization to use field and label selectors. AuthorizeWithSelectors featuregate.Feature = "AuthorizeWithSelectors" + // owner: @benluddy + // kep: https://kep.k8s.io/4222 + // + // Enables CBOR as a supported encoding for requests and responses, and as the + // preferred storage encoding for custom resources. + // + // This feature is currently PRE-ALPHA and MUST NOT be enabled outside of integration tests. + TestOnlyCBORServingAndStorage featuregate.Feature = "TestOnlyCBORServingAndStorage" + // owner: @serathius // Enables concurrent watch object decoding to avoid starving watch cache when conversion webhook is installed. ConcurrentWatchObjectDecode featuregate.Feature = "ConcurrentWatchObjectDecode" @@ -238,6 +247,7 @@ const ( func init() { runtime.Must(utilfeature.DefaultMutableFeatureGate.Add(defaultKubernetesFeatureGates)) runtime.Must(utilfeature.DefaultMutableFeatureGate.AddVersioned(defaultVersionedKubernetesFeatureGates)) + runtime.Must(utilfeature.TestOnlyMutableFeatureGate.AddVersioned(testOnlyVersionedKubernetesFeatureGates)) } // defaultVersionedKubernetesFeatureGates consists of all known Kubernetes-specific feature keys with VersionedSpecs. @@ -410,3 +420,12 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate // defaultKubernetesFeatureGates consists of legacy unversioned Kubernetes-specific feature keys. // Please do not add to this struct and use defaultVersionedKubernetesFeatureGates instead. var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{} + +// testOnlyVersionedKubernetesFeatureGates consists of features that require programmatic enablement +// for integration testing, but have not yet graduated to alpha in a release and must not be enabled +// by a runtime option. +var testOnlyVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate.VersionedSpecs{ + TestOnlyCBORServingAndStorage: { + {Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha}, + }, +} diff --git a/staging/src/k8s.io/apiserver/pkg/server/config.go b/staging/src/k8s.io/apiserver/pkg/server/config.go index e906f3a1ae5..7130bc3a04a 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/config.go +++ b/staging/src/k8s.io/apiserver/pkg/server/config.go @@ -742,7 +742,7 @@ func (c *RecommendedConfig) Complete() CompletedConfig { return c.Config.Complete(c.SharedInformerFactory) } -var allowedMediaTypes = []string{ +var defaultAllowedMediaTypes = []string{ runtime.ContentTypeJSON, runtime.ContentTypeYAML, runtime.ContentTypeProtobuf, @@ -755,6 +755,10 @@ func (c completedConfig) New(name string, delegationTarget DelegationTarget) (*G if c.Serializer == nil { return nil, fmt.Errorf("Genericapiserver.New() called with config.Serializer == nil") } + allowedMediaTypes := defaultAllowedMediaTypes + if utilfeature.TestOnlyFeatureGate.Enabled(genericfeatures.TestOnlyCBORServingAndStorage) { + allowedMediaTypes = append(allowedMediaTypes, runtime.ContentTypeCBOR) + } for _, info := range c.Serializer.SupportedMediaTypes() { var ok bool for _, mt := range allowedMediaTypes { diff --git a/staging/src/k8s.io/apiserver/pkg/server/config_test.go b/staging/src/k8s.io/apiserver/pkg/server/config_test.go index 75314e2cafa..a1e4d6f2dfd 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/config_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/config_test.go @@ -29,6 +29,7 @@ import ( "time" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" @@ -40,12 +41,14 @@ import ( "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/server/healthz" utilfeature "k8s.io/apiserver/pkg/util/feature" utilversion "k8s.io/apiserver/pkg/util/version" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/component-base/tracing" "k8s.io/klog/v2/ktesting" netutils "k8s.io/utils/net" @@ -419,3 +422,22 @@ func TestNewErrorForbiddenSerializer(t *testing.T) { t.Errorf("unexpected error: %v", err) } } + +func TestNewFeatureGatedSerializer(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.TestOnlyFeatureGate, features.TestOnlyCBORServingAndStorage, true) + + config := NewConfig(serializer.NewCodecFactory(scheme, serializer.WithSerializer(func(creater runtime.ObjectCreater, typer runtime.ObjectTyper) runtime.SerializerInfo { + return runtime.SerializerInfo{ + MediaType: "application/cbor", + MediaTypeType: "application", + MediaTypeSubType: "cbor", + } + }))) + config.ExternalAddress = "192.168.10.4:443" + config.EffectiveVersion = utilversion.NewEffectiveVersion("") + config.LoopbackClientConfig = &rest.Config{} + + if _, err := config.Complete(nil).New("test", NewEmptyDelegate()); err != nil { + t.Errorf("unexpected error: %v", err) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go b/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go index 00a9e099ba7..7c061042aab 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go +++ b/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go @@ -31,3 +31,15 @@ var ( // Top-level commands/options setup that needs to modify this feature gate should use DefaultMutableFeatureGate. DefaultFeatureGate featuregate.FeatureGate = DefaultMutableFeatureGate ) + +var ( + // TestOnlyMutableFeatureGate is a mutable version of TestOnlyFeatureGate. Only top-level + // commands/options setup and the k8s.io/component-base/featuregate/testing package should + // make use of this. + TestOnlyMutableFeatureGate featuregate.MutableVersionedFeatureGate = featuregate.NewFeatureGate() + + // TestOnlyFeatureGate is a shared global FeatureGate for features that have not yet + // graduated to alpha and require programmatic feature enablement for pre-alpha integration + // testing without exposing the feature as a runtime option. + TestOnlyFeatureGate featuregate.FeatureGate = TestOnlyMutableFeatureGate +) diff --git a/test/featuregates_linter/test_data/versioned_feature_list.yaml b/test/featuregates_linter/test_data/versioned_feature_list.yaml index 842389216be..1f521fc2c54 100644 --- a/test/featuregates_linter/test_data/versioned_feature_list.yaml +++ b/test/featuregates_linter/test_data/versioned_feature_list.yaml @@ -1228,6 +1228,12 @@ lockToDefault: false preRelease: Beta version: "1.32" +- name: TestOnlyCBORServingAndStorage + versionedSpecs: + - default: false + lockToDefault: false + preRelease: Alpha + version: "1.32" - name: TopologyAwareHints versionedSpecs: - default: false diff --git a/test/integration/framework/cbor.go b/test/integration/framework/cbor.go new file mode 100644 index 00000000000..03025531bf1 --- /dev/null +++ b/test/integration/framework/cbor.go @@ -0,0 +1,70 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package framework + +import ( + "testing" + + apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver" + metainternalscheme "k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/runtime/serializer/cbor" + "k8s.io/apiserver/pkg/features" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + aggregatorscheme "k8s.io/kube-aggregator/pkg/apiserver/scheme" + "k8s.io/kubernetes/pkg/api/legacyscheme" +) + +// EnableCBORForTest patches global state to enable the CBOR serializer and reverses those changes +// at the end of the test. As a risk mitigation, integration tests are initially written this way so +// that integration tests can be implemented fully and incrementally before exposing options +// (including feature gates) that can enable CBOR at runtime. After integration test coverage is +// complete, feature gates will be introduced to completely supersede this mechanism. +func EnableCBORServingAndStorageForTest(tb testing.TB) { + featuregatetesting.SetFeatureGateDuringTest(tb, utilfeature.TestOnlyFeatureGate, features.TestOnlyCBORServingAndStorage, true) + + newCBORSerializerInfo := func(creater runtime.ObjectCreater, typer runtime.ObjectTyper) runtime.SerializerInfo { + return runtime.SerializerInfo{ + MediaType: "application/cbor", + MediaTypeType: "application", + MediaTypeSubType: "cbor", + Serializer: cbor.NewSerializer(creater, typer), + StrictSerializer: cbor.NewSerializer(creater, typer, cbor.Strict(true)), + StreamSerializer: &runtime.StreamSerializerInfo{ + Framer: cbor.NewFramer(), + Serializer: cbor.NewSerializer(creater, typer, cbor.Transcode(false)), + }, + } + } + + // Codecs for built-in types are constructed at package initialization time and read by + // value from REST storage providers. + codecs := map[*runtime.Scheme]*serializer.CodecFactory{ + legacyscheme.Scheme: &legacyscheme.Codecs, + metainternalscheme.Scheme: &metainternalscheme.Codecs, + aggregatorscheme.Scheme: &aggregatorscheme.Codecs, + apiextensionsapiserver.Scheme: &apiextensionsapiserver.Codecs, + } + + for scheme, factory := range codecs { + original := *factory // shallow copy of original value + tb.Cleanup(func() { *codecs[scheme] = original }) + *codecs[scheme] = serializer.NewCodecFactory(scheme, serializer.WithSerializer(newCBORSerializerInfo)) + } +} From ea13190d8bd3a4bb3e82055b529aa7599ae5c6e1 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Wed, 23 Oct 2024 16:36:37 -0400 Subject: [PATCH 5/7] Add test-only client feature gates for CBOR. As with the apiserver feature gate for CBOR as a serving and storage encoding, the client feature gates for CBOR are being initially added through a test-only feature gate instance that is not wired to environment variables or to command-line flags and is intended only to be enabled programmatically from integration tests. The test-only instance will be removed as part of alpha graduation and replaced by conventional client feature gating. --- .../src/k8s.io/client-go/dynamic/scheme.go | 30 ++- .../src/k8s.io/client-go/dynamic/simple.go | 13 +- .../src/k8s.io/client-go/features/features.go | 44 ++++- .../client-go/features/known_features.go | 21 +++ .../integration/client/dynamic_client_test.go | 178 ++++++++++++++++-- test/integration/framework/cbor.go | 34 ++++ 6 files changed, 298 insertions(+), 22 deletions(-) diff --git a/staging/src/k8s.io/client-go/dynamic/scheme.go b/staging/src/k8s.io/client-go/dynamic/scheme.go index 869002284d9..dbee05312ea 100644 --- a/staging/src/k8s.io/client-go/dynamic/scheme.go +++ b/staging/src/k8s.io/client-go/dynamic/scheme.go @@ -21,7 +21,9 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer/cbor" "k8s.io/apimachinery/pkg/runtime/serializer/json" + "k8s.io/client-go/features" ) var basicScheme = runtime.NewScheme() @@ -35,11 +37,8 @@ func init() { metav1.AddToGroupVersion(parameterScheme, versionV1) } -// basicNegotiatedSerializer is used to handle discovery and error handling serialization -type basicNegotiatedSerializer struct{} - -func (s basicNegotiatedSerializer) SupportedMediaTypes() []runtime.SerializerInfo { - return []runtime.SerializerInfo{ +func newBasicNegotiatedSerializer() basicNegotiatedSerializer { + supportedMediaTypes := []runtime.SerializerInfo{ { MediaType: "application/json", MediaTypeType: "application", @@ -54,6 +53,27 @@ func (s basicNegotiatedSerializer) SupportedMediaTypes() []runtime.SerializerInf }, }, } + if features.TestOnlyFeatureGates.Enabled(features.TestOnlyClientAllowsCBOR) { + supportedMediaTypes = append(supportedMediaTypes, runtime.SerializerInfo{ + MediaType: "application/cbor", + MediaTypeType: "application", + MediaTypeSubType: "cbor", + Serializer: cbor.NewSerializer(unstructuredCreater{basicScheme}, unstructuredTyper{basicScheme}), + StreamSerializer: &runtime.StreamSerializerInfo{ + Serializer: cbor.NewSerializer(basicScheme, basicScheme, cbor.Transcode(false)), + Framer: cbor.NewFramer(), + }, + }) + } + return basicNegotiatedSerializer{supportedMediaTypes: supportedMediaTypes} +} + +type basicNegotiatedSerializer struct { + supportedMediaTypes []runtime.SerializerInfo +} + +func (s basicNegotiatedSerializer) SupportedMediaTypes() []runtime.SerializerInfo { + return s.supportedMediaTypes } func (s basicNegotiatedSerializer) EncoderForVersion(encoder runtime.Encoder, gv runtime.GroupVersioner) runtime.Encoder { diff --git a/staging/src/k8s.io/client-go/dynamic/simple.go b/staging/src/k8s.io/client-go/dynamic/simple.go index 51d96e692fb..b476714053e 100644 --- a/staging/src/k8s.io/client-go/dynamic/simple.go +++ b/staging/src/k8s.io/client-go/dynamic/simple.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/features" "k8s.io/client-go/rest" "k8s.io/client-go/util/consistencydetector" "k8s.io/client-go/util/watchlist" @@ -45,9 +46,17 @@ var _ Interface = &DynamicClient{} // appropriate dynamic client defaults set. func ConfigFor(inConfig *rest.Config) *rest.Config { config := rest.CopyConfig(inConfig) - config.AcceptContentTypes = "application/json" + config.ContentType = "application/json" - config.NegotiatedSerializer = basicNegotiatedSerializer{} // this gets used for discovery and error handling types + config.AcceptContentTypes = "application/json" + if features.TestOnlyFeatureGates.Enabled(features.TestOnlyClientAllowsCBOR) { + config.AcceptContentTypes = "application/json;q=0.9,application/cbor;q=1" + if features.TestOnlyFeatureGates.Enabled(features.TestOnlyClientPrefersCBOR) { + config.ContentType = "application/cbor" + } + } + + config.NegotiatedSerializer = newBasicNegotiatedSerializer() if config.UserAgent == "" { config.UserAgent = rest.DefaultKubernetesUserAgent() } diff --git a/staging/src/k8s.io/client-go/features/features.go b/staging/src/k8s.io/client-go/features/features.go index afb67f509eb..19056df147b 100644 --- a/staging/src/k8s.io/client-go/features/features.go +++ b/staging/src/k8s.io/client-go/features/features.go @@ -18,9 +18,11 @@ package features import ( "errors" + "fmt" + "sync" + "sync/atomic" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "sync/atomic" ) // NOTE: types Feature, FeatureSpec, prerelease (and its values) @@ -141,3 +143,43 @@ var ( // should use AddFeaturesToExistingFeatureGates followed by ReplaceFeatureGates. featureGates = &atomic.Value{} ) + +// TestOnlyFeatureGates is a distinct registry of pre-alpha client features that must not be +// included in runtime wiring to command-line flags or environment variables. It exists as a risk +// mitigation to allow only programmatic enablement of CBOR serialization for integration testing +// purposes. +// +// TODO: Once all required integration test coverage is complete, this will be deleted and the +// test-only feature gates will be replaced by normal feature gates. +var TestOnlyFeatureGates = &testOnlyFeatureGates{ + features: map[Feature]bool{ + TestOnlyClientAllowsCBOR: false, + TestOnlyClientPrefersCBOR: false, + }, +} + +type testOnlyFeatureGates struct { + lock sync.RWMutex + features map[Feature]bool +} + +func (t *testOnlyFeatureGates) Enabled(feature Feature) bool { + t.lock.RLock() + defer t.lock.RUnlock() + + enabled, ok := t.features[feature] + if !ok { + panic(fmt.Sprintf("test-only feature %q not recognized", feature)) + } + return enabled +} + +func (t *testOnlyFeatureGates) Set(feature Feature, enabled bool) error { + t.lock.Lock() + defer t.lock.Unlock() + if _, ok := t.features[feature]; !ok { + return fmt.Errorf("test-only feature %q not recognized", feature) + } + t.features[feature] = enabled + return nil +} diff --git a/staging/src/k8s.io/client-go/features/known_features.go b/staging/src/k8s.io/client-go/features/known_features.go index 0c972a46fd5..9a6a7364573 100644 --- a/staging/src/k8s.io/client-go/features/known_features.go +++ b/staging/src/k8s.io/client-go/features/known_features.go @@ -41,6 +41,27 @@ const ( // owner: @nilekhc // alpha: v1.30 InformerResourceVersion Feature = "InformerResourceVersion" + + // owner: @benluddy + // kep: https://kep.k8s.io/4222 + // + // If disabled, clients configured to accept "application/cbor" will instead accept + // "application/json" with the same relative preference, and clients configured to write + // "application/cbor" or "application/apply-patch+cbor" will instead write + // "application/json" or "application/apply-patch+yaml", respectively. + // + // This feature is currently PRE-ALPHA and MUST NOT be enabled outside of integration tests. + TestOnlyClientAllowsCBOR Feature = "TestOnlyClientAllowsCBOR" + + // owner: @benluddy + // kep: https://kep.k8s.io/4222 + // + // If enabled AND TestOnlyClientAllowsCBOR is also enabled, the default request content type + // (if not explicitly configured) and the dynamic client's request content type both become + // "application/cbor". + // + // This feature is currently PRE-ALPHA and MUST NOT be enabled outside of integration tests. + TestOnlyClientPrefersCBOR Feature = "TestOnlyClientPrefersCBOR" ) // defaultKubernetesFeatureGates consists of all known Kubernetes-specific feature keys. diff --git a/test/integration/client/dynamic_client_test.go b/test/integration/client/dynamic_client_test.go index 0db8c55e611..f4d59ac7ba2 100644 --- a/test/integration/client/dynamic_client_test.go +++ b/test/integration/client/dynamic_client_test.go @@ -20,11 +20,13 @@ import ( "context" "encoding/json" "fmt" + "net/http" "reflect" "testing" "time" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/fields" @@ -38,6 +40,7 @@ import ( "k8s.io/client-go/dynamic" clientset "k8s.io/client-go/kubernetes" clientscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" "k8s.io/kubernetes/test/integration/framework" ) @@ -55,12 +58,12 @@ func TestDynamicClient(t *testing.T) { resource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} // Create a Pod with the normal client - pod := &v1.Pod{ + pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test", }, - Spec: v1.PodSpec{ - Containers: []v1.Container{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ { Name: "test", Image: "test-image", @@ -134,16 +137,16 @@ func TestDynamicClientWatch(t *testing.T) { t.Fatalf("unexpected error creating dynamic client: %v", err) } - resource := v1.SchemeGroupVersion.WithResource("events") + resource := corev1.SchemeGroupVersion.WithResource("events") - mkEvent := func(i int) *v1.Event { + mkEvent := func(i int) *corev1.Event { name := fmt.Sprintf("event-%v", i) - return &v1.Event{ + return &corev1.Event{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", Name: name, }, - InvolvedObject: v1.ObjectReference{ + InvolvedObject: corev1.ObjectReference{ Namespace: "default", Name: name, }, @@ -276,24 +279,171 @@ func TestUnstructuredExtract(t *testing.T) { } -func unstructuredToPod(obj *unstructured.Unstructured) (*v1.Pod, error) { +func unstructuredToPod(obj *unstructured.Unstructured) (*corev1.Pod, error) { json, err := runtime.Encode(unstructured.UnstructuredJSONScheme, obj) if err != nil { return nil, err } - pod := new(v1.Pod) - err = runtime.DecodeInto(clientscheme.Codecs.LegacyCodec(v1.SchemeGroupVersion), json, pod) + pod := new(corev1.Pod) + err = runtime.DecodeInto(clientscheme.Codecs.LegacyCodec(corev1.SchemeGroupVersion), json, pod) pod.Kind = "" pod.APIVersion = "" return pod, err } -func unstructuredToEvent(obj *unstructured.Unstructured) (*v1.Event, error) { +func unstructuredToEvent(obj *unstructured.Unstructured) (*corev1.Event, error) { json, err := runtime.Encode(unstructured.UnstructuredJSONScheme, obj) if err != nil { return nil, err } - event := new(v1.Event) - err = runtime.DecodeInto(clientscheme.Codecs.LegacyCodec(v1.SchemeGroupVersion), json, event) + event := new(corev1.Event) + err = runtime.DecodeInto(clientscheme.Codecs.LegacyCodec(corev1.SchemeGroupVersion), json, event) return event, err } + +func TestDynamicClientCBOREnablement(t *testing.T) { + for _, tc := range []struct { + name string + serving bool + allowed bool + preferred bool + wantRequestContentType string + wantRequestAccept string + wantResponseContentType string + wantResponseStatus int + wantStatusError bool + }{ + { + name: "sends cbor accepts both gets cbor", + serving: true, + allowed: true, + preferred: true, + wantRequestContentType: "application/cbor", + wantRequestAccept: "application/json;q=0.9,application/cbor;q=1", + wantResponseContentType: "application/cbor", + wantResponseStatus: http.StatusCreated, + wantStatusError: false, + }, + { + name: "sends cbor accepts both gets 415", + serving: false, + allowed: true, + preferred: true, + wantRequestContentType: "application/cbor", + wantRequestAccept: "application/json;q=0.9,application/cbor;q=1", + wantResponseContentType: "application/json", + wantResponseStatus: http.StatusUnsupportedMediaType, + wantStatusError: true, + }, + { + name: "sends json accepts both gets cbor", + serving: true, + allowed: true, + preferred: false, + wantRequestContentType: "application/json", + wantRequestAccept: "application/json;q=0.9,application/cbor;q=1", + wantResponseContentType: "application/cbor", + wantResponseStatus: http.StatusCreated, + wantStatusError: false, + }, + { + name: "sends json accepts both gets json", + serving: false, + allowed: true, + preferred: false, + wantRequestContentType: "application/json", + wantRequestAccept: "application/json;q=0.9,application/cbor;q=1", + wantResponseContentType: "application/json", + wantResponseStatus: http.StatusCreated, + wantStatusError: false, + }, + { + name: "sends json accepts json gets json with serving enabled", + serving: true, + allowed: false, + preferred: false, + wantRequestContentType: "application/json", + wantRequestAccept: "application/json", + wantResponseContentType: "application/json", + wantResponseStatus: http.StatusCreated, + wantStatusError: false, + }, + { + name: "sends json accepts json gets json with serving disabled", + serving: false, + allowed: false, + preferred: false, + wantRequestContentType: "application/json", + wantRequestAccept: "application/json", + wantResponseContentType: "application/json", + wantResponseStatus: http.StatusCreated, + wantStatusError: false, + }, + { + name: "sends json without both gates enabled", + serving: true, + allowed: false, + preferred: true, + wantRequestContentType: "application/json", + wantRequestAccept: "application/json", + wantResponseContentType: "application/json", + wantResponseStatus: http.StatusCreated, + wantStatusError: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.serving { + framework.EnableCBORServingAndStorageForTest(t) + } + + server := kubeapiservertesting.StartTestServerOrDie(t, nil, framework.DefaultTestServerFlags(), framework.SharedEtcd()) + defer server.TearDownFn() + + framework.SetTestOnlyCBORClientFeatureGatesForTest(t, tc.allowed, tc.preferred) + + config := rest.CopyConfig(server.ClientConfig) + config.Wrap(func(rt http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(request *http.Request) (*http.Response, error) { + response, err := rt.RoundTrip(request) + if got := response.Request.Header.Get("Content-Type"); got != tc.wantRequestContentType { + t.Errorf("want request content type %q, got %q", tc.wantRequestContentType, got) + } + if got := response.Request.Header.Get("Accept"); got != tc.wantRequestAccept { + t.Errorf("want request accept %q, got %q", tc.wantRequestAccept, got) + } + if got := response.Header.Get("Content-Type"); got != tc.wantResponseContentType { + t.Errorf("want response content type %q, got %q", tc.wantResponseContentType, got) + } + if got := response.StatusCode; got != tc.wantResponseStatus { + t.Errorf("want response status %d, got %d", tc.wantResponseStatus, got) + } + return response, err + }) + }) + client, err := dynamic.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + + _, err = client.Resource(corev1.SchemeGroupVersion.WithResource("namespaces")).Create( + context.TODO(), + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-dynamic-client-cbor-enablement", + }, + }, + }, + metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, + ) + switch { + case tc.wantStatusError && errors.IsUnsupportedMediaType(err): + // ok + case !tc.wantStatusError && err == nil: + // ok + default: + t.Errorf("unexpected error: %v", err) + } + }) + } +} diff --git a/test/integration/framework/cbor.go b/test/integration/framework/cbor.go index 03025531bf1..3ad7e498a99 100644 --- a/test/integration/framework/cbor.go +++ b/test/integration/framework/cbor.go @@ -26,11 +26,45 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer/cbor" "k8s.io/apiserver/pkg/features" utilfeature "k8s.io/apiserver/pkg/util/feature" + clientfeatures "k8s.io/client-go/features" featuregatetesting "k8s.io/component-base/featuregate/testing" aggregatorscheme "k8s.io/kube-aggregator/pkg/apiserver/scheme" "k8s.io/kubernetes/pkg/api/legacyscheme" ) +// SetTestOnlyCBORClientFeatureGatesForTest overrides the CBOR client feature gates in the test-only +// client feature gate instance for the duration of a test. The CBOR client feature gates are +// temporarily registered in their own feature gate instance that does not include runtime wiring to +// command-line flags or environment variables in order to mitigate the risk of enabling a new +// encoding before all integration tests have been demonstrated to pass. +// +// This will be removed as an alpha requirement. The client feature gates will be registered with +// the existing feature gate instance and tests will use +// k8s.io/client-go/features/testing.SetFeatureDuringTest (which unlike +// k8s.io/component-base/featuregate/testing.SetFeatureGateDuringTest does not accept a feature gate +// instance as a parameter). +func SetTestOnlyCBORClientFeatureGatesForTest(tb testing.TB, allowed, preferred bool) { + originalAllowed := clientfeatures.TestOnlyFeatureGates.Enabled(clientfeatures.TestOnlyClientAllowsCBOR) + tb.Cleanup(func() { + if err := clientfeatures.TestOnlyFeatureGates.Set(clientfeatures.TestOnlyClientAllowsCBOR, originalAllowed); err != nil { + tb.Fatal(err) + } + }) + if err := clientfeatures.TestOnlyFeatureGates.Set(clientfeatures.TestOnlyClientAllowsCBOR, allowed); err != nil { + tb.Fatal(err) + } + + originalPreferred := clientfeatures.TestOnlyFeatureGates.Enabled(clientfeatures.TestOnlyClientPrefersCBOR) + tb.Cleanup(func() { + if err := clientfeatures.TestOnlyFeatureGates.Set(clientfeatures.TestOnlyClientPrefersCBOR, originalPreferred); err != nil { + tb.Fatal(err) + } + }) + if err := clientfeatures.TestOnlyFeatureGates.Set(clientfeatures.TestOnlyClientPrefersCBOR, preferred); err != nil { + tb.Fatal(err) + } +} + // EnableCBORForTest patches global state to enable the CBOR serializer and reverses those changes // at the end of the test. As a risk mitigation, integration tests are initially written this way so // that integration tests can be implemented fully and incrementally before exposing options From 3e1b6aaf41f2e05146e313d47ec9c288bc554a56 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Thu, 17 Oct 2024 08:53:45 -0400 Subject: [PATCH 6/7] Export meta internal version scheme for testing. Codecs is already exported, but in order for tests to construct an alternate CodecFactory for meta's internal version types, they either need to be able to reference the scheme or to construct a parallel scheme, and a parallel scheme construction risks going out of sync with the way the package-scoped scheme object is initialized. --- .../pkg/apis/meta/internalversion/scheme/register.go | 8 ++++---- .../apis/meta/internalversion/scheme/register_test.go | 10 +++++----- .../apis/meta/internalversion/scheme/roundtrip_test.go | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go index 472a9aeb236..585d7f44bd1 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go @@ -24,16 +24,16 @@ import ( ) // Scheme is the registry for any type that adheres to the meta API spec. -var scheme = runtime.NewScheme() +var Scheme = runtime.NewScheme() // Codecs provides access to encoding and decoding for the scheme. -var Codecs = serializer.NewCodecFactory(scheme) +var Codecs = serializer.NewCodecFactory(Scheme) // ParameterCodec handles versioning of objects that are converted to query parameters. -var ParameterCodec = runtime.NewParameterCodec(scheme) +var ParameterCodec = runtime.NewParameterCodec(Scheme) // Unlike other API groups, meta internal knows about all meta external versions, but keeps // the logic for conversion private. func init() { - utilruntime.Must(internalversion.AddToScheme(scheme)) + utilruntime.Must(internalversion.AddToScheme(Scheme)) } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register_test.go index da6982c90bf..e17f28a7a0e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register_test.go @@ -37,11 +37,11 @@ func TestListOptions(t *testing.T) { Watch: true, } out := &metainternalversion.ListOptions{} - if err := scheme.Convert(in, out, nil); err != nil { + if err := Scheme.Convert(in, out, nil); err != nil { t.Fatal(err) } actual := &metav1.ListOptions{} - if err := scheme.Convert(out, actual, nil); err != nil { + if err := Scheme.Convert(out, actual, nil); err != nil { t.Fatal(err) } if !reflect.DeepEqual(in, actual) { @@ -54,16 +54,16 @@ func TestListOptions(t *testing.T) { {FieldSelector: "a!!!"}, } { out = &metainternalversion.ListOptions{} - if err := scheme.Convert(failingObject, out, nil); err == nil { + if err := Scheme.Convert(failingObject, out, nil); err == nil { t.Errorf("%d: unexpected conversion: %#v", i, out) } } // verify kind registration - if gvks, unversioned, err := scheme.ObjectKinds(in); err != nil || unversioned || gvks[0] != metav1.SchemeGroupVersion.WithKind("ListOptions") { + if gvks, unversioned, err := Scheme.ObjectKinds(in); err != nil || unversioned || gvks[0] != metav1.SchemeGroupVersion.WithKind("ListOptions") { t.Errorf("unexpected: %v %v %v", gvks[0], unversioned, err) } - if gvks, unversioned, err := scheme.ObjectKinds(out); err != nil || unversioned || gvks[0] != metainternalversion.SchemeGroupVersion.WithKind("ListOptions") { + if gvks, unversioned, err := Scheme.ObjectKinds(out); err != nil || unversioned || gvks[0] != metainternalversion.SchemeGroupVersion.WithKind("ListOptions") { t.Errorf("unexpected: %v %v %v", gvks[0], unversioned, err) } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/roundtrip_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/roundtrip_test.go index ee048e5a554..028942c6a8f 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/roundtrip_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/roundtrip_test.go @@ -24,5 +24,5 @@ import ( ) func TestRoundTrip(t *testing.T) { - roundtrip.RoundTripTestForScheme(t, scheme, fuzzer.Funcs) + roundtrip.RoundTripTestForScheme(t, Scheme, fuzzer.Funcs) } From 77401d7073b275ea50b4f6c156298395a567b2ef Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Thu, 24 Oct 2024 10:27:03 -0400 Subject: [PATCH 7/7] Add CBOR variant of admission webhook integration test. The existing admission webhook integration test provides good coverage of serving built-in resources and custom resources, including subresources. Serialization concerns, including roundtrippability, of built-in types have existing test coverage; the CBOR variant of the admission webhook integration test additionally exercises client and server codec wiring. --- .../admissionwebhook/admission_test.go | 28 +++++--- test/integration/framework/cbor.go | 69 +++++++++++++++++++ 2 files changed, 89 insertions(+), 8 deletions(-) diff --git a/test/integration/apiserver/admissionwebhook/admission_test.go b/test/integration/apiserver/admissionwebhook/admission_test.go index 3ab0e8f5916..a29fc0b5e1f 100644 --- a/test/integration/apiserver/admissionwebhook/admission_test.go +++ b/test/integration/apiserver/admissionwebhook/admission_test.go @@ -20,7 +20,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "encoding/json" "fmt" "io" "net/http" @@ -49,6 +48,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" @@ -446,16 +446,25 @@ func (w *warningHandler) HandleWarningHeader(code int, agent string, message str // TestWebhookAdmissionWithWatchCache tests communication between API server and webhook process. func TestWebhookAdmissionWithWatchCache(t *testing.T) { - testWebhookAdmission(t, true) + testWebhookAdmission(t, true, func(testing.TB, *rest.Config) {}) } // TestWebhookAdmissionWithoutWatchCache tests communication between API server and webhook process. func TestWebhookAdmissionWithoutWatchCache(t *testing.T) { - testWebhookAdmission(t, false) + testWebhookAdmission(t, false, func(testing.TB, *rest.Config) {}) +} + +func TestWebhookAdmissionWithCBOR(t *testing.T) { + framework.EnableCBORServingAndStorageForTest(t) + framework.SetTestOnlyCBORClientFeatureGatesForTest(t, true, true) + testWebhookAdmission(t, false, func(t testing.TB, config *rest.Config) { + config.Wrap(framework.AssertRequestResponseAsCBOR(t)) + }) } // testWebhookAdmission tests communication between API server and webhook process. -func testWebhookAdmission(t *testing.T, watchCache bool) { +func testWebhookAdmission(t *testing.T, watchCache bool, reconfigureClient func(testing.TB, *rest.Config)) { + // holder communicates expectations to webhooks, and results from webhooks holder := &holder{ t: t, @@ -528,10 +537,6 @@ func testWebhookAdmission(t *testing.T, watchCache bool) { } // gather resources to test - dynamicClient, err := dynamic.NewForConfig(clientConfig) - if err != nil { - t.Fatal(err) - } _, resources, err := client.Discovery().ServerGroupsAndResources() if err != nil { t.Fatalf("Failed to get ServerGroupsAndResources with error: %+v", err) @@ -640,6 +645,13 @@ func testWebhookAdmission(t *testing.T, watchCache bool) { for _, verb := range []string{"create", "update", "patch", "connect", "delete", "deletecollection"} { if shouldTestResourceVerb(gvr, resource, verb) { t.Run(verb, func(t *testing.T) { + clientConfig := rest.CopyConfig(clientConfig) + reconfigureClient(t, clientConfig) + dynamicClient, err := dynamic.NewForConfig(clientConfig) + if err != nil { + t.Fatal(err) + } + count++ holder.reset(t) testFunc := getTestFunc(gvr, verb) diff --git a/test/integration/framework/cbor.go b/test/integration/framework/cbor.go index 3ad7e498a99..8bbb003bc59 100644 --- a/test/integration/framework/cbor.go +++ b/test/integration/framework/cbor.go @@ -17,6 +17,9 @@ limitations under the License. package framework import ( + "bytes" + "io" + "net/http" "testing" apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver" @@ -24,9 +27,11 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/runtime/serializer/cbor" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/features" utilfeature "k8s.io/apiserver/pkg/util/feature" clientfeatures "k8s.io/client-go/features" + "k8s.io/client-go/transport" featuregatetesting "k8s.io/component-base/featuregate/testing" aggregatorscheme "k8s.io/kube-aggregator/pkg/apiserver/scheme" "k8s.io/kubernetes/pkg/api/legacyscheme" @@ -102,3 +107,67 @@ func EnableCBORServingAndStorageForTest(tb testing.TB) { *codecs[scheme] = serializer.NewCodecFactory(scheme, serializer.WithSerializer(newCBORSerializerInfo)) } } + +// AssertRequestResponseAsCBOR returns a transport.WrapperFunc that will report a test error if a +// non-empty request or response body contains data that does not appear to be CBOR-encoded. +func AssertRequestResponseAsCBOR(t testing.TB) transport.WrapperFunc { + recognizer := cbor.NewSerializer(runtime.NewScheme(), runtime.NewScheme()) + + unsupportedPatchContentTypes := sets.New( + "application/json-patch+json", + "application/merge-patch+json", + "application/strategic-merge-patch+json", + ) + + return func(rt http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(request *http.Request) (*http.Response, error) { + if request.Body != nil && !unsupportedPatchContentTypes.Has(request.Header.Get("Content-Type")) { + requestbody, err := io.ReadAll(request.Body) + if err != nil { + t.Error(err) + } + recognized, _, err := recognizer.RecognizesData(requestbody) + if err != nil { + t.Error(err) + } + if len(requestbody) > 0 && !recognized { + t.Errorf("non-cbor request: 0x%x", requestbody) + } + request.Body = io.NopCloser(bytes.NewReader(requestbody)) + } + + response, rterr := rt.RoundTrip(request) + if rterr != nil { + return response, rterr + } + + // We can't synchronously inspect streaming responses, so tee to a buffer + // and inspect it at the end of the test. + var buf bytes.Buffer + response.Body = struct { + io.Reader + io.Closer + }{ + Reader: io.TeeReader(response.Body, &buf), + Closer: response.Body, + } + t.Cleanup(func() { + recognized, _, err := recognizer.RecognizesData(buf.Bytes()) + if err != nil { + t.Error(err) + } + if buf.Len() > 0 && !recognized { + t.Errorf("non-cbor response: 0x%x", buf.Bytes()) + } + }) + + return response, rterr + }) + } +} + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +}