From d44a243e9bd6eec2b42e569c4b10f01eb791eb1f Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 3 Apr 2023 21:10:17 -0400 Subject: [PATCH] Add tests for UnexpectedResponseError This adds a testServer implementation and some http.Handlers that can test behaviors that are difficult or unreasonable to add to the real HTTP API server. For example, the `closingHandler` intentionally provides partial results to test the `additional` error pathway. --- api/error_unexpected_response_test.go | 268 ++++++++++++++++++++++++++ api/go.mod | 1 + api/go.sum | 2 + go.mod | 2 +- go.sum | 3 +- 5 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 api/error_unexpected_response_test.go diff --git a/api/error_unexpected_response_test.go b/api/error_unexpected_response_test.go new file mode 100644 index 00000000000..18688b02f2a --- /dev/null +++ b/api/error_unexpected_response_test.go @@ -0,0 +1,268 @@ +package api_test + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "testing/iotest" + "time" + + "github.com/felixge/httpsnoop" + "github.com/hashicorp/nomad/api" + "github.com/shoenig/test/must" + "github.com/shoenig/test/portal" +) + +const mockNamespaceBody = `{"Capabilities":null,"CreateIndex":1,"Description":"Default shared namespace","Hash":"C7UbjDwBK0dK8wQq7Izg7SJIzaV+lIo2X7wRtzY3pSw=","Meta":null,"ModifyIndex":1,"Name":"default","Quota":""}` + +func TestUnexpectedResponseError(t *testing.T) { + t.Parallel() + a := testServer(t) + cfg := api.DefaultConfig() + cfg.Address = a + + c, e := api.NewClient(cfg) + must.NoError(t, e) + + type testCase struct { + testFunc func() + statusCode *int + body *int + } + + // ValidateServer ensures that the testServer handles the default namespace + // correctly. This ensures that the routing rule for this path is at least + // correct and that the testServer is passing its address to the client + // properly. + t.Run("ValidateServer", func(t *testing.T) { + n, _, err := c.Namespaces().Info("default", nil) + must.NoError(t, err) + var ns api.Namespace + err = unmock(t, mockNamespaceBody, &ns) + must.NoError(t, err) + must.Eq(t, ns, *n) + }) + + // WrongStatus tests that an UnexpectedResponseError is generated and filled + // with the correct data when a response code that the API client wasn't + // looking for is returned by the server. + t.Run("WrongStatus", func(t *testing.T) { + t.Parallel() + n, _, err := c.Namespaces().Info("badStatus", nil) + must.Nil(t, n) + must.Error(t, err) + t.Logf("err: %v", err) + + ure, ok := err.(api.UnexpectedResponseError) + must.True(t, ok) + + must.True(t, ure.HasStatusCode()) + must.Eq(t, http.StatusAccepted, ure.StatusCode()) + + must.True(t, ure.HasStatusText()) + must.Eq(t, http.StatusText(http.StatusAccepted), ure.StatusText()) + + must.True(t, ure.HasBody()) + must.Eq(t, mockNamespaceBody, ure.Body()) + }) + + // NotFound tests that an UnexpectedResponseError is generated and filled + // with the correct data when a `404 Not Found`` is returned to the API + // client, since the requireOK wrapper doesn't "expect" 404s. + t.Run("NotFound", func(t *testing.T) { + t.Parallel() + n, _, err := c.Namespaces().Info("wat", nil) + must.Nil(t, n) + must.Error(t, err) + t.Logf("err: %v", err) + + ure, ok := err.(api.UnexpectedResponseError) + must.True(t, ok) + + must.True(t, ure.HasStatusCode()) + must.Eq(t, http.StatusNotFound, ure.StatusCode()) + + must.True(t, ure.HasStatusText()) + must.Eq(t, http.StatusText(http.StatusNotFound), ure.StatusText()) + + must.True(t, ure.HasBody()) + must.Eq(t, "Namespace not found", ure.Body()) + }) + + // EarlyClose tests what happens when an error occurs during the building of + // the UnexpectedResponseError using FromHTTPRequest. + t.Run("EarlyClose", func(t *testing.T) { + t.Parallel() + n, _, err := c.Namespaces().Info("earlyClose", nil) + must.Nil(t, n) + must.Error(t, err) + + t.Logf("e: %v\n", err) + ure, ok := err.(api.UnexpectedResponseError) + must.True(t, ok) + + must.True(t, ure.HasStatusCode()) + must.Eq(t, http.StatusInternalServerError, ure.StatusCode()) + + must.True(t, ure.HasStatusText()) + must.Eq(t, http.StatusText(http.StatusInternalServerError), ure.StatusText()) + + must.True(t, ure.HasAdditional()) + must.ErrorContains(t, err, "the body might be truncated") + + must.True(t, ure.HasBody()) + must.Eq(t, "{", ure.Body()) // The body is truncated to the first byte + }) +} + +// testServer creates a httptest.Server that can be used to serve simple mock +// data, which is faster than starting a real Nomad agent. +func testServer(t *testing.T) string { + grabber := portal.New(t) + ports := grabber.Grab(1) + must.Len(t, 1, ports) + + mux := http.NewServeMux() + mux.Handle("/v1/namespace/earlyClose", closingHandler(http.StatusInternalServerError, mockNamespaceBody)) + mux.Handle("/v1/namespace/badStatus", testHandler(http.StatusAccepted, mockNamespaceBody)) + mux.Handle("/v1/namespace/default", testHandler(http.StatusOK, mockNamespaceBody)) + mux.Handle("/v1/namespace/", testNotFoundHandler("Namespace not found")) + mux.Handle("/v1/namespace", http.NotFoundHandler()) + mux.Handle("/v1", http.NotFoundHandler()) + mux.Handle("/", testHandler(http.StatusOK, "ok")) + + lMux := testLogRequestHandler(t, mux) + ts := httptest.NewUnstartedServer(lMux) + ts.Config.Addr = fmt.Sprintf("127.0.0.1:%d", ports[0]) + + t.Logf("starting mock server on %s", ts.Config.Addr) + ts.Start() + t.Cleanup(func() { + t.Log("stopping mock server") + ts.Close() + }) + + // Test the server + tc := ts.Client() + resp, err := tc.Get(func() string { p, _ := url.JoinPath(ts.URL, "/"); return p }()) + must.NoError(t, err) + defer resp.Body.Close() + b, err := io.ReadAll(resp.Body) + must.NoError(t, err) + t.Logf("checking testServer, got resp: %s", b) + + // If we get here, the testServer is running and ready for requests. + return ts.URL +} + +// addMockHeaders sets the common Nomad headers to values sufficient to be +// parsed into api.QueryMeta +func addMockHeaders(h http.Header) { + h.Add("X-Nomad-Knownleader", "true") + h.Add("X-Nomad-Lastcontact", "0") + h.Add("X-Nomad-Index", "1") + h.Add("Content-Type", "application/json") +} + +// testNotFoundHandler creates a testHandler preconfigured with status code 404. +func testNotFoundHandler(b string) http.Handler { return testHandler(http.StatusNotFound, b) } + +// testNotFoundHandler creates a testHandler preconfigured with status code 200. +func testOKHandler(b string) http.Handler { return testHandler(http.StatusOK, b) } + +// testHandler is a helper function that writes a Nomad-like server response +// with the necessary headers to make the API client happy +func testHandler(sc int, b string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + addMockHeaders(w.Header()) + w.WriteHeader(sc) + w.Write([]byte(b)) + }) +} + +// closingHandler is a handler that terminates the response body early in the +// reading process +func closingHandler(sc int, b string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + + // We need a misbehaving reader to test network effects when collecting + // the http.Response data into a UnexpectedResponseError + er := iotest.TimeoutReader( // TimeoutReader throws an error on the second read + iotest.OneByteReader( // OneByteReader yields a byte at a time, causing multiple reads + strings.NewReader(mockNamespaceBody), + ), + ) + + // We need to set content-length to the true value it _should_ be so the + // API-side reader knows it's a short read. + w.Header().Set("content-length", fmt.Sprint(len(mockNamespaceBody))) + addMockHeaders(w.Header()) + w.WriteHeader(sc) + + // Using io.Copy to send the data into w prevents golang from setting the + // content-length itself. + io.Copy(w, er) + }) +} + +// testLogRequestHandler wraps a http.Handler with a logger that writes to the +// test log output +func testLogRequestHandler(t *testing.T, h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t := t + // call the original http.Handler wrapped in a httpsnoop + m := httpsnoop.CaptureMetrics(h, w, r) + ri := HTTPReqInfo{ + uri: r.URL.String(), + method: r.Method, + ipaddr: ipAddrFromRemoteAddr(r.RemoteAddr), + code: m.Code, + duration: m.Duration, + size: m.Written, + userAgent: r.UserAgent(), + } + t.Logf(ri.String()) + }) +} + +// HTTPReqInfo holds all the information used to log a request to the testserver +type HTTPReqInfo struct { + method string + uri string + referer string + ipaddr string + code int + size int64 + duration time.Duration + userAgent string +} + +func (i HTTPReqInfo) String() string { + return fmt.Sprintf( + "method=%q uri=%q referer=%q ipaddr=%q code=%d size=%d duration=%q userAgent=%q", + i.method, i.uri, i.referer, i.ipaddr, i.code, i.size, i.duration, i.userAgent, + ) +} + +// ipAddrFromRemoteAddr removes the port from the address:port in remote addr +func ipAddrFromRemoteAddr(s string) string { + idx := strings.LastIndex(s, ":") + if idx == -1 { + return s + } + return s[:idx] +} + +// unmock attempts to unmarshal a given mock json body into dst, which should +// be a pointer to the correct API struct. +func unmock(t *testing.T, src string, dst any) error { + if err := json.Unmarshal([]byte(src), dst); err != nil { + return fmt.Errorf("error unmarshaling mock: %w", err) + } + return nil +} diff --git a/api/go.mod b/api/go.mod index 07878f4899a..41408f692c0 100644 --- a/api/go.mod +++ b/api/go.mod @@ -4,6 +4,7 @@ go 1.19 require ( github.com/docker/go-units v0.5.0 + github.com/felixge/httpsnoop v1.0.3 github.com/gorilla/websocket v1.5.0 github.com/hashicorp/cronexpr v1.1.1 github.com/hashicorp/go-cleanhttp v0.5.2 diff --git a/api/go.sum b/api/go.sum index 774b4b80682..9294df301f0 100644 --- a/api/go.sum +++ b/api/go.sum @@ -3,6 +3,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/docker/go-units v0.5.0 h1:69rxXcBk27SvSaaxTtLh/8llcHD8vYHT7WSdRZ/jvr4= github.com/docker/go-units v0.5.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= +github.com/felixge/httpsnoop v1.0.3 h1:s/nj+GCswXYzN5v2DpNMuMQYe+0DDwt5WVCU6CWBdXk= +github.com/felixge/httpsnoop v1.0.3/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/gorilla/websocket v1.5.0 h1:PPwGk2jz7EePpoHN/+ClbZu8SPxiqlu12wZP/3sWmnc= diff --git a/go.mod b/go.mod index f29f0266314..4adf6fb6344 100644 --- a/go.mod +++ b/go.mod @@ -188,7 +188,7 @@ require ( github.com/docker/go-metrics v0.0.1 // indirect github.com/docker/libtrust v0.0.0-20160708172513-aabc10ec26b7 // indirect github.com/fatih/color v1.13.0 - github.com/felixge/httpsnoop v1.0.1 // indirect + github.com/felixge/httpsnoop v1.0.3 // indirect github.com/go-ole/go-ole v1.2.6 // indirect github.com/godbus/dbus/v5 v5.1.0 // indirect github.com/gogo/protobuf v1.3.2 // indirect diff --git a/go.sum b/go.sum index c5babc86123..523b10a874f 100644 --- a/go.sum +++ b/go.sum @@ -591,8 +591,9 @@ github.com/fatih/color v1.13.0 h1:8LOYc1KYPPmyKMuN8QV2DNRWNbLo6LZ0iLs8+mlH53w= github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk= github.com/fatih/structs v1.1.0 h1:Q7juDM0QtcnhCpeyLGQKyg4TOIghuNXrkL32pHAUMxo= github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= -github.com/felixge/httpsnoop v1.0.1 h1:lvB5Jl89CsZtGIWuTcDM1E/vkVs49/Ml7JJe07l8SPQ= github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= +github.com/felixge/httpsnoop v1.0.3 h1:s/nj+GCswXYzN5v2DpNMuMQYe+0DDwt5WVCU6CWBdXk= +github.com/felixge/httpsnoop v1.0.3/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= github.com/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k= github.com/frankban/quicktest v1.10.0/go.mod h1:ui7WezCLWMWxVWr1GETZY3smRy0G4KWq9vcPtJmFl7Y= github.com/frankban/quicktest v1.11.3/go.mod h1:wRf/ReqHper53s+kmmSZizM8NamnL3IM0I9ntUbOk+k=