Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add compression handler to server handler #5433

Closed
wants to merge 2 commits into from
Closed

feat: add compression handler to server handler #5433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e01386e
add compression handler to server handler
burnerlee Dec 2, 2022
898f461
update tests to check for gzip compression
burnerlee Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ require (
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/snappy v0.0.4 // indirect
github.com/google/flatbuffers v1.12.1 // indirect
github.com/gorilla/handlers v1.5.1 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that these were recently archived, I'm wondering what other options would be.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about just include the handlers internally I see the implementation of gorilla/handlers is not dependent on another gorilla package https://github.com/gorilla/handlers/blob/v1.5.1/compress.go#L63

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a possibility. Sorry there are so many things in flight here right now, please note #5483.

github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.0 // indirect
github.com/inconshreveable/mousetrap v1.0.1 // indirect
github.com/klauspost/compress v1.13.6 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ github.com/envoyproxy/go-control-plane v0.9.9-0.20210512163311-63b5d3c536b0/go.m
github.com/envoyproxy/go-control-plane v0.9.10-0.20210907150352-cf90f659a021/go.mod h1:AFq3mo9L8Lqqiid3OhADV3RfLJnjiw63cSpi+fDTRC0=
github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
github.com/felixge/httpsnoop v1.0.2 h1:+nS9g82KMXccJ/wp0zyRW9ZBHFETmMGtkk+2CTTrW4o=
github.com/felixge/httpsnoop v1.0.2/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
github.com/fortytw2/leaktest v1.3.0 h1:u8491cBMTQ8ft8aeV+adlcytMZylmA5nnwwkRZjI8vw=
Expand Down Expand Up @@ -237,6 +238,7 @@ github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+
github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+vpHVxEJEs9eg=
github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
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.8.0 h1:i40aqfkR1h2SlN9hojwV5ZA91wcXFOvkdNIeFDP5koI=
github.com/gorilla/mux v1.8.0/go.mod h1:DVbg23sWSpFRCP0SfiEN6jmj59UnW/n46BH5rLB71So=
github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
Expand Down
27 changes: 20 additions & 7 deletions runtime/logging_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
package runtime

import (
"bytes"
"compress/gzip"
"context"
"io"
"net/http"
"net/http/httptest"
"net/url"
Expand Down Expand Up @@ -99,21 +102,22 @@ func TestRequestLogging(t *testing.T) {
<-initChannel

tests := []struct {
path string
acceptEncoding string
expected string
path string
acceptEncoding string
expected string
contentEncoding string
}{
{
"/metrics", "gzip", "[compressed payload]",
"/metrics", "gzip", "HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.", "gzip",
},
{
"/metrics", "*/*", "HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.", // rest omitted
"/metrics", "*/*", "HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.", "", // rest omitted
},
{ // accept-encoding does not matter for "our" handlers -- they don't compress
"/v1/data", "gzip", "{\"result\":{}}",
"/v1/data", "gzip", "{\"result\":{}}", "gzip",
},
{ // accept-encoding does not matter for pprof: it's always protobuf
"/debug/pprof/cmdline", "*/*", "[binary payload]",
"/debug/pprof/cmdline", "*/*", "[binary payload]", "",
},
}

Expand All @@ -129,6 +133,9 @@ func TestRequestLogging(t *testing.T) {
if exp, act := http.StatusOK, rec.Result().StatusCode; exp != act {
t.Errorf("GET %s: expected HTTP %d, got %d", tc.path, exp, act)
}
if exp, act := tc.contentEncoding, rec.Result().Header.Get("Content-Encoding"); exp != act {
t.Errorf("GET %s: expected HTTP Response Header %s, got %s", tc.path, exp, act)
}
}

cancel()
Expand All @@ -141,6 +148,12 @@ func TestRequestLogging(t *testing.T) {
for _, ent := range entriesForReq(ents, i) {
if ent.Message == "Sent response." {
act := ent.Fields["resp_body"].(string)
if tc.acceptEncoding == "gzip" {
reader := bytes.NewReader([]byte(act))
gzreader, _ := gzip.NewReader(reader)
output, _ := io.ReadAll(gzreader)
act = string(output)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adjusts the test, but if you're running opa with

opa run --server --log-level debug

will it spill gzip binary data into your logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes if the client sends the request with Accept-Encoding header set to gzip, then it would do so. But isn't it the intended behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 spewing binary data into the logs is not the intended behaviour. For the metrics endpoint, we'll log "binary payload" instead -- this is what the test was about. For the data endpoints, I think we should include the decompressed data in the debug logs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose decompressing what we've previously compressed to log it is also kind of weird, and an indicator that logging should be wired differently. 🤔

}
if !strings.Contains(act, tc.expected) {
t.Errorf("expected %q in resp_body field, got %q", tc.expected, act)
}
Expand Down
6 changes: 6 additions & 0 deletions server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"sync"
"time"

ghandlers "github.com/gorilla/handlers"
"github.com/gorilla/mux"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
Expand Down Expand Up @@ -184,6 +185,7 @@ func (s *Server) Init(ctx context.Context) (*Server, error) {

// authorizer, if configured, needs the iCache to be set up already
s.Handler = s.initHandlerAuth(s.Handler)
s.Handler = s.initHandlerCompression(s.Handler)
s.DiagnosticHandler = s.initHandlerAuth(s.DiagnosticHandler)

return s, s.store.Commit(ctx, txn)
Expand Down Expand Up @@ -657,6 +659,10 @@ func (s *Server) initHandlerAuth(handler http.Handler) http.Handler {
return handler
}

func (s *Server) initHandlerCompression(handler http.Handler) http.Handler {
return ghandlers.CompressHandler(handler)
}

func (s *Server) initRouters() {
mainRouter := s.router
if mainRouter == nil {
Expand Down
22 changes: 22 additions & 0 deletions vendor/github.com/gorilla/handlers/LICENSE
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 56 additions & 0 deletions vendor/github.com/gorilla/handlers/README.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

74 changes: 74 additions & 0 deletions vendor/github.com/gorilla/handlers/canonical.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.