-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add EDNS0 support #3131
Add EDNS0 support #3131
Conversation
97e31ea
to
71479d4
Compare
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.
LGTM, if we can add another trim test that exercises the EDNS path directly.
command/agent/dns_test.go
Outdated
@@ -4377,16 +4418,16 @@ func TestDNS_Compression_trimUDPResponse(t *testing.T) { | |||
t.Parallel() | |||
config := &DefaultConfig().DNSConfig | |||
|
|||
m := dns.Msg{} | |||
trimUDPResponse(config, &m) | |||
req, m := dns.Msg{}, dns.Msg{} |
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.
Can you add another trim case? This has been a notorious source of subtle bugs so is worth poking a bit.
command/agent/dns.go
Outdated
@@ -525,9 +538,17 @@ func syncExtra(index map[string]dns.RR, resp *dns.Msg) { | |||
// 1035. Enforce an arbitrary limit that can be further ratcheted down by | |||
// config, and then make sure the response doesn't exceed 512 bytes. Any extra | |||
// records will be trimmed along with answers. | |||
func trimUDPResponse(config *DNSConfig, resp *dns.Msg) (trimmed bool) { | |||
func trimUDPResponse(config *DNSConfig, req, resp *dns.Msg) (trimmed bool) { |
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.
Actually, if you pass maxSize
in here instead of req
this would be simpler to test on its own.
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.
That API simplifies the method signature a bit, but it's quite easy to make an empty dns.Msg
. It makes all the calls to trimUDPResponse
a lot longer with more duplication to extract whether it's an edns message and pull out the max size. Each call prior to TrimUDPResponse has to parse out the req, which seemed like a lot of duplication.
This is a refactor of GH-1980. Originally I tried to do a straight rebase, but the code has changed too much.
71479d4
to
171b43e
Compare
This is a refactor of GH-1980. Originally I tried to do a straight rebase, but the code has changed too much.