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

 Replying to [comment:14 jelte]:

 > Ack, proposal:
 > [bug] jelte
 > b10-auth no longer tries to send DDNS UPDATE messages to b10-ddns if
 b10-ddns is not running. Sending an UPDATE to BIND 10 that is not
 configured to run DDNS will now result in a response with rcode NOTIMP
 instead of SERVFAIL.

 Looks okay.

 > > '''auth.spec'''
 > >
 > > - Are these expected to be invoked by a user, too?  If not, what if it
 > >   happens?
 >
 > They are not, but we need some form of 'internal' designator for
 commands, so we can verify them but reject them for the user. There is an
 old ticket for that, #941.

 In that case I'd at least clarify in the command_description that they
 are not supposed to be invoked by users.

 > > '''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).
 >
 > They are not necessary, I tend to default to writing accessors over
 direct manipulation of members. Removed.

 In general I prefer providing accessors too because it's usually
 safer.  But in this case it seemed to be overkilling because
 `ddns_forwarder_` etc are essentially a private member of the
 `AuthSrv` class (using a separate struct/class as pimpl is just an
 idiom to hide these details from the users of the `AuthSrv` class).

 > > - 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.
 >
 > yes that is the price of not wanting to forward packets when it is not
 running (or more generally, in order to make use of inter-module
 communication you need to rely on inter-module communication, and that the
 communicating agents play nice)
 >
 > what exactly did you have in mind?

 I don't have a specific idea or proposal right now.  But I guess the
 bind10 process keeps track of more (if it's not sufficient currently)
 details of which module is running and which is not, and how many if a
 module runs multiple instances, and if a module needs to know whether
 or not another module is running (per module or per instance) it gets
 updates on that information from bind10.  The current shutting down
 mechanism will be extended so that it will be more robust and the
 bind10 process can use it.

 > > - shouldn't we (also) check "start update" has been sent immediately
 > >   after the creation of DDNS?  Likewise, I guess we should test
 >
 > that was already tested, added a comment (though the easiest way to test
 the next one included an extra check for this as well)

 I knew check_session_start_forwarder_called() is called in
 test_session_msg() etc, but I thought it's doesn't clearly indicate
 it's done "immediately" after the DDNS creation (depending on what
 other things setUp() does, "start forwarder" may actually be sent
 after some specific event that setUp() happens to invoke.  This is
 actually not the case in the current test code, but the distance
 between the creation and the check seems to make it less reliable.

 One possibility is to do this check within setUp() (a bit redundant to
 do this for every test, though).  Another is to create a separate test
 case where we create a separate `DDNSServer` object with a separate
 `MyCCSession`, and do check_session_start_forwarder_called() for these
 pair immediate after the creation.

 Comments on the revised code follow:

 '''general'''

 - This didn't occur to me in the first review, but I now wonder how
   this notification works with multiple instances of b10-auth.  For
   example, I guess all of them responds to "start forwarding"; could
   it cause any disruption?  Even if not, I suspect it's possible that
   only a subset of auth instance succeeds in setting up the forwarder
   (although it may not be so different from the case where a single
   auth instance fails).  Unless it causes a trouble in normal
   operation, I think it's okay to leave this open, but this seems to
   be another topic in the context of general framework.
 - I've made some editorial cleanups.

 '''auth_src_unittest'''

 - This comment doesn't parse to me:
 {{{#!cpp
     // Test that AuthSrv returns NOTIMP before ddns forwarder is created,
     // that is is connected when the start_ddns_forwarder command is sent,
     // and that is it is no longer connected and returns NOTIMP after
     // the stop_ddns_forwarding command is sent.
 }}}
   s/that is is/that it is/? and s/that is it is/that it is/?

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