From 56c794028f26b8dd74abbc71a1cb9c6671eeffed Mon Sep 17 00:00:00 2001 From: fm Date: Fri, 11 Nov 2022 19:41:46 +0800 Subject: [PATCH] fix: fix log nil error when http request failed --- errors/errorhandling.go | 5 +- errors/errorhandling_test.go | 113 ++++++++++++++++++++++++++++++++- plugin/client/plugin_client.go | 9 ++- 3 files changed, 119 insertions(+), 8 deletions(-) diff --git a/errors/errorhandling.go b/errors/errorhandling.go index 764ad55c4..34f437732 100644 --- a/errors/errorhandling.go +++ b/errors/errorhandling.go @@ -49,6 +49,7 @@ func AsStatusCode(err error) int { // AsStatusError transform resty response to status error func AsStatusError(response *resty.Response, grs ...schema.GroupResource) error { + // adding GroupResource as a "optional" parameter only // should never provide more than one gr := RESTClientGroupResource @@ -66,7 +67,9 @@ func AsStatusError(response *resty.Response, grs ...schema.GroupResource) error ) if err, ok := response.Error().(error); ok { - statusError.ErrStatus.Message = err.Error() + if originalErr := err.Error(); originalErr != "" { + statusError.ErrStatus.Message = originalErr + } } return statusError diff --git a/errors/errorhandling_test.go b/errors/errorhandling_test.go index 05b023548..fbb8b4617 100644 --- a/errors/errorhandling_test.go +++ b/errors/errorhandling_test.go @@ -20,11 +20,12 @@ import ( goerrors "errors" "fmt" "net/http" - + "net/http/httptest" "testing" + "github.com/go-resty/resty/v2" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -49,3 +50,111 @@ func TestAsStatusCode(t *testing.T) { g.Expect(AsStatusCode(fmt.Errorf("anyerror"))).To(Equal(http.StatusInternalServerError)) g.Expect(AsStatusCode(errors.NewBadRequest("bad"))).To(Equal(http.StatusBadRequest)) } + +var _ = Describe("Test AsStatusError", func() { + var ( + server *httptest.Server + response *resty.Response + ) + + newTestServer := func(f func(w http.ResponseWriter, r *http.Request)) { + mux := http.NewServeMux() + server = httptest.NewServer(mux) + mux.HandleFunc("/test", f) + } + + BeforeEach(func() { + server = &httptest.Server{} + response = &resty.Response{} + }) + + JustBeforeEach(func() { + response, _ = resty.New().R().Get(server.URL + "/test") + }) + + When("server return 200", func() { + BeforeEach(func() { + newTestServer(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + DeferCleanup(func() { + server.Close() + }) + }) + + It("should return nil", func() { + Expect(response.StatusCode()).Should(Equal(http.StatusOK)) + Expect(AsStatusError(response)).To(BeNil()) + }) + }) + + When("server return 301", func() { + BeforeEach(func() { + newTestServer(func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Location", "https://127.0.0.1") + w.WriteHeader(http.StatusMovedPermanently) + }) + DeferCleanup(func() { + server.Close() + }) + }) + + It("rawResponse should be nil", func() { + Expect(response.RawResponse).To(BeNil()) + }) + }) + + When("server return 400", func() { + BeforeEach(func() { + newTestServer(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte("test body")) + }) + DeferCleanup(func() { + server.Close() + }) + }) + + It("should return 400", func() { + Expect(response.StatusCode()).To(Equal(http.StatusBadRequest)) + statusError := AsStatusError(response).(*errors.StatusError) + Expect(int(statusError.ErrStatus.Code)).To(Equal(http.StatusBadRequest)) + Expect(statusError.ErrStatus.Message).To(ContainSubstring("the server rejected our request for an unknown reason")) + }) + }) + When("server return 403", func() { + BeforeEach(func() { + newTestServer(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + w.Write([]byte("test forbidden error")) + }) + DeferCleanup(func() { + server.Close() + }) + }) + + It("should return 403", func() { + Expect(response.StatusCode()).To(Equal(http.StatusForbidden)) + statusError := AsStatusError(response).(*errors.StatusError) + Expect(int(statusError.ErrStatus.Code)).To(Equal(http.StatusForbidden)) + Expect(statusError.ErrStatus.Message).To(ContainSubstring("test forbidden error")) + }) + }) + When("server return 500", func() { + BeforeEach(func() { + newTestServer(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + DeferCleanup(func() { + server.Close() + }) + }) + + It("should return 500", func() { + Expect(response.StatusCode()).To(Equal(http.StatusInternalServerError)) + statusError := AsStatusError(response).(*errors.StatusError) + Expect(int(statusError.ErrStatus.Code)).To(Equal(http.StatusInternalServerError)) + Expect(statusError.ErrStatus.Message).To(ContainSubstring("an error on the server (\"\") has prevented the request from succeeding")) + }) + }) +}) diff --git a/plugin/client/plugin_client.go b/plugin/client/plugin_client.go index a788d67f5..23b50eca8 100644 --- a/plugin/client/plugin_client.go +++ b/plugin/client/plugin_client.go @@ -22,19 +22,18 @@ import ( "strings" "github.com/go-resty/resty/v2" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" - duckv1 "knative.dev/pkg/apis/duck/v1" - metav1alpha1 "github.com/katanomi/pkg/apis/meta/v1alpha1" "github.com/katanomi/pkg/client" perrors "github.com/katanomi/pkg/errors" "github.com/katanomi/pkg/tracing" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + duckv1 "knative.dev/pkg/apis/duck/v1" ) var ( defaultOptions = []OptionFunc{ - ErrorOpts(&errors.StatusError{}), + ErrorOpts(&metav1.Status{}), HeaderOpts("Content-Type", "application/json"), } )