Merge pull request kubernetes#128641 from benluddy/e2e-cbor-client-compat

KEP-4222: Fix JSON fallback for clients using default content-type and add E2E client test.

Author: k8s-ci-robot

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

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()

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")
+		})
+	})
+})

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"))
 

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)
+			}
+		})
 	}
 }