From ea13190d8bd3a4bb3e82055b529aa7599ae5c6e1 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Wed, 23 Oct 2024 16:36:37 -0400 Subject: [PATCH] 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