Compress all DNS responses #971

epipho · 2015-05-22T23:06:40Z

No description provided.

limit from RFC 1035

ryanbreen · 2015-05-22T23:08:47Z

Could we get some tests for this?

armon · 2015-05-23T00:16:53Z

This requires a lot more testing. In my experience before, the DNS library would cause corruption issues when compression was enabled.

grobie · 2015-05-23T01:53:44Z

I'd be very interested to hear more about the corruption issues you've seen in order to test for such cases in our infrastructure. Can you remember under which circumstances these might happen? We're using miekg/dns as well for our DNS server implementation (on top of consul) and to my knowledge haven't had any issues so far with compression.

epipho · 2015-05-23T05:00:38Z

Sorry about that, meant to open a local PR between branches. Tests will be coming soon.

I have been running a good number of manual tests so far and have seen no hint of corruption. The example given in #854 compresses identically to the result returned by the 8.8.8.8 DNS server. Do you know of any specific examples that caused corruption in the past? It may also only be necessary to enable compression on the recursor queries, leaving it disabled for all other response messages.

.

armon · 2015-05-27T18:52:51Z

@grobie @epipho It's been a while... But if memory serves, the compression issue hit when doing recursion for clients. The actual responses we generate didn't cause issues, but I think some of the more edge case record types did. Never saw it with A, AAAA, CNAME or SRV.

grobie · 2015-05-29T01:34:31Z

Thanks Armon. We don't do any recursion on our service discovery DNS
servers (yet), good to know.
On May 27, 2015 2:52 PM, "Armon Dadgar" [email protected] wrote:

@grobie https://github.com/grobie @epipho https://github.com/epipho
It's been a while... But if memory serves, the compression issue hit when
doing recursion for clients. The actual responses we generate didn't cause
issues, but I think some of the more edge case record types did. Never saw
it with A, AAAA, CNAME or SRV.

—
Reply to this email directly or view it on GitHub
#971 (comment).

discordianfish · 2015-06-04T19:01:04Z

Is there a ETA for getting this merged and release? See my comment on #854 (comment) - it's IMO really severe issue.

sstarcher · 2015-06-05T02:06:34Z

@discordianfish I build consul 0.5.2 with this patch and removed my "search ec2.internal" from /etc/resolv.conf along with setting only a single nameserver of 127.0.0.1.

Even with compression turned on I still get "FATA[0000] Get https://registry-1.docker.io/v1/repositories/library/busybox/tags: dial tcp: lookup registry-1.docker.io on 127.0.0.1:53: cannot unmarshal DNS message" from docker

discordianfish · 2015-06-05T10:15:52Z

@sstarcher Yes, looks like the responses are still too large. This PR doesn't fix the actual issue, it just makes it less likely to hit it.

epipho · 2015-06-05T14:32:04Z

I have not been able to craft a decent test that validates this fix as I cannot get the test DNS server to reliably return a response that is too large. We have been using it without issues but it sounds like @sstarcher still sees some.

@discordianfish do you believe miekg/dns#216 will fix that issue as well?

discordianfish · 2015-06-05T14:35:43Z

Oh I missed that, I tried the fix and it worked for me - at least for my test tool.
@sstarcher Are you sure you used the patched version?

Beside that, I'd consider this a bug in miekg/dns (see the issue I opened). It seems idiomatic to write the dns message received from Exchange() to the client, but since miekg/dns doesn't set the compress flag even if it was compressed, it yields a message to large.

sstarcher · 2015-06-05T14:59:10Z

@discordianfish I'm sure I compiled turbine's compress-dns branch
"./command/agent/dns.go: m.Compress = true
./command/agent/dns.go: m.Compress = true
./command/agent/dns.go: r.Compress = true
./command/agent/dns.go: m.Compress = true
"

I also checked the version of my binary it was something along the lines of 0.5.2.#####

I will re-attempt on monday.

discordianfish · 2015-06-05T15:08:14Z

@sstarcher I just confirmed that it fixes the problems for me: master is broken but with this branch it works.

sstarcher · 2015-06-08T14:24:46Z

@discordianfish I again rebuilt it using the compress-dns branch from turbine and I still get
FATA[0000] Get https://registry-1.docker.io/v1/repositories/library/busybox/tags: dial tcp: lookup registry-1.docker.io on 127.0.0.1:53: cannot unmarshal DNS message

root@ip-10-0-10-59:~/.gvm/pkgsets/go1.4/global/src/github.com/turbine/consul# ./bin/consul version
Consul v0.5.2-13-g08b7313
Consul Protocol: 3 (Understands back to: 1)
root@ip-10-0-10-59:~/.gvm/pkgsets/go1.4/global/src/github.com/turbine/consul# ./bin/consul info
WARNING: It is highly recommended to set GOMAXPROCS higher than 1

agent:
    check_monitors = 0
    check_ttls = 0
    checks = 0
    services = 3
build:
    prerelease =
    revision = 08b7313d
    version = 0.5.2

discordianfish · 2015-06-08T16:55:54Z

@sstarcher That's weird.. Maybe you can provide a tcpdump?

discordianfish · 2015-06-08T17:01:35Z

@miekg suggests to set the compress flag in consul, not wait for him to change this upstream.
@armon Can we get this merged soon? Even if it's not fixing the issue for all, it's definitely a pretty clear bug which breaks everyone using docker and consul as recursor.

sstarcher · 2015-06-08T17:23:01Z

I edited my last post I should've said it was the unmarshall error again not the no such host error.

Here is a tcpdump of traffic on 53

17:22:09.017814 IP 10.0.10.4.domain > ip-10-0-10-199.node.us-east-1.consul.52227: 24183 3/0/0 A 10.0.20.188, A 10.0.10.15, A 10.0.20.239 (89)
17:22:09.017856 IP 10.0.10.4.domain > ip-10-0-10-199.node.us-east-1.consul.52227: 24457 0/0/0 (41)
17:22:11.012427 IP ip-10-0-10-199.node.us-east-1.consul.59108 > 10.0.0.2.domain: 9408+ A? registry-1.docker.io. (38)
17:22:11.012528 IP ip-10-0-10-199.node.us-east-1.consul.56231 > 10.0.0.2.domain: 39053+ AAAA? registry-1.docker.io. (38)
17:22:11.013844 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.59108: 9408 7/0/0 CNAME us-east-1-elbio-rm5bon1qaeo4-623296237.us-east-1.elb.amazonaws.com., A 54.172.137.222, A 107.21.22.134, A 52.0.31.125, A 52.5.4.145, A 52.6.136.158, A 54.164.219.90 (214)
17:22:11.014081 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.56231: 39053 1/1/0 CNAME us-east-1-elbio-rm5bon1qaeo4-623296237.us-east-1.elb.amazonaws.com. (200)
17:22:11.014445 IP ip-10-0-10-199.node.us-east-1.consul.59633 > 10.0.0.2.domain: 35794+ A? registry-1.docker.io. (38)
17:22:11.014817 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.59633: 35794 7/0/0 CNAME us-east-1-elbio-rm5bon1qaeo4-623296237.us-east-1.elb.amazonaws.com., A 54.164.219.90, A 54.172.137.222, A 107.21.22.134, A 52.0.31.125, A 52.5.4.145, A 52.6.136.158 (214)
17:22:11.015868 IP ip-10-0-10-199.node.us-east-1.consul.40609 > 10.0.0.2.domain: 21181+ A? index.docker.io. (33)
17:22:11.015943 IP ip-10-0-10-199.node.us-east-1.consul.59027 > 10.0.0.2.domain: 38952+ AAAA? index.docker.io. (33)
17:22:11.016956 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.59027: 38952 0/1/0 (103)
17:22:11.017117 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.40609: 21181 1/0/0 A 162.242.195.84 (49)
17:22:11.144753 IP ip-10-0-10-199.node.us-east-1.consul.56472 > 10.0.0.2.domain: 11298+ A? registry-1.docker.io. (38)
17:22:11.145042 IP ip-10-0-10-199.node.us-east-1.consul.48252 > 10.0.0.2.domain: 10235+ AAAA? registry-1.docker.io. (38)
17:22:11.145112 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.56472: 11298 7/0/0 CNAME us-east-1-elbio-rm5bon1qaeo4-623296237.us-east-1.elb.amazonaws.com., A 52.6.136.158, A 54.164.219.90, A 54.172.137.222, A 107.21.22.134, A 52.0.31.125, A 52.5.4.145 (214)
17:22:11.145408 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.48252: 10235 1/1/0 CNAME us-east-1-elbio-rm5bon1qaeo4-623296237.us-east-1.elb.amazonaws.com. (200)
17:22:11.145646 IP ip-10-0-10-199.node.us-east-1.consul.59490 > 10.0.0.2.domain: 25933+ A? registry-1.docker.io. (38)
17:22:11.145967 IP 10.0.0.2.domain > ip-10-0-10-199.node.us-east-1.consul.59490: 25933 7/0/0 CNAME us-east-1-elbio-rm5bon1qaeo4-623296237.us-east-1.elb.amazonaws.com., A 52.5.4.145, A 52.6.136.158, A 54.164.219.90, A 54.172.137.222, A 107.21.22.134, A 52.0.31.125 (214)

miekg · 2015-06-08T18:27:41Z

I don't really think this is a bug in go dns. Compressing a message is basically a "per-client-connection' decision. You should check the client's EDNS0 buffer size and the message size (the Len() function) to see if "it fits". It not set Compress to true (or just do this always). If it still does not fit set the TC bit (when transport != UDP)

ryanuber · 2015-06-24T22:49:51Z

command/agent/dns.go

@@ -625,6 +627,7 @@ func (d *DNSServer) handleRecurse(resp dns.ResponseWriter, req *dns.Msg) {
 	var err error
 	for _, recursor := range d.recursors {
 		r, rtt, err = c.Exchange(req, recursor)
+		r.Compress = true


This should probably be moved after the error check, because r may be nil in that case and cause a nil pointer dereference.

epipho · 2015-06-24T23:23:20Z

Hello everyone. Just fixed a silly mistake nil pointer crash in 04bba18 where we were accessing the response returned by exchange before we had checked the error. If a DNS recursion failed, it would cause consul to panic and crash.

If you are using this patch, please upgrade immediatly.

epipho · 2015-06-25T04:46:01Z

@ryanuber awesome, it looks like you reviewed and posted literally right as we were discovering that issue.

ryanuber · 2015-08-20T17:35:55Z

Just checking in here - I think we can get this in if some tests could be crafted to ensure that we are actually compressing the messages. I also think it would be worthwhile to make the adjustments @miekg suggests rather than just compressing everything and hoping the messages fit into the buffer. This would also ease the hesitation, since most messages should be small enough to fit without compression.

epipho · 2015-08-20T18:41:39Z

I will look into both after I get back from vacation on 9/1.

slackpad · 2016-01-15T01:04:45Z

Hi @epipho checking in on this one - is there still interest in getting this merged?

sstarcher · 2016-01-15T01:07:28Z

I'm still interested as I had to stop using consul as the direct DNS due to this and another issue.

epipho · 2016-01-27T15:21:50Z

@slackpad indeed. Sorry I had github notifications turned off and didn't see it. I just merged 0.6.3 into my fork and am doing testing to make sure there are no regressions.

jordan-acosta · 2016-02-17T21:07:56Z

@epipho , found this PR after running into this problem at work. Wondering if this will be merged soon. Is there anything I can do to help push this along?

This is kind of an edge case for us (only happens in our dev environment, and we're not sure why,) but it would make our lives a lot easier if this were merged?

jordan-acosta · 2016-02-17T23:10:52Z

It doesn't look like this change checks the length of the response. Is it possible that, even with the compression, it could still exceed 512 bytes?

slackpad · 2016-03-09T07:26:26Z

I'm going to close this out in favor of #1734 which was just merged. It directly enforces the UDP size limit which I think was the root problem here, and avoids any extra client burden / complexity around compression. I think we'd need to enforce the limit even if we did enable compression, so this is a good first step, even if compression is warranted later. It doesn't seem necessary, though, because of random shuffling, even getting 1 or 2 responses back instead of the usual 3 should work fine in most cases.

epipho · 2016-03-11T05:22:29Z

I don't believe #1734 addresses all the cases. It looks like it will address a service lookup returning a response that is too large but will still fail if an upstream DNS server returns a large response. We found this happens when querying large ELBs (10+ A records sometimes) where the upstream DNS server will send a compressed UDP response (that is under 512 bytes) but Consul will decompress it before forwarding it on, making it too large for Go to accept.

slackpad · 2016-03-11T05:26:21Z

Ah interesting - I'll reopen so we can look at that case as well.

slackpad · 2016-03-11T05:38:00Z

Since Consul-generated messages should be sized by the current fix perhaps we should just compress in the forwarding case if the incoming message is also compressed.

epipho · 2016-03-11T05:53:39Z

Seems reasonable. I was also thinking we may want to do some of the following to ensure correctness / usefulness in as many cases as possible.

Pass through the truncation flag from the incoming message regardless of the value of enable_truncate Since we don't know what the upstream DNS server is doing it could cause undesirable behavior to silently drop this flag.
Compress udp messages that come in that are already over 512 bytes (non-conforming server?)
Use the trim method from Make sure UDP DNS responses aren't larger than allowed #1734 to trim records / set truncation if the compressed size is still over. Not sure on this one, could violate the principle of least surprise.

alexjh · 2016-05-31T17:37:14Z

@slackpad I've been doing some testing as I'm hitting this same docker + consul bug. I don't think the DNS library makes the compressed state of the incoming message available.

miekg · 2016-05-31T17:45:42Z

No it doesn't because compression is hop by hop so it doesn't make sense to
relay this when you are a proxy.

I don't get why consul just compresses everything. Compression has been in
the DNS standard since 1984.
On 31 May 2016 7:37 p.m., "Alex Harford" [email protected] wrote:

@slackpad https://github.com/slackpad I've been doing some testing as
I'm hitting this same docker + consul bug. I don't think the DNS library
makes the compressed state of the incoming message available.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#971 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAVkW_3zhBkwprX1sN0VSn7gdJW6QZhVks5qHHHigaJpZM4Elqx3
.

alexjh · 2016-05-31T22:34:44Z

@epipho:

Compress udp messages that come in that are already over 512 bytes (non-conforming server?)

Like miekg said, it makes sense to compress everything. I think that would be a good start and would fix the docker bug that a lot of people are seeing.

@ryanuber: I don't think tests for compression are needed as that's already covered by the tests in the dns library.

slackpad · 2016-06-01T00:11:59Z

I'd be ok with compressing everything now that we do have the trim fix for the responses being too large (compression wasn't a true fix for that). We could add a config option to turn off compression for people with obscure setups who might be affected by the default change, but default it to on.

beneshed · 2016-06-19T16:41:57Z

is there a work around?

Compress all DNS responses to prevent them from going over the 512 byte

08b7313

limit from RFC 1035

epipho mentioned this pull request May 26, 2015

Compress all DNS responses turbine/consul#1

Merged

discordianfish mentioned this pull request Jun 5, 2015

Exchange() transparently uncompresses message without compress flag set miekg/dns#216

Closed

discordianfish mentioned this pull request Jun 10, 2015

Add protocol (re)negotiation for automated upgrades #1017

Closed

cjhubert mentioned this pull request Jun 14, 2015

go 1.4.2 cannot parse some dns responses from consul #854

Closed

divideandconquer mentioned this pull request Jun 23, 2015

Panic on DNS lookup #1023

Closed

ryanuber reviewed Jun 24, 2015
View reviewed changes

Fix panic where r.Compress would be set before checking for errors.

04bba18

epipho force-pushed the compress-dns branch from ea4ab10 to 04bba18 Compare June 24, 2015 23:19

This was referenced Feb 18, 2016

Make sure UDP DNS responses aren't larger than allowed #1733

Closed

Make sure UDP DNS responses aren't larger than allowed #1734

Closed

slackpad closed this Mar 9, 2016

slackpad reopened this Mar 11, 2016

slackpad added the type/bug Feature does not function as expected label Mar 18, 2016

slackpad self-assigned this Mar 18, 2016

alexjh mentioned this pull request Jun 6, 2016

Compress all DNS responses #2096

Closed

slackpad mentioned this pull request Aug 11, 2016

Compresses all DNS responses by default. #2266

Merged

slackpad closed this in #2266 Aug 11, 2016

AaronFriel mentioned this pull request Feb 11, 2022

net: 512 byte DNS response size limit causes "cannot unmarshal DNS" error golang/go#51127

Closed

ekmixon mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 ekmixon/consul#516

Open

Compress all DNS responses #971

Compress all DNS responses #971

Conversation

epipho commented May 22, 2015

ryanbreen commented May 22, 2015

armon commented May 23, 2015

grobie commented May 23, 2015

epipho commented May 23, 2015

armon commented May 27, 2015

grobie commented May 29, 2015

discordianfish commented Jun 4, 2015

sstarcher commented Jun 5, 2015

discordianfish commented Jun 5, 2015

epipho commented Jun 5, 2015

discordianfish commented Jun 5, 2015

sstarcher commented Jun 5, 2015

discordianfish commented Jun 5, 2015

sstarcher commented Jun 8, 2015

discordianfish commented Jun 8, 2015

discordianfish commented Jun 8, 2015

sstarcher commented Jun 8, 2015

miekg commented Jun 8, 2015

ryanuber Jun 24, 2015

Choose a reason for hiding this comment

epipho commented Jun 24, 2015

epipho commented Jun 25, 2015

ryanuber commented Aug 20, 2015

epipho commented Aug 20, 2015

slackpad commented Jan 15, 2016

sstarcher commented Jan 15, 2016

epipho commented Jan 27, 2016

jordan-acosta commented Feb 17, 2016

jordan-acosta commented Feb 17, 2016

slackpad commented Mar 9, 2016

epipho commented Mar 11, 2016

slackpad commented Mar 11, 2016

slackpad commented Mar 11, 2016

epipho commented Mar 11, 2016

alexjh commented May 31, 2016

miekg commented May 31, 2016

alexjh commented May 31, 2016

slackpad commented Jun 1, 2016

beneshed commented Jun 19, 2016