BIND 10 #1986: b10-auth shouldn't try to forward update requests without ddns running

[BIND 10 Development]

#1986: b10-auth shouldn't try to forward update requests without ddns running
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120731
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  b10-auth                           |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  7
            Defect Severity:  N/A    |
Feature Depending on Ticket:  DDNS   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''other/general'''

 - I made a few small editorial changes.
 - I think we need a changelog for this.
 - About the protocol: What if start/stop_ddns_forwarder takes place
   multiple times without having the other?
 - Are we eventually going to generalize the notification mechanism?  I
   think the concept is pretty generic, so ideally we have a unified
   framework and each module just uses the common interface.

 '''auth.spec'''

 - Are these expected to be invoked by a user, too?  If not, what if it
   happens?
 - I'd use 'message(s)' instead of 'packets':
 {{{
         "command_description": "(Re)start internal forwarding of DDNS
 Update packets. This is automatically called if b10-ddns is started.",
 }}}
   same for the stop counterpart.

 '''auth_messages.mes'''

 Just a comment: I wonder whether these messages could be INFO, because
 it should be sufficiently rare and reports some important changes on
 how the system will work.

 '''auth_src.cc'''

 - Is `hasDDNSForwarder()` necessary?  Although the policy already
   seems to be inconsistent (and I may be one of those introducing the
   confusion), since the entire `AuthSrvImpl` is essentially a private
   member of the `AuthSrv` class I don't see the strong need for
   introducing further accessibility barrier within the impl class.
   And, if we make ddns_forwarder_ public, there won't be need for the
   explicit accessor either.  (and I then noticed the same discussion
   applies to the crete/destroy methods in the `Impl` class).

 '''auth_src.h'''

 - I'd use 'message' instead of 'packet' where appropriate.

 '''auth_srv_unittest'''

 shouldn't we test various new cases that can now happen?  e.g., make
 sure auth returns NOTIMP if forwarder isn't created.

 I also guess we should probably confirm the new auth commands actually
 create/destroy the forwarder at the unit test level.

 '''ddns.py'''

 - I'd do s/packets/messges/
 {{{#!python
         # Notify Auth server that DDNS update packets can now be forwarded
         self.__notify_start_forwarder()
 }}}
   same for docstring for `__notify_start/stop_forwarder`, and some
   other places too.

 - command_handler: not only for this case, but I'm not a fan of
   duplicate hardcoding things like `auth_started`.  maybe we can
   consider a generic, cleaner and unified way to handle these cases at
   this opportunity?  (maybe a hardening topic?)

 - `__notify_start_forwarder`: what if group_send/recvmsg raises an
   exception?  Same for stop.
 - `__notify_stop_forwarder`: docstring doesn't seem to be correct
   (copy-paste error).

 - If start_forwarder in response to auth_started fails, it can happen
   both auth and ddns are running but update requests will keep
   resulting in NOTIMP.  I wouldn't require this be resolved in this
   ticket, but I think we need to define design principles of inter
   module communication on, e.g., how much of reliability we'd expect
   and what each module should do to ensure that.

 '''ddns_messages.mes'''

 - 'DDNS_START_FOWARDER_ERROR' should be '...FORWARDER...'.  And this
   seems to suggest we don't have sufficient tests.
 - the description of the error message: I'd describe how this could
   happen and what the operator should do if this message is printed.

 '''ddns_test'''

 - check_session_stop_forwarder_called: docstring isn't correct (copy
   paste)
 - As noted above, test cases where sendmsg etc fail are missing.
 - shouldn't we (also) check "start update" has been sent immediately
   after the creation of DDNS?  Likewise, I guess we should test
   the 'auth_started' command really triggers this message (for that
   matter, this command doesn't seem to be tested at all).

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