Opened 3 years ago

Last modified 3 years 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
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 rwall 7 months ago.
Trying to reacquaint myself with the remaining EDNS tasks

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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.

Changed 7 months ago by rwall

Trying to reacquaint myself with the remaining EDNS tasks

Note: See TracTickets for help on using tickets.