From a77f4c7ba2e761461daaf115a38903fc91916dd6 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Thu, 7 Nov 2024 00:05:03 -0500 Subject: [PATCH 1/2] Fix content type fallback when a client defaults to CBOR. With the ClientsAllowCBOR client-go feature gate enabled, a 415 response to a CBOR-encoded REST causes all subsequent requests from the client to fall back to a JSON request encoding. This mechanism had only worked as intended when CBOR was explicitly configured in the ClientContentConfig. When both ClientsAllowCBOR and ClientsPreferCBOR are enabled, an unconfigured (empty) content type defaults to CBOR instead of JSON. Both ways of configuring a client to use the CBOR request encoding are now subject to the same fallback mechanism. --- staging/src/k8s.io/client-go/rest/client.go | 33 +++++--- staging/src/k8s.io/client-go/rest/request.go | 14 ++-- test/integration/client/client_test.go | 84 ++++++++++++-------- 3 files changed, 77 insertions(+), 54 deletions(-) diff --git a/staging/src/k8s.io/client-go/rest/client.go b/staging/src/k8s.io/client-go/rest/client.go index 393a4166913..159caa13fab 100644 --- a/staging/src/k8s.io/client-go/rest/client.go +++ b/staging/src/k8s.io/client-go/rest/client.go @@ -238,7 +238,8 @@ func (c *RESTClient) Delete() *Request { // APIVersion returns the APIVersion this RESTClient is expected to use. func (c *RESTClient) APIVersion() schema.GroupVersion { - return c.content.GetClientContentConfig().GroupVersion + config, _ := c.content.GetClientContentConfig() + return config.GroupVersion } // requestClientContentConfigProvider observes HTTP 415 (Unsupported Media Type) responses to detect @@ -257,25 +258,35 @@ type requestClientContentConfigProvider struct { } // GetClientContentConfig returns the ClientContentConfig that should be used for new requests by -// this client. -func (p *requestClientContentConfigProvider) GetClientContentConfig() ClientContentConfig { +// this client and true if the request ContentType was selected by default. +func (p *requestClientContentConfigProvider) GetClientContentConfig() (ClientContentConfig, bool) { + config := p.base + + defaulted := config.ContentType == "" + if defaulted { + config.ContentType = "application/json" + } + if !clientfeatures.FeatureGates().Enabled(clientfeatures.ClientsAllowCBOR) { - return p.base + return config, defaulted + } + + if defaulted && clientfeatures.FeatureGates().Enabled(clientfeatures.ClientsPreferCBOR) { + config.ContentType = "application/cbor" } if sawUnsupportedMediaTypeForCBOR := p.sawUnsupportedMediaTypeForCBOR.Load(); !sawUnsupportedMediaTypeForCBOR { - return p.base + return config, defaulted } - if mediaType, _, _ := mime.ParseMediaType(p.base.ContentType); mediaType != runtime.ContentTypeCBOR { - return p.base + if mediaType, _, _ := mime.ParseMediaType(config.ContentType); mediaType != runtime.ContentTypeCBOR { + return config, defaulted } - config := p.base - // The default ClientContentConfig sets ContentType to CBOR and the client has previously - // received an HTTP 415 in response to a CBOR request. Override ContentType to JSON. + // The effective ContentType is CBOR and the client has previously received an HTTP 415 in + // response to a CBOR request. Override ContentType to JSON. config.ContentType = runtime.ContentTypeJSON - return config + return config, defaulted } // UnsupportedMediaType reports that the server has responded to a request with HTTP 415 Unsupported diff --git a/staging/src/k8s.io/client-go/rest/request.go b/staging/src/k8s.io/client-go/rest/request.go index 65942aa1233..0ec90ad188b 100644 --- a/staging/src/k8s.io/client-go/rest/request.go +++ b/staging/src/k8s.io/client-go/rest/request.go @@ -156,14 +156,10 @@ func NewRequest(c *RESTClient) *Request { timeout = c.Client.Timeout } - contentConfig := c.content.GetClientContentConfig() - contentTypeNotSet := len(contentConfig.ContentType) == 0 - if contentTypeNotSet { - contentConfig.ContentType = "application/json" - if clientfeatures.FeatureGates().Enabled(clientfeatures.ClientsAllowCBOR) && clientfeatures.FeatureGates().Enabled(clientfeatures.ClientsPreferCBOR) { - contentConfig.ContentType = "application/cbor" - } - } + // A request needs to know whether the content type was explicitly configured or selected by + // default in order to support the per-request Protobuf override used by clients generated + // with --prefers-protobuf. + contentConfig, contentTypeDefaulted := c.content.GetClientContentConfig() r := &Request{ c: c, @@ -176,7 +172,7 @@ func NewRequest(c *RESTClient) *Request { warningHandler: c.warningHandler, contentConfig: contentConfig, - contentTypeNotSet: contentTypeNotSet, + contentTypeNotSet: contentTypeDefaulted, } r.setAcceptHeader() diff --git a/test/integration/client/client_test.go b/test/integration/client/client_test.go index 54cad7d10b3..7f51c941798 100644 --- a/test/integration/client/client_test.go +++ b/test/integration/client/client_test.go @@ -2037,44 +2037,60 @@ func TestUnsupportedMediaTypeCircuitBreaker(t *testing.T) { server := kubeapiservertesting.StartTestServerOrDie(t, nil, framework.DefaultTestServerFlags(), framework.SharedEtcd()) t.Cleanup(server.TearDownFn) - config := rest.CopyConfig(server.ClientConfig) - config.ContentType = "application/cbor" - config.AcceptContentTypes = "application/json" + for _, tc := range []struct { + name string + contentType string + }{ + { + name: "default content type", + contentType: "", + }, + { + name: "explicit content type", + contentType: "application/cbor", + }, + } { + t.Run(tc.name, func(t *testing.T) { + config := rest.CopyConfig(server.ClientConfig) + config.ContentType = tc.contentType + config.AcceptContentTypes = "application/json" - client, err := corev1client.NewForConfig(config) - if err != nil { - t.Fatal(err) - } + client, err := corev1client.NewForConfig(config) + if err != nil { + t.Fatal(err) + } - if _, err := client.Namespaces().Create( - context.TODO(), - &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-client-415"}}, - metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, - ); !apierrors.IsUnsupportedMediaType(err) { - t.Errorf("expected to receive unsupported media type on first cbor request, got: %v", err) - } + if _, err := client.Namespaces().Create( + context.TODO(), + &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-client-415"}}, + metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, + ); !apierrors.IsUnsupportedMediaType(err) { + t.Errorf("expected to receive unsupported media type on first cbor request, got: %v", err) + } - // Requests from this client should fall back from application/cbor to application/json. - if _, err := client.Namespaces().Create( - context.TODO(), - &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-client-415"}}, - metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, - ); err != nil { - t.Errorf("expected to receive nil error on subsequent cbor request, got: %v", err) - } + // Requests from this client should fall back from application/cbor to application/json. + if _, err := client.Namespaces().Create( + context.TODO(), + &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-client-415"}}, + metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, + ); err != nil { + t.Errorf("expected to receive nil error on subsequent cbor request, got: %v", err) + } - // The circuit breaker trips on a per-client basis, so it should not begin tripped for a - // fresh client with identical config. - client, err = corev1client.NewForConfig(config) - if err != nil { - t.Fatal(err) - } + // The circuit breaker trips on a per-client basis, so it should not begin tripped for a + // fresh client with identical config. + client, err = corev1client.NewForConfig(config) + if err != nil { + t.Fatal(err) + } - if _, err := client.Namespaces().Create( - context.TODO(), - &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-client-415"}}, - metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, - ); !apierrors.IsUnsupportedMediaType(err) { - t.Errorf("expected to receive unsupported media type on cbor request with fresh client, got: %v", err) + if _, err := client.Namespaces().Create( + context.TODO(), + &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-client-415"}}, + metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}, + ); !apierrors.IsUnsupportedMediaType(err) { + t.Errorf("expected to receive unsupported media type on cbor request with fresh client, got: %v", err) + } + }) } } From 42d3e9752c605f7ea99b21425f2f32fe44a6ade8 Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Thu, 7 Nov 2024 00:06:24 -0500 Subject: [PATCH 2/2] Add E2E test for CBOR client compatibility with older apiservers. Clients must be able to use CBOR without a guarantee that all apiservers support it. The apiserver aggregation layer avoids changing in any way that would require an aggregated apiservers to be updated. This end-to-end test verifies that a client's content negotiation behaviors continue to work over time when communicating with a 1.17 sample-apiserver. --- test/e2e/apimachinery/protocol.go | 77 ++++++++++++++++++++++++++++++- test/e2e/feature/feature.go | 4 ++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/test/e2e/apimachinery/protocol.go b/test/e2e/apimachinery/protocol.go index b40f52275a0..491f5c15b97 100644 --- a/test/e2e/apimachinery/protocol.go +++ b/test/e2e/apimachinery/protocol.go @@ -26,12 +26,21 @@ import ( v1 "k8s.io/api/core/v1" apierrors "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" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/dynamic" + clientfeatures "k8s.io/client-go/features" + clientfeaturestesting "k8s.io/client-go/features/testing" "k8s.io/client-go/kubernetes" - admissionapi "k8s.io/pod-security-admission/api" - + aggregatorclientset "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset" + "k8s.io/kubernetes/test/e2e/feature" "k8s.io/kubernetes/test/e2e/framework" + imageutils "k8s.io/kubernetes/test/utils/image" + admissionapi "k8s.io/pod-security-admission/api" + samplev1alpha1 "k8s.io/sample-apiserver/pkg/apis/wardle/v1alpha1" + samplev1alpha1client "k8s.io/sample-apiserver/pkg/generated/clientset/versioned/typed/wardle/v1alpha1" ) var _ = SIGDescribe("client-go should negotiate", func() { @@ -96,3 +105,67 @@ var _ = SIGDescribe("client-go should negotiate", func() { }) } }) + +var _ = SIGDescribe("CBOR", feature.CBOR, func() { + f := framework.NewDefaultFramework("cbor") + f.NamespacePodSecurityLevel = admissionapi.LevelBaseline + + // Must be serial to avoid conflict with other tests that set up a sample apiserver. + f.It("clients remain compatible with the 1.17 sample-apiserver", f.WithSerial(), func(ctx context.Context) { + clientfeaturestesting.SetFeatureDuringTest(g.GinkgoTB(), clientfeatures.ClientsAllowCBOR, true) + clientfeaturestesting.SetFeatureDuringTest(g.GinkgoTB(), clientfeatures.ClientsPreferCBOR, true) + + clientConfig, err := framework.LoadConfig() + framework.ExpectNoError(err) + + aggregatorClient, err := aggregatorclientset.NewForConfig(clientConfig) + framework.ExpectNoError(err) + + dynamicClient, err := dynamic.NewForConfig(clientConfig) + framework.ExpectNoError(err) + + objectNames := generateSampleAPIServerObjectNames(f.Namespace.Name) + g.DeferCleanup(func(ctx context.Context) { + cleanupSampleAPIServer(ctx, f.ClientSet, aggregatorClient, objectNames, samplev1alpha1.SchemeGroupVersion.Version+"."+samplev1alpha1.SchemeGroupVersion.Group) + }) + SetUpSampleAPIServer(ctx, f, aggregatorClient, imageutils.GetE2EImage(imageutils.APIServer), objectNames, samplev1alpha1.SchemeGroupVersion.Group, samplev1alpha1.SchemeGroupVersion.Version) + + flunder := samplev1alpha1.Flunder{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-flunder", + }, + } + + g.By("making requests with a generated client", func() { + sampleClient, err := samplev1alpha1client.NewForConfig(clientConfig) + framework.ExpectNoError(err) + + _, err = sampleClient.Flunders(f.Namespace.Name).List(ctx, metav1.ListOptions{LabelSelector: "a,!a"}) + framework.ExpectNoError(err, "Failed to list with generated client") + + _, err = sampleClient.Flunders(f.Namespace.Name).Create(ctx, &flunder, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + o.Expect(err).To(o.MatchError(apierrors.IsUnsupportedMediaType, "Expected 415 (Unsupported Media Type) response on first write with generated client")) + + _, err = sampleClient.Flunders(f.Namespace.Name).Create(ctx, &flunder, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + framework.ExpectNoError(err, "Expected subsequent writes to succeed with generated client") + }) + + g.By("making requests with a dynamic client", func() { + unstructuredFlunderContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&flunder) + framework.ExpectNoError(err) + unstructuredFlunder := &unstructured.Unstructured{Object: unstructuredFlunderContent} + + flunderDynamicClient := dynamicClient.Resource(samplev1alpha1.SchemeGroupVersion.WithResource("flunders")).Namespace(f.Namespace.Name) + + list, err := flunderDynamicClient.List(ctx, metav1.ListOptions{LabelSelector: "a,!a"}) + framework.ExpectNoError(err, "Failed to list with dynamic client") + o.Expect(list.GetObjectKind().GroupVersionKind()).To(o.Equal(samplev1alpha1.SchemeGroupVersion.WithKind("FlunderList"))) + + _, err = flunderDynamicClient.Create(ctx, unstructuredFlunder, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + o.Expect(err).To(o.MatchError(apierrors.IsUnsupportedMediaType, "Expected 415 (Unsupported Media Type) response on first write with dynamic client")) + + _, err = flunderDynamicClient.Create(ctx, unstructuredFlunder, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + framework.ExpectNoError(err, "Expected subsequent writes to succeed with dynamic client") + }) + }) +}) diff --git a/test/e2e/feature/feature.go b/test/e2e/feature/feature.go index 557d0930cee..d9cf9d47f46 100644 --- a/test/e2e/feature/feature.go +++ b/test/e2e/feature/feature.go @@ -34,6 +34,10 @@ var ( // TODO: document the feature (owning SIG, when to use this feature for a test) BoundServiceAccountTokenVolume = framework.WithFeature(framework.ValidFeatures.Add("BoundServiceAccountTokenVolume")) + // Owner: sig-api-machinery + // Marks tests that exercise the CBOR data format for serving or storage. + CBOR = framework.WithFeature(framework.ValidFeatures.Add("CBOR")) + // TODO: document the feature (owning SIG, when to use this feature for a test) CloudProvider = framework.WithFeature(framework.ValidFeatures.Add("CloudProvider"))