BIND 10 #1414: Zonemgr/secondary_zones[]/class required

[BIND 10 Development]

#1414: Zonemgr/secondary_zones[]/class required
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20111220
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  configuration                      |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  2
Feature Depending on Ticket:  none   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:8 jinmei]:
 > '''zonemgr'''
 >
 > - not really for this branch, but I think we should rather check the
 >   given name is a valid domain name (same for the RR class, btw) by
 >   converting it into a Name object, then dump it to text (lower-cased
 >   with ending period):
 > {{{#!python
 >                     # Be tolerant to sclerotic users who forget the
 final dot
 >                     if name[-1] != '.':
 >                         name = name + '.'
 > }}}
 >   xfrout does this, btw.  See _get_transfer_acl().  It can be deferred
 >   to a separate ticket, though.  I mentioned it simply because I
 >   noticed it while I reading the patch.
 >

 Might as well fix it now, added to-and-from conversion, with exception
 catch to provide a slightly better error (and to convert to one
 exception). Added a few tests as well (but not extensively for every
 possible bad dname)

 > '''zonemgr_test'''
 >
 > - I'd integrate FakeConfig and FakeCCSession (and "MyCCSession"
 >   defined and used somewhere else in the test).  See the attached diff
 >   for concrete ideas.  The point is:
 >   - eliminate hardcoding as much as possible
 >   - integrate functionality FakeConfig and MyCCSession into
 >     FakeCCSession, thus eliminating the need for the former(s)
 >     completely.
 >

 I applied your patch as-is. It did require one extra addition; it
 introduces SPECFILE_LOCATION, which is fine, but then we must also set
 B10_FROM_BUILD in the Makefile test call, so that it calculates the
 correct path for it. Added that too.

 > - (this is not really for this branch) While thinking about this further
 >   refactoring, I realized this part didn't seem to make sense:
 > {{{#!python
 >         # This one does not exist
 >         config['secondary_zones'] = \
 >             zone_list_from_name_classes(["example.net", "CH"])
 >         self.zone_refresh.update_config_data(config, self.cc_session)
 >         self.assertFalse(("example.net.", "CH") in
 >                          self.zone_refresh._zonemgr_refresh_info)
 >         # Simply skip loading soa for the zone, the other configs should
 be updated successful
 >         self.assertFalse(("example.net.", "IN") in
 >                          self.zone_refresh._zonemgr_refresh_info)
 > }}}
 >   (this is after refactoring, but I believe it preserves the test
 >   semantics)  First, I suspect `["example.net", "CH"]` should have
 >   been `[("example.net", "CH")]`.  But then the subsequent test failed.
 >   I couldn't even be sure what exactly these sequences tried to test,
 >   so I simply kept the original semantics even though it didn't make
 >   sense to me.
 >

 I have no idea whether it was intentional, but what it did was add to one-
 letter names with one-letter classes, and then making sure it didn't set
 the original one. Yeah that's weird, and most probably wrong.

 Again, might as well fix it now; I've changed the test; what it now does
 is set the 'normal' name example.net, but class CH (and use the weirdly
 named but conveniently defined ZONE_NAME_CLASS1_CH for that), then checks
 whether the zonemgr did not make IN of it.

 Whatever the test was supposed to do, it probably changed :p

 Removed the duplicate test after 'skip soa'. No idea what that was for.


 > '''Others'''
 >
 > - I've made a couple of minor editorial cleanups.

 thanks

 > - I believe we need a changelog entry for this.

 [bug]   jelte
 Fixed a bug where adding Zonemgr/secondary_zones without explicitely
 setting the class value of the added zone resulted in a cryptic error in
 bindctl ("Error: class"). It will now correctly default to IN if not set.
 This also adds better checks on the name value, and a better error when it
 is bad.

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