[1451] address review comments

src/bin/ddns/b10-ddns.8

@@ -31,12 +31,12 @@
 b10-ddns \- Dynamic DNS update service
 .SH "SYNOPSIS"
 .HP \w'\fBb10\-ddns\fR\ 'u
-\fBb10\-ddns\fR [\fB\-v\fR] [\fB\-\-verbose\fR]
+\fBb10\-ddns\fR [\fB\-v\fR | \fB\-\-verbose\fR]
 .SH "DESCRIPTION"
 .PP
 The
 \fBb10\-ddns\fR
-daemon provides the BIND 10 DDNS update service\&. Normally it is started by the
+daemon provides the BIND 10 Dynamic Update (DDNS) service, as specified in RFC 2136\&. Normally it is started by the
 \fBbind10\fR(8)
 boss process\&. When the
 \fBb10\-auth\fR
@@ -54,13 +54,21 @@ will exit\&.
 \fBb10\-ddns\fR
 receives its configurations from
 \fBb10-cfgmgr\fR(8)\&.
+.SH "ARGUMENTS"
+.PP
+The arguments are as follows:
+.PP
+\fB\-v\fR, \fB\-\-verbose\fR
+.RS 4
+This value is ignored at this moment, but is provided for compatibility with the bind10 Boss process
+.RE
 .SH "CONFIGURATION AND COMMANDS"
 .PP
 The configurable settings are:
 .PP
 
-\fITODO\fR
-TODO
+\fIzones\fR
+The zones option is a named set of zones that can be updated with DDNS\&. Each entry has one element called update_acls, which itself is a list of ACLs that define update permissions\&. By default this is empty; DDNS must be explicitely enabled per zone\&.
 .PP
 The module commands are:
 .PP

src/bin/ddns/b10-ddns.xml

@@ -44,15 +44,17 @@
   <refsynopsisdiv>
     <cmdsynopsis>
       <command>b10-ddns</command>
-      <arg><option>-v</option></arg>
-      <arg><option>--verbose</option></arg>
+        <group choice="opt">
+          <arg choice="plain"><option>-v</option></arg>
+          <arg choice="plain"><option>--verbose</option></arg>
+        </group>
     </cmdsynopsis>
   </refsynopsisdiv>
 
   <refsect1>
     <title>DESCRIPTION</title>
     <para>The <command>b10-ddns</command> daemon provides the BIND 10
-      DDNS update service.
+      Dynamic Update (DDNS) service, as specified in RFC 2136.
       Normally it is started by the
       <citerefentry><refentrytitle>bind10</refentrytitle><manvolnum>8</manvolnum></citerefentry>
       boss process.
@@ -75,16 +77,40 @@
   </refsect1>
 
   <refsect1>
+    <title>ARGUMENTS</title>
+
+    <para>The arguments are as follows:</para>
+
+    <variablelist>
+
+      <varlistentry>
+        <term>
+          <option>-v</option>,
+          <option>--verbose</option>
+        </term>
+        <listitem>
+          <para>
+            This value is ignored at this moment, but is provided for
+            compatibility with the bind10 Boss process
+          </para>
+        </listitem>
+      </varlistentry>
+    </variablelist>
+  </refsect1>
+
+  <refsect1>
     <title>CONFIGURATION AND COMMANDS</title>
     <para>
       The configurable settings are:
     </para>
     <para>
-      <varname>TODO</varname>
-      TODO
+      <varname>zones</varname>
+      The zones option is a named set of zones that can be updated with
+      DDNS. Each entry has one element called update_acls, which itself
+      is a list of ACLs that define update permissions.
+      By default this is empty; DDNS must be explicitely enabled per zone.
     </para>
 
-<!-- TODO: formating -->
     <para>
       The module commands are:
     </para>

src/bin/ddns/ddns.py.in

@@ -35,7 +35,7 @@ logger = isc.log.Logger("ddns")
 
 DATA_PATH = bind10_config.DATA_PATH
 if "B10_FROM_BUILD" in os.environ:
-    DATA_PATH = DATA_PATH + "/src/bin/ddns"
+    DATA_PATH = os.environ['B10_FROM_BUILD'] + "/src/bin/ddns"
 SPECFILE_LOCATION = DATA_PATH + "/ddns.spec"
 
 
@@ -75,14 +75,22 @@ class DDNSSession:
         pass
 
 class DDNSServer:
-    def __init__(self):
+    def __init__(self, cc_session = None):
         '''
         Initialize the DDNS Server.
         This sets up a ModuleCCSession for the BIND 10 system.
+        Parameters:
+        cc_session: If None (default), a new ModuleCCSession will be set up
+                    If specified, the given session will be used. This is
+                    mainly used for testing.
         '''
-        self._cc = isc.config.ModuleCCSession(SPECFILE_LOCATION,
-                                              self.config_handler,
-                                              self.command_handler)
+        if cc_session is not None:
+            self._cc = cc_session
+        else:
+            self._cc = isc.config.ModuleCCSession(SPECFILE_LOCATION,
+                                                  self.config_handler,
+                                                  self.command_handler)
+
         self._config_data = self._cc.get_full_config()
         self._cc.start()
         self._shutdown = False
@@ -102,7 +110,7 @@ class DDNSServer:
             self.shutdown()
             answer = create_answer(0)
         else:
-            answer = create_answer(1, "Unknown command:" + str(cmd))
+            answer = create_answer(1, "Unknown command: " + str(cmd))
         return answer
 
     def shutdown(self):
@@ -122,18 +130,23 @@ class DDNSServer:
         while not self._shutdown:
             self._cc.check_command(False)
 
-ddns_server = None
-
-def signal_handler(signal, frame):
+def create_signal_handler(ddns_server):
     '''
-    Handler for process signals. Since only signals to shut down are sent
-    here, the actual signal is not checked and the server is simply shut
-    down.
+    This creates a signal_handler for use in set_signal_handler, which
+    shuts down the given DDNSServer (or any object that has a shutdown()
+    method)
     '''
-    if ddns_server is not None:
-        ddns_server.shutdown()
+    def signal_handler(signal, frame):
+        '''
+        Handler for process signals. Since only signals to shut down are sent
+        here, the actual signal is not checked and the server is simply shut
+        down.
+        '''
+        if ddns_server is not None:
+            ddns_server.shutdown()
+    return signal_handler
 
-def set_signal_handler():
+def set_signal_handler(signal_handler):
     '''
     Sets the signal handler(s).
     '''
@@ -152,13 +165,15 @@ if '__main__' == __name__:
         parser = OptionParser()
         set_cmd_options(parser)
         (options, args) = parser.parse_args()
-        VERBOSE_MODE = options.verbose
+        if options.verbose:
+            print("[b10-ddns] Warning: -v verbose option is ignored at this point.")
 
         ddns_server = DDNSServer()
-        set_signal_handler()
+        logger.debug(10, DDNS_STOPPED_BY_KEYBOARD)
+        set_signal_handler(create_signal_handler(ddns_server))
         ddns_server.run()
     except KeyboardInterrupt:
-        logger.INFO(DDNS_STOPPED_BY_KEYBOARD)
+        logger.info(DDNS_STOPPED_BY_KEYBOARD)
     except SessionError as e:
         logger.error(DDNS_CC_SESSION_ERROR, str(e))
     except ModuleCCSessionError as e:
@@ -167,7 +182,3 @@ if '__main__' == __name__:
         logger.error(DDNS_CONFIG_ERROR, str(e))
     except SessionTimeout as e:
         logger.error(DDNS_CC_SESSION_TIMEOUT_ERROR)
-
-    if ddns_server:
-        ddns_server.shutdown()
-

src/bin/ddns/tests/ddns_test.py

@@ -17,10 +17,55 @@
 
 import unittest
 import isc
+import ddns
+import isc.config
 
-class TestInitialization(unittest.TestCase):
-    def test_noop(self):
-        self.assertTrue(True)
+class MyCCSession(isc.config.ConfigData):
+    '''Fake session with minimal interface compliance'''
+    def __init__(self):
+        module_spec = isc.config.module_spec_from_file(
+            ddns.SPECFILE_LOCATION)
+        isc.config.ConfigData.__init__(self, module_spec)
+        self._started = False
+
+    def start(self):
+        '''Called by DDNSServer initialization, but not used in tests'''
+        self._started = True
+
+class TestDDNSServer(unittest.TestCase):
+    def setUp(self):
+        cc_session = MyCCSession()
+        self.assertFalse(cc_session._started)
+        self.ddns_server = ddns.DDNSServer(cc_session)
+        self.assertTrue(cc_session._started)
+
+    def test_config_handler(self):
+        # Config handler does not do anything yet, but should at least
+        # return 'ok' for now.
+        new_config = {}
+        answer = self.ddns_server.config_handler(new_config)
+        self.assertEqual((0, None), isc.config.parse_answer(answer))
+
+    def test_shutdown_command(self):
+        '''Test whether the shutdown command works'''
+        self.assertFalse(self.ddns_server._shutdown)
+        answer = self.ddns_server.command_handler('shutdown', None)
+        self.assertEqual((0, None), isc.config.parse_answer(answer))
+        self.assertTrue(self.ddns_server._shutdown)
+
+    def test_command_handler(self):
+        '''Test some commands.'''
+        # this command should not exist
+        answer = self.ddns_server.command_handler('bad_command', None)
+        self.assertEqual((1, 'Unknown command: bad_command'),
+                         isc.config.parse_answer(answer))
+
+    def test_signal_handler(self):
+        '''Test whether signal_handler calls shutdown()'''
+        signal_handler = ddns.create_signal_handler(self.ddns_server)
+        self.assertFalse(self.ddns_server._shutdown)
+        signal_handler(None, None)
+        self.assertTrue(self.ddns_server._shutdown)
 
 if __name__== "__main__":
     isc.log.resetUnitTestRootLogger()