From fc313b7f8ff7a40b6946078f794651d4916a9dbd Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 22 May 2023 11:45:31 -0400 Subject: [PATCH] [api] Return a shapely error for unexpected response (#16743) * Add UnexpectedResultError to nomad/api This allows users to perform additional status-based behavior by rehydrating the error using `errors.As` inside of consumers. --- .changelog/16743.txt | 3 + api/api.go | 31 +-- api/error_unexpected_response.go | 175 +++++++++++++++++ api/error_unexpected_response_test.go | 266 ++++++++++++++++++++++++++ api/go.mod | 1 + api/go.sum | 2 + api/internal/testutil/ports.go | 23 +++ api/internal/testutil/server.go | 4 +- api/namespace_test.go | 17 ++ api/operator.go | 12 +- api/variables.go | 39 ---- go.mod | 2 +- go.sum | 3 +- 13 files changed, 508 insertions(+), 70 deletions(-) create mode 100644 .changelog/16743.txt create mode 100644 api/error_unexpected_response.go create mode 100644 api/error_unexpected_response_test.go create mode 100644 api/internal/testutil/ports.go diff --git a/.changelog/16743.txt b/.changelog/16743.txt new file mode 100644 index 000000000..45cef4ba7 --- /dev/null +++ b/.changelog/16743.txt @@ -0,0 +1,3 @@ +```release-note:improvement +api: return a structured error for unexpected responses +``` diff --git a/api/api.go b/api/api.go index 1bf97f05f..ac755e254 100644 --- a/api/api.go +++ b/api/api.go @@ -895,13 +895,16 @@ func (c *Client) websocket(endpoint string, q *QueryOptions) (*websocket.Conn, * conn, resp, err := dialer.Dial(rhttp.URL.String(), rhttp.Header) // check resp status code, as it's more informative than handshake error we get from ws library - if resp != nil && resp.StatusCode != 101 { + if resp != nil && resp.StatusCode != http.StatusSwitchingProtocols { var buf bytes.Buffer if resp.Header.Get("Content-Encoding") == "gzip" { greader, err := gzip.NewReader(resp.Body) if err != nil { - return nil, nil, fmt.Errorf("Unexpected response code: %d", resp.StatusCode) + return nil, nil, newUnexpectedResponseError( + fromStatusCode(resp.StatusCode), + withExpectedStatuses([]int{http.StatusSwitchingProtocols}), + withError(err)) } io.Copy(&buf, greader) } else { @@ -909,7 +912,11 @@ func (c *Client) websocket(endpoint string, q *QueryOptions) (*websocket.Conn, * } resp.Body.Close() - return nil, nil, fmt.Errorf("Unexpected response code: %d (%s)", resp.StatusCode, buf.Bytes()) + return nil, nil, newUnexpectedResponseError( + fromStatusCode(resp.StatusCode), + withExpectedStatuses([]int{http.StatusSwitchingProtocols}), + withBody(fmt.Sprint(buf.Bytes())), + ) } return conn, resp, err @@ -1129,24 +1136,6 @@ func encodeBody(obj interface{}) (io.Reader, error) { return buf, nil } -// requireOK is used to wrap doRequest and check for a 200 -func requireOK(d time.Duration, resp *http.Response, e error) (time.Duration, *http.Response, error) { - if e != nil { - if resp != nil { - resp.Body.Close() - } - return d, nil, e - } - if resp.StatusCode != 200 { - var buf bytes.Buffer - _, _ = io.Copy(&buf, resp.Body) - _ = resp.Body.Close() - body := strings.TrimSpace(buf.String()) - return d, nil, fmt.Errorf("Unexpected response code: %d (%s)", resp.StatusCode, body) - } - return d, resp, nil -} - // Context returns the context used for canceling HTTP requests related to this query func (o *QueryOptions) Context() context.Context { if o != nil && o.ctx != nil { diff --git a/api/error_unexpected_response.go b/api/error_unexpected_response.go new file mode 100644 index 000000000..4dc4bc7fb --- /dev/null +++ b/api/error_unexpected_response.go @@ -0,0 +1,175 @@ +package api + +import ( + "bytes" + "fmt" + "io" + "net/http" + "strings" + "time" + + "golang.org/x/exp/slices" +) + +// UnexpectedResponseError tracks the components for API errors encountered when +// requireOK and requireStatusIn's conditions are not met. +type UnexpectedResponseError struct { + expected []int + statusCode int + statusText string + body string + err error + additional error +} + +func (e UnexpectedResponseError) HasExpectedStatuses() bool { return len(e.expected) > 0 } +func (e UnexpectedResponseError) ExpectedStatuses() []int { return e.expected } +func (e UnexpectedResponseError) HasStatusCode() bool { return e.statusCode != 0 } +func (e UnexpectedResponseError) StatusCode() int { return e.statusCode } +func (e UnexpectedResponseError) HasStatusText() bool { return e.statusText != "" } +func (e UnexpectedResponseError) StatusText() string { return e.statusText } +func (e UnexpectedResponseError) HasBody() bool { return e.body != "" } +func (e UnexpectedResponseError) Body() string { return e.body } +func (e UnexpectedResponseError) HasError() bool { return e.err != nil } +func (e UnexpectedResponseError) Unwrap() error { return e.err } +func (e UnexpectedResponseError) HasAdditional() bool { return e.additional != nil } +func (e UnexpectedResponseError) Additional() error { return e.additional } +func newUnexpectedResponseError(src unexpectedResponseErrorSource, opts ...unexpectedResponseErrorOption) UnexpectedResponseError { + nErr := src() + for _, opt := range opts { + opt(nErr) + } + if nErr.statusText == "" { + // the stdlib's http.StatusText function is a good place to start + nErr.statusFromCode(http.StatusText) + } + + return *nErr +} + +// Use textual representation of the given integer code. Called when status text +// is not set using the WithStatusText option. +func (e UnexpectedResponseError) statusFromCode(f func(int) string) { + e.statusText = f(e.statusCode) + if !e.HasStatusText() { + e.statusText = "unknown status code" + } +} + +func (e UnexpectedResponseError) Error() string { + var eTxt strings.Builder + eTxt.WriteString("Unexpected response code") + if e.HasBody() || e.HasStatusCode() { + eTxt.WriteString(": ") + } + if e.HasStatusCode() { + eTxt.WriteString(fmt.Sprint(e.statusCode)) + if e.HasBody() { + eTxt.WriteRune(' ') + } + } + if e.HasBody() { + eTxt.WriteString(fmt.Sprintf("(%s)", e.body)) + } + + if e.HasAdditional() { + eTxt.WriteString(fmt.Sprintf(". Additionally, an error occurred while constructing this error (%s); the body might be truncated or missing.", e.additional.Error())) + } + + return eTxt.String() +} + +// UnexpectedResponseErrorOptions are functions passed to NewUnexpectedResponseError +// to customize the created error. +type unexpectedResponseErrorOption func(*UnexpectedResponseError) + +// withError allows the addition of a Go error that may have been encountered +// while processing the response. For example, if there is an error constructing +// the gzip reader to process a gzip-encoded response body. +func withError(e error) unexpectedResponseErrorOption { + return func(u *UnexpectedResponseError) { u.err = e } +} + +// withBody overwrites the Body value with the provided custom value +func withBody(b string) unexpectedResponseErrorOption { + return func(u *UnexpectedResponseError) { u.body = b } +} + +// withStatusText overwrites the StatusText value the provided custom value +func withStatusText(st string) unexpectedResponseErrorOption { + return func(u *UnexpectedResponseError) { u.statusText = st } +} + +// withExpectedStatuses provides a list of statuses that the receiving function +// expected to receive. This can be used by API callers to provide more feedback +// to end-users. +func withExpectedStatuses(s []int) unexpectedResponseErrorOption { + return func(u *UnexpectedResponseError) { u.expected = slices.Clone(s) } +} + +// unexpectedResponseErrorSource provides the basis for a NewUnexpectedResponseError. +type unexpectedResponseErrorSource func() *UnexpectedResponseError + +// fromHTTPResponse read an open HTTP response, drains and closes its body as +// the data for the UnexpectedResponseError. +func fromHTTPResponse(resp *http.Response) unexpectedResponseErrorSource { + return func() *UnexpectedResponseError { + u := new(UnexpectedResponseError) + + if resp != nil { + // collect and close the body + var buf bytes.Buffer + if _, e := io.Copy(&buf, resp.Body); e != nil { + u.additional = e + } + + // Body has been tested as safe to close more than once + _ = resp.Body.Close() + body := strings.TrimSpace(buf.String()) + + // make and return the error + u.statusCode = resp.StatusCode + u.statusText = strings.TrimSpace(strings.TrimPrefix(resp.Status, fmt.Sprint(resp.StatusCode))) + u.body = body + } + return u + } +} + +// fromStatusCode attempts to resolve the status code to status text using +// the resolving function provided inside of the NewUnexpectedResponseError +// implementation. +func fromStatusCode(sc int) unexpectedResponseErrorSource { + return func() *UnexpectedResponseError { return &UnexpectedResponseError{statusCode: sc} } +} + +// doRequestWrapper is a function that wraps the client's doRequest method +// and can be used to provide error and response handling +type doRequestWrapper = func(time.Duration, *http.Response, error) (time.Duration, *http.Response, error) + +// requireOK is used to wrap doRequest and check for a 200 +func requireOK(d time.Duration, resp *http.Response, e error) (time.Duration, *http.Response, error) { + f := requireStatusIn(http.StatusOK) + return f(d, resp, e) +} + +// requireStatusIn is a doRequestWrapper generator that takes expected HTTP +// response codes and validates that the received response code is among them +func requireStatusIn(statuses ...int) doRequestWrapper { + return func(d time.Duration, resp *http.Response, e error) (time.Duration, *http.Response, error) { + if e != nil { + if resp != nil { + _ = resp.Body.Close() + } + return d, nil, e + } + + for _, status := range statuses { + if resp.StatusCode == status { + return d, resp, nil + } + } + + return d, nil, newUnexpectedResponseError(fromHTTPResponse(resp), withExpectedStatuses(statuses)) + } +} diff --git a/api/error_unexpected_response_test.go b/api/error_unexpected_response_test.go new file mode 100644 index 000000000..8c1d53687 --- /dev/null +++ b/api/error_unexpected_response_test.go @@ -0,0 +1,266 @@ +package api_test + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/netip" + "net/url" + "strings" + "testing" + "testing/iotest" + "time" + + "github.com/felixge/httpsnoop" + "github.com/hashicorp/nomad/api" + "github.com/hashicorp/nomad/api/internal/testutil" + "github.com/shoenig/test/must" +) + +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) { + testutil.Parallel(t) + a := mockserver(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 mock server handles the default namespace + // correctly. This ensures that the routing rule for this path is at least + // correct and that the mock server 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) { + testutil.Parallel(t) + 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) { + testutil.Parallel(t) + 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) { + testutil.Parallel(t) + 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 + }) +} + +// mockserver creates a httptest.Server that can be used to serve simple mock +// data, which is faster than starting a real Nomad agent. +func mockserver(t *testing.T) string { + port := testutil.PortAllocator.One() + + 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", port) + + 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 mock server, got resp: %s", b) + + // If we get here, the mock server 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) { + // 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 mock server +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 +// in case of a parse error, the original value is returned unparsed +func ipAddrFromRemoteAddr(s string) string { + if ap, err := netip.ParseAddrPort(s); err == nil { + return ap.Addr().String() + } + return s +} + +// 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 f09b2cfae..d770dc914 100644 --- a/api/go.mod +++ b/api/go.mod @@ -4,6 +4,7 @@ go 1.20 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 d26ca4447..e6c51619e 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/api/internal/testutil/ports.go b/api/internal/testutil/ports.go new file mode 100644 index 000000000..632ba89e8 --- /dev/null +++ b/api/internal/testutil/ports.go @@ -0,0 +1,23 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package testutil + +import ( + "fmt" + + "github.com/shoenig/test/portal" +) + +type fatalTester struct{} + +func (t *fatalTester) Fatalf(msg string, args ...any) { + panic(fmt.Sprintf(msg, args...)) +} + +// PortAllocator is used to acquire unused ports for testing real network +// listeners. +var PortAllocator = portal.New( + new(fatalTester), + portal.WithAddress("127.0.0.1"), +) diff --git a/api/internal/testutil/server.go b/api/internal/testutil/server.go index 292941619..b6abd6dad 100644 --- a/api/internal/testutil/server.go +++ b/api/internal/testutil/server.go @@ -28,7 +28,6 @@ import ( "github.com/hashicorp/nomad/api/internal/testutil/discover" testing "github.com/mitchellh/go-testing-interface" "github.com/shoenig/test/must" - "github.com/shoenig/test/portal" "github.com/shoenig/test/wait" ) @@ -107,8 +106,7 @@ type ServerConfigCallback func(c *TestServerConfig) // defaultServerConfig returns a new TestServerConfig struct pre-populated with // usable config for running as server. func defaultServerConfig(t testing.T) *TestServerConfig { - grabber := portal.New(t) - ports := grabber.Grab(3) + ports := PortAllocator.Grab(3) logLevel := "ERROR" if envLogLevel := os.Getenv("NOMAD_TEST_LOG_LEVEL"); envLogLevel != "" { diff --git a/api/namespace_test.go b/api/namespace_test.go index 9d4d92b32..40ae22b54 100644 --- a/api/namespace_test.go +++ b/api/namespace_test.go @@ -4,6 +4,8 @@ package api import ( + "errors" + "net/http" "testing" "github.com/hashicorp/nomad/api/internal/testutil" @@ -145,3 +147,18 @@ func TestNamespaces_List(t *testing.T) { must.Len(t, 1, resp) must.Eq(t, ns2.Name, resp[0].Name) } + +func TestNamespace_NotFound(t *testing.T) { + testutil.Parallel(t) + + c, s := makeClient(t, nil, nil) + defer s.Stop() + namespaces := c.Namespaces() + + var ure UnexpectedResponseError + _, _, e := namespaces.Info("dummy", nil) + + ok := errors.As(e, &ure) + must.True(t, ok) + must.Eq(t, http.StatusNotFound, ure.StatusCode()) +} diff --git a/api/operator.go b/api/operator.go index ba8d41cec..32faf3546 100644 --- a/api/operator.go +++ b/api/operator.go @@ -6,8 +6,8 @@ package api import ( "encoding/json" "errors" - "fmt" "io" + "net/http" "strconv" "strings" "time" @@ -341,13 +341,15 @@ func (op *Operator) LicenseGet(q *QueryOptions) (*LicenseReply, *QueryMeta, erro } defer resp.Body.Close() - if resp.StatusCode == 204 { + if resp.StatusCode == http.StatusNoContent { return nil, nil, errors.New("Nomad Enterprise only endpoint") } - if resp.StatusCode != 200 { - body, _ := io.ReadAll(resp.Body) - return nil, nil, fmt.Errorf("Unexpected response code: %d (%s)", resp.StatusCode, body) + if resp.StatusCode != http.StatusOK { + return nil, nil, newUnexpectedResponseError( + fromHTTPResponse(resp), + withExpectedStatuses([]int{http.StatusOK, http.StatusNoContent}), + ) } err = json.NewDecoder(resp.Body).Decode(&reply) diff --git a/api/variables.go b/api/variables.go index 91dc13cf4..86458c13a 100644 --- a/api/variables.go +++ b/api/variables.go @@ -4,14 +4,11 @@ package api import ( - "bytes" "encoding/json" "errors" "fmt" - "io" "net/http" "strings" - "time" ) const ( @@ -457,39 +454,3 @@ type ErrCASConflict struct { func (e ErrCASConflict) Error() string { return fmt.Sprintf("cas conflict: expected ModifyIndex %v; found %v", e.CheckIndex, e.Conflict.ModifyIndex) } - -// doRequestWrapper is a function that wraps the client's doRequest method -// and can be used to provide error and response handling -type doRequestWrapper = func(time.Duration, *http.Response, error) (time.Duration, *http.Response, error) - -// requireStatusIn is a doRequestWrapper generator that takes expected HTTP -// response codes and validates that the received response code is among them -func requireStatusIn(statuses ...int) doRequestWrapper { - fn := func(d time.Duration, resp *http.Response, e error) (time.Duration, *http.Response, error) { - if e != nil { - if resp != nil { - _ = resp.Body.Close() - } - return d, nil, e - } - - for _, status := range statuses { - if resp.StatusCode == status { - return d, resp, nil - } - } - - return d, nil, generateUnexpectedResponseCodeError(resp) - } - return fn -} - -// generateUnexpectedResponseCodeError creates a standardized error -// when the the API client's newRequest method receives an unexpected -// HTTP response code when accessing the variable's HTTP API -func generateUnexpectedResponseCodeError(resp *http.Response) error { - var buf bytes.Buffer - _, _ = io.Copy(&buf, resp.Body) - _ = resp.Body.Close() - return fmt.Errorf("Unexpected response code: %d (%s)", resp.StatusCode, buf.Bytes()) -} diff --git a/go.mod b/go.mod index d709f08c4..3244d3e7b 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 d583f19a6..e4e98815c 100644 --- a/go.sum +++ b/go.sum @@ -574,8 +574,9 @@ github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL 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/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=