DrFaust92 opened a new pull request, #1115:
URL: https://github.com/apache/yunikorn-core/pull/1115

   > **Note:** JIRA pending — I don't have an ASF JIRA account. Happy to 
retitle to `[YUNIKORN-XXXX] ...` once a ticket is filed (or if a committer 
files one). Opening as a draft for that reason; the change itself is complete 
and tested.
   
   ## What is this PR for?
   
   The gzip middleware added to the REST router in 1.9.0 compresses any 
response whose buffered body exceeds `minCompressionSize`, without checking 
whether the wrapped handler had already encoded the body itself.
   
   `/ws/v1/metrics` is served by `promhttp.Handler()`, which performs its 
**own** gzip content negotiation: when the client sends `Accept-Encoding: gzip` 
it compresses the payload and sets `Content-Encoding: gzip`. The middleware 
then sees the same `Accept-Encoding`, buffers promhttp's already-gzipped bytes, 
finds them over the threshold, and compresses a second time. `switchToGzip` 
uses `Header().Set`, so the response still advertises a single 
`Content-Encoding: gzip` while carrying a doubly encoded body.
   
   Clients decode once and get gzip framing instead of the payload. For 
Prometheus this means **every scrape of `/ws/v1/metrics` fails to parse and the 
target is marked down** — scheduler metrics are lost entirely for any scraper 
that advertises gzip support, which is the default.
   
   Reproduced against a running 1.9.0 scheduler:
   
   ```console
   $ curl -s -D- -o body.bin http://scheduler:9080/ws/v1/metrics
   200 OK, 69714 bytes, "# HELP go_gc_duration_seconds ..."
   
   $ curl -s -D- -o body.gz -H 'Accept-Encoding: gzip' 
http://scheduler:9080/ws/v1/metrics
   200 OK, Content-Encoding: gzip, 8839 bytes
   
   $ gunzip -c body.gz | file -
   /dev/stdin: gzip compressed data          # <-- still gzip after one decode
   
   $ gunzip -c body.gz | gunzip -c | head -1
   # HELP go_gc_duration_seconds ...         # <-- payload only after two
   ```
   
   ## What type of PR is it?
   
   Bug Fix
   
   ## Todos
   
   - [ ] File JIRA and retitle
   
   ## What is the Jira issue?
   
   Pending — see note above.
   
   ## How should this be tested?
   
   `go test ./pkg/webservice/`
   
   Adds `pkg/webservice/gzip_test.go`, which did not exist before. 
`TestCompressResponseSelfEncodedHandler` is the regression test — it fails on 
current `master` with the extra gzip framing visible in the diff, and passes 
with the fix:
   
   ```
   --- FAIL: TestCompressResponseSelfEncodedHandler (0.02s)
       gzip_test.go:109: assertion failed:
           --- payload
           +++ →
             []uint8{
           +    0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, ...
                ... // 16320 identical bytes
           +    0x01, 0x00, 0x00, 0xff, 0xff, 0xdc, 0x26, 0x2b, 0x3c, 0x00, 
0x40, 0x00, 0x00,
             }
   ```
   
   The self-encoded fixture is deliberately high-entropy. This matters: a 
compressible body gzips down *below* `minCompressionSize`, the middleware never 
commits to a second pass, and the test would pass against the very bug it is 
meant to catch. The helper asserts its own gzipped size exceeds the threshold 
so the fixture cannot silently regress into being vacuous.
   
   The remaining tests cover the pre-existing paths (compress over threshold, 
pass through under threshold, no gzip requested, status-code preservation) and 
`clientAcceptsGzip`.
   
   `go vet`, `gofmt`, `make license-check` and the full `pkg/webservice` suite 
are clean.
   
   ## Fix
   
   Treat a `Content-Encoding` set by the handler as a signal that the body is 
already encoded, and pass the response through untouched. The buffered bytes 
are flushed verbatim via a new `passThrough` helper, which `finalize` now 
reuses for the below-threshold path.
   
   This keeps metrics responses compressed — promhttp still gzips them, just 
once — and leaves the behaviour of every handler that does not set 
`Content-Encoding` unchanged.
   
   An alternative would be to pass `promhttp.HandlerOpts{DisableCompression: 
true}` in `getMetrics`, but that only patches the one endpoint that happens to 
self-encode today; the middleware would still corrupt any future handler that 
does the same. Fixing the middleware seemed like the right layer.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to