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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:7 jinmei]:
 > '''higher level points'''
 >
 > - we'll need a changelog entry for this.
 >

 oh yes, i propose:
 [func] Xfrin configuration now contains a master server setting per zone,
 and the 'general' master_addr has been removed.  Both xfrin and xfrout
 also have a tsig_key setting (per zone).

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

 Initially i thought the latter, and i started out modifying zonemgr.  But
 then it got unclear (as discussed in the call last week), and I restarted
 doing it on a module-that-actually-needs-the-data basis (i.e. you set
 masters in xfrin, slaves in xfrout, etc).  Zonemgr is strange imo, and i
 am half thinking that in the direction it's currently going we might as
 well put all its logic in xfrin anyway.  There is another change i want
 that makes this a bit more 'usable', that is to not have a List as the
 'zones' config, but a direct dict (where the keys are the zone names), so
 that essentially, you are not configuring the 'zone' as such in xfrin and
 xfrout (and zonemgr), but you configure xfrin-specific settings *for* the
 zone, if you catch my drift.

 Side note, though the current bindctl modifies internal config values
 directly, afaik we are still thinking about having an abstraction between
 the UI and the actual settings (so that settings live where they are used,
 but on the configuration side they may be grouped on a conceptual basis),
 but that's further away I guess.  And I do not have a clear view on how
 this would work with extensibility yet.

 > - 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.
 >

 Yes, and there may not be any settings that make sense (well, perhaps the
 soa value, but you don't have that yet).  It now errors if retransfer is
 given without a master argument and the zone is not known in the
 configuration.

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

 yes.  But since that would also require some more advanced
 fallback-to-next master logic, I omitted it for this task.

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

 really? for me it fails if i try to set values for an item that was not
 added yet.  Is the above a transcript from your session or a copy of my
 example?

 > '''xfrin.py'''
 >
 > - I'd add (pydoc) comment to XfrinConfigException
 >

 Ack, done

 > - 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?
 >

 Oh yes you are right we should take it from spec.
 Changed this.  Added a ConfigData.get_default_value() so modules don't
 have to poke around in specification internals.


 > - 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.
 >

 Good points, done. Class too btw.

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

 While internally it would simply replace it indeed, it could lead to
 unexpected behaviour, so I added a check for it.

 > - 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?
 >

 yeah, i changed it to restore the previous values now

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

 Added this check for 'retransfer'.  In 'notify' the value is actually not
 used yet (and with reason; we should only use the address/port from
 whatever we got the notify from if it matches one of the values we have in
 our configuration.  As long as we can only set one value, it makes no
 sense to use the one given.  In fact I believe it would be a security hole
 if we did.  Added a few extram comments about this to xfrin.py.in).


 >  - we'll probably need test cases for invalid zone names (and perhaps
 >    invalid tsig key spec too)

 ack, done, although i did not add test for every possible bad zone name.

 >  - I'd use 192.0.2.X instead of 1.1.1.1; likewise I'd use
 >    test.example.com instead of test com.
 >

 ohyeah, i just copied the values but changed them now, also added some
 checks to see that the config is actually taken over.

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

 Kind of depends on what we mean with 'optional' here; in my view,
 'optional' does not mean that the user does not need to set it
 explicitely; it means that it can be omitted entirely (with different
 behaviour than when it is not omitted); for instance, tsig_key is
 optional, if not set, no TSIG is used. But we always need a port number
 for a master, so port number is not optional. We don't want the user to
 have to set it explicitely, so if this is not done it will default to 53.

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