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:10 jinmei]:
 > Replying to [comment:9 jelte]:
 >

 skipping the initial part, all good points (and i had plans myself for
 some of them as well, especially relating to the unspecified commands,
 which was actually a surprise to me and we should certainly not have that,
 but outside the scope of this ticket indeed, will defer this to list or
 ticket).


 > '''xfrin_test.py'''
 >
 > - MockCC.get_default_value: maybe it's better to use TEST_RRCLASS_STR
 >   instead of hardcoded 'IN'
 >

 ack

 > - MockXfrin.xfrin_start: wouldn't it be better to pass the given
 >   check_soa to the super class rather than resetting it to True?
 > {{{
 >         return Xfrin.xfrin_start(self, zone_name, rrclass, db_file,
 >                                  master_addrinfo, tsig_key_str,
 >                                  check_soa=True)
 > }}}
 >   (i.e., removing '=True')
 >

 woops, of course.

 > - MockXfrin.xfrin_start: the following attributes don't seem to be
 >   used: xfrin_started_zone_name, xfrin_started_rrclass,
 >   xfrin_started_tsig_key_str
 >

 oh yeah i wasn't sure we needed them in our set of tests, removed them

 > - test_command_handler_retransfer_short_command2: this is now
 >   equivalent to test_command_handler_retransfer_short_command1 and
 >   redundant.  Previously it added a trailing dot to the zone name to
 >   see if the behavior is the same.  You may still want to do this
 >   check somewhere, but since this test now expects a failure (due
 >   to a different reason) I think such a test should go to somewhere
 >   else.  as a result this test case should probably be just removed.
 >

 yes, removed short_command2, renamed 3 to 2, and added a copy of 3 that
 does the same test without the trailing dot.

 > - test_command_handler_retransfer_quota: better to use
 >   TEST_MASTER_IPV4_ADDRESS than hardcoding '127.0.0.1'
 >

 ack

 > - test_command_handler_notify_known_zone: the line number in the
 >   comment doesn't make sense to me.  Such hardcoded line numbers are
 >   quit susceptible to further changes, so I'd suggest using a bit
 >   stabler tag (e.g. a method name)
 > {{{
 >         # would at this point not make sense, see the TODO in
 >         # xfrin.py.in:542)
 > }}}
 >

 ok

 > - I think it would be better to check a case of invalid tsig string.
 >   It should be a trivial copy-and-modify of existing tests, so I wrote
 >   it myself and pushed it.
 >

 ok thanks

 > '''xfrin.py'''
 >
 > (These are actually specific instances of the general issue I
 > mentioned at the beginning, so depending on how we handle it we may
 > defer them to a separate ticket)
 >
 > - _check_zone_class: do we need to consider the case where
 >    zone_class_str is None?  If it's a non optional config parameter,
 >    this seems to be treated as an error (just like the case where the
 >    zone name or master address isn't specified).
 >
 > - Likewise, do we need to have DEFAULT_MASTER_PORT in Xfrin and use it
 >   as a last resort?  Can't we basically rely on the validity check of
 >   the config module and simply treat it as an error if it's unexpectedly
 >   unspecified?
 >

 We do, at the very least, we do for now.  Not for config (since it is
 indeed non-optional), but for command parsing (where the zone class is
 optional, and we don't have that same defaults stuff (yet)).

 I've added a module constand DEFAULT_ZONE_CLASS, and added a comment to
 both these constants why we need them (now), and a TODO to have a solution
 similar to what we have for config options.

 > === ZoneInfo ===
 >  - is it okay to omit module_cc (in which case None is used)?  no one
 >    seems to benefit from omitting it, and if it's omitted and 'class'
 >    isn't specified in config_data, it will cause disruption.

 ack, removed default None

 >  - is there any reason for storing tsigkey as string?  This way we
 >    need to convert it to a TSIGKey object every time we use it.
 >  - similarly, do we need to maintain self.master_addr_str and
 >    self.master_port_str separately from self.master_{addr, port}?
 >

 These values are now no longer stored as strings.  I refactored the
 ZoneInfo class a bit, as the initializer got way too long )it now calls a
 set of set_foo(foo_str) functions that do any checking/default getting.

 name_str and class_str are still stored separately, added TODO for those
 (but it's a bit tied into the rest of the (original) code)

 > === Xfrin ===
 >
 >  - _get_zone_info: it seems the if-else clause could be simplified:
 > {{{
 >         return self._zones.get(key)
 > }}}
 >

 ack. local variable key is then also not necessary anymore

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

 >  - do we need the indirection of _get_all_zone_info()?  it seems to me
 >    we can simply use _zones in config_handler() (both are in the same
 >    class), which is (currently) the only user of this method.  Same
 >    for _set_all_zone_info().
 >

 no, we don't.  It was mostly to keep operations on _zones on the same
 level.  Removed them.  Also noticed that 'the other' value could still be
 updated even if the config as a whole was rejected, so it now reverts that
 too (if we add more options that may fail, we will probably want to
 generalize this, either through a backup_config object, a pending_config
 object, and/or a set of calls (not to mention that we also need a bigger
 three-phase-commit type thing))

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

 >  - _parse_zone_name_and_class: as commented above, it would be better
 >     to treat the case of '!rrclass' as an error.  (note: this is
 >     another instance of the "general issues")
 >

 also see earlier

 >  - _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 :)
 changed it to use is None

 >    we could actually unify the check (assuming we trust the default
 >    RRClass provided by the config module + xfrin spec).
 >
 >  - _parse_master_and_port(): same point for the port: can't we treat the
 >    case of 'port is None' as an error?
 >

 yes. But both depend on the defaults thingy for commands, so right now,
 no.

 btw, with the values no longer stored in zoneinfo as strings, i changed
 this method to only parse them when given as a command argument (and
 otherwise directly use the zoneinfo members), and removed build_addr_info.

 It still needs to convert back to string at the end, but i figured i
 refactored enough as a side effect for this ticket, and am considering
 ways to use ZoneInfo for command arguments as well (and then pass that to
 the actual xfrin functions instead of these ad-hoc tuples), but ha no
 complete idea for this yet.

 > '''config_data.py'''
 >
 > we probably need a test for get_default_value inside the config module.
 > but note also that we may want to introduce a different interface to
 > handle config/command defaults (see the discussion at the beginning).
 >
 > '''ChangeLog'''
 >
 > - I think we need to add "*" because it changes the UI quite a lot.
 > - Some short example of how to add a zone may be helpful.

 ok

 > - I didn't understand the "xfrout" part: "Both xfrin and xfrout
 >   also have a tsig_key setting (per zone)."
 >

 well I added tsig_key to xfrout, but it did not have any other tsig
 related code yet, so it was mostly just the addition in the spec, as i did
 not want to add too much code that was not used yet.  Updated it to at
 least convert the tsig key.

 it also had no config-error checking whatsoever so i added something kind
 of similar to what we have in xfrin.

 The code here is not 'as done' as in xfrin, and what there is shows quite
 a few similarities, so we may want to refactor it later.  Having said
 that, I do expect them to diverge, and i would like to first know how
 before we do that :)

 > '''Other minor points'''
 >
 > - As we briefly discussed on jabber, I noticed the copyright year was
 >   changed from 2009 to 2011.  There doesn't seem to be consensus about
 >   it or it's not even clear whether we should have a consensus, so I'm
 >   just making a note about this and leave it to you.
 >

 made it into 2009-2011 (and 2010-2011), don't really care too much, but we
 may want to have a guideline on this

 > - I made some minor editorial changes and pushed them.  I believe the
 >   intent is clear enough so I don't explain details.
 >

 ok thanks

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