BIND 10 trac1986, updated. 22c45facd92929d66723796127b3a51e77c6c9f1 [1986] Adressed review comments (b10-ddns side)
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Jul 18 13:22:52 UTC 2012
The branch, trac1986 has been updated
via 22c45facd92929d66723796127b3a51e77c6c9f1 (commit)
via 3eb46f5f9ed333e791ead5d759810546aefef4a9 (commit)
from ea44fe72161fa65fef97d128b28f1223a8762d99 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 22c45facd92929d66723796127b3a51e77c6c9f1
Author: Jelte Jansen <jelte at isc.org>
Date: Wed Jul 18 15:18:08 2012 +0200
[1986] Adressed review comments (b10-ddns side)
- fix log messages
- added more send/recv error catches
- more tests
commit 3eb46f5f9ed333e791ead5d759810546aefef4a9
Author: Jelte Jansen <jelte at isc.org>
Date: Wed Jul 18 12:19:54 2012 +0200
[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
-----------------------------------------------------------------------
Summary of changes:
src/bin/auth/auth.spec.pre.in | 4 +-
src/bin/auth/auth_srv.cc | 54 ++++++++------------
src/bin/auth/auth_srv.h | 2 +-
src/bin/auth/tests/auth_srv_unittest.cc | 83 +++++++++++++++++++++++++++++++
src/bin/ddns/ddns.py.in | 38 ++++++++------
src/bin/ddns/ddns_messages.mes | 31 ++++++++++--
src/bin/ddns/tests/ddns_test.py | 36 +++++++++++++-
7 files changed, 190 insertions(+), 58 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/auth/auth.spec.pre.in b/src/bin/auth/auth.spec.pre.in
index 0ed00ac..567381a 100644
--- a/src/bin/auth/auth.spec.pre.in
+++ b/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": []
}
],
diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc
index 8b2ef9c..e8176c3 100644
--- a/src/bin/auth/auth_srv.cc
+++ b/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();
+ }
}
diff --git a/src/bin/auth/auth_srv.h b/src/bin/auth/auth_srv.h
index 2ccf245..d40a490 100644
--- a/src/bin/auth/auth_srv.h
+++ b/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.
diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc
index da75377..6865c52 100644
--- a/src/bin/auth/tests/auth_srv_unittest.cc
+++ b/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);
+}
+
}
diff --git a/src/bin/ddns/ddns.py.in b/src/bin/ddns/ddns.py.in
index b84ba29..708b07c 100755
--- a/src/bin/ddns/ddns.py.in
+++ b/src/bin/ddns/ddns.py.in
@@ -228,7 +228,7 @@ class DDNSServer:
# Outstanding TCP context: fileno=>(context_obj, dst)
self._tcp_ctxs = {}
- # Notify Auth server that DDNS update packets can now be forwarded
+ # Notify Auth server that DDNS update messages can now be forwarded
self.__notify_start_forwarder()
class InternalError(Exception):
@@ -384,7 +384,7 @@ class DDNSServer:
Do NOT call this to initialize shutdown, use trigger_shutdown().
'''
- # tell Auth not to forward UPDATE packets anymore
+ # tell Auth not to forward UPDATE messages anymore
self.__notify_stop_forwarder()
# tell the ModuleCCSession to send a message that this module is
# stopping.
@@ -542,22 +542,28 @@ class DDNSServer:
return True
def __notify_start_forwarder(self):
- '''Notify auth that DDNS Update packets can now be forwarded'''
- seq = self._cc._session.group_sendmsg(create_command(
- "start_ddns_forwarder"), AUTH_MODULE_NAME)
- answer, _ = self._cc._session.group_recvmsg(False, seq)
- rcode, error_msg = parse_answer(answer)
- if rcode != 0:
- logger.error(DDNS_START_FORWARDER_ERROR, error_msg)
+ '''Notify auth that DDNS Update messages can now be forwarded'''
+ try:
+ seq = self._cc._session.group_sendmsg(create_command(
+ "start_ddns_forwarder"), AUTH_MODULE_NAME)
+ answer, _ = self._cc._session.group_recvmsg(False, seq)
+ rcode, error_msg = parse_answer(answer)
+ if rcode != 0:
+ logger.error(DDNS_START_FORWARDER_ERROR, error_msg)
+ except (SessionTimeout, SessionError, ProtocolError) as ex:
+ logger.error(DDNS_START_FORWARDER_FAIL, ex)
def __notify_stop_forwarder(self):
- '''Notify auth that DDNS Update packets can now be forwarded'''
- seq = self._cc._session.group_sendmsg(create_command(
- "stop_ddns_forwarder"), AUTH_MODULE_NAME)
- answer, _ = self._cc._session.group_recvmsg(False, seq)
- rcode, error_msg = parse_answer(answer)
- if rcode != 0:
- logger.error(DDNS_STOP_FORWARDER_ERROR, error_msg)
+ '''Notify auth that DDNS Update messages should no longer be forwarded'''
+ try:
+ seq = self._cc._session.group_sendmsg(create_command(
+ "stop_ddns_forwarder"), AUTH_MODULE_NAME)
+ answer, _ = self._cc._session.group_recvmsg(False, seq)
+ rcode, error_msg = parse_answer(answer)
+ if rcode != 0:
+ logger.error(DDNS_STOP_FORWARDER_ERROR, error_msg)
+ except (SessionTimeout, SessionError, ProtocolError) as ex:
+ logger.error(DDNS_STOP_FORWARDER_FAIL, ex)
def __notify_auth(self, zname, zclass):
'''Notify auth of the update, if necessary.'''
diff --git a/src/bin/ddns/ddns_messages.mes b/src/bin/ddns/ddns_messages.mes
index cf896e5..d128361 100644
--- a/src/bin/ddns/ddns_messages.mes
+++ b/src/bin/ddns/ddns_messages.mes
@@ -192,11 +192,24 @@ be completed, after which the process will exit.
The ddns process has successfully started and is now ready to receive commands
and updates.
-% DDNS_START_FOWARDER_ERROR Error from b10-auth when requesting DDNS UPDATE forwarding: %1
+% DDNS_START_FORWARDER_ERROR Error from b10-auth when requesting DDNS UPDATE forwarding: %1
There was an error response from b10-auth to the command to start
forwarding DDNS UPDATE messages to b10-ddns.
+It is almost certain that DDNS UPDATE messages are not handled now, and that
+they will be responded to with a NOTIMP error code, even though b10-ddns is
+running.
The error message is printed, and additional information may be found in
-the b10-auth log output.
+the b10-auth log output. Since this is an error that is sent by the
+b10-auth module, it should have more information as to what the actual
+problem was.
+
+% DDNS_START_FORWARDER_FAIL Error sending request for DDNS UPDATE forwarding to b10-auth: %1
+There was an error when attempting to send b10-auth the request to forward
+DDNS UPDATE messages to the b10-ddns module. This points to an internal
+problem using the inter-module messaging system.
+This needs to be inspected, as it is almost certain that DDNS UPDATE messages
+are not handled now.
+The specific error is printed in the log message.
% DDNS_STOPPED ddns server has stopped
The ddns process has successfully stopped and is no longer listening for or
@@ -206,12 +219,24 @@ handling commands or updates, and will now exit.
There was a keyboard interrupt signal to stop the ddns process. The
process will now shut down.
-% DDNS_STOP_FOWARDER_ERROR Error from b10-auth when requesting to stop DDNS UPDATE forwarding: %1
+% DDNS_STOP_FORWARDER_ERROR Error from b10-auth when requesting to stop DDNS UPDATE forwarding: %1
There was an error response from b10-auth to the command to stop
forwarding DDNS UPDATE messages to b10-ddns.
+This specific error may not be fatal; instead of returning NOTIMP for future
+DDNS UPDATE messages, it will return SERVFAIL. However, this does point to
+an underlying problem in the messaging system, and should be inspected.
The error message is printed, and additional information may be found in
the b10-auth log output.
+% DDNS_STOP_FORWARDER_FAIL Error sending request to stop DDNS UPDATE forwarding to b10-auth: %1
+There was an error when attempting to send b10-auth the request to stop
+forwarding DDNS UPDATE messages to the b10-ddns module. This points to an
+internal problem using the inter-module messaging system.
+This specific error may not be fatal; instead of returning NOTIMP for future
+DDNS UPDATE messages, it will return SERVFAIL. However, this does point to
+an underlying problem in the messaging system, and should be inspected.
+The specific error is printed in the log message.
+
% DDNS_UNCAUGHT_EXCEPTION uncaught exception of type %1: %2
The b10-ddns process encountered an uncaught exception and will now shut
down. This is indicative of a programming error and should not happen under
diff --git a/src/bin/ddns/tests/ddns_test.py b/src/bin/ddns/tests/ddns_test.py
index 9cb3196..35ea641 100755
--- a/src/bin/ddns/tests/ddns_test.py
+++ b/src/bin/ddns/tests/ddns_test.py
@@ -1183,7 +1183,6 @@ class TestDDNSSession(unittest.TestCase):
'''Check that the command 'start_ddns_forwarder' has been called
This test removes said message from the sent message queue.
'''
-
sent_msg, sent_group = self.__cc_session._sent_msg.pop(0)
sent_cmd = sent_msg['command']
self.assertEqual('Auth', sent_group)
@@ -1194,7 +1193,7 @@ class TestDDNSSession(unittest.TestCase):
self.__cc_session._recvmsg_called = 0
def check_session_stop_forwarder_called(self):
- '''Check that the command 'start_ddns_forwarder' has been called
+ '''Check that the command 'stop_ddns_forwarder' has been called
This test removes said message from the sent message queue.
'''
# check the last message sent
@@ -1207,6 +1206,8 @@ class TestDDNSSession(unittest.TestCase):
def test_session_msg(self):
'''Test post update communication with other modules.'''
+ # Check that start_ddns_forwarder has been called upon
+ # initialization
self.check_session_start_forwarder_called()
# Normal cases, confirming communication takes place iff update
@@ -1306,6 +1307,37 @@ class TestDDNSSession(unittest.TestCase):
# check the result
self.check_session(UPDATE_DROP)
+ def test_session_start_stop_forwarder_failures(self):
+ '''Check that we don't crash if the server reports an error
+ setting up or closing down the DDNS UPDATE message forwarder,
+ or if there is an exception from the message queue.'''
+ self.__cc_session._answer_code = 1
+ self.server._DDNSServer__notify_start_forwarder()
+ self.server._DDNSServer__notify_stop_forwarder()
+
+ for exc in [ SessionError("sessionerror"),
+ SessionTimeout("sessiontimeout"),
+ ProtocolError("protocolerror") ]:
+ self.__cc_session._recvmsg_exception = exc
+ self.server._DDNSServer__notify_start_forwarder()
+ self.server._DDNSServer__notify_stop_forwarder()
+ self.__cc_session._recvmsg_exception = None
+
+ self.__cc_session._sendmsg_exception = exc
+ self.server._DDNSServer__notify_start_forwarder()
+ self.server._DDNSServer__notify_stop_forwarder()
+ self.__cc_session._recvmsg_exception = None
+
+ def test_session_auth_started(self):
+ '''Check that 'start_ddns_forwarder' is sent when the notification
+ 'auth_started' is received'''
+ # It should have already been called during init
+ self.check_session_start_forwarder_called()
+
+ # auth_started message should trigger it again
+ answer = self.server.command_handler('auth_started', None)
+ self.check_session_start_forwarder_called()
+
class TestMain(unittest.TestCase):
def setUp(self):
self._server = MyDDNSServer()
More information about the bind10-changes
mailing list