From 36ee69609eb7be3bb8d26e48e41dcce864f45bc0 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Thu, 13 Jul 2017 17:56:01 +0200 Subject: [PATCH] fix: double compression. --- glide.lock | 6 +- glide.yaml | 2 + middlewares/compress.go | 24 ++-- middlewares/compress_test.go | 110 +++++++++++++++--- vendor/github.com/NYTimes/gziphandler/gzip.go | 59 +++++----- .../github.com/NYTimes/gziphandler/wrapper.go | 58 +++++++++ 6 files changed, 198 insertions(+), 61 deletions(-) create mode 100644 vendor/github.com/NYTimes/gziphandler/wrapper.go diff --git a/glide.lock b/glide.lock index f8442e172..586c9b450 100644 --- a/glide.lock +++ b/glide.lock @@ -1,4 +1,4 @@ -hash: cebc972cf87c4b0a8f86801f38750c51b09c8dee3bf62bb48f8eaa6ab7946352 +hash: df3bba260c0e5c3183741ab4aca2ae551a5c6d9ba11f4e05b90554a9ffad96ad updated: 2017-06-29T16:47:14.848940186+02:00 imports: - name: cloud.google.com/go @@ -320,7 +320,9 @@ imports: - name: github.com/mvdan/xurls version: db96455566f05ffe42bd6ac671f05eeb1152b45d - name: github.com/NYTimes/gziphandler - version: 22d4470af89e09998fc16b35029df973932df4ae + version: 316adfc72ed3b0157975917adf62ba2dc31842ce + repo: https://github.com/containous/gziphandler.git + vcs: git - name: github.com/ogier/pflag version: 45c278ab3607870051a2ea9040bb85fcb8557481 - name: github.com/opencontainers/runc diff --git a/glide.yaml b/glide.yaml index fc582e5e3..0d8f43971 100644 --- a/glide.yaml +++ b/glide.yaml @@ -87,6 +87,8 @@ import: vcs: git - package: github.com/abbot/go-http-auth - package: github.com/NYTimes/gziphandler + repo: https://github.com/containous/gziphandler.git + vcs: git - package: github.com/docker/leadership - package: github.com/satori/go.uuid version: ^1.1.0 diff --git a/middlewares/compress.go b/middlewares/compress.go index 5df1dd9c9..455c959d6 100644 --- a/middlewares/compress.go +++ b/middlewares/compress.go @@ -1,13 +1,11 @@ package middlewares import ( + "compress/gzip" "net/http" "github.com/NYTimes/gziphandler" -) - -const ( - contentEncodingHeader = "Content-Encoding" + "github.com/containous/traefik/log" ) // Compress is a middleware that allows redirection @@ -15,17 +13,13 @@ type Compress struct{} // ServerHTTP is a function used by Negroni func (c *Compress) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) { - if isEncoded(r.Header) { - next.ServeHTTP(rw, r) - } else { - newGzipHandler := gziphandler.GzipHandler(next) - newGzipHandler.ServeHTTP(rw, r) - } + gzipHandler(next).ServeHTTP(rw, r) } -func isEncoded(headers http.Header) bool { - header := headers.Get(contentEncodingHeader) - // According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding, - // content is not encoded if the header 'Content-Encoding' is empty or equals to 'identity'. - return header != "" && header != "identity" +func gzipHandler(h http.Handler) http.Handler { + wrapper, err := gziphandler.NewGzipHandler(gzip.DefaultCompression, gziphandler.DefaultMinSize, &gziphandler.GzipResponseWriterWrapper{}) + if err != nil { + log.Error(err) + } + return wrapper(h) } diff --git a/middlewares/compress_test.go b/middlewares/compress_test.go index c83bd3b1d..8f0fc6333 100644 --- a/middlewares/compress_test.go +++ b/middlewares/compress_test.go @@ -1,36 +1,39 @@ package middlewares import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" "github.com/NYTimes/gziphandler" + "github.com/codegangsta/negroni" "github.com/containous/traefik/testhelpers" "github.com/stretchr/testify/assert" ) const ( - acceptEncodingHeader = "Accept-Encoding" - varyHeader = "Vary" - gzip = "gzip" + acceptEncodingHeader = "Accept-Encoding" + contentEncodingHeader = "Content-Encoding" + varyHeader = "Vary" + gzipValue = "gzip" ) func TestShouldCompressWhenNoContentEncodingHeader(t *testing.T) { handler := &Compress{} req := testhelpers.MustNewRequest(http.MethodGet, "http://localhost", nil) - req.Header.Add(acceptEncodingHeader, gzip) + req.Header.Add(acceptEncodingHeader, gzipValue) baseBody := generateBytes(gziphandler.DefaultMinSize) next := func(rw http.ResponseWriter, r *http.Request) { rw.Write(baseBody) } - rw := httptest.NewRecorder() + rw := httptest.NewRecorder() handler.ServeHTTP(rw, req, next) - assert.Equal(t, gzip, rw.Header().Get(contentEncodingHeader)) + assert.Equal(t, gzipValue, rw.Header().Get(contentEncodingHeader)) assert.Equal(t, acceptEncodingHeader, rw.Header().Get(varyHeader)) if assert.ObjectsAreEqualValues(rw.Body.Bytes(), baseBody) { @@ -42,28 +45,105 @@ func TestShouldNotCompressWhenContentEncodingHeader(t *testing.T) { handler := &Compress{} req := testhelpers.MustNewRequest(http.MethodGet, "http://localhost", nil) - req.Header.Add(acceptEncodingHeader, gzip) - req.Header.Add(contentEncodingHeader, gzip) - - baseBody := generateBytes(gziphandler.DefaultMinSize) + req.Header.Add(acceptEncodingHeader, gzipValue) + fakeCompressedBody := generateBytes(gziphandler.DefaultMinSize) next := func(rw http.ResponseWriter, r *http.Request) { - rw.Write(baseBody) + rw.Header().Add(contentEncodingHeader, gzipValue) + rw.Header().Add(varyHeader, acceptEncodingHeader) + rw.Write(fakeCompressedBody) } rw := httptest.NewRecorder() handler.ServeHTTP(rw, req, next) - assert.Equal(t, "", rw.Header().Get(contentEncodingHeader)) - assert.Equal(t, "", rw.Header().Get(varyHeader)) + assert.Equal(t, gzipValue, rw.Header().Get(contentEncodingHeader)) + assert.Equal(t, acceptEncodingHeader, rw.Header().Get(varyHeader)) - assert.EqualValues(t, rw.Body.Bytes(), baseBody) + assert.EqualValues(t, rw.Body.Bytes(), fakeCompressedBody) +} + +func TestShouldNotCompressWhenNoAcceptEncodingHeader(t *testing.T) { + handler := &Compress{} + + req := testhelpers.MustNewRequest(http.MethodGet, "http://localhost", nil) + + fakeBody := generateBytes(gziphandler.DefaultMinSize) + next := func(rw http.ResponseWriter, r *http.Request) { + rw.Write(fakeBody) + } + + rw := httptest.NewRecorder() + handler.ServeHTTP(rw, req, next) + + assert.Empty(t, rw.Header().Get(contentEncodingHeader)) + assert.EqualValues(t, rw.Body.Bytes(), fakeBody) +} + +func TestIntegrationShouldNotCompressWhenContentAlreadyCompressed(t *testing.T) { + fakeCompressedBody := generateBytes(100000) + + handler := func(rw http.ResponseWriter, r *http.Request) { + rw.Header().Add(contentEncodingHeader, gzipValue) + rw.Header().Add(varyHeader, acceptEncodingHeader) + rw.Write(fakeCompressedBody) + } + + comp := &Compress{} + + negro := negroni.New(comp) + negro.UseHandlerFunc(handler) + ts := httptest.NewServer(negro) + defer ts.Close() + + client := &http.Client{} + req := testhelpers.MustNewRequest(http.MethodGet, ts.URL, nil) + req.Header.Add(acceptEncodingHeader, gzipValue) + + resp, err := client.Do(req) + assert.NoError(t, err, "there should be no error") + + assert.Equal(t, gzipValue, resp.Header.Get(contentEncodingHeader)) + assert.Equal(t, acceptEncodingHeader, resp.Header.Get(varyHeader)) + + body, err := ioutil.ReadAll(resp.Body) + assert.EqualValues(t, fakeCompressedBody, body) +} + +func TestIntegrationShouldCompressWhenAcceptEncodingHeaderIsPresent(t *testing.T) { + fakeBody := generateBytes(100000) + + handler := func(rw http.ResponseWriter, r *http.Request) { + rw.Write(fakeBody) + } + + comp := &Compress{} + + negro := negroni.New(comp) + negro.UseHandlerFunc(handler) + ts := httptest.NewServer(negro) + defer ts.Close() + + client := &http.Client{} + req := testhelpers.MustNewRequest(http.MethodGet, ts.URL, nil) + req.Header.Add(acceptEncodingHeader, gzipValue) + + resp, err := client.Do(req) + assert.NoError(t, err, "there should be no error") + + assert.Equal(t, gzipValue, resp.Header.Get(contentEncodingHeader)) + assert.Equal(t, acceptEncodingHeader, resp.Header.Get(varyHeader)) + + body, err := ioutil.ReadAll(resp.Body) + if assert.ObjectsAreEqualValues(body, fakeBody) { + assert.Fail(t, "expected a compressed body", "got %v", body) + } } func generateBytes(len int) []byte { var value []byte for i := 0; i < len; i++ { - value = append(value, 0x61) + value = append(value, 0x61+byte(i)) } return value } diff --git a/vendor/github.com/NYTimes/gziphandler/gzip.go b/vendor/github.com/NYTimes/gziphandler/gzip.go index cccf79de7..e21205f75 100644 --- a/vendor/github.com/NYTimes/gziphandler/gzip.go +++ b/vendor/github.com/NYTimes/gziphandler/gzip.go @@ -3,6 +3,7 @@ package gziphandler import ( "bufio" "compress/gzip" + "errors" "fmt" "io" "net" @@ -97,6 +98,7 @@ func (w *GzipResponseWriter) Write(b []byte) (int, error) { } // Save the write into a buffer for later use in GZIP responseWriter (if content is long enough) or at close with regular responseWriter. + // On the first write, w.buf changes from nil to a valid slice w.buf = append(w.buf, b...) // If the global writes are bigger than the minSize, compression is enable. @@ -122,7 +124,9 @@ func (w *GzipResponseWriter) startGzip() error { w.Header().Del(contentLength) // Write the header to gzip response. - w.writeHeader() + if w.code != 0 { + w.ResponseWriter.WriteHeader(w.code) + } // Initialize the GZIP response. w.init() @@ -146,14 +150,6 @@ func (w *GzipResponseWriter) WriteHeader(code int) { w.code = code } -// writeHeader uses the saved code to send it to the ResponseWriter. -func (w *GzipResponseWriter) writeHeader() { - if w.code == 0 { - w.code = http.StatusOK - } - w.ResponseWriter.WriteHeader(w.code) -} - // init graps a new gzip writer from the gzipWriterPool and writes the correct // content encoding header. func (w *GzipResponseWriter) init() { @@ -166,19 +162,18 @@ func (w *GzipResponseWriter) init() { // Close will close the gzip.Writer and will put it back in the gzipWriterPool. func (w *GzipResponseWriter) Close() error { - // Buffer not nil means the regular response must be returned. - if w.buf != nil { - w.writeHeader() - // Make the write into the regular response. - _, writeErr := w.ResponseWriter.Write(w.buf) - // Returns the error if any at write. - if writeErr != nil { - return fmt.Errorf("gziphandler: write to regular responseWriter at close gets error: %q", writeErr.Error()) - } - } - - // If the GZIP responseWriter is not set no needs to close it. if w.gw == nil { + // Gzip not trigged yet, write out regular response. + if w.code != 0 { + w.ResponseWriter.WriteHeader(w.code) + } + if w.buf != nil { + _, writeErr := w.ResponseWriter.Write(w.buf) + // Returns the error if any at write. + if writeErr != nil { + return fmt.Errorf("gziphandler: write to regular responseWriter at close gets error: %q", writeErr.Error()) + } + } return nil } @@ -236,12 +231,22 @@ func NewGzipLevelHandler(level int) (func(http.Handler) http.Handler, error) { // NewGzipLevelAndMinSize behave as NewGzipLevelHandler except it let the caller // specify the minimum size before compression. func NewGzipLevelAndMinSize(level, minSize int) (func(http.Handler) http.Handler, error) { + return NewGzipHandler(level, minSize, &GzipResponseWriter{}) +} + +// NewGzipHandler behave as NewGzipLevelHandler except it let the caller +// specify the minimum size before compression and a GzipWriter. +func NewGzipHandler(level, minSize int, gw GzipWriter) (func(http.Handler) http.Handler, error) { if level != gzip.DefaultCompression && (level < gzip.BestSpeed || level > gzip.BestCompression) { return nil, fmt.Errorf("invalid compression level requested: %d", level) } if minSize < 0 { - return nil, fmt.Errorf("minimum size must be more than zero") + return nil, errors.New("minimum size must be more than zero") } + if gw == nil { + return nil, errors.New("the GzipWriter must be defined") + } + return func(h http.Handler) http.Handler { index := poolIndex(level) @@ -249,13 +254,9 @@ func NewGzipLevelAndMinSize(level, minSize int) (func(http.Handler) http.Handler w.Header().Add(vary, acceptEncoding) if acceptsGzip(r) { - gw := &GzipResponseWriter{ - ResponseWriter: w, - index: index, - minSize: minSize, - - buf: []byte{}, - } + gw.SetResponseWriter(w) + gw.setIndex(index) + gw.setMinSize(minSize) defer gw.Close() h.ServeHTTP(gw, r) diff --git a/vendor/github.com/NYTimes/gziphandler/wrapper.go b/vendor/github.com/NYTimes/gziphandler/wrapper.go new file mode 100644 index 000000000..51e532284 --- /dev/null +++ b/vendor/github.com/NYTimes/gziphandler/wrapper.go @@ -0,0 +1,58 @@ +package gziphandler + +import ( + "bufio" + "net" + "net/http" +) + +const ( + contentEncodingHeader = "Content-Encoding" +) + +// ---------- + +// http.ResponseWriter +// http.Hijacker +type GzipWriter interface { + Header() http.Header + Write([]byte) (int, error) + WriteHeader(int) + Hijack() (net.Conn, *bufio.ReadWriter, error) + Close() error + SetResponseWriter(http.ResponseWriter) + setIndex(int) + setMinSize(int) +} + +func (w *GzipResponseWriter) SetResponseWriter(rw http.ResponseWriter) { + w.ResponseWriter = rw +} + +func (w *GzipResponseWriter) setIndex(index int) { + w.index = index +} + +func (w *GzipResponseWriter) setMinSize(minSize int) { + w.minSize = minSize +} + +// -------- + +type GzipResponseWriterWrapper struct { + GzipResponseWriter +} + +func (g *GzipResponseWriterWrapper) Write(b []byte) (int, error) { + if g.gw == nil && isEncoded(g.Header()) { + return g.ResponseWriter.Write(b) + } + return g.GzipResponseWriter.Write(b) +} + +func isEncoded(headers http.Header) bool { + header := headers.Get(contentEncodingHeader) + // According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding, + // content is not encoded if the header 'Content-Encoding' is empty or equals to 'identity'. + return header != "" && header != "identity" +}