Opened 3 years ago

Closed 13 months ago

#5675 enhancement closed fixed (fixed)

Create a new Message-like class that includes space for EDNS information

Reported by: exarkun Owned by: rwall
Priority: normal Milestone: EDNS0
Component: names Keywords: edns
Cc: p.mayers@… Branch: branches/edns-message-5675-4
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

EDNS extends DNS with a couple new statically defined fields and an arbitrary extension mechanism. This information is logically part of the DNS Message itself, but is represented on the network as a new record type.

We should introduce a new message class which exposes the additional information present in an OPT record. It should be safe to deserialize any DNS message using this class, even regular DNS messages (non-EDNS messages). When deserializing EDNS messages, though, the fields of the OPT record should be made available directly on the Message itself and the OPT record should not be present in the list of additional records (where it might end up in a naive implementation, since that's where it appears in the wire format).

It may make sense for some portion of the implementation of this new class to be shared with the implementation of the existing message class. It may make sense for these parts to be factored into a new shared base class. Some parts of the existing message class are undesirable (the redundancy between encode/decode and fromStr/toStr, the failure to use class methods where appropriate, the obscurely named attributes), thus I do not recommend having the new class subclass the old class directly.

Attachments (4)

edns-message-5675-1.patch (37.3 KB) - added by rwall 18 months ago.
Unfinished implementation of EDNSMessage which wraps and can be substituted for Message
edns-message-5675.OPTHeader-integration.patch (13.3 KB) - added by rwall 18 months ago.
Start using _OPTHeader from source:branches/opt-record-5668-3
test_dns.failures.txt (1.9 KB) - added by lukmdo 18 months ago.
two test failing (MessageStandardEncodingTests.test_ednsDecode, MessageStandardEncodingTests.test_ednsEncode)
edns-message-5675-examples.patch (22.2 KB) - added by rwall 15 months ago.
Latest edns examples as a patch against edns-message-5675-4

Download all attachments as: .zip

Change History (28)

comment:1 Changed 19 months ago by rwall

  • Author set to rwall
  • Branch set to branches/edns-message-5675

(In [38441]) Branching to 'edns-message-5675'

Changed 18 months ago by rwall

Unfinished implementation of EDNSMessage which wraps and can be substituted for Message

comment:2 Changed 18 months ago by rwall

  • Keywords review added

Ready for review in attachment:edns-message-5675-1.patch

This isn't finished and I don't want a full code review. I'd just like
some guidance / feedback on the following:

  1. EDNSMessage
    1. attribute names / constructor args: I originally tried to improve the API by using more meaningful attribute names (eg responseCode instead of rCode)
      1. But this makes it difficult to drop EDNSMessage in place of Message (eg in dns.DNSMixin, server.DNSFactory) without making lots of changes elsewhere
      2. Is it worth choosing good names and providing some sort of adapter?
    2. decode / encode: I've left all the decode and encode logic in the original Message class. Does this make sense or should I / can I refactor Message and put the decode / encode stuff in a Mixin maybe?
    3. branch scope: I'm thinking that this branch should provide just enough to allow twistd dns to properly reject EDNS queries. That might sound strange, but it is described in https://tools.ietf.org/html/rfc6891#section-7
      1. Currently I found that twistd dns returns your EDNS message to you in the NXDOMAIN case. This is because it doesn't reset the additional records list before sending the message back.
      2. So I thought it would be nice goal for this branch to fix that bug and simply send back the proper FORMERR rcode when an EDNS query is received - and defer proper EDNS responses to another branch.
  2. Testing
    1. My tests have got quite convoluted.
    2. I'm trying to run as many tests as possible against both Message and EDNSMessage. Is that a worthwhile goal? Are there better methods than putting them in a mixin? The indirection of self.messageFactory and self.messageDecoder, makes the tests difficult to read...I think.
    3. I've also put the test data (bytes / dns wireformat and the corresponding Message constructor args) in a separate class. I thought it would be useful to be able to reuse that data (the byte string examples at least). Are there better ways of doing this?
    4. The existing Message tests make assertions about each of the Message attributes (eg assertEqual(m1.rCode, m2.rCode)) where as I'm just relying on the FancyEqMixin. Is that Ok?
    5. I've added FancyEqMixin to the original Message for ease of testing. Is that against the compatibility policy?

Anyway, that's all for now.

If anyone has time to comment I'd appreciate some feedback.

-RichardW.

comment:3 Changed 18 months ago by rwall

  • Branch changed from branches/edns-message-5675 to branches/edns-message-5675-2

(In [38826]) Branching to 'edns-message-5675-2'

comment:4 Changed 18 months ago by rwall

Ready for review in log:branches/edns-message-5675-2

Following on from my comment:2, I've now trimmed and simplified things
a bit.

  1. EDNSMessage is now private so that I'm free to make changes in future EDNS related branches.
  2. I've narrowed the EDNSMessage API down to just toStr and fromStr, plus the various attributes that were already present.
    1. There didn't seem any need to implement IEncodable and internally, DNSDatagramProtocol and DNSProtocol are using the to/fromStr API anyway.
  3. I've simplified the tests. Breaking them into multiple test cases and where possible also running the tests against dns.Message.
  4. Build Results
    1. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/edns-message-5675-2
    2. All the tests seem to pass and _EDNSMessage does seem to be a good drop in replacement for Message.

Next I'm going to actually start implementing more of the EDNS
specific attributes.

But first I might go back to #5668 and re-implement OPTRecord as a
wrapper around an RRHeader instance.

I'd still appreciate some feedback / guidance if anyone has time to
glance at my branch.

-RichardW.

Changed 18 months ago by rwall

Start using _OPTHeader from source:branches/opt-record-5668-3

Changed 18 months ago by lukmdo

two test failing (MessageStandardEncodingTests.test_ednsDecode, MessageStandardEncodingTests.test_ednsEncode)

comment:5 follow-up: Changed 18 months ago by lukmdo

comment:6 Changed 17 months ago by rwall

  • Branch changed from branches/edns-message-5675-2 to branches/edns-message-5675-3

(In [39552]) Branching to 'edns-message-5675-3'

comment:7 in reply to: ↑ 5 Changed 17 months ago by rwall

Thanks for the review lukmdo.

ISTR you were reviewing this at the EuroPython Twisted sprint - sorry
for not replying sooner.

Replying to lukmdo:

The failing tests were because I'd got my class definition in the wrong place. Now fixed r39554

  • the twisted/names/test/test_dns.py:3202 sendMessage does not
  • look like needed

Removed. r39555.

The OPTHeader (#5668) branch is now merged so I've merged this
branch forward.

There's still a lot to do and I'll continue to work on it in the
background. But I'll leave the ticket in review.

If you or anyone else has time to look at the code and comment on the
general approach I'd appreciate some feedback.

I've also added some example scripts which demo how the EDNS APIs
might be used.

comment:8 Changed 16 months ago by rwall

  • Branch changed from branches/edns-message-5675-3 to branches/edns-message-5675-4

(In [39732]) Branching to 'edns-message-5675-4'

comment:9 Changed 16 months ago by rwall

(In [39733]) Merge forward to pick up #6680. Refs: #6680, #5675

comment:10 Changed 15 months ago by philmayers

  • Owner set to philmayers

comment:11 follow-up: Changed 15 months ago by philmayers

  • Keywords review removed
  • Owner changed from philmayers to rwall

Ok, so this is my first review - I'm trying to emulate the general tone/content I've seen other places, but please forgive if I come across wrong; I think the work is good and useful. Also please forgive my relative ignorance on the review process - some of the following may not be applicable.

On with the show:

  1. Is it useful to
    assert dnssecOK in (True, False)
    
    ...or should we just test for truth? Not a major issue, but maybe an assert should be accompanied by a "dnssecOK must be True or False" message?
  2. I note the tests do real network activity - if I understand correctly, best practice is to use a pseudo transport and avoid real networking in tests?
  3. I'm particularly uncomfortable about probing a specific list of servers out on the real internet for a response (but I want to avoid bikeshedding!)
  4. The EDNSMessage class looks fine, though I assume the leading _ will disappear? ;o)
  5. Missing @since: in the epytext comments - but not sure at which stage this gets added.
  6. Missing shebang in source:doc/names/examples/test_edns_compliance.py - note existing tests e.g. TestDnsTests check example source for shebang, maybe do that?
  7. Missing/useful-to-have epytext in the EDNS{Stream,Datagram}Protocol and related class docstrings?
  8. twistedchecker has some complaints for edns.py:
    W9002:  1,0: Missing a reference to test module in header
    W9012: 93,0: Expected 2 blank lines, found 1
    W9208: 88,0:EDNSClientFactory: Missing docstring
    
    It was mostly useless on dns.py because it's old & crufty code but I think:
    W9204:2366,4:_EDNSMessage.toMessage: Missing epytext markup @return for return value
    
    ...is relevant
  9. As far as I can tell the coding standard is otherwise followed (if someone could review my review here, I'd be grateful!)
  10. I'm uncertain if a topfiles/NEWS item is needed at this stage?
  11. I don't have a py3 environment to test in, but I'm not sure if t.names passes Py3 tests at all.
  12. I agree that the hard-coding of the message class in the old protocol/factory code is unfortunate; is your plan to file tickets for this (as per ticket:5668#comment:21) after this one? In the meantime this looks like a reasonable approach and avoids touching the old code.
  13. The general approach looks good, and I can confirm the code behaves as expected and looks good on the wire.
  14. That said I note txdig.py is setting AD in queries; see http://tools.ietf.org/html/rfc4035#section-4.6 which suggests to me that AD=0 in queries should be the norm (wireshark complains about it in the DNS analyzer too!)
  15. The API seems fine; I wonder if a ticket to deprecate the old Message at some (distant) point is useful?
  16. Does it need a buildbot run?

The above minor things aside it looks good to merge, to me. I'll clear "review" and assign back to you, but won't be offended if you want more info from myself or someone else.

comment:12 Changed 15 months ago by philmayers

  • Cc p.mayers@… added

comment:13 in reply to: ↑ 11 Changed 15 months ago by rwall

  • Status changed from new to assigned

Replying to philmayers:

Ok, so this is my first review - I'm trying to emulate the general
tone/content I've seen other places, but please forgive if I come
across wrong; I think the work is good and useful. Also please
forgive my relative ignorance on the review process - some of the
following may not be applicable

Phil,

thanks for looking at the branch. It's a great review.

My answers and comments below...

On with the show:

  1. Is it useful to
    assert dnssecOK in (True, False)
    
    ...or should we just test for truth? Not a major issue, but maybe an assert should be accompanied by a "dnssecOK must be True or False" message?

I wasn't sure about this either, here's my thinking:

  • _EDNSMessage has 16 constructor arguments.
  • Many with shortened names
  • May be supplied as positional arguments
  • So try extra hard to give the user early warning if they accidentally supply rCode in place of recAv

Another question is whether I should take the opportunity to use
readable argument names. I've chosen to copy the names from
dns.Message for convenience and compatibility, but there aren't many
places in Twisted that construct new Messages so it wouldn't be hard
to change the names. And for people using Message in their own
scripts, we could provide an adapter...maybe.

Another thing to consider is whether it might be better to use
t.p.constants.NamedConstants instead of all these arguments.

Then we supply a set of Named constants to the _EDNSMessage
constructor...is that a good idea?

  1. I note the tests do real network activity - if I understand correctly, best practice is to use a pseudo transport and avoid real networking in tests?
  2. I'm particularly uncomfortable about probing a specific list of servers out on the real internet for a response (but I want to avoid bikeshedding!)

I think you're talking about "test_edns_compliance". That's not
actually one of the unit tests. It's just an example which is supposed
to demonstrate how the new _EDNSMessage API can be used.

  1. The EDNSMessage class looks fine, though I assume the leading _ will disappear? ;o)

Actually no. My plan was to keep it private until I've tackled #5670
and #5669.

If I make it public now and it turns out not to be a good API then
I'll have to go through the deprecation procedure etc.

  1. Missing @since: in the epytext comments - but not sure at which stage this gets added.
  2. Missing shebang in source:doc/names/examples/test_edns_compliance.py - note existing tests e.g. TestDnsTests check example source for shebang, maybe do that?

I'll fix those. Thanks.

  1. Missing/useful-to-have epytext in the EDNS{Stream,Datagram}Protocol and related class docstrings?
  2. twistedchecker has some complaints for edns.py:
    W9002:  1,0: Missing a reference to test module in header
    W9012: 93,0: Expected 2 blank lines, found 1
    W9208: 88,0:EDNSClientFactory: Missing docstring
    
    It was mostly useless on dns.py because it's old & crufty code but I think:
    W9204:2366,4:_EDNSMessage.toMessage: Missing epytext markup @return for return value
    
    ...is relevant

The edns.py module is not actually going to be merged in this branch.

It's just my way of demonstrating (along with the example scripts) how
the _EDNSMessage API is going to be used and that it is usable.

  1. As far as I can tell the coding standard is otherwise followed (if someone could review my review here, I'd be grateful!)
  2. I'm uncertain if a topfiles/NEWS item is needed at this stage?

I always forget that. I'll add one.

  1. I don't have a py3 environment to test in, but I'm not sure if t.names passes Py3 tests at all.

The best thing is to check the buildbot results.

There's usually a link at the top of every ticket that has an active
branch. But the results seem to have gone missing in this case,
probably because the ticket has been waiting in the review queue for
so long.

I've forced a new build. The py3 tests do seem ok...for once.

  1. I agree that the hard-coding of the message class in the old protocol/factory code is unfortunate; is your plan to file tickets for this (as per ticket:5668#comment:21) after this one? In the meantime this looks like a reasonable approach and avoids touching the old code.

Ah that's in edns.py I think. Yes I'll create a new ticket for
that. But I need to think about whether to create a new _EDNSProtocol
class rather than trying to modify or re-use the existing Protocols.

  1. The general approach looks good, and I can confirm the code behaves as expected and looks good on the wire.

I know you contributed some EDNS / DNSSEC patches a long time ago.

What was your use-case at the time and do you think the _EDNSMessage
in this branch would have met your requirements?

  1. That said I note txdig.py is setting AD in queries; see http://tools.ietf.org/html/rfc4035#section-4.6 which suggests to me that AD=0 in queries should be the norm (wireshark complains about it in the DNS analyzer too!)

Yeah I thought it seemed wrong, but I was just copying dig, whose man page says this...

+[no]adflag

Set [do not set] the AD (authentic data) bit in the query. This
requests the server to return whether all of the answer and
authority sections have all been validated as secure according to
the security policy of the server. AD=1 indicates that all records
have been validated as secure and the answer is not from a OPT-OUT
range. AD=0 indicate that some part of the answer was insecure or
not validated. This bit is set by default.

  1. The API seems fine; I wonder if a ticket to deprecate the old Message at some (distant) point is useful?

I think so, yes. Although a non-EDNS message class might still be
useful for eg simulating and testing compatibility with old non-EDNS
servers and clients.

  1. Does it need a buildbot run?

See above.

The above minor things aside it looks good to merge, to me. I'll
clear "review" and assign back to you, but won't be offended if you
want more info from myself or someone else.

Thanks. I'll make your suggested changes and resubmit for another review.

If you've got any further thoughts or useful info, let me know. I really
appreciate your input.

-RichardW.

Changed 15 months ago by rwall

Latest edns examples as a patch against edns-message-5675-4

comment:14 Changed 15 months ago by rwall

  • Keywords review added
  • Owner rwall deleted
  • Status changed from assigned to new

Ready for another review in log:branches/edns-message-5675-4

I've removed all the example scripts and supporting module.

I've fixed various twistedchecker warnings.

Decided not to change the EDNSMessage constructor argument names...for now ...until I see how I'm going to use it in place of dns.Message...in the next ticket.

I was worried about my testing of EDNSMessage encoding and decoding. I considered implementing tests which check the binary bit values at specific ranges and offsets in the message but then decided against it. r40040
I spoke to people in #powerdns and it seems that no-one else goes to these lengths, so twisted won't be any worse tested than other DNS servers.

So I'm sure this API isn't perfect, but it's all private so I can change it as necessary in the next branch for #5670

Build results:

comment:15 Changed 15 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:16 follow-up: Changed 15 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to rwall
  • Status changed from assigned to new

Heya rwall, thanks for working on this.

Overall, I like this branch a lot. Below are some comments, though I don't know if I gave the test suite as thorough an examination as it deserves (by the time I got that far into the patch I was well beyond the line count threshold where I can continue to meaningfully absorb new information).

  1. twisted/names/dns.py
    1. Please link to the spec defining EBADVERSION
    2. I'll echo philmayers comments about the treatment of the arguments in to _EDNSMessage.__init__ (as well as the treatment of dnssecOK in _OPTHeader.__init__).
      1. Most definitively, I can at least say that assert shouldn't be used anywhere in Twisted. If special type checking is required here, it should be done with an if statement and a raise statement.Does this parameter/attribute merit special handling beyond what others get? None of the existing parameters to this __init__ get type checked or coerced.
      2. Apart from that, the coercion appears redundant if the type check is going to remain (bool(True) is True and bool(False) is False - oh wait, I get it, 0 in (True, False) is True; gah).
      3. Earlier you mentioned an idea about improving the handling of these numerous flags. I think that's a good idea to pursue. Consider this idea (not necessarily for this ticket, since making these APIs private means we have plenty of time to improve them with follow-up work) - __init__ should accept a FlagConstant collapsing as many of these flags as possible (looks like there are 8 parameters that would all combine into 1) into a single value. We could also think about whether opCode, rCode, and ednsVersion merit more special handling (perhaps a Values constant instead of int) and For compatibility, _EDNSMessage can also:
        1. provide an alternate constructor with a signature more like its current __init__ signature - this would be an API more like a drop-in replacement for Message.__init__. This can also include whatever extra safety checks seem useful to help developers avoid subtle mistakes (though hopefully we can also encourage them to switch to the nicer API).
        2. provide properties that inspect the flag value in order to provide attribute compatibility with Message instances
    3. I'm curious what the intended use of _EDNSMessage._decodingErrors might be. Do you imagine a public interface to this information someday? My first thought on reading this is that raising an exception on this case (perhaps wrapping a partially initialized _EDNSMessage) is the more obvious way to expose this.
  2. twisted/names/test/test_dns.py
    1. Some docstrings say @type: Mixed - I don't think this is meaningful. I would omit these - maybe unless twistedchecker is going to complain. Then, it occurs to me, perhaps what is meant here is object (this is a lame trick though, it's hard to see the value if declaring that any type of object at all is allowed rather than just not declaring the type - but maybe there is some).
    2. I'm definitely not a fan of all the looping going on in verifyConstructorArgument - the result of which is that the test suite may need to be run many times (with intervening fixes) to learn whether everything is working well.
    3. in test_rCode - is 123 a good alternate value to test? This isn't an rCode defined by any spec as far as I know.
    4. test_fromMessage and test_toMessage look somewhat incomplete
  3. More boringly... :)
    1. Docstrings need to be reflowed to a wider column
    2. Did you file a bug against twistedchecker for the invalid test_dns error on <https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1348/steps/run-twistedchecker/logs/new%20twistedchecker%20errors>?

Two things I particularly liked in the tests are the attempts to apply the same tests to both dns.Message and dns._EDNSMessage as well as the continued use of the pattern of explicitly defined cases (represented by the classes like MessageEmpty). Something I could imagine (one day) doing with the latter is:

  1. explicitly define and declare the interface all those classes provide
  2. write a test method generator (maybe with testSuite?) that takes a list of such providers

I think after addressing these points this branch will almost certainly be ready to merge. Given that some of the review points are somewhat more like questions than suggestions for changes though, I wouldn't mind reading/discussing your responses to them before the merge happens.

Thanks again.

comment:17 in reply to: ↑ 16 Changed 15 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

exarkun,

Thanks for the review. I really appreciate your feedback.

I've added some comments below...

Replying to exarkun:

  1. Please link to the spec defining EBADVERSION

Done. r40212.

  1. Most definitively, I can at least say that assert shouldn't be used anywhere in Twisted. If special type checking is required here, it

Removed all assert statements. r40213.

  1. Apart from that, the coercion appears redundant if the type check is going to remain (bool(True) is True and bool(False) is False - oh wait, I get it, 0 in (True, False) is True; gah).

Removed all use of bool. r40214.

  1. Earlier you mentioned an idea about improving the handling of these numerous flags. I think that's a good idea to pursue. Consider this

See #6778

  1. I'm curious what the intended use of _EDNSMessage._decodingErrors might be. Do you imagine a public interface to this information someday? My first thought on reading this is that raising an exception on this case (perhaps wrapping a partially initialized _EDNSMessage) is the more obvious way to expose this.

Removed in r40215.

It was in anticipation of sending the correct FORMERR response when multiple OPT
records are found in a message.

I hadn't really thought it through and it doesn't need to be dealt with in this
ticket.

See ticket:5669#comment:1 and ticket:5669#comment:2

  1. twisted/names/test/test_dns.py
  1. Some docstrings say @type: Mixed - I don't think this is meaningful.

Done. r40216.

  1. I'm definitely not a fan of all the looping going on in verifyConstructorArgument - the result of which is that the test suite may need to be run many times (with intervening fixes) to learn whether everything is working well.

Removed the invalidInputs loop in r40217, which isn't necessary now that I
removed the assert statements and bool coercion.

Removed the inputTests assert loop in (r40218) verifyConstructor argument which
isn't necessary having removed bool coercion.

  1. in test_rCode - is 123 a good alternate value to test? This isn't an rCode defined by any spec as far as I know.

Fixed in r40219.

  1. test_fromMessage and test_toMessage look somewhat incomplete

Those functions were introduced quite late in the branch as a way of testing the
opt records that get added to dns.Message during encoding.

They are fully tested by way of the toStr and fromStr.

Ideally, the majority of EDNSMessage encoding / decoding tests would use these
and compare the resulting messages, instead of testing the resulting bytes.

The trouble is that dns.Message doesn't implement FancyEqMixin, so it's
difficult to test the result of toMessage.

The other reason is that by only testing for bytes, I can re-use the same test
data for dns.Message and _EDNSMessage...maybe that was just a lack of
imagination on my part though.

But you're right, those two tests are fairly useless. So what I've done now is
replace those with a series of tests that demonstrate that fromStr calls
fromMessage and that toStr calls toMessage. And having established that link,
the existing tests for toStr and fromStr are known to be exercising the
to/fromMessage functions.

  1. More boringly... :)
    1. Docstrings need to be reflowed to a wider column

Done. r40228.

  1. Did you file a bug against twistedchecker for the invalid test_dns error on <https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1348/steps/run-twistedchecker/logs/new%20twistedchecker%20errors>?

Tom already reported it:

Two things I particularly liked in the tests are the attempts to apply the
same tests to both dns.Message and dns._EDNSMessage as well as the
continued use of the pattern of explicitly defined cases (represented by the
classes like MessageEmpty). Something I could imagine (one day) doing with
the latter is:

  1. explicitly define and declare the interface all those classes provide
  2. write a test method generator (maybe with testSuite?) that takes a list of such providers

Yeah, that's a nice idea.

I'd also prefer if the various Test Message classes didn't have so much
overlap.

All changes are in the same branch log:branches/edns-message-5675-4

And here are the latest build results which look ok apart from an unrelated
failure on the freebsd builder.:

-RichardW.

comment:18 Changed 13 months ago by rwall

  • Milestone set to EDNS0

comment:19 Changed 13 months ago by rwall

Make sure the repr for EDNSMessage follows the same rules as for Message in #6847. ie show only the attributes which are non-default and the flags which are set.

comment:20 Changed 13 months ago by rwall

Here's a branch which combines all the EDNS and DNSSEC changes that
are currently up for review.

I've hacked together some new EDNS versions of protocol, client and
server. Implemented as proxies for the existing classes.

I've also added some examples which demo the new APIs.

  • twistd -noy doc/names/examples/edns_server.tac.py
  • TARGET=127.0.0.1,10053,twistd ./bin/trial doc/names/examples/test_edns_compliance.py
  • python doc/names/examples/txdig.py -s 8.8.8.8 com DNSKEY

Hopefully this gives some context for the changes in this branch.

comment:21 follow-up: Changed 13 months ago by tom.prince

  • Keywords review removed
  1. TrunCation: Is there any reason for the odd capitalization?
  2. @type without @param is ignored, so queries, answers, authorityy and additional should get @param.
  3. I realized that all of EDNSMessage is currently private, but it will likely be made public at some point. I'm wondering if toMessage and fromMessage will want to be made public at that point, or if toStr and fromStr want to be the public interface? I would consider making them private here now, so that if/when EDNSMessage is made public, they aren't as well.
  4. I'm curious what the point of the _messageFactory class attribute is? (Which, incidentally, isn't documented.)
  5. I would be inclined to make multiple OPT records fail nosily, rather than silently.
  6. Consider filing a ticket for merging MessageComparable into Message.
  7. In test_fromStrCallsFromMessage a function with a docstring doesn't need pass.
  8. Is there any reason for MessageStandardEncodingTests.messageFactory to accept positional arguments?
  9. (optional) test_fromMessageCopiesSections: Is it valuable to test none of the lists are the same? I would have expected something like
         for attrName in (...):
            if getattr(standardMessage, attrName) is getattr(ednsMessage, attrName):
              duplicates.append(AttrName)
    

That is, only comparing corresponding attributes, and using is instead of id.

  • It occurs to me, that if fromMessage is private and only called from fromStr, that the underlying Message is never exposed, so the lists could be stolen. (If it did, the documentation for fromMessage should say that it does.)
  1. I would be inclined to split EDNSMessageStandardEncodingTests and EDNSMessageTests into a base class that doesn't inherit from TestCase and a EDNSMessage specific subclass that does. Also, I would document exactly what assumption the base class makes about its subclasses.

Please merge after addressing the above comments and suggestions to your own satisfaction.

Random musings.

  1. I'm sad about all the abbreviations in attributes. But this is just copied from Message. It would be nice to change them, but if this needs to be compatible with Message, that isn't possible. A couple of options (in later tickets):
    • Maybe this doesn't need to be compatible.
    • Change them, but provided aliases for the old options (either here, or on Message too). Possibly deprecating the old ones.
    • Do the constructors need to be compatible? The arguments could be changed without changing the attributes. :/
  2. I'm sad at fromStr not being a class method. Again, we should consider (at some later point), whether we need to present something compatible with Message as part of the eventual public interface.

comment:22 Changed 13 months ago by tom.prince

  • Owner set to rwall

comment:23 in reply to: ↑ 21 Changed 13 months ago by rwall

  • Status changed from new to assigned

Replying to tom.prince:

  1. TrunCation: Is there any reason for the odd capitalization?

There is a reason, but not a good one. I copied these docstrings directly from
the RFC. They use that capitalisation because they refer later to the TC
bit. Similarly, they refer to the RD and RA bits.

I changed it. r40897

  1. @type without @param is ignored, so queries, answers, authorityy and additional should get @param.

Done. r40898

  1. I realized that all of EDNSMessage is currently private, but it will likely be made public at some point. I'm wondering if toMessage and fromMessage will want to be made public at that point, or if toStr and fromStr want to be the public interface? I would consider making them private here now, so that if/when EDNSMessage is made public, they aren't as well.

I agree. Done. r40899

  1. I'm curious what the point of the _messageFactory class attribute is? (Which, incidentally, isn't documented.)

It's there to allow me to test that _EDNSMessage.fromStr instantiates Message with the correct arguments.

Maybe I've gone over the top with these tests. Something I want to discuss with you in Bristol.

Anyway, I documented it and left it for now.

  1. I would be inclined to make multiple OPT records fail nosily, rather than silently.

Yeah, that's a tricky one. I originally thought the same. The counter arguments
are:

  1. Don't want someone to be able to crash your DNS server by sending you a malformed message (there's currently no handling of decoding exceptions).
  2. If you're writing a DNS diagnostic tool or compliance tool it's useful to be able to examine the message and report what's wrong.

I also considered logging a warning, but chatting with exarkun and we thought it
was probably not a good idea to allow someone to spam your log files by sending
malformed messages.

  1. Consider filing a ticket for merging MessageComparable into Message.

Already done. See #6848 which is up for review.

  1. In test_fromStrCallsFromMessage a function with a docstring doesn't need pass.

Done. r40900

  1. Is there any reason for MessageStandardEncodingTests.messageFactory to accept positional arguments?

No reason. Better if it doesn't. Avoids any confusion. r40901

  1. (optional) test_fromMessageCopiesSections: Is it valuable to test none of the lists are the same? I would have expected something like
    for attrName in (...):
       if getattr(standardMessage, attrName) is getattr(ednsMessage, attrName):
       duplicates.append(AttrName)
    

That is, only comparing corresponding attributes, and using is instead of id.

  • It occurs to me, that if fromMessage is private and only called from fromStr, that the underlying Message is never exposed, so the lists could be stolen. (If it did, the documentation for fromMessage should say that it does.)

I think it's a valuable test, despite to/fromMessage now being private.

But you're right. The test was over complicated. I've simplified it like you suggested. r40902

  1. I would be inclined to split EDNSMessageStandardEncodingTests and

EDNSMessageTests into a base class that doesn't inherit from TestCase and a
EDNSMessage specific subclass that does. Also, I would document exactly what
assumption the base class makes about its subclasses.

Good idea. r40903 and r40904.

Please merge after addressing the above comments and suggestions to your own satisfaction.

Thanks for the review Tom!

Random musings.

  1. I'm sad about all the abbreviations in attributes. But this is just copied from Message. It would be nice to change them, but if this needs to be compatible with Message, that isn't possible. A couple of options (in later tickets):
    • Maybe this doesn't need to be compatible.
    • Change them, but provided aliases for the old options (either here, or on Message too). Possibly deprecating the old ones.
    • Do the constructors need to be compatible? The arguments could be changed without changing the attributes. :/
  2. I'm sad at fromStr not being a class method. Again, we should consider (at some later point), whether we need to present something compatible with Message as part of the eventual public interface.

I agree about the constructor arguments and attribute names. They don't
necessarily need to be compatible, but my original thought was that I'd leave
them the same so that EDNSMessage can be dropped in place of Message without too
many changes to the surrounding code.

Once this is merged, it should be easier to see how it's all going to fit
together.

Ok. I'll merge if there are no further comments and the builds go green.

Thanks again.

-RichardW.

comment:24 Changed 13 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [40906]) Merge edns-message-5675-4

Author: rwall
Reviewers: lukmdo, philmayers, exarkun, tom.prince
Fixes: #5675

twisted.names.dns._EDNSMessage. A private API for encoding and decoding Extended
DNS messages.

Note: See TracTickets for help on using tickets.