From d061e8eadf91e1d03cfc942d55fc825ddb5b50b7 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 12 Jan 2022 15:17:16 -0600 Subject: [PATCH 1/2] deps: swap gzip handler for gorilla This has been pinned since the Go modules migration, because the nytimes gzip handler was modified in version v1.1.0 in a way that is no longer compatible. Pretty sure it is this commit: https://github.com/nytimes/gziphandler/commit/c551b6c3b4b976eafa1a18220b5e21692784d8e2 Instead use handler.CompressHandler from gorilla, which is a web toolkit we already make use of for other things. --- command/agent/http.go | 10 ++-------- go.mod | 4 ++-- go.sum | 8 ++++++-- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/command/agent/http.go b/command/agent/http.go index a3c082661e9..d33be8e85c9 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -14,8 +14,8 @@ import ( "strings" "time" - "github.com/NYTimes/gziphandler" assetfs "github.com/elazarl/go-bindata-assetfs" + "github.com/gorilla/handlers" "github.com/gorilla/websocket" "github.com/hashicorp/go-connlimit" log "github.com/hashicorp/go-hclog" @@ -86,12 +86,6 @@ func NewHTTPServers(agent *Agent, config *Config) ([]*HTTPServer, error) { var srvs []*HTTPServer var serverInitializationErrors error - // Handle requests with gzip compression - gzip, err := gziphandler.GzipHandlerWithOpts(gziphandler.MinSize(0)) - if err != nil { - return srvs, err - } - // Get connection handshake timeout limit handshakeTimeout, err := time.ParseDuration(config.Limits.HTTPSHandshakeTimeout) if err != nil { @@ -160,7 +154,7 @@ func NewHTTPServers(agent *Agent, config *Config) ([]*HTTPServer, error) { // Create HTTP server with timeouts httpServer := http.Server{ Addr: srv.Addr, - Handler: gzip(mux), + Handler: handlers.CompressHandler(mux), ConnState: makeConnState(config.TLSConfig.EnableHTTP, handshakeTimeout, maxConns), ErrorLog: newHTTPServerLogger(srv.logger), } diff --git a/go.mod b/go.mod index da71b424c02..980101f08ea 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,6 @@ go 1.17 // Pinned dependencies are noted in github.com/hashicorp/nomad/issues/11826 replace ( github.com/Microsoft/go-winio => github.com/endocrimes/go-winio v0.4.13-0.20190628114223-fb47a8b41948 - github.com/NYTimes/gziphandler => github.com/NYTimes/gziphandler v1.0.0 github.com/apparentlymart/go-textseg/v12 => github.com/apparentlymart/go-textseg/v12 v12.0.0 github.com/hashicorp/go-discover => github.com/hashicorp/go-discover v0.0.0-20210818145131-c573d69da192 github.com/hashicorp/hcl => github.com/hashicorp/hcl v1.0.1-0.20201016140508-a07e7d50bbee @@ -18,7 +17,6 @@ replace github.com/hashicorp/nomad/api => ./api require ( github.com/LK4D4/joincontext v0.0.0-20171026170139-1724345da6d5 github.com/Microsoft/go-winio v0.4.17 - github.com/NYTimes/gziphandler v1.0.1 github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e github.com/armon/go-metrics v0.3.10 github.com/aws/aws-sdk-go v1.42.27 @@ -41,6 +39,7 @@ require ( github.com/golang/protobuf v1.5.2 github.com/golang/snappy v0.0.4 github.com/google/go-cmp v0.5.6 + github.com/gorilla/handlers v1.5.1 github.com/gorilla/websocket v1.4.2 github.com/gosuri/uilive v0.0.4 github.com/grpc-ecosystem/go-grpc-middleware v1.2.1-0.20200228141219-3ce3d519df39 @@ -179,6 +178,7 @@ require ( github.com/docker/libtrust v0.0.0-20160708172513-aabc10ec26b7 // indirect github.com/envoyproxy/go-control-plane v0.10.0 // indirect github.com/envoyproxy/protoc-gen-validate v0.6.2 // indirect + github.com/felixge/httpsnoop v1.0.1 // indirect github.com/go-ole/go-ole v1.2.4 // indirect github.com/godbus/dbus/v5 v5.0.4 // indirect github.com/gogo/protobuf v1.3.2 // indirect diff --git a/go.sum b/go.sum index db3894de963..e0738ae83f2 100644 --- a/go.sum +++ b/go.sum @@ -114,8 +114,8 @@ github.com/Microsoft/hcsshim v0.8.23 h1:47MSwtKGXet80aIn+7h4YI6fwPmwIghAnsx2aOUr github.com/Microsoft/hcsshim v0.8.23/go.mod h1:4zegtUJth7lAvFyc6cH2gGQ5B3OFQim01nnU2M8jKDg= github.com/Microsoft/hcsshim/test v0.0.0-20201218223536-d3e5debf77da/go.mod h1:5hlzMzRKMLyo42nCZ9oml8AdTlq/0cvIaBv6tK1RehU= github.com/Microsoft/hcsshim/test v0.0.0-20210227013316-43a75bb4edd3/go.mod h1:mw7qgWloBUl75W/gVH3cQszUg1+gUITj7D6NY7ywVnY= -github.com/NYTimes/gziphandler v1.0.0 h1:OswZCvpiFsNRCbeapdJxDuikAqVXTgV7XAht8S9olZo= -github.com/NYTimes/gziphandler v1.0.0/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= +github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= +github.com/NYTimes/gziphandler v1.0.1/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU= github.com/PuerkitoBio/purell v1.0.0/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= @@ -445,6 +445,8 @@ 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/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k= github.com/frankban/quicktest v1.4.0/go.mod h1:36zfPVQyHxymz4cH7wlDmVwDrJuljRB60qkgn7rorfQ= github.com/frankban/quicktest v1.10.0/go.mod h1:ui7WezCLWMWxVWr1GETZY3smRy0G4KWq9vcPtJmFl7Y= @@ -618,6 +620,8 @@ github.com/gophercloud/gophercloud v0.1.0 h1:P/nh25+rzXouhytV2pUHBb65fnds26Ghl8/ github.com/gophercloud/gophercloud v0.1.0/go.mod h1:vxM41WHh5uqHVBMZHzuwNOHh8XEoIEcSTewFxm1c5g8= github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY= github.com/gorilla/handlers v0.0.0-20150720190736-60c7bfde3e33/go.mod h1:Qkdc/uu4tH4g6mTK6auzZ766c4CA0Ng8+o/OAirnOIQ= +github.com/gorilla/handlers v1.5.1 h1:9lRY6j8DEeeBT10CvO9hGW0gmky0BprnvDI5vfhUHH4= +github.com/gorilla/handlers v1.5.1/go.mod h1:t8XrUpc4KVXb7HGyJ4/cEnwQiaxrX/hz1Zv/4g96P1Q= github.com/gorilla/mux v1.7.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs= github.com/gorilla/mux v1.7.4/go.mod h1:DVbg23sWSpFRCP0SfiEN6jmj59UnW/n46BH5rLB71So= github.com/gorilla/mux v1.8.0 h1:i40aqfkR1h2SlN9hojwV5ZA91wcXFOvkdNIeFDP5koI= From e2ab16847d4663c70db309a1931fdf56761894df Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Thu, 13 Jan 2022 09:57:36 -0600 Subject: [PATCH 2/2] deps: adjust to gzip handler zero length response body After swapping gzip handler to use the gorilla library, we must account for a quirk in how zero/minimal length response bodies are delivered. The previous gzip handler was configured to compress all responses regardless of size - even if the data was zero length or below the network MTU. This behavior changed in [v1.1.0](https://github.com/nytimes/gziphandler/commit/c551b6c3b4b976eafa1a18220b5e21692784d8e2#diff-de723e6602cc2f16f7a9d85fd89d69954edc12a49134dab8901b10ee06d1879d) which is why we could not upgrade. The Nomad HTTP Client mutates the http.Response.Body object, making a strong assumption that if the Content-Encoding header is set to "gzip", the response will be readable via gzip decoder. This is no longer true for the nytimes gzip handler, and is also not true for the gorilla gzip handler. It seems in practice this only makes a difference on the /v1/operator/license endpoint which returns an empty response in OSS Nomad. The fix here is to simply not wrap the response body reader if we encounter an io.EOF while creating the gzip reader - indicating there is no data to decode. --- .changelog/11843.txt | 3 +++ api/api.go | 48 +++++++++++++++++++++++++++---------------- api/api_test.go | 49 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 18 deletions(-) create mode 100644 .changelog/11843.txt diff --git a/.changelog/11843.txt b/.changelog/11843.txt new file mode 100644 index 00000000000..1d62d1a61a2 --- /dev/null +++ b/.changelog/11843.txt @@ -0,0 +1,3 @@ +```release-note:improvement +deps: use gorilla package for gzip http handler +``` diff --git a/api/api.go b/api/api.go index 8719cab2c39..d8b90cc8dc8 100644 --- a/api/api.go +++ b/api/api.go @@ -741,33 +741,45 @@ func (c *Client) doRequest(r *request) (time.Duration, *http.Response, error) { if err != nil { return 0, nil, err } + start := time.Now() resp, err := c.httpClient.Do(req) diff := time.Since(start) // If the response is compressed, we swap the body's reader. - if resp != nil && resp.Header != nil { - var reader io.ReadCloser - switch resp.Header.Get("Content-Encoding") { - case "gzip": - greader, err := gzip.NewReader(resp.Body) - if err != nil { - return 0, nil, err - } + if zipErr := c.autoUnzip(resp); zipErr != nil { + return 0, nil, zipErr + } - // The gzip reader doesn't close the wrapped reader so we use - // multiCloser. - reader = &multiCloser{ - reader: greader, - inorderClose: []io.Closer{greader, resp.Body}, - } - default: - reader = resp.Body + return diff, resp, err +} + +// autoUnzip modifies resp in-place, wrapping the response body with a gzip +// reader if the Content-Encoding of the response is "gzip". +func (*Client) autoUnzip(resp *http.Response) error { + if resp == nil || resp.Header == nil { + return nil + } + + if resp.Header.Get("Content-Encoding") == "gzip" { + zReader, err := gzip.NewReader(resp.Body) + if err == io.EOF { + // zero length response, do not wrap + return nil + } else if err != nil { + // some other error (e.g. corrupt) + return err + } + + // The gzip reader does not close an underlying reader, so use a + // multiCloser to make sure response body does get closed. + resp.Body = &multiCloser{ + reader: zReader, + inorderClose: []io.Closer{zReader, resp.Body}, } - resp.Body = reader } - return diff, resp, err + return nil } // rawQuery makes a GET request to the specified endpoint but returns just the diff --git a/api/api_test.go b/api/api_test.go index a845f641054..3f83e716c93 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -1,10 +1,13 @@ package api import ( + "bytes" + "compress/gzip" "context" "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -568,3 +571,49 @@ func TestClient_HeaderRaceCondition(t *testing.T) { require.Equal(2, <-c, "goroutine request should have two headers") require.Len(conf.Headers, 1, "config headers should not mutate") } + +func TestClient_autoUnzip(t *testing.T) { + var client *Client = nil + + try := func(resp *http.Response, exp error) { + err := client.autoUnzip(resp) + require.Equal(t, exp, err) + } + + // response object is nil + try(nil, nil) + + // response.Body is nil + try(new(http.Response), nil) + + // content-encoding is not gzip + try(&http.Response{ + Header: http.Header{"Content-Encoding": []string{"text"}}, + }, nil) + + // content-encoding is gzip but body is empty + try(&http.Response{ + Header: http.Header{"Content-Encoding": []string{"gzip"}}, + Body: io.NopCloser(bytes.NewBuffer([]byte{})), + }, nil) + + // content-encoding is gzip but body is invalid gzip + try(&http.Response{ + Header: http.Header{"Content-Encoding": []string{"gzip"}}, + Body: io.NopCloser(bytes.NewBuffer([]byte("not a zip"))), + }, errors.New("unexpected EOF")) + + // sample gzip payload + var b bytes.Buffer + w := gzip.NewWriter(&b) + _, err := w.Write([]byte("hello world")) + require.NoError(t, err) + err = w.Close() + require.NoError(t, err) + + // content-encoding is gzip and body is gzip data + try(&http.Response{ + Header: http.Header{"Content-Encoding": []string{"gzip"}}, + Body: io.NopCloser(&b), + }, nil) +}