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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:15 jinmei]:
 >
 > > > '''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.
 >

 done (and that they will be removed as public commands in the future)

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

 ah, right, yes, IIRC that was part of the intended msgq changes. I was
 thinking more generally of communication protocols between modules (even
 one-shot module-to-module commands)

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

 oh actually it is a relatively light check that can be performed in setup,
 moved it.
 (the only potential problem is that other tests might fail on this if
 there is a bug here, if we see that as a problem, but if any fail this
 would need to be fixed anyway)

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

 ack

 > - I've made some editorial cleanups.
 >

 of course :)

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

 ack, also made explicit references to forwarder and auth in an attempt to
 be more clear

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