Opened 10 years ago

Closed 10 years ago

#5455 enhancement closed duplicate (duplicate)

fix some incorrect dns behaviors

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

Description (last modified by Jean-Paul Calderone)

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 Bob Novas 10 years ago.

Download all attachments as: .zip

Change History (6)

Changed 10 years ago by Bob Novas

comment:1 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added; REVIEW removed

comment:2 Changed 10 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:3 Changed 10 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Bob Novas

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: 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 10 years ago by Itamar Turner-Trauring

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:

comment:5 Changed 10 years ago by Bob Novas

Resolution: duplicate
Status: newclosed

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.