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:12 jinmei]:
 > '''other/general'''
 >
 > - I made a few small editorial changes.

 thnx

 > - I think we need a changelog for this.

 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.

 > - About the protocol: What if start/stop_ddns_forwarder takes place
 >   multiple times without having the other?

 For multiple starts, it results in the connection being re-established (to
 cover the case where b10-ddns dies without being able to send its goodbye
 message). For multiple stops, it is a no-op after the first one (or before
 start has even happened).

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

 I'm not against it, but have no current idea as to how.

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

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

 done

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

 I am not heavily opposed, but I chose (low) debug, to not add to the
 number of startup messages.

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

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

 only one case, right? fixed.

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

 oh, right, kind of forgot about that given the system tests.

 Added a test for this, which also checks behaviour is correct when sending
 multiple starts and stops.

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

 done

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

 yeah, I didn't want to write a wrapper just for a message string at this
 moment, but we can certainly consider a better way to do this.

 > - `__notify_start_forwarder`: what if group_send/recvmsg raises an
 >   exception?  Same for stop.

 catching it and logging error now

 > - `__notify_stop_forwarder`: docstring doesn't seem to be correct
 >   (copy-paste error).
 >

 fixed

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

 > '''ddns_messages.mes'''
 >
 > - 'DDNS_START_FOWARDER_ERROR' should be '...FORWARDER...'.  And this
 >   seems to suggest we don't have sufficient tests.

 hmm, indeed. Fixed and added (also for exceptions from sendmsg/recvmsg)

 > - the description of the error message: I'd describe how this could
 >   happen and what the operator should do if this message is printed.
 >

 added about the same info as for when exceptions are raised from
 send/recv, but for the real cause of the error one should check auth's
 logs, this module won't know any more than that it got an error back.

 > '''ddns_test'''
 >
 > - check_session_stop_forwarder_called: docstring isn't correct (copy
 >   paste)

 fixed


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

 >   the 'auth_started' command really triggers this message (for that
 >   matter, this command doesn't seem to be tested at all).

 added

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