Opened 8 years ago

Closed 6 years ago

#2276 enhancement closed fixed (fixed)

Changes to TwistedNames to make it support NAPTR records

Reported by: agatonsax Owned by:
Priority: normal Milestone:
Component: names Keywords:
Cc: therve Branch: branches/names-naptr-2276-2
(diff, github, buildbot, log)
Author: exarkun, agatonsax Launchpad Bug:

Description

There is presently no support for NAPTR records in either the client or the server code and I needed both. So I have made some small modifications to the code such that TwistedNames would support NAPTR records both on the client and the server side.

Attachments (5)

common.py.patch (748 bytes) - added by agatonsax 8 years ago.
Patch to names/common.oy
dns.py.patch (4.3 KB) - added by agatonsax 8 years ago.
Patch to names/common.oy
test_names.py.patch (1.2 KB) - added by agatonsax 8 years ago.
Patch to names/testnames.py
patch.1 (4.2 KB) - added by agatonsax 6 years ago.
Added documentation and test
patch.2.1 (6.0 KB) - added by agatonsax 6 years ago.
Replaced patch.1 with a new version that will fix what's needed

Download all attachments as: .zip

Change History (22)

Changed 8 years ago by agatonsax

Patch to names/common.oy

Changed 8 years ago by agatonsax

Patch to names/common.oy

Changed 8 years ago by agatonsax

Patch to names/testnames.py

comment:1 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

comment:2 Changed 6 years ago by therve

  • author set to therve
  • Branch set to branches/names-naptr-2276

(In [23395]) Branching to 'names-naptr-2276'

comment:3 Changed 6 years ago by therve

(In [23396]) Check the patched in, with some small fixes.

Refs #2276

comment:4 Changed 6 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to agatonsax

First, thanks a lot for your contribution. I'm sorry we lost some much time handling it. I think it's already in pretty good shape, but it would need more documentation and tests to land in trunk. Are you willing to help on these topics? Thanks again.

comment:5 Changed 6 years ago by therve

(Received by mail:)

Could do that!
What would the process be ?

Awesome :). First, it would be great if we could discuss it here. Basically, your patch needs:

  • unit tests for all inserted code.
  • docstrings, for every new methods and attributes.

The classes Charstr and Record_NAPTR need direct tests, not test throught the API. Also, it would be great to test some failure conditions of lookupNAPTR.

The following patched should be against the branch I created (names-naptr-2276) for convenience.

Don't hesitate to come back to me if you have more questions.

Thanks!

Changed 6 years ago by agatonsax

Added documentation and test

comment:6 Changed 6 years ago by therve

  • Keywords review added
  • Owner agatonsax deleted

comment:7 follow-up: Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to agatonsax

Great, thanks a lot for the documentation effort. I think we're almost there. Last points:

  • the compDict parameter of Charstr.encode is not tested
  • client.lookupNamingAuthorityPointer is not tested.

Back to you.

Changed 6 years ago by agatonsax

Replaced patch.1 with a new version that will fix what's needed

comment:8 in reply to: ↑ 7 Changed 6 years ago by agatonsax

Replying to therve:

Great, thanks a lot for the documentation effort. I think we're almost there. Last points:

  • the compDict parameter of Charstr.encode is not tested

compDict should not be used for Charstr, since the compression algorithm specified in RFC1037 only is valid for domain names. So I have 'removed' it.

  • client.lookupNamingAuthorityPointer is not tested.

It is now.

comment:9 Changed 6 years ago by therve

(In [23500]) Apply patch

Refs #2276

comment:10 Changed 6 years ago by therve

  • Keywords review added
  • Owner agatonsax deleted

comment:11 Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to agatonsax

OK I was hoping someone with better DNS knowledge would take a look, but everyone is pretty busy.

It looks good to me, but as I don't know at all this, I'd like to be convinced it's good. If you can you provide one or two small examples with twisted code and an external thing (dig?), I'll be happy to merge that.

Thanks and sorry for the delay.

comment:12 Changed 6 years ago by exarkun

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

comment:13 Changed 6 years ago by exarkun

  • author changed from therve to exarkun
  • Branch changed from branches/names-naptr-2276 to branches/names-naptr-2276-2

(In [24371]) Branching to 'names-naptr-2276-2'

comment:14 Changed 6 years ago by exarkun

  • author changed from exarkun to exarkun, agatonsax
  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

Seems good to me as well. I made some minor changes, including some new tests and a fix for the hashability of Charstr. I'm not sure what kind of example you were hoping for. How's this?

exarkun@charm:/tmp$ cat zone
zone = [SOA('foobar'), NAPTR('foobar', 1, 2, 's', '/foo/bar/i', '')]
exarkun@charm:/tmp$ cat zone
zone = [SOA('foobar'), NAPTR('foobar', 1, 2, 's', '/foo/bar/i', '')]
exarkun@charm:/tmp$ twistd dns --port 2053 --pyzone zone
exarkun@charm:/tmp$ dig -t any foobar @localhost -p 2053

; <<>> DiG 9.3.2-P2.1 <<>> -t any foobar @localhost -p 2053
; (1 server found)
;; global options:  printcmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 26891
;; flags: qr aa rd; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;foobar.                                IN      ANY

;; ANSWER SECTION:
foobar.                 0       IN      SOA     . . 0 0 0 0 0
foobar.                 0       IN      NAPTR   1 2 "s" "/foo/bar/i" "" .

;; Query time: 1 msec
;; SERVER: 127.0.0.1#2053(127.0.0.1)
;; WHEN: Fri Jul 25 14:24:10 2008
;; MSG SIZE  rcvd: 89

exarkun@charm:/tmp$

comment:15 Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Cool, please merge.

comment:16 Changed 6 years ago by exarkun

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

(In [24505]) Merge names-naptr-2276-2

Author: agatonsax, exarkun
Reviewer: exarkun, therve
Fixes: #2276

Add support for the NAPTR record type to twisted.names.

comment:17 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.