Skip to content

Commit

Permalink
deps: adjust to gzip handler zero length response body
Browse files Browse the repository at this point in the history
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](nytimes/gziphandler@c551b6c#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.
  • Loading branch information
shoenig committed Jan 18, 2022
1 parent 7bd8416 commit 6a91c89
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 18 deletions.
3 changes: 3 additions & 0 deletions .changelog/11843.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
deps: use gorilla package for gzip http handler
```
48 changes: 30 additions & 18 deletions api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 49 additions & 0 deletions api/api_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package api

import (
"bytes"
"compress/gzip"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
"net/url"
Expand Down Expand Up @@ -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)
}

0 comments on commit 6a91c89

Please sign in to comment.