Opened 4 years ago

Last modified 4 years ago

#6839 enhancement assigned

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

Reported by: Richard Wall Owned by: Richard Wall
Priority: normal Milestone: EDNS0
Component: names Keywords:
Cc: Branch: branches/dns-protocol-message-factory-6839-2
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

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:

Attachments (1)

edns-protocols-6839.diff (10.2 KB) - added by Richard Wall 16 months ago.
Trying to reacquaint myself with the remaining EDNS tasks

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by Richard Wall

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

typo in title

comment:2 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/dns-protocol-message-factory-6839

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

comment:3 Changed 4 years ago by Richard Wall

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 4 years ago by Richard Wall

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 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall
  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 4 years ago by Richard Wall

Branch: branches/dns-protocol-message-factory-6839branches/dns-protocol-message-factory-6839-2

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

comment:7 in reply to:  5 Changed 4 years ago by Richard Wall

Status: newassigned

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.

Changed 16 months ago by Richard Wall

Attachment: edns-protocols-6839.diff added

Trying to reacquaint myself with the remaining EDNS tasks

Note: See TracTickets for help on using tickets.