Instead of removing all RRs on Truncated, attempt to unpack

[jameshartig]

This Pull Request fixes the TODO(miek): this isn't the best strategy! regarding Truncated messages received. Instead of just returning no RRs it's now attempting to do as many RRs as possible until it gets an error then returning ErrTruncated along with the partial response. This allows someone to naively just check err != nil if they didn't know about truncated messages or someone could check if err == ErrTruncated additionally and proceed with the truncated response if they're okay with a truncated response.

More realistically, I don't think servers will be returning partial responses (like how skydns removes the Extra first), but it supports that. If a non-partial response is returned (that was truncated) then no error is returned. I can make it still return a ErrTruncated but since there was no error unpacking the message, I didn't think it made sense to return an error.

[miekg]

Nice!

If a non-partial response is returned (that was truncated) then no error is returned. I can make it still return a ErrTruncated but since there was no error unpacking the message, I didn't think it made sense to return an error.

So if a server sends a truncated response, but it just happens to fall within an RRset boundery no error is returned? If think in that case we should still return ErrTruncated...

[jameshartig]

I've updated the PR and made it so it will return an ErrTruncated no matter if it ran into an error during RRSets or not. The only exception to this is if it failed to unpack the questions, if any errors occur during unpacking questions, it returns that error.

I've updated the tests to test for both of those cases.

[miekg]

unpackRRslice? Also don't think this needs to be exported.

[jameshartig]

Gotcha, I only exported it because UnpackRR was exported.

[jameshartig]

Okay I just updated the PR and addressed your comments.

[bboreham]

I'm not so sure that erroring on a correctly-decoded response that the server declared to be truncated is correct. Suppose someone is using DNS for load-spreading, and has many machines behind the same name; do you want users of your library to receive an error purely dependent on the value of 'many' compared to the size of their UDP buffer?

[miekg]

But if you don't make it an error you can get the situation where a response happens to be correctly decoded (because the last RR just ended on byte 512 for example), while in other cases the parsing would fail and you would return an error.

I don't understand your example.

[bboreham]

OK, maybe I misunderstand the behaviour of DNS servers; does the TC bit mean the response was arbitrarily truncated at the limit of the buffer, and never that the server truncated the response to return a whole number of RRs that fit in the buffer? RFC1035 doesn't seem to nail it either way.

[miekg]

I'm not sure actually. Think it is left to the server implementation which could do either.

This is one of those times when (DNS) people do the "Let's see what BIND does"

[rade]

From RFC2181

Where TC is set, the partial RRSet that would not completely fit may be left in the response. When a DNS client receives a reply with TC set, it should ignore that response, and query again, using a mechanism, such as a TCP connection, that will permit larger replies.

So clients should not be looking at the contents when TC is set. No idea whether that reflects reality.

[miekg]

A partial RRset means the packet should be completely valid. Dunno... think the current approach is most sensible @bboreham. Willing to drop the if truncated then err, but I think that is more complex than just always returning ErrTruncated.

[rade]

A partial RRset means the packet should be completely valid.

says who?

[miekg]

I think I read that in "the partial RRSet", i.e the RRset is partial, but the RRs in the set are complete.