Repository navigation
fix(server): compress responses larger than 512 bytes - #2213
Merged
Merged
Conversation
Since #2102 the server lets Truncate decide compression, and Truncate clears Compress whenever the message already fits uncompressed. Fitting the buffer the client advertised is not sufficient: that client may be a forwarder relaying our answer to a stub resolver with a smaller buffer, and it can only truncate what it received. A query arriving via such a forwarder carries the forwarder's OPT record, so getMaxResponseSize returns its buffer (1232 or more) rather than 512. An eight-record A answer for a 45-byte name is 562 bytes uncompressed and 202 compressed: both fit 1232, so we now send the uncompressed one, and the forwarder has to truncate it for its non-EDNS0 clients -- typically by dropping every section, leaving them with TC and no answer at all. Before #2102 the same answer went out compressed, fit 512, and was relayed intact. Compress anything that would not fit a bare 512-byte UDP message so it survives that hop. Below that threshold there is nothing to gain, so the compression-map alloc #2102 set out to avoid is still skipped for the short responses that make up the bulk of the traffic. Fixes #2212
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts DNS response compression behavior to improve interoperability with forwarders and non‑EDNS stub resolvers by ensuring responses that exceed the classic 512‑byte UDP size get compressed even if they fit the client-advertised EDNS0 buffer.
Changes:
- Forces
dns.Msg.Compress = truewhen an otherwise-uncompressed response would exceed 512 bytes. - Adds a regression test covering the “fits EDNS0 buffer but exceeds 512 uncompressed” forwarder scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/client_query.go |
Enables compression for responses whose uncompressed size exceeds 512 bytes after truncation to the client buffer. |
server/server_test.go |
Adds a test ensuring responses in the forwarder scenario pack to ≤ 512 bytes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+99
to
+103
| // what it received. Compress anything that wouldn't fit a bare 512-byte UDP message so it | ||
| // survives that hop; below that there is nothing to gain and the compression map costs more | ||
| // than the bytes it would save. | ||
| if !res.Compress && res.Len() > dns.MinMsgSize { | ||
| res.Compress = true |
Comment on lines
+1513
to
+1520
| // guard the scenario: it fits the advertised 1232 uncompressed, so Truncate | ||
| // leaves the answer complete and sees no reason to compress. | ||
| Expect(resp.Res.Truncated).Should(BeFalse()) | ||
| Expect(resp.Res.Answer).Should(HaveLen(20)) | ||
|
|
||
| packed, err := resp.Res.Pack() | ||
| Expect(err).Should(Succeed()) | ||
| Expect(len(packed)).Should(BeNumerically("<=", dns.MinMsgSize)) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2213 +/- ##
==========================================
+ Coverage 87.98% 87.99% +0.01%
==========================================
Files 126 126
Lines 9878 9880 +2
==========================================
+ Hits 8691 8694 +3
Misses 924 924
+ Partials 263 262 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…gful Assert the answer really exceeds 512 bytes uncompressed, so shrinking the RR set can't leave the test passing without exercising the scenario.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2212.
Since #2102 the server lets
Truncatedecide compression, andTruncateclearsCompresswhenever the message already fits uncompressed. Fitting the buffer the client advertised turns out not to be sufficient: that client may be a forwarder relaying our answer to a stub resolver with a smaller buffer, and it can only truncate what it received.A query arriving through such a forwarder carries the forwarder's OPT record, so
getMaxResponseSizereturns its buffer (1232 or more) rather than 512. For the reporter's name, eight A records are 562 bytes uncompressed and 202 compressed — both fit 1232, so we send the uncompressed one, and the forwarder has to truncate it for its non-EDNS0 clients. Most do that by dropping every section, so the client is left with TC and no answer at all. Before #2102 the same response went out compressed, fit in 512, and was relayed intact.Compressing anything that wouldn't fit a bare 512-byte UDP message restores that, while keeping #2102's optimization where it pays: short responses still skip the compression map.
Reproduction
dig +noedns→ dnsmasq (add-subnet=32,128, forwarding to blocky) → blocky → upstream returning 8 A records:Both match the reporter's v0.34 and v0.31 output byte for byte.
Also fixed
The same change bounds response size in general. Sixty A records, client advertising 4096:
A 3734-byte UDP response fragments on a 1500-byte MTU path, and it raises the amplification factor for anything reachable from outside.
Tests
New case in
server_test.gounderresponse compression: a response that fits the advertised 1232-byte buffer uncompressed but exceeds 512 must pack to 512 bytes or less, with the answer section intact. It fails onmainand passes here. The two existing cases from #2102 are unchanged — a single-A-record response still goes out uncompressed.go test ./server/... ./resolver/... ./util/...is green.