diff --git a/pkg/client/restclient/client_test.go b/pkg/client/restclient/client_test.go index 009a2febcca..65f8492d28e 100644 --- a/pkg/client/restclient/client_test.go +++ b/pkg/client/restclient/client_test.go @@ -79,13 +79,52 @@ func TestDoRequestFailed(t *testing.T) { testServer := httptest.NewServer(&fakeHandler) defer testServer.Close() + c, err := restClient(testServer) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + err = c.Get().Do().Error() + if err == nil { + t.Errorf("unexpected non-error") + } + ss, ok := err.(errors.APIStatus) + if !ok { + t.Errorf("unexpected error type %v", err) + } + actual := ss.Status() + if !reflect.DeepEqual(status, &actual) { + t.Errorf("Unexpected mis-match: %s", diff.ObjectReflectDiff(status, &actual)) + } +} + +func TestDoRawRequestFailed(t *testing.T) { + status := &unversioned.Status{ + Code: http.StatusNotFound, + Status: unversioned.StatusFailure, + Reason: unversioned.StatusReasonNotFound, + Message: "the server could not find the requested resource", + Details: &unversioned.StatusDetails{ + Causes: []unversioned.StatusCause{ + {Type: unversioned.CauseTypeUnexpectedServerResponse, Message: "unknown"}, + }, + }, + } + expectedBody, _ := runtime.Encode(testapi.Default.Codec(), status) + fakeHandler := utiltesting.FakeHandler{ + StatusCode: 404, + ResponseBody: string(expectedBody), + T: t, + } + testServer := httptest.NewServer(&fakeHandler) + defer testServer.Close() + c, err := restClient(testServer) if err != nil { t.Fatalf("unexpected error: %v", err) } body, err := c.Get().Do().Raw() - if err == nil || body != nil { + if err == nil || body == nil { t.Errorf("unexpected non-error: %#v", body) } ss, ok := err.(errors.APIStatus) @@ -93,12 +132,8 @@ func TestDoRequestFailed(t *testing.T) { t.Errorf("unexpected error type %v", err) } actual := ss.Status() - expected := *status - // The decoder will apply the default Version and Kind to the Status. - expected.APIVersion = "v1" - expected.Kind = "Status" - if !reflect.DeepEqual(&expected, &actual) { - t.Errorf("Unexpected mis-match: %s", diff.ObjectDiff(status, &actual)) + if !reflect.DeepEqual(status, &actual) { + t.Errorf("Unexpected mis-match: %s", diff.ObjectReflectDiff(status, &actual)) } } diff --git a/pkg/client/restclient/request.go b/pkg/client/restclient/request.go index afddea093bf..0f17d0badc0 100644 --- a/pkg/client/restclient/request.go +++ b/pkg/client/restclient/request.go @@ -930,33 +930,21 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu } } - // 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. - err := runtime.DecodeInto(decoder, body, status) - if err == nil && len(status.Status) > 0 { - isStatusResponse = true - } - switch { case resp.StatusCode == http.StatusSwitchingProtocols: // no-op, we've been upgraded case resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusPartialContent: - if !isStatusResponse { - return Result{err: r.transformUnstructuredResponseError(resp, req, body)} + // calculate an unstructured error from the response which the Result object may use if the caller + // did not return a structured error. + retryAfter, _ := retryAfterSeconds(resp) + err := r.newUnstructuredResponseError(body, isTextResponse(resp), resp.StatusCode, req.Method, retryAfter) + return Result{ + body: body, + contentType: contentType, + statusCode: resp.StatusCode, + decoder: decoder, + err: err, } - return Result{err: errors.FromObject(status)} - } - - // If the server gave us a status back, look at what it was. - success := resp.StatusCode >= http.StatusOK && resp.StatusCode <= http.StatusPartialContent - if isStatusResponse && (status.Status != unversioned.StatusSuccess && !success) { - // "Failed" requests are clearly just an error and it makes sense to return them as such. - return Result{err: errors.FromObject(status)} } return Result{ @@ -967,6 +955,9 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu } } +// maxUnstructuredResponseTextBytes is an upper bound on how much output to include in the unstructured error. +const maxUnstructuredResponseTextBytes = 2048 + // transformUnstructuredResponseError handles an error from the server that is not in a structured form. // It is expected to transform any response that is not recognizable as a clear server sent error from the // K8S API using the information provided with the request. In practice, HTTP proxies and client libraries @@ -987,20 +978,29 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu // TODO: introduce transformation of generic http.Client.Do() errors that separates 4. func (r *Request) transformUnstructuredResponseError(resp *http.Response, req *http.Request, body []byte) error { if body == nil && resp.Body != nil { - if data, err := ioutil.ReadAll(resp.Body); err == nil { + if data, err := ioutil.ReadAll(&io.LimitedReader{R: resp.Body, N: maxUnstructuredResponseTextBytes}); err == nil { body = data } } + retryAfter, _ := retryAfterSeconds(resp) + return r.newUnstructuredResponseError(body, isTextResponse(resp), resp.StatusCode, req.Method, retryAfter) +} + +// newUnstructuredResponseError instantiates the appropriate generic error for the provided input. It also logs the body. +func (r *Request) newUnstructuredResponseError(body []byte, isTextResponse bool, statusCode int, method string, retryAfter int) error { + // cap the amount of output we create + if len(body) > maxUnstructuredResponseTextBytes { + body = body[:maxUnstructuredResponseTextBytes] + } glog.V(8).Infof("Response Body: %#v", string(body)) message := "unknown" - if isTextResponse(resp) { + if isTextResponse { message = strings.TrimSpace(string(body)) } - retryAfter, _ := retryAfterSeconds(resp) return errors.NewGenericServerResponse( - resp.StatusCode, - req.Method, + statusCode, + method, unversioned.GroupResource{ Group: r.content.GroupVersion.Group, Resource: r.resource, @@ -1064,15 +1064,31 @@ func (r Result) Raw() ([]byte, error) { return r.body, r.err } -// Get returns the result as an object. +// Get returns the result as an object, which means it passes through the decoder. +// If the returned object is of type Status and has .Status != StatusSuccess, the +// additional information in Status will be used to enrich the error. func (r Result) Get() (runtime.Object, error) { if r.err != nil { - return nil, r.err + // Check whether the result has a Status object in the body and prefer that. + return nil, r.Error() } if r.decoder == nil { return nil, fmt.Errorf("serializer for %s doesn't exist", r.contentType) } - return runtime.Decode(r.decoder, r.body) + + // decode, but if the result is Status return that as an error instead. + out, _, err := r.decoder.Decode(r.body, nil, nil) + if err != nil { + return nil, err + } + switch t := out.(type) { + case *unversioned.Status: + // any status besides StatusSuccess is considered an error. + if t.Status != unversioned.StatusSuccess { + return nil, errors.FromObject(t) + } + } + return out, nil } // StatusCode returns the HTTP status code of the request. (Only valid if no @@ -1083,14 +1099,31 @@ func (r Result) StatusCode(statusCode *int) Result { } // Into stores the result into obj, if possible. If obj is nil it is ignored. +// If the returned object is of type Status and has .Status != StatusSuccess, the +// additional information in Status will be used to enrich the error. func (r Result) Into(obj runtime.Object) error { if r.err != nil { - return r.err + // Check whether the result has a Status object in the body and prefer that. + return r.Error() } if r.decoder == nil { return fmt.Errorf("serializer for %s doesn't exist", r.contentType) } - return runtime.DecodeInto(r.decoder, r.body, obj) + + out, _, err := r.decoder.Decode(r.body, nil, obj) + if err != nil || out == obj { + return err + } + // if a different object is returned, see if it is Status and avoid double decoding + // the object. + switch t := out.(type) { + case *unversioned.Status: + // any status besides StatusSuccess is considered an error. + if t.Status != unversioned.StatusSuccess { + return errors.FromObject(t) + } + } + return nil } // WasCreated updates the provided bool pointer to whether the server returned @@ -1101,7 +1134,29 @@ func (r Result) WasCreated(wasCreated *bool) Result { } // Error returns the error executing the request, nil if no error occurred. +// If the returned object is of type Status and has Status != StatusSuccess, the +// additional information in Status will be used to enrich the error. // See the Request.Do() comment for what errors you might get. func (r Result) Error() error { + // if we have received an unexpected server error, and we have a body and decoder, we can try to extract + // a Status object. + if r.err == nil || !errors.IsUnexpectedServerError(r.err) || len(r.body) == 0 || r.decoder == nil { + return r.err + } + + // attempt to convert the body into a Status object + // to be backwards compatible with old servers that do not return a version, default to "v1" + out, _, err := r.decoder.Decode(r.body, &unversioned.GroupVersionKind{Version: "v1"}, nil) + if err != nil { + glog.V(5).Infof("body was not decodable (unable to check for Status): %v", err) + return r.err + } + switch t := out.(type) { + case *unversioned.Status: + // because we default the kind, we *must* check for StatusFailure + if t.Status == unversioned.StatusFailure { + return errors.FromObject(t) + } + } return r.err } diff --git a/pkg/client/restclient/request_test.go b/pkg/client/restclient/request_test.go index 3bf5de8d650..514be7c2be0 100755 --- a/pkg/client/restclient/request_test.go +++ b/pkg/client/restclient/request_test.go @@ -41,6 +41,7 @@ import ( "k8s.io/kubernetes/pkg/runtime" "k8s.io/kubernetes/pkg/runtime/serializer/streaming" "k8s.io/kubernetes/pkg/util/clock" + "k8s.io/kubernetes/pkg/util/diff" "k8s.io/kubernetes/pkg/util/flowcontrol" "k8s.io/kubernetes/pkg/util/httpstream" "k8s.io/kubernetes/pkg/util/intstr" @@ -582,7 +583,8 @@ func TestTransformUnstructuredError(t *testing.T) { Resource string Name string - ErrFn func(error) bool + ErrFn func(error) bool + Transformed error }{ { Resource: "foo", @@ -626,9 +628,46 @@ func TestTransformUnstructuredError(t *testing.T) { }, ErrFn: apierrors.IsBadRequest, }, + { + // status in response overrides transformed result + Req: &http.Request{}, + Res: &http.Response{StatusCode: http.StatusBadRequest, Body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"Status","apiVersion":"v1","status":"Failure","code":404}`)))}, + ErrFn: apierrors.IsBadRequest, + Transformed: &apierrors.StatusError{ + ErrStatus: unversioned.Status{Status: unversioned.StatusFailure, Code: http.StatusNotFound}, + }, + }, + { + // successful status is ignored + Req: &http.Request{}, + Res: &http.Response{StatusCode: http.StatusBadRequest, Body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"Status","apiVersion":"v1","status":"Success","code":404}`)))}, + ErrFn: apierrors.IsBadRequest, + }, + { + // empty object does not change result + Req: &http.Request{}, + Res: &http.Response{StatusCode: http.StatusBadRequest, Body: ioutil.NopCloser(bytes.NewReader([]byte(`{}`)))}, + ErrFn: apierrors.IsBadRequest, + }, + { + // we default apiVersion for backwards compatibility with old clients + // TODO: potentially remove in 1.7 + Req: &http.Request{}, + Res: &http.Response{StatusCode: http.StatusBadRequest, Body: ioutil.NopCloser(bytes.NewReader([]byte(`{"kind":"Status","status":"Failure","code":404}`)))}, + ErrFn: apierrors.IsBadRequest, + Transformed: &apierrors.StatusError{ + ErrStatus: unversioned.Status{Status: unversioned.StatusFailure, Code: http.StatusNotFound}, + }, + }, + { + // we do not default kind + Req: &http.Request{}, + Res: &http.Response{StatusCode: http.StatusBadRequest, Body: ioutil.NopCloser(bytes.NewReader([]byte(`{"status":"Failure","code":404}`)))}, + ErrFn: apierrors.IsBadRequest, + }, } - for _, testCase := range testCases { + for i, testCase := range testCases { r := &Request{ content: defaultContentConfig(), serializers: defaultSerializers(), @@ -641,12 +680,40 @@ func TestTransformUnstructuredError(t *testing.T) { t.Errorf("unexpected error: %v", err) continue } + if !apierrors.IsUnexpectedServerError(err) { + t.Errorf("%d: unexpected error type: %v", i, err) + } if len(testCase.Name) != 0 && !strings.Contains(err.Error(), testCase.Name) { t.Errorf("unexpected error string: %s", err) } if len(testCase.Resource) != 0 && !strings.Contains(err.Error(), testCase.Resource) { t.Errorf("unexpected error string: %s", err) } + + // verify Error() properly transforms the error + transformed := result.Error() + expect := testCase.Transformed + if expect == nil { + expect = err + } + if !reflect.DeepEqual(expect, transformed) { + t.Errorf("%d: unexpected Error(): %s", i, diff.ObjectReflectDiff(expect, transformed)) + } + + // verify result.Get properly transforms the error + if _, err := result.Get(); !reflect.DeepEqual(expect, err) { + t.Errorf("%d: unexpected error on Get(): %s", i, diff.ObjectReflectDiff(expect, err)) + } + + // verify result.Into properly handles the error + if err := result.Into(&api.Pod{}); !reflect.DeepEqual(expect, err) { + t.Errorf("%d: unexpected error on Into(): %s", i, diff.ObjectReflectDiff(expect, err)) + } + + // verify result.Raw leaves the error in the untransformed state + if _, err := result.Raw(); !reflect.DeepEqual(result.err, err) { + t.Errorf("%d: unexpected error on Raw(): %s", i, diff.ObjectReflectDiff(expect, err)) + } } }