Opened 3 years ago

Closed 2 years ago

#5455 enhancement closed duplicate (duplicate)

fix some incorrect dns behaviors

Reported by: BobNovas Owned by: BobNovas
Priority: normal Milestone:
Component: names Keywords: DNS, BIND
Cc: Branch:
Author: Bob Novas Launchpad Bug:

Description (last modified by exarkun)

This patch, applied to twisted 11.1.0 after the patch in 5454, does the following:

  1. Causes FileAuthority to search for a name suffixed with a trailing period if it doesn't find the name in records. The reason for this is that if records are created from a bind file, they may be created with a trailing dot, but FileAuthority will never find them since it will never search for a name with a trailing dot.
  2. Allows a DNS Secondary server to use a primary DNS server that is serving on a port other than port 53.
  3. Fixes problems with parsing a bind format zone file. The previous parser was simply wrong. It regarded leading whitespace as irrelevant, when in fact leading whitespace is significant. leading whitespace indicates the absence of a name.
  4. Fixes a bug that others have noticed with connectionLost getting called on UDP protocol. This bug happens when a DNS query issued over UDP is retried on TCP. Given no further DNS activity, 30 seconds after the TCP query completes, connectionLost is called on the UDP protcol and, absent this method, raises an exception.

Attachments (1)

add-server-behavior-and-misc-03.patch (6.2 KB) - added by BobNovas 3 years ago.

Download all attachments as: .zip

Change History (6)

Changed 3 years ago by BobNovas

comment:1 Changed 3 years ago by exarkun

  • Keywords review added; REVIEW removed

comment:2 Changed 3 years ago by exarkun

  • Description modified (diff)

comment:3 Changed 3 years ago by itamar

  • Keywords review removed
  • Owner set to BobNovas

Thanks for submitting these bug fixes!

2 and 4 sound good just from the description. Sadly, I personally do not know enough about DNS to comment on 1 and 3 as part of this review without brushing up on the subject. Before doing the research, I'd want to see unit tests -- none of the four changes you include here have unit tests, which are required in order for them to be merged, and tests make it much easier to see if code is correct.

More minor issues:

  1. You should take a look at the coding standard: http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html. E.g. you shouldn't name have variables like_this, but rather likeThis. Following this is necessary to get the patch in, but I've sometimes just cleaned up small patches like this one when I apply them to the repository. For big patches better to get it right from the start, if possible.
  2. You might not want to name a variable type, since that's a Python built-in.

comment:4 Changed 3 years ago by itamar

Oh, I should also note that having a separate ticket for each bug fix would allow me to e.g. easily review fixes 2 and 4 while leaving 1 and 3 for someone who understands DNS better.

You might find this useful when testing the timeout bug:
http://twistedmatrix.com/documents/current/core/howto/trial.html#auto9

comment:5 Changed 2 years ago by BobNovas

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

This ticket was split into 4 tickets for review and hence duplicates them. The tickets are:

5467 - Fix BindAuthority for bind files with absolute names
5468 - Add port parameter to DNS secondary server setup
5470 - Fix BindAuthority's parsing of a bind zone file
5471 - Add necessary but missing connectionLost method to client.Resolver

Note: See TracTickets for help on using tickets.