EDNS option backwards compatibility

[cdeccio]

Thanks for dnspython! I like the addition of the new EDNS option classes, like CookieOption and NSIDOption. However, code that uses earlier versions of dnspython relied on GenericOption and thus used the data member. With the introduction of the more specific option classes, messages now are populated with these more specific options, and there is no data member, so the code fails. For example, consider the following "legacy" code:

import dns.edns
import dns.message
import dns.query
msg = dns.message.make_query('example.com', 'A', use_edns=0, options=[dns.edns.GenericOption(otype=3, data=b'')])
response = dns.query.udp(msg, '8.8.8.8')
nsid_opt = [o for o in response.options if o.otype == dns.edns.NSID][0]
print(nsid_opt.data)

When I run it with dnspython < 2.6, it runs just fine. But when I run it with dnspython >= 2.6, this is what happens:

Traceback (most recent call last):
    print(nsid_opt.data)
          ^^^^^^^^^^^^^
AttributeError: 'NSIDOption' object has no attribute 'data'

See dnsviz/dnsviz#151 and dnsviz/dnsviz#134

I'm having to hack these up with code like this:

        # Backwards compatibility with dnspython < 2.6
        if hasattr(dns.edns, 'NSIDOption'):
            # dnspython >= 2.6 with NSIDOption
            data = nsid_opt.nsid
        else:
            # dnspython < 2.6 with Generic Option
            data = nsid_opt.data

So does it make sense to leave some backwards compatibility, and/or was this part of the design? What do you recommend?

[rthalley]

I honestly didn't think about this aspect of backwards compatibility, so sorry! Though this is kind of an inherent problem with new options and new rdata types. And while we could maybe hack things by letting data refer to raw bytes, I think code would still be fragile (e.g. if we output the proper rdata text format for a new type and something expected generic format).

So I'm not sure how to spare code that is supporting types before dnspython does, though we could certainly make the working-around easier. E.g. just deleting the specific implementation from the table dnspython uses for EDNS options would fix all instances in an application.

[bwelling]

It's probably too late for this specific case, but would adding an equivalent to dns.rdata.Rdata.to_generic() for EDNS options help in the future?

With that, if you're writing code using a generic option whose implementation might one day be replaced with a custom class, you could unconditionally call to_generic() on it to use the generic form.

[cdeccio]

I honestly didn't think about this aspect of backwards compatibility, so sorry!

No worries. I'm not exactly a prime example of software engineering and versioning, so... 🥴 And anyway, how can I complain about free software. Thanks for all your work on dnspython!

Though this is kind of an inherent problem with new options and new rdata types.

Yes, I suppose so. But part of me sort of expected it to "just work" because I assumed (without looking closely at the code) that the new types were and would be subclasses of GenericOption, so I could use the "raw" interface (e.g., the data member) or the more specific interface. But of course that's just now how they were implemented.

[cdeccio]

It's probably too late for this specific case, but would adding an equivalent to dns.rdata.Rdata.to_generic() for EDNS options help in the future?

With that, if you're writing code using a generic option whose implementation might one day be replaced with a custom class, you could unconditionally call to_generic() on it to use the generic form.

Right, for these two cases (NSIDOption and CookieOption) the ship has sailed, but dns.rdata.Rdata.to_generic() is an option (pun intended) and/or you could pass a boolean to from_wire() and from_text() (or somewhere more appropriate) that tells dnspython whether to use GenericOption when parsing the options or something more specific.

As far as I can tell, this is really only an issue when creating messages from wire or text or something, not when instantiating messages using the constructor.

[cdeccio]

I just wanted to point out one other related/supporting issue:

From dns.edns.py in CookieOption.__init__():

        if len(client) != 8:
            raise ValueError("client cookie must be 8 bytes")
        if len(server) != 0 and (len(server) < 8 or len(server) > 32):
            raise ValueError("server cookie must be empty or between 8 and 32 bytes")

If this ValueError is raised from deep in the from_wire() method, I suppose it will be caught with continue_on_errors, but that definitely changes behavior for something like DNSViz which is supposed to analyze the option and determine correctness of the option (see for example https://github.com/dnsviz/dnsviz/blob/master/dnsviz/analysis/offline.py#L1243). I don't know how many other reverse dependencies might care about this, but this is definitely a case where instantiating CookieOption vs. GenericOption changes the overall behavior. Even dns.rdata.Rdata.to_generic() won't save this because the instantiation will happen and the ValueError will be raised before there is ever a chance to call to_generic() -- unless there is a use_generic_option_class boolean argument that could be passed to from_wire() and from_text(), as I suggested in my earlier comment.

[bwelling]

Right, for these two cases (NSIDOption and CookieOption) the ship has sailed, but dns.rdata.Rdata.to_generic() is an option (pun intended) and/or you could pass a boolean to from_wire() and from_text() (or somewhere more appropriate) that tells dnspython whether to use GenericOption when parsing the options or something more specific.

As far as I can tell, this is really only an issue when creating messages from wire or text or something, not when instantiating messages using the constructor.

I'm not sure if such a parameter would make sense. Either it would be:

• a boolean that controlled all option processing - this would mean that if some new option was added, and you wanted to ensure that the new option was generic, you'd be forced to use the generic form of all of the options that your application might already support.
• some sort of specification of which options should be parsed as generic, and which should not. This would be overly complicated and impractical.

[rthalley]

This is probably overly complicated, but... along the lines of "some sort of specification of which options should be parsed as generic, and which should not", there could be a library global that was the "dnspython generic version level". This would default to the current version, as in general I think applications would want to get non-generic classes. But more complicated applications could set a version level, e.g. DNSPYTHON_VERSION_2_6, and then all types added after DNSPYTHON_VERSION_2_6 would be forced to generic. This would stop the more complicated applications from breaking.

[cdeccio]

This is probably overly complicated, but... along the lines of "some sort of specification of which options should be parsed as generic, and which should not", there could be a library global that was the "dnspython generic version level". This would default to the current version, as in general I think applications would want to get non-generic classes. But more complicated applications could set a version level, e.g. DNSPYTHON_VERSION_2_6, and then all types added after DNSPYTHON_VERSION_2_6 would be forced to generic. This would stop the more complicated applications from breaking.

That could work at least for my purposes. That would allow the developer to use generic options indefinitely (except for version 2.7, and to some extent 2.6; still trying to figure out what to do with those). But to Brian's point, I think it's perfectly reasonable for it to be an all-or-none (generic or not) choice. So maybe it doesn't need to include which dnspython version, which could actually be overly complicated; maybe it's just a global USE_GENERIC_OPTIONS. Is it really just message parsing from text and wire that this affects?

[rthalley]

I merged #1145, which is all I want to do at the current time, as the other things seem excessively complex now that we have a way to ensure generic if you need it.