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:9 jelte]:

 I'll first reply to selected parts of your response.  For some of the
 others, see comments on the revised code.  For other parts that are
 not covered either in the explicit response or in the code comment, I
 have no further comment.

 > Replying to [comment:7 jinmei]:

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

 I think I vaguely understand the idea.  In any case I'm okay with the
 current way.  As we have more concrete examples we'll have a more
 clear idea about how we should manage the information.

 > > - I couldn't reproduce the step you showed:
 [...]
 > > 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?

 Maybe it was my misoperation.  I tried it again and this time I got
 the same result.

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

 Yes, I understand the security implication (it was me who first
 noticed that in a previous xfrin implementation and disabled that:-).
 My original comment was intended to say "we should use configured
 address/port (for security reasons) and we should test to confirm
 that".

 Now, specific comments on the revised branch:

 '''About default config/command parameters'''

 (This part is long and may be beyond the scope of this ticket.  I'm
 okay with deferring the discussion/development for this to a separate
 ticket)

 After looking at it more closely, I'm now feeling it's not clear who
 is responsible for applying the default or for checking the existence
 of mandatory parameters.  It first occurred to me "why do we need to
 write down these checks or apply the default when something is
 unspecified within xfrin?  Isn't it a job of ccsession?"  Then I
 noticed various related issues:

 - the python version of ModuleCCSession class doesn't validate
   incoming commands unlike the C++ version.  This might be because
   some commands (like "notify") are "internal" and aren't listed in
   the spec file, but IMO we should centralize the command syntax
   information (and if we don't want to hide something from general UI,
   add a property field for that purpose in the spec file) rather than
   having the application act differently.
 - ModuleCCSession actually doesn't support completing config/command
   for (mandatory) parameters that has the default when they are
   unspecified.   The latest version provides get_default_value(), but
   the caller still has to do the job of applying the default when
   something is missing.
 - bindctl doesn't seem to use the spec default while it checks
   missing mandatory parameter by itself (but this would be far beyond
   the scope of this ticket)
 - It's not clear what it means if a parameter is "optional" but has
   (meaningful) default.  In the case of xfrin, they are "zone_class"
   and "port" for the retransfer command, and are essentially mandatory
   parameters, and xfrin uses hardocoded defaults (which happen to be
   consistent with the ones in the spec file).  For "master", xfrin
   uses the address configured in zone when it's missing, and it's
   actually optional, but the default value will never make sense (it's
   an invalid string as an IPv6/v4 address).

 I think a better solution is:
 - add syntax validation to
 ModuleCCSession.check_command_without_recvmsg().
   if necessary, extend the spec file so that it will include "hidden"
   commands, too (as noted above).
 - add an ability to ModuleCCSession to complete config/command params
   when some mandatory params with defaults are missing.  Maybe it can
   be part of check_command_without_recvmsg() and/or it can provide a
   separate API, say, ModuleCCSession.complete_command(args), which
   would apply all defaults for unspecified mandatory parameters.
 - the application generally relies on the check/completion of
   ModuleCCSession.  In particular, the application doesn't care about
   how to apply default values when something that has a default is
   missing.  but in terms of the defense in depth the app may at least
   want to check if a mandatory parameter is really provided.

 '''xfrin_test.py'''

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

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

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

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

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

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

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

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

 === 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.
  - 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}?

 === Xfrin ===

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

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

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

  - _parse_zone_name_and_class and _parse_master_and_port do a
    redundant job of extracting and validating RRclass.

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

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

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

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

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

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