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):

 Replying to [comment:13 jelte]:

 === Xfrin ===

 > >  - is there any reason we cannot directly use pairs of (Name, RRClass)
 > >    for the key of _zones{}?  It seems to be an unnecessary overhead to
 > >    convert them to string every time.
 >
 > fundamentally, there isn't.  However, our python wrappers would need to
 be
 > extended to do this.  I propose we make a ticket for that too (i don't
 > think it's too hard, just out of scope)

 I can live with that.  But I just realized there are some minor case
 problems if we keep using strings as keys: case-sensitiveness.
 Consider we store a zone named "example.com", and then a user tries to
 trigger a retransfer specifying "EXAMPLE.COM".  Not actually checked,
 but I suspect it will fail.

 One quick-hack fix would be to convert the name to down-cased one
 within _check_zone_name:
 {{{
 def _check_zone_name(zone_name_str):
     """Checks if the given zone name is a valid domain name, and returns
     it as a Name object. Raises an XfrinException if it is not."""
     try:
         # Later we'll compare zone names as string via to_text(), so
         # to avoid any unexpected comparison result due to case issues
         # we down case the name here.
         return Name(zone_name_str, True)
 }}}

 (and I guess we need a test case for it)

 Or, if we declare this is quite minor and less likely to happen in
 practice (and defer everything to the later ticket), I'm also okay
 with that.

 > >  - _parse_zone_name_and_class and _parse_master_and_port do a
 > >    redundant job of extracting and validating RRclass.
 >
 > it was not entirely the same, it raised a different exception (due to
 > different usage contexts), however, this made me think

 Hmm, okay, but if the string is invalid the first exception will be
 raised so the end result should be identical (right)?

 > XfrinConfigException was a bad name, as it was really only about zone
 > information data, so I changed the exception to XfrinZoneInfoException
 > ('something is wrong with the given zone information').  And the _parse
 > function now calls the same check function.

 And, with that change, I think we can really remove the redundancy.
 I'm attaching a proposed patch.

 > >  - _parse_zone_name_and_class: I also noticed you use 'not XXX' to
 > >    mean "not specified", rather than "is None".  This happens when
 > >    the given string is actually "", rather than "unspecified".  But
 > >    since these will be rejected via the Name()/RRClass() constructors,
 >
 > according to git, that's your code, btw :)

 Oh, is it?  I admit I myself sometimes am confused about None and not:-)

 Comments on the revised code follow:

 '''xfrin_test.py'''
  - in test_command_handler_zones(), zonesN should better be renamed to
    "config1" or "xfrin_config1", etc, because now they are not only
    for zones.

 '''xfrin.py.in'''

 - XfrinZoneInfoException: "when a command does not have the required
   or bad arguments" doesn't seem to parse reasonably. "when a command
   does not have a required argument or has bad arguments"?

 - in set_master_port, should we worry about if the default spec value
   is out of range for a valid port number (e.g. >= 2^16)?
   same for set_zone_class. (Sorry I should have pointed these before.)
   I'm okay with ignoring it as an unlikely scenario (at least for now).

 - Zoneinfo.set_name: is this TODO still active?  From a quick check it
   seems we can safely remove this line:
 {{{
         self.name_str = name_str
 }}}

 '''Other points'''
 - xfrout tests failed for me.  We could fix it, but based on the
   discussion at the sprint planning, I'd rather suggest deferring the
   whole xfrout part to a separate ticket and concentrate on finishing
   xfrin.  at the moment I've not closely looked at the xfrout code.
 - my previous comment about missing test for get_default_value()
   doesn't seem to be addressed.

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