BIND 10 #1643: TSIG configuration syntax should be as consistent as possible for auth and xfrout

[BIND 10 Development]

#1643: TSIG configuration syntax should be as consistent as possible for auth and
xfrout
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120306
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  configuration                      |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 The branch generally looks okay.  I've made a few minor editorial
 changes including some trivial corrections in code comments.  I have
 some other small things to note, which follow:

 '''general'''

 Ideally, I'd like to confirm actual zone transfer using TSIG via
 system tests.  Since configuration involves multiple processes, and
 also we now have additional indirection to the global key ring,
 I'm afraid it's more likely to have a system-level bug that cannot be
 detected via unittests.  This could be a separate deferred ticket
 though.

 '''ccsession.py'''

 - Please explain a bit more rationale about this change.  It's not
   crystal clear.  (maybe we need to add the implication to the
   add_remote_config() description)
 {{{
     * The config callback should be called after the module is ready.
 }}}

 - not really for this branch, but `_add_remote_config_internal` seems
   to ignore some error cases:
   - non-0 rcode or value is None
   - when validate_config() fails

 - add_remote_config_by_name: 'env' doesn't seem to be used and can be `_`:
 {{{#!python
             answer, env = self._session.group_recvmsg(False, seq)
 }}}
   (not a big deal though)

 '''ccsession_test.py'''

 - _internal_remote_module_with_custom_config: this comment (still)
    refers to add_remote_config even if the method is generalized:
 {{{#!python
         # override the default config value for "item1".
 add_remote_config()
 }}}
 - error cases do not seem to be tested like this one:
 {{{#!python
                 if module_spec.get_module_name() != module_name:
                     raise ModuleCCSessionError("Module name mismatch: " +
                                                module_name + " and " +
 module_spec.get_module_name())
 }}}
   (you may also want to run pycoverage)

 '''tsig_keyring.py'''

 - is it okay not to provide any logging?

 - `Updater._keyring` could be "declared" as private, i.e.,
   '__keyring'?  Same for `_update()`.
 - for naming consistency, "keyring()" may be better named "get_keyring()"?
 - did you actually mean "duplicate", not "duplicity"?
 {{{
     Raised when a key can not be added. This usually means there's a
     duplicity.
 }}}
   in case you mean it: according to my dictionary "duplicity" is
   uncountable, so it should be "there's duplicity".  Same comment
   applies to the comment in test_update_bad.

 - do you mean "i.e." instead of "eg."?  ('eg. = for example' doesn't
   make much sense to me in this context)
 {{{
         The parameters are there just to match the signature which
         the callback should have (eg. they are ignored).
 }}}

 '''tsig_keyring_test.py'''

 - is it an okay behavior to do init_keyring() after deinit?  If so,
   should we test that case too?
 - I'd test either/both of loading/updating a keyring that has multiple
   keys.  I'd also test whether we can make the keyring empty on update.

 '''xfrout_test.py'''

 - Shouldn't we check if the global keyring is actually used in xfrout?

 '''bind10-guide'''
 - do you mean 'para', instead of 'param'?
 {{{
     <param>Both Xfrout and Auth will use the system wide keyring to check
     TSIGs in the incomming messages and to sign responses.</param>
 }}}

 '''changelog'''

 - s/tsig_kes/tsig_key_ring/
 {{{
 However, the old
 configuration of Xfrout/tsig_kes need to be removed for Xfrout to
 work.
 }}}

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