From 203cf2be6f728378ae196dee749a3bcf6db76da7 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 12 Jul 2016 23:44:55 -0400 Subject: [PATCH] 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. --- pkg/client/restclient/config.go | 3 + pkg/client/restclient/request.go | 64 +++++++---- pkg/client/restclient/request_test.go | 159 ++++++++++++++++++++++++++ pkg/client/typed/dynamic/client.go | 1 + 4 files changed, 204 insertions(+), 23 deletions(-) diff --git a/pkg/client/restclient/config.go b/pkg/client/restclient/config.go index ee956dcd649..aa9cc520268 100644 --- a/pkg/client/restclient/config.go +++ b/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, diff --git a/pkg/client/restclient/request.go b/pkg/client/restclient/request.go index 289c82d8ef0..dd1bbb35a53 100644 --- a/pkg/client/restclient/request.go +++ b/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, diff --git a/pkg/client/restclient/request_test.go b/pkg/client/restclient/request_test.go index 5e9730d9384..1288bb50a07 100755 --- a/pkg/client/restclient/request_test.go +++ b/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 diff --git a/pkg/client/typed/dynamic/client.go b/pkg/client/typed/dynamic/client.go index 3a6aae71c47..5b8e1481ebc 100644 --- a/pkg/client/typed/dynamic/client.go +++ b/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)