BIND 10 #811: Put TSIG field into list of zones we should manage.

[BIND 10 Development]

#811: Put TSIG field into list of zones we should manage.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:         |            Resolution:
  blocker                            |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  Unclassified                       |  Estimated Difficulty:  2.0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:  tsig   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''higher level points'''

 - we'll need a changelog entry for this.

 - are we going to manage the zone info in various places (zonemgr,
   xfrin, auth, and perhaps xfrout)?  Or would there be "master" source
   of the information (in zonemgr?) and others just use a copy (of a
   subset of it)?

 - I tend to agree that we should reject the case of unspecified master
   address.  at least the current default (127.0.0.1) doesn't make
   sense.

 - shouldn't we be able to specify multiple masters?  is it a plan for
   future extension? (I'm okay with that)

 - I couldn't reproduce the step you showed:
 {{{
 > config show Xfrin
 Xfrin/transfers_in      10      integer (default)
 Xfrin/zones     []      list    (default)
 > config add Xfrin/zones
 > config show Xfrin/zones
 Xfrin/zones[0]/name     ""      string  (default)
 [...]
 }}}

 For me, "config add Xfrin/zones" didn't seem to make any effect (but I
 was able to add zone info by config set Xfrin/zones[0]/..., so it was
 not a big problem)

 '''xfrin.py'''

 - I'd add (pydoc) comment to XfrinConfigException

 - I'd avoid using hardcoded magic values ('IN') in the main source
   code like this:
 {{{
         self.class_str = config_data.get('class') or 'IN'
 }}}
   I'd at least like them to be defined as a module specific constant
   at the head of the source file.  further, I wonder whether this
   default should be in the spec file and xfrin should simply refer to
   it?

 - why do we bother with trailing dot like this?
 {{{
         # add the root dot if the user forgot
         if len(self.name) > 0 and self.name[-1] != '.':
             self.name += '.'
 }}}
   Wouldn't it be better to store it as Name objects?  On a related
   note, what if the specified zone is not a valid domain name
   (e.g. "badname..example")?  If we convert it to Name at an earlier
   stage, we can detect the error at that point.

   Probably the same point applies to tsig key, too.

 - _add_zone_info: should we worry about overriding existing info?
   (probably not as this is called in the context of "replace
   everything", but just checking)

 - config_handler doesn't provide the "strong exception guarantee".
   for example, if we add a few zones successfully and then find a
   syntax error for another zone, an incomplete zone list will remain
   in xfrin.  Should we care about such a case?

 '''xfrin_test.py'''
  - in test_command_handler_notify_known_zone, I'd check if the
    configured address/port is used instead of those specified in the
    command (when they are different)
  - we'll probably need test cases for invalid zone names (and perhaps
    invalid tsig key spec too)
  - I'd use 192.0.2.X instead of 1.1.1.1; likewise I'd use
    test.example.com instead of test com.

 '''xfrin.spec'''
  - can't zones[]/class,master_port be optional?  their defaults seem
    to be quite typical.

-- 
Ticket URL: <http://bind10.isc.org/ticket/811#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development