-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
feat: add compression handler to server handler #5433
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,10 @@ | |
package runtime | ||
|
||
import ( | ||
"bytes" | ||
"compress/gzip" | ||
"context" | ||
"io" | ||
"net/http" | ||
"net/http/httptest" | ||
"net/url" | ||
|
@@ -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]", "", | ||
}, | ||
} | ||
|
||
|
@@ -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() | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adjusts the test, but if you're running opa with
will it spill gzip binary data into your logs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes if the client sends the request with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.