Opened 12 months ago

Last modified 12 months ago

#6839 enhancement assigned

t.n.dns.DNSProtocol and DNSDatagramProtocol should allow an alternative Message parser to be used

Reported by: rwall Owned by: rwall
Priority: normal Milestone: EDNS0
Component: names Keywords:
Cc: Branch: branches/dns-protocol-message-factory-6839-2
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

In #5675 we've got a new EDNSMessage parser.

But dns.DNSDatagramProtocol and DNSProtocol (TCP) are hardcoded to use the original dns.Message for decoding and encoding wire messages.

Add a new "messageFactory" constructor argument which allows us to supply dns.EDNSMessage (or a factory function).

Make sure all factory arguments are supplied as keywords. This will also allow us to supply a functools.partial instance, where the EDNS specific constructor arguments have been preassigned; without worrying about argument order.

See:

Change History (7)

comment:1 Changed 12 months ago by rwall

  • Summary changed from t.n.dns.DNSProtocol and DNSDatagramProtocol should allow and alternative Message parser to be provided to t.n.dns.DNSProtocol and DNSDatagramProtocol should allow an alternative Message parser to be used

typo in title

comment:2 Changed 12 months ago by rwall

  • Author set to rwall
  • Branch set to branches/dns-protocol-message-factory-6839

(In [40743]) Branching to 'dns-protocol-message-factory-6839'

comment:3 Changed 12 months ago by rwall

  • Keywords review added

Ready for review in log:branches/dns-protocol-message-factory-6839

  • Added a messageFactory argument to the constructor of dns.Message
  • This will allow me to supply dns.EDNSMessage once it's merged #5675

Build results:

comment:4 Changed 12 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:5 follow-up: Changed 12 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall
  1. I wonder if the appropriate interface for the messageFactory arguments is "something that acts like a Message constructor? There are two things done with it
    • Create a message from some keyword arguments and some queries.
    • Create a message from some bytes on the wire.

It occurs that we could perhaps address the issue of Message.fromStr not being a class method making messageFactory something that had a more specific interface (and provide something that constructs Messages appropriately).


  1. DNSProtocolSahredTestsMixin should document the assumption it makes about what subclasses setUp do. (It is unfortunate that the best way we have of handling reuse is by subclassing, but if we are using it, we should make it as clear as possible what is expected of subclasses.
    • I also wonder if it would be clearer to have subclasses that implement *just* what the shared base-class needs, and then have a seperate class for the other tests.

Please resubmit for review after addressing the above points.

comment:6 Changed 12 months ago by rwall

  • Branch changed from branches/dns-protocol-message-factory-6839 to branches/dns-protocol-message-factory-6839-2

(In [40907]) Branching to 'dns-protocol-message-factory-6839-2'

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

  • Status changed from new to assigned

Replying to tom.prince:

  1. I wonder if the appropriate interface for the messageFactory arguments is

"something that acts like a Message constructor?

Well I've updated the docstring for a start. r40910

There are two things done

with it

  • Create a message from some keyword arguments and some queries.
  • Create a message from some bytes on the wire. It occurs that we could perhaps address the issue of Message.fromStr not

being a class method making messageFactory something that had a more
specific interface (and provide something that constructs Messages
appropriately).

I need to give this some more thought. I'll post some responses later.

  1. DNSProtocolSahredTestsMixin should document the assumption it makes about

what subclasses setUp do. (It is unfortunate that the best way we have of
handling reuse is by subclassing, but if we are using it, we should make it
as clear as possible what is expected of subclasses.

Done. r40911

But I agree. And having to document it demonstrates how ugly the tests are. I'd

rather not rely on setUp, but I was trying to fit in with existing tests. Maybe
that's a mistake....which is what you're saying in the next comment.

  • I also wonder if it would be clearer to have subclasses that implement

*just* what the shared base-class needs, and then have a seperate class
for the other tests.

Done. r40912

Please resubmit for review after addressing the above points.

Thanks for the review.

Note: See TracTickets for help on using tickets.