Merge pull request #28933 from smarterclayton/accept_content_types

Automatic merge from submit-queue

Use response content-type on restclient errors

Also allow a new AcceptContentTypes field to allow the client to ask for
a fallback serialization when getting responses from the server. This
allows a new client to ask for protobuf and JSON, falling back to JSON
when necessary.

The changes to request.go allow error responses from non-JSON servers to
be properly decoded.

@wojtek-t - also alters #28910 slightly (this is better output)

pkg/client/restclient/config.go

@@ -131,6 +131,9 @@ type TLSClientConfig struct {
 }
 
 type ContentConfig struct {
+	// AcceptContentTypes specifies the types the client will accept and is optional.
+	// If not set, ContentType will be used to define the Accept header
+	AcceptContentTypes string
 	// ContentType specifies the wire format used to communicate with the server.
 	// This value will be set as the Accept header on requests made to the server, and
 	// as the default content type on any object sent to the server. If not set,

pkg/client/restclient/request.go

@@ -18,6 +18,7 @@ package restclient
 
 import (
 	"bytes"
+	"encoding/hex"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -143,7 +144,10 @@ func NewRequest(client HTTPClient, verb string, baseURL *url.URL, versionedAPIPa
 		backoffMgr:  backoff,
 		throttle:    throttle,
 	}
-	if len(content.ContentType) > 0 {
+	switch {
+	case len(content.AcceptContentTypes) > 0:
+		r.SetHeader("Accept", content.AcceptContentTypes)
+	case len(content.ContentType) > 0:
 		r.SetHeader("Accept", content.ContentType+", */*")
 	}
 	return r
@@ -888,16 +892,49 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu
 			body = data
 		}
 	}
-	glog.V(8).Infof("Response Body: %#v", string(body))
+
+	if glog.V(8) {
+		switch {
+		case bytes.IndexFunc(body, func(r rune) bool { return r < 0x0a }) != -1:
+			glog.Infof("Response Body:\n%s", hex.Dump(body))
+		default:
+			glog.Infof("Response Body: %s", string(body))
+		}
+	}
+
+	// verify the content type is accurate
+	contentType := resp.Header.Get("Content-Type")
+	decoder := r.serializers.Decoder
+	if len(contentType) > 0 && (decoder == nil || (len(r.content.ContentType) > 0 && contentType != r.content.ContentType)) {
+		mediaType, params, err := mime.ParseMediaType(contentType)
+		if err != nil {
+			return Result{err: errors.NewInternalError(err)}
+		}
+		decoder, err = r.serializers.RenegotiatedDecoder(mediaType, params)
+		if err != nil {
+			// if we fail to negotiate a decoder, treat this as an unstructured error
+			switch {
+			case resp.StatusCode == http.StatusSwitchingProtocols:
+				// no-op, we've been upgraded
+			case resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusPartialContent:
+				return Result{err: r.transformUnstructuredResponseError(resp, req, body)}
+			}
+			return Result{
+				body:        body,
+				contentType: contentType,
+				statusCode:  resp.StatusCode,
+			}
+		}
+	}
 
 	// Did the server give us a status response?
 	isStatusResponse := false
+	status := &unversioned.Status{}
 	// Because release-1.1 server returns Status with empty APIVersion at paths
 	// to the Extensions resources, we need to use DecodeInto here to provide
 	// default groupVersion, otherwise a status response won't be correctly
 	// decoded.
-	status := &unversioned.Status{}
-	err := runtime.DecodeInto(r.serializers.Decoder, body, status)
+	err := runtime.DecodeInto(decoder, body, status)
 	if err == nil && len(status.Status) > 0 {
 		isStatusResponse = true
 	}
@@ -919,25 +956,6 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu
 		return Result{err: errors.FromObject(status)}
 	}
 
-	contentType := resp.Header.Get("Content-Type")
-	var decoder runtime.Decoder
-	if contentType == r.content.ContentType {
-		decoder = r.serializers.Decoder
-	} else {
-		mediaType, params, err := mime.ParseMediaType(contentType)
-		if err != nil {
-			return Result{err: errors.NewInternalError(err)}
-		}
-		decoder, err = r.serializers.RenegotiatedDecoder(mediaType, params)
-		if err != nil {
-			return Result{
-				body:        body,
-				contentType: contentType,
-				statusCode:  resp.StatusCode,
-			}
-		}
-	}
-
 	return Result{
 		body:        body,
 		contentType: contentType,

pkg/client/restclient/request_test.go

@@ -19,6 +19,7 @@ package restclient
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -283,6 +284,9 @@ func defaultSerializers() Serializers {
 		Decoder:             testapi.Default.Codec(),
 		StreamingSerializer: testapi.Default.Codec(),
 		Framer:              runtime.DefaultFramer,
+		RenegotiatedDecoder: func(contentType string, params map[string]string) (runtime.Decoder, error) {
+			return testapi.Default.Codec(), nil
+		},
 	}
 }
 
@@ -413,6 +417,161 @@ func TestTransformResponse(t *testing.T) {
 	}
 }
 
+type renegotiator struct {
+	called      bool
+	contentType string
+	params      map[string]string
+	decoder     runtime.Decoder
+	err         error
+}
+
+func (r *renegotiator) invoke(contentType string, params map[string]string) (runtime.Decoder, error) {
+	r.called = true
+	r.contentType = contentType
+	r.params = params
+	return r.decoder, r.err
+}
+
+func TestTransformResponseNegotiate(t *testing.T) {
+	invalid := []byte("aaaaa")
+	uri, _ := url.Parse("http://localhost")
+	testCases := []struct {
+		Response *http.Response
+		Data     []byte
+		Created  bool
+		Error    bool
+		ErrFn    func(err error) bool
+
+		ContentType       string
+		Called            bool
+		ExpectContentType string
+		Decoder           runtime.Decoder
+		NegotiateErr      error
+	}{
+		{
+			ContentType: "application/json",
+			Response: &http.Response{
+				StatusCode: 401,
+				Header:     http.Header{"Content-Type": []string{"application/json"}},
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Error: true,
+			ErrFn: func(err error) bool {
+				return err.Error() != "aaaaa" && apierrors.IsUnauthorized(err)
+			},
+		},
+		{
+			ContentType: "application/json",
+			Response: &http.Response{
+				StatusCode: 401,
+				Header:     http.Header{"Content-Type": []string{"application/protobuf"}},
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Decoder: testapi.Default.Codec(),
+
+			Called:            true,
+			ExpectContentType: "application/protobuf",
+
+			Error: true,
+			ErrFn: func(err error) bool {
+				return err.Error() != "aaaaa" && apierrors.IsUnauthorized(err)
+			},
+		},
+		{
+			ContentType: "application/json",
+			Response: &http.Response{
+				StatusCode: 500,
+				Header:     http.Header{"Content-Type": []string{"application/,others"}},
+			},
+			Decoder: testapi.Default.Codec(),
+
+			Error: true,
+			ErrFn: func(err error) bool {
+				return err.Error() == "Internal error occurred: mime: expected token after slash" && err.(apierrors.APIStatus).Status().Code == 500
+			},
+		},
+		{
+			// no negotiation when no content type specified
+			Response: &http.Response{
+				StatusCode: 200,
+				Header:     http.Header{"Content-Type": []string{"text/any"}},
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Decoder: testapi.Default.Codec(),
+		},
+		{
+			// no negotiation when no response content type specified
+			ContentType: "text/any",
+			Response: &http.Response{
+				StatusCode: 200,
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Decoder: testapi.Default.Codec(),
+		},
+		{
+			// unrecognized content type is not handled
+			ContentType: "application/json",
+			Response: &http.Response{
+				StatusCode: 404,
+				Header:     http.Header{"Content-Type": []string{"application/unrecognized"}},
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Decoder: testapi.Default.Codec(),
+
+			NegotiateErr:      fmt.Errorf("aaaa"),
+			Called:            true,
+			ExpectContentType: "application/unrecognized",
+
+			Error: true,
+			ErrFn: func(err error) bool {
+				return err.Error() != "aaaaa" && apierrors.IsNotFound(err)
+			},
+		},
+	}
+	for i, test := range testCases {
+		serializers := defaultSerializers()
+		negotiator := &renegotiator{
+			decoder: test.Decoder,
+			err:     test.NegotiateErr,
+		}
+		serializers.RenegotiatedDecoder = negotiator.invoke
+		contentConfig := defaultContentConfig()
+		contentConfig.ContentType = test.ContentType
+		r := NewRequest(nil, "", uri, "", contentConfig, serializers, nil, nil)
+		if test.Response.Body == nil {
+			test.Response.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
+		}
+		result := r.transformResponse(test.Response, &http.Request{})
+		_, err := result.body, result.err
+		hasErr := err != nil
+		if hasErr != test.Error {
+			t.Errorf("%d: unexpected error: %t %v", i, test.Error, err)
+			continue
+		} else if hasErr && test.Response.StatusCode > 399 {
+			status, ok := err.(apierrors.APIStatus)
+			if !ok {
+				t.Errorf("%d: response should have been transformable into APIStatus: %v", i, err)
+				continue
+			}
+			if int(status.Status().Code) != test.Response.StatusCode {
+				t.Errorf("%d: status code did not match response: %#v", i, status.Status())
+			}
+		}
+		if test.ErrFn != nil && !test.ErrFn(err) {
+			t.Errorf("%d: error function did not match: %v", i, err)
+		}
+		if negotiator.called != test.Called {
+			t.Errorf("%d: negotiator called %t != %t", i, negotiator.called, test.Called)
+		}
+		if !test.Called {
+			continue
+		}
+		if negotiator.contentType != test.ExpectContentType {
+			t.Errorf("%d: unexpected content type: %s", i, negotiator.contentType)
+		}
+	}
+}
+
 func TestTransformUnstructuredError(t *testing.T) {
 	testCases := []struct {
 		Req *http.Request

pkg/client/typed/dynamic/client.go

@@ -58,6 +58,7 @@ func NewClient(conf *restclient.Config) (*Client, error) {
 
 	// TODO: it's questionable that this should be using anything other than unstructured schema and JSON
 	conf.ContentType = runtime.ContentTypeJSON
+	conf.AcceptContentTypes = runtime.ContentTypeJSON
 	streamingInfo, _ := api.Codecs.StreamingSerializerForMediaType("application/json;stream=watch", nil)
 	conf.NegotiatedSerializer = serializer.NegotiatedSerializerWrapper(runtime.SerializerInfo{Serializer: codec}, streamingInfo)