potential 2.0.0 incompatibility: message.from_wire()

[pspacek]

I will attempt to list incompatibilities found while testing master branch (to-be-2.0.0).

This does not mean it has to be fixed, but we should think about each on case-by-case basis.

[pspacek]

• dns.message.from_wire raises dns.message.Truncated exception - formerly it just returned the message and it was up to the higher layers to decide what to do with it

I think the old behavior made more sense because there are use-cases where TC=1 answers need to be analyzed beyond "TC=1 is present".

[rthalley]

Which use cases are you thinking of? Also, if the authority has truncated in the bad way, e.g. in the middle of something, from_wire() would still be likely to raise an exception. More dangerous are the cases where the authority has created a partial RRset.

Perhaps we should have a raise_on_truncation flag to from_wire() with a default of True, and possibly to dns.query.udp()? Then people who wanted to do extra processing of a TC message, could but the normal use case would be safer. I think in typical use cases, if we don't raise by default then people will probably forget to check TC and possibly get a partial RRset without knowing it.

[pspacek]

Use-cases: Integration and interoperability testing for DNS software:

• https://gitlab.labs.nic.cz/knot/deckard
• https://gitlab.labs.nic.cz/knot/respdiff
  Both projects need to parse UDP message as it arrived and compare with expected answer from upstream.

New flag is fine with me, if you are okay with breaking API. Personally I consider this more invasive change than removing search list processing :-)

[pspacek]

Thinking about it a bit more, I think that for query.udp() interface it is expected that it returns the message as it arrived, while resolve() interface should handle truncation internally and return complete answer.

[rthalley]

As #284 shows, it's not a simple thing if query.udp() "returns the message as it arrived" as from_wire of a TC'd message may itself cause exceptions, and if we let those exceptions escape it messes up the resolver and also prevents the caller from understanding the "truncatedness" of the response. I suppose we could have a "raise_on_truncated" default=False flag to dns.query.udp() and dns.message.from_wire(), and then have the resolver set it to True. That would be backwards compatible and still solve the problem for the resolver, but it's still really the wrong default for most uses.

I considered making from_wire smarter, but this is hard as the code currently has no way to tell the difference between a FormError caused by truncation and one caused by other badness. It might be possible to fix that, but I'm not certain how hard it would be.

I suppose if we adopt "you must ask for the Truncated" exception now, that leaves us room to get smarter in the future.

[pspacek]

Okay, now I can see the concern. The way how we handle this kind of mess in Knot DNS is that we parse header first and only if it seems "sensible enough" we continue parsing the rest.

That might be one option how to handle TC bit in resolver code, but it might be too convoluted and we could stick to flag in from_wire() as you suggest.

Again, I'm okay with making it incompatible change and going ahead with safe default, i.e. raise_on_truncated=True. I just wanted to highlight it so we can consciously decide we are okay with breaking this particular API.

[pspacek]

Oh wait, I have even better idea ;-)

What about providing wrapper function, something like query.udp_fallback_tcp() which does UDP query, checks TC bit and retries if necessary? It can be very dumb and just bitwise-check TC bit before calling from_wire().

I've coded exactly this function several times already in various projects... and apparently #370 (comment) I'm not the only one who could use it.

If we had this udp_fallback_tcp wrapper it could return DNSMessage + flag indicating if it came over UDP or TCP. That would perfectly match what is needed in resolver and also would be useful elsewhere.

[rthalley]

Just brainstorming here, but another thing we could do is put the partially parsed response into the truncated exception. Basically

# parse header here; if not ok there is no hope of anything useful so just raise FormError
try:
    # parse the rest of the message
except Exception:
    if TC bit was set:
         raise Truncated(response)
    else:
         raise

This is still incompatible, but is safe and information rich, in that we are returning as much info as we could get from the message in the Truncated case. It also doesn't require any extra flags, which is nice. A slight drawback is that it doesn't differentiate between "we saw a proper FormError before we got to the part that was truncated" and "we saw a FormError due to truncation". I don't think that matters much as that's unlikely to happen and retry on TC in such a case is pretty harmless.

[rthalley]

Your wrapper idea could be convenient, but it doesn't change the inherent danger of partial RRsets in all the code that's doing dns.query.udp() and not checking TC or falling back to TCP. No idea how much code that is, but...

[pspacek]

Well, all the old code has the problem for years now and either dealt with it somehow or never saw truncated answer. For new code we can simply document "you should use udp_fallback_tcp unless you want to see partial answers", which is I believe sufficient.

[rthalley]

I meant to make a branch with proposed changes and submit a PR but I goofed with git and pushed it to master, so everyone can review it there. If we hate it, I will revert.

[rthalley]

Summarizing:

Default behavior is same as before 1.16.0
If you ask for a truncation exception, it will give you one and include as much of the message as was successfully parsed.
The resolver asks for truncation
udp_with_fallback() helper was added

I haven't changed the resolver to use udp_with_fallback yet

[pspacek]

TC handling now seems to work on acc5693. I'm going to close this and open a new issue for other incompatibilities.