[1986] Address review comments, auth part

- changed a number of 'DDNS packet' to 'DDNS message'
- remove accessors in already private class AuthSrvImpl
- added unittests for start/stop ddns forwarder

src/bin/auth/auth.spec.pre.in

@@ -134,12 +134,12 @@
       },
       {
         "command_name": "start_ddns_forwarder",
-        "command_description": "(Re)start internal forwarding of DDNS Update packets. This is automatically called if b10-ddns is started.",
+        "command_description": "(Re)start internal forwarding of DDNS Update messages. This is automatically called if b10-ddns is started.",
         "command_args": []
       },
       {
         "command_name": "stop_ddns_forwarder",
-        "command_description": "Stop internal forwarding of DDNS Update packets. This is automatically called if b10-ddns is stopped.",
+        "command_description": "Stop internal forwarding of DDNS Update messages. This is automatically called if b10-ddns is stopped.",
         "command_args": []
       }
     ],

src/bin/auth/auth_srv.cc

@@ -238,10 +238,6 @@ public:
                        auto_ptr<TSIGContext> tsig_context);
     bool processUpdate(const IOMessage& io_message);
 
-    bool hasDDNSForwarder();
-    void createDDNSForwarder();
-    void destroyDDNSForwarder();
-
     IOService io_service_;
 
     MessageRenderer renderer_;
@@ -275,6 +271,14 @@ public:
     /// isc:config::ModuleSpec::validateStatistics.
     void registerStatisticsValidator();
 
+    /// Socket session forwarder for dynamic update requests
+    BaseSocketSessionForwarder& ddns_base_forwarder_;
+
+    /// Holder for the DDNS Forwarder, which is used to send
+    /// DDNS messages to b10-ddns, but can be set to empty if
+    /// b10-ddns is not running
+    boost::scoped_ptr<SocketSessionForwarderHolder> ddns_forwarder_;
+
     /// \brief Resume the server
     ///
     /// This is a wrapper call for DNSServer::resume(done), if 'done' is true,
@@ -290,6 +294,7 @@ public:
     void resumeServer(isc::asiodns::DNSServer* server,
                       isc::dns::Message& message,
                       bool done);
+
 private:
     std::string db_file_;
 
@@ -302,10 +307,6 @@ private:
     bool xfrout_connected_;
     AbstractXfroutClient& xfrout_client_;
 
-    // Socket session forwarder for dynamic update requests
-    BaseSocketSessionForwarder& ddns_base_forwarder_;
-    boost::scoped_ptr<SocketSessionForwarderHolder> ddns_forwarder_;
-
     /// Increment query counter
     void incCounter(const int protocol);
 
@@ -324,10 +325,10 @@ AuthSrvImpl::AuthSrvImpl(const bool use_cache,
     statistics_timer_(io_service_),
     counters_(),
     keyring_(NULL),
-    xfrout_connected_(false),
-    xfrout_client_(xfrout_client),
     ddns_base_forwarder_(ddns_forwarder),
-    ddns_forwarder_(NULL)
+    ddns_forwarder_(NULL),
+    xfrout_connected_(false),
+    xfrout_client_(xfrout_client)
 {
     // cur_datasrc_ is automatically initialized by the default constructor,
     // effectively being an empty (sqlite) data source.  once ccsession is up
@@ -660,7 +661,7 @@ AuthSrv::processMessage(const IOMessage& io_message, Message& message,
             send_answer = impl_->processNotify(io_message, message, buffer,
                                                tsig_context);
         } else if (opcode == Opcode::UPDATE()) {
-            if (impl_->hasDDNSForwarder()) {
+            if (impl_->ddns_forwarder_) {
                 send_answer = impl_->processUpdate(io_message);
             } else {
                 makeErrorMessage(impl_->renderer_, message, buffer,
@@ -888,26 +889,6 @@ AuthSrvImpl::processNotify(const IOMessage& io_message, Message& message,
 }
 
 bool
-AuthSrvImpl::hasDDNSForwarder() {
-    return (ddns_forwarder_);
-}
-
-void
-AuthSrvImpl::createDDNSForwarder() {
-    LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_START_DDNS_FORWARDER);
-    ddns_forwarder_.reset(
-        new SocketSessionForwarderHolder("update", ddns_base_forwarder_));
-}
-
-void
-AuthSrvImpl::destroyDDNSForwarder() {
-    if (ddns_forwarder_) {
-        LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_STOP_DDNS_FORWARDER);
-        ddns_forwarder_.reset();
-    }
-}
-
-bool
 AuthSrvImpl::processUpdate(const IOMessage& io_message) {
     // Push the update request to a separate process via the forwarder.
     // On successful push, the request shouldn't be responded from b10-auth,
@@ -1062,12 +1043,17 @@ AuthSrv::setTSIGKeyRing(const boost::shared_ptr<TSIGKeyRing>* keyring) {
 
 void
 AuthSrv::createDDNSForwarder() {
-    impl_->createDDNSForwarder();
+    LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_START_DDNS_FORWARDER);
+    impl_->ddns_forwarder_.reset(
+        new SocketSessionForwarderHolder("update", impl_->ddns_base_forwarder_));
 }
 
 void
 AuthSrv::destroyDDNSForwarder() {
-    impl_->destroyDDNSForwarder();
+    if (impl_->ddns_forwarder_) {
+        LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_STOP_DDNS_FORWARDER);
+        impl_->ddns_forwarder_.reset();
+    }
 }
 
 

src/bin/auth/auth_srv.h

@@ -422,7 +422,7 @@ public:
     ///
     /// Until this method is called (it is called when the
     /// start_ddns_forwarder command is sent to b10-auth), b10-auth will
-    /// respond to UPDATE packets with a NOTIMP rcode.
+    /// respond to UPDATE messages with a NOTIMP rcode.
     /// If the internal forwarder was already created, it is destroyed and
     /// created again. This is useful for instance when b10-ddns is shut
     /// down and restarted.

src/bin/auth/tests/auth_srv_unittest.cc

@@ -31,6 +31,7 @@
 
 #include <datasrc/memory_datasrc.h>
 #include <auth/auth_srv.h>
+#include <auth/command.h>
 #include <auth/common.h>
 #include <auth/statistics.h>
 
@@ -1642,4 +1643,86 @@ TEST_F(AuthSrvTest, DDNSForwardClose) {
     EXPECT_FALSE(ddns_forwarder.isConnected());
 }
 
+namespace {
+    // Send a basic command without arguments, and check the response has
+    // result code 0
+    void sendSimpleCommand(AuthSrv& server, const std::string&command) {
+        ConstElementPtr response = execAuthServerCommand(server,
+                                                         command,
+                                                         ConstElementPtr());
+        int command_result = -1;
+        isc::config::parseAnswer(command_result, response);
+        EXPECT_EQ(0, command_result);
+    }
+} // end anonymous namespace
+
+TEST_F(AuthSrvTest, DDNSForwardCreateDestroy) {
+    // Test that AuthSrv returns NOTIMPL 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 NOTIMPL after
+    // the stop_ddns_forwarding command is sent.
+    scoped_ptr<AuthSrv> tmp_server(new AuthSrv(true, xfrout, ddns_forwarder));
+
+    // Prepare update message to send
+    UnitTestUtil::createRequestMessage(request_message, Opcode::UPDATE(),
+                                       default_qid, Name("example.com"),
+                                       RRClass::IN(), RRType::SOA());
+    createRequestPacket(request_message, IPPROTO_UDP);
+
+    // before creating forwarder. isConnected() should be false and
+    // rcode to UPDATE should be NOTIMP
+    parse_message->clear(Message::PARSE);
+    tmp_server->processMessage(*io_message, *parse_message, *response_obuffer,
+                               &dnsserv);
+    EXPECT_FALSE(ddns_forwarder.isConnected());
+    EXPECT_TRUE(dnsserv.hasAnswer());
+    headerCheck(*parse_message, default_qid, Rcode::NOTIMP(),
+                Opcode::UPDATE().getCode(), QR_FLAG, 0, 0, 0, 0);
+
+    // now create forwarder
+    sendSimpleCommand(*tmp_server, "start_ddns_forwarder");
+
+    // our mock does not respond, and since auth is supposed to send it on,
+    // there should now be no result when an UPDATE is sent
+    parse_message->clear(Message::PARSE);
+    tmp_server->processMessage(*io_message, *parse_message, *response_obuffer,
+                               &dnsserv);
+    EXPECT_FALSE(dnsserv.hasAnswer());
+    EXPECT_TRUE(ddns_forwarder.isConnected());
+
+    // If we send a start again, the connection should be recreated,
+    // visible because isConnected() reports false until an actual message
+    // has been forwarded
+    sendSimpleCommand(*tmp_server, "start_ddns_forwarder");
+
+    EXPECT_FALSE(ddns_forwarder.isConnected());
+    parse_message->clear(Message::PARSE);
+    tmp_server->processMessage(*io_message, *parse_message, *response_obuffer,
+                               &dnsserv);
+    EXPECT_FALSE(dnsserv.hasAnswer());
+    EXPECT_TRUE(ddns_forwarder.isConnected());
+
+    // Now tell it to stop forwarder, should respond with NOTIMP again
+    sendSimpleCommand(*tmp_server, "stop_ddns_forwarder");
+
+    parse_message->clear(Message::PARSE);
+    tmp_server->processMessage(*io_message, *parse_message, *response_obuffer,
+                               &dnsserv);
+    EXPECT_FALSE(ddns_forwarder.isConnected());
+    EXPECT_TRUE(dnsserv.hasAnswer());
+    headerCheck(*parse_message, default_qid, Rcode::NOTIMP(),
+                Opcode::UPDATE().getCode(), QR_FLAG, 0, 0, 0, 0);
+
+    // Sending stop again should make no difference
+    sendSimpleCommand(*tmp_server, "stop_ddns_forwarder");
+
+    parse_message->clear(Message::PARSE);
+    tmp_server->processMessage(*io_message, *parse_message, *response_obuffer,
+                               &dnsserv);
+    EXPECT_FALSE(ddns_forwarder.isConnected());
+    EXPECT_TRUE(dnsserv.hasAnswer());
+    headerCheck(*parse_message, default_qid, Rcode::NOTIMP(),
+                Opcode::UPDATE().getCode(), QR_FLAG, 0, 0, 0, 0);
+}
+
 }