BIND 10 trac2977, updated. 4d6b150d0605cec1086f05c7557c7990e4524188 [2977] Addressed review comments.
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Jul 5 12:47:24 UTC 2013
The branch, trac2977 has been updated
via 4d6b150d0605cec1086f05c7557c7990e4524188 (commit)
from 7076a02b24e63b0fa542af760585e6ad95cc421f (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 4d6b150d0605cec1086f05c7557c7990e4524188
Author: Marcin Siodelski <marcin at isc.org>
Date: Fri Jul 5 14:47:05 2013 +0200
[2977] Addressed review comments.
In particular, added a doUpdate version which supports TSIG. It is also
possible to specify Transport layer protocol preferred.
-----------------------------------------------------------------------
Summary of changes:
src/bin/d2/dns_client.cc | 78 +++++++++++++++++---
src/bin/d2/dns_client.h | 72 ++++++++++++++++---
src/bin/d2/tests/dns_client_unittests.cc | 114 +++++++++++++++++++++++++++---
3 files changed, 236 insertions(+), 28 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/d2/dns_client.cc b/src/bin/d2/dns_client.cc
index 80065f3..6a6c39d 100644
--- a/src/bin/d2/dns_client.cc
+++ b/src/bin/d2/dns_client.cc
@@ -15,6 +15,7 @@
#include <d2/dns_client.h>
#include <d2/d2_log.h>
#include <dns/messagerenderer.h>
+#include <limits>
namespace isc {
namespace d2 {
@@ -31,8 +32,9 @@ const size_t DEFAULT_BUFFER_SIZE = 128;
using namespace isc::util;
using namespace isc::asiolink;
using namespace isc::asiodns;
+using namespace isc::dns;
-// This class provides the implementation for the DNSClient. This allows to
+// This class provides the implementation for the DNSClient. This allows for
// the separation of the DNSClient interface from the implementation details.
// Currently, implementation uses IOFetch object to handle asynchronous
// communication with the DNS. This design may be revisited in the future. If
@@ -47,9 +49,13 @@ public:
// A caller-supplied external callback which is invoked when DNS message
// exchange is complete or interrupted.
DNSClient::Callback* callback_;
+ // A Transport Layer protocol used to communicate with a DNS.
+ DNSClient::Protocol proto_;
+ // Constructor and Destructor
DNSClientImpl(D2UpdateMessagePtr& response_placeholder,
- DNSClient::Callback* callback);
+ DNSClient::Callback* callback,
+ const DNSClient::Protocol proto);
virtual ~DNSClientImpl();
// This internal callback is called when the DNS update message exchange is
@@ -58,23 +64,45 @@ public:
// type, representing a response from the server is set.
virtual void operator()(asiodns::IOFetch::Result result);
+ // Starts asynchronous DNS Update.
void doUpdate(asiolink::IOService& io_service,
const asiolink::IOAddress& ns_addr,
const uint16_t ns_port,
D2UpdateMessage& update,
- const int wait = -1);
+ const unsigned int wait);
// This function maps the IO error to the DNSClient error.
DNSClient::Status getStatus(const asiodns::IOFetch::Result);
};
DNSClientImpl::DNSClientImpl(D2UpdateMessagePtr& response_placeholder,
- DNSClient::Callback* callback)
+ DNSClient::Callback* callback,
+ const DNSClient::Protocol proto)
: in_buf_(new OutputBuffer(DEFAULT_BUFFER_SIZE)),
- response_(response_placeholder), callback_(callback) {
+ response_(response_placeholder), callback_(callback), proto_(proto) {
+
+ // @todo At some point we may need to implement TCP. It should be straight
+ // forward but would require a bunch of new unit tests. That's why we
+ // currently disable TCP. Once implemented the check below should be
+ // removed.
+ if (proto_ == DNSClient::TCP) {
+ isc_throw(isc::NotImplemented, "TCP is currently not supported as a"
+ << " Transport protocol for DNS Updates; please use UDP");
+ }
+
+ // Given that we already eliminated the possibility that TCP is used, it
+ // would be sufficient to check that (proto != DNSClient::UDP). But, once
+ // support TCP is added the check above will disappear and the extra check
+ // will be needed here anyway. Why not add it now?
+ if (proto_ != DNSClient::TCP && proto_ != DNSClient::UDP) {
+ isc_throw(isc::NotImplemented, "invalid transport protocol type '"
+ << proto_ << "' specified for DNS Updates");
+ }
+
if (!response_) {
isc_throw(BadValue, "a pointer to an object to encapsulate the DNS"
" server must be provided; found NULL value");
+
}
}
@@ -132,7 +160,7 @@ DNSClientImpl::doUpdate(IOService& io_service,
const IOAddress& ns_addr,
const uint16_t ns_port,
D2UpdateMessage& update,
- const int wait) {
+ const unsigned int wait) {
// A renderer is used by the toWire function which creates the on-wire data
// from the DNS Update message. A renderer has its internal buffer where it
// renders data by default. However, this buffer can't be directly accessed.
@@ -151,8 +179,12 @@ DNSClientImpl::doUpdate(IOService& io_service,
// communication with the DNS server. The last but one argument points to
// this object as a completion callback for the message exchange. As a
// result operator()(Status) will be called.
+
+ // Timeout value is explicitly cast to the int type to avoid warnings about
+ // overflows when doing implicit cast. It should have been checked by the
+ // caller that the unsigned timeout value will fit into int.
IOFetch io_fetch(IOFetch::UDP, io_service, msg_buf, ns_addr, ns_port,
- in_buf_, this, wait);
+ in_buf_, this, static_cast<int>(wait));
// Post the task to the task queue in the IO service. Caller will actually
// run these tasks by executing IOService::run.
io_service.post(io_fetch);
@@ -160,24 +192,50 @@ DNSClientImpl::doUpdate(IOService& io_service,
DNSClient::DNSClient(D2UpdateMessagePtr& response_placeholder,
- Callback* callback)
- : impl_(new DNSClientImpl(response_placeholder, callback)) {
+ Callback* callback, const DNSClient::Protocol proto)
+ : impl_(new DNSClientImpl(response_placeholder, callback, proto)) {
}
DNSClient::~DNSClient() {
delete (impl_);
}
+unsigned int
+DNSClient::getMaxTimeout() {
+ static const unsigned int max_timeout = std::numeric_limits<int>::max();
+ return (max_timeout);
+}
+
+void
+DNSClient::doUpdate(IOService&,
+ const IOAddress&,
+ const uint16_t,
+ D2UpdateMessage&,
+ const unsigned int,
+ const dns::TSIGKey&) {
+ isc_throw(isc::NotImplemented, "TSIG is currently not supported for"
+ "DNS Update message");
+}
+
void
DNSClient::doUpdate(IOService& io_service,
const IOAddress& ns_addr,
const uint16_t ns_port,
D2UpdateMessage& update,
- const int wait) {
+ const unsigned int wait) {
+ // The underlying implementation which we use to send DNS Updates uses
+ // signed integers for timeout. If we want to avoid overflows we need to
+ // respect this limitation here.
+ if (wait > getMaxTimeout()) {
+ isc_throw(isc::BadValue, "A timeout value for DNS Update request must"
+ " not exceed " << getMaxTimeout()
+ << ". Provided timeout value is '" << wait << "'");
+ }
impl_->doUpdate(io_service, ns_addr, ns_port, update, wait);
}
+
} // namespace d2
} // namespace isc
diff --git a/src/bin/d2/dns_client.h b/src/bin/d2/dns_client.h
index cbd2986..faac44b 100644
--- a/src/bin/d2/dns_client.h
+++ b/src/bin/d2/dns_client.h
@@ -21,6 +21,7 @@
#include <util/buffer.h>
#include <asiodns/io_fetch.h>
+#include <dns/tsig.h>
namespace isc {
namespace d2 {
@@ -35,7 +36,7 @@ class DNSClientImpl;
///
/// Communication with the DNS server is asynchronous. Caller must provide a
/// callback, which will be invoked when the response from the DNS server is
-/// received, a timeout has occured or IO service has been stopped for any
+/// received, a timeout has occurred or IO service has been stopped for any
/// reason. The caller-supplied callback is called by the internal callback
/// operator implemented by @c DNSClient. This callback is responsible for
/// initializing the @c D2UpdateMessage instance which encapsulates the response
@@ -45,9 +46,24 @@ class DNSClientImpl;
/// Caller must supply a pointer to the @c D2UpdateMessage object, which will
/// encapsulate DNS response, through class constructor. An exception will be
/// thrown if the pointer is not initialized by the caller.
+///
+/// @todo Ultimately, this class will support both TCP and UDP Transport.
+/// Currently only UDP is supported and can be specified as a preferred
+/// protocol. @c DNSClient constructor will throw an exception if TCP is
+/// specified. Once both protocols are supported, the @c DNSClient logic will
+/// try to obey caller's preference. However, it may use the other protocol if
+/// on its own discretion, when there is a legitimate reason to do so. For
+/// example, if communication with the server using preferred protocol fails.
class DNSClient {
public:
+ /// @brief Transport layer protocol used by a DNS Client to communicate
+ /// with a server.
+ enum Protocol {
+ UDP,
+ TCP
+ };
+
/// @brief A status code of the DNSClient.
enum Status {
SUCCESS, ///< Response received and is ok.
@@ -70,8 +86,8 @@ public:
/// @brief Function operator implementing a callback.
///
- /// @param result an @c asiodns::IOFetch::Result object representing
- /// IO status code.
+ /// @param status a @c DNSClient::Status enum representing status code
+ /// of DNSClient operation.
virtual void operator()(DNSClient::Status status) = 0;
};
@@ -82,7 +98,10 @@ public:
/// @param callback Pointer to an object implementing @c DNSClient::Callback
/// class. This object will be called when DNS message exchange completes or
/// if an error occurs. NULL value disables callback invocation.
- DNSClient(D2UpdateMessagePtr& response_placeholder, Callback* callback);
+ /// @param proto caller's preference regarding Transport layer protocol to
+ /// be used by DNS Client to communicate with a server.
+ DNSClient(D2UpdateMessagePtr& response_placeholder, Callback* callback,
+ const Protocol proto = UDP);
/// @brief Virtual destructor, does nothing.
~DNSClient();
@@ -99,7 +118,44 @@ private:
//@}
public:
- /// @brief Start asynchronous DNS Update.
+
+ /// @brief Returns maximal allowed timeout value accepted by
+ /// @c DNSClient::doUpdate.
+ ///
+ /// @return maximal allowed timeout value accepted by @c DNSClient::doUpdate
+ static unsigned int getMaxTimeout();
+
+ /// @brief Start asynchronous DNS Update with TSIG.
+ ///
+ /// This function starts asynchronous DNS Update and returns. The DNS Update
+ /// will be executed by the specified IO service. Once the message exchange
+ /// with a DNS server is complete, timeout occurs or IO operation is
+ /// interrupted, the caller-supplied callback function will be invoked.
+ ///
+ /// An address and port of the DNS server is specified through the function
+ /// arguments so as the same instance of the @c DNSClient can be used to
+ /// initiate multiple message exchanges.
+ ///
+ /// @param io_service IO service to be used to run the message exchange.
+ /// @param ns_addr DNS server address.
+ /// @param ns_port DNS server port.
+ /// @param update A DNS Update message to be sent to the server.
+ /// @param wait A timeout (in seconds) for the response. If a response is
+ /// not received within the timeout, exchange is interrupted. This value
+ /// must not exceed maximal value for 'int' data type.
+ /// @param tsig_key An @c isc::dns::TSIGKey object representing TSIG
+ /// context which will be used to render the DNS Update message.
+ ///
+ /// @todo Implement TSIG Support. Currently any attempt to call this
+ /// function will result in exception.
+ void doUpdate(asiolink::IOService& io_service,
+ const asiolink::IOAddress& ns_addr,
+ const uint16_t ns_port,
+ D2UpdateMessage& update,
+ const unsigned int wait,
+ const dns::TSIGKey& tsig_key);
+
+ /// @brief Start asynchronous DNS Update without TSIG.
///
/// This function starts asynchronous DNS Update and returns. The DNS Update
/// will be executed by the specified IO service. Once the message exchange
@@ -115,13 +171,13 @@ public:
/// @param ns_port DNS server port.
/// @param update A DNS Update message to be sent to the server.
/// @param wait A timeout (in seconds) for the response. If a response is
- /// not received within the timeout, exchange is interrupted. A negative
- /// value disables timeout.
+ /// not received within the timeout, exchange is interrupted. This value
+ /// must not exceed maximal value for 'int' data type.
void doUpdate(asiolink::IOService& io_service,
const asiolink::IOAddress& ns_addr,
const uint16_t ns_port,
D2UpdateMessage& update,
- const int wait = -1);
+ const unsigned int wait);
private:
DNSClientImpl* impl_; ///< Pointer to DNSClient implementation.
diff --git a/src/bin/d2/tests/dns_client_unittests.cc b/src/bin/d2/tests/dns_client_unittests.cc
index d969104..8fc6d73 100644
--- a/src/bin/d2/tests/dns_client_unittests.cc
+++ b/src/bin/d2/tests/dns_client_unittests.cc
@@ -16,8 +16,10 @@
#include <d2/dns_client.h>
#include <asiodns/io_fetch.h>
#include <asiodns/logger.h>
+#include <asiolink/interval_timer.h>
#include <dns/rcode.h>
#include <dns/rrclass.h>
+#include <dns/tsig.h>
#include <asio/ip/udp.hpp>
#include <asio/socket_base.hpp>
#include <boost/bind.hpp>
@@ -41,15 +43,22 @@ namespace {
const char* TEST_ADDRESS = "127.0.0.1";
const uint16_t TEST_PORT = 5301;
const size_t MAX_SIZE = 1024;
+const long TEST_TIMEOUT = 5 * 1000;
// @brief Test Fixture class.
//
// This test fixture class implements DNSClient::Callback so as it can be
// installed as a completion callback for tests it implements. This callback
// is called when a DDNS transaction (send and receive) completes. This allows
-// for the callback function to direcetly access class members. In particular,
+// for the callback function to directly access class members. In particular,
// the callback function can access IOService on which run() was called and
// call stop() on it.
+//
+// Many of the tests defined here schedule execution of certain tasks and block
+// until tasks are completed or a timeout is hit. However, if timeout is not
+// properly handled a task may be hanging for a long time. In order to prevent
+// it, the asiolink::IntervalTimer is used to break a running test if test
+// timeout is hit. This will result in test failure.
class DNSClientTest : public virtual ::testing::Test, DNSClient::Callback {
public:
IOService service_;
@@ -59,6 +68,7 @@ public:
DNSClientPtr dns_client_;
bool corrupt_response_;
bool expect_response_;
+ asiolink::IntervalTimer test_timer_;
// @brief Constructor.
//
@@ -72,10 +82,15 @@ public:
: service_(),
status_(DNSClient::SUCCESS),
corrupt_response_(false),
- expect_response_(true) {
+ expect_response_(true),
+ test_timer_(service_) {
asiodns::logger.setSeverity(log::INFO);
response_.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
dns_client_.reset(new DNSClient(response_, this));
+
+ // Set the test timeout to break any running tasks if they hang.
+ test_timer_.setup(boost::bind(&DNSClientTest::testTimeoutHandler, this),
+ TEST_TIMEOUT);
}
// @brief Destructor.
@@ -88,7 +103,7 @@ public:
// @brief Exchange completion callback.
//
// This callback is called when the exchange with the DNS server is
- // complete or an error occured. This includes the occurence of a timeout.
+ // complete or an error occurred. This includes the occurrence of a timeout.
//
// @param status A status code returned by DNSClient.
virtual void operator()(DNSClient::Status status) {
@@ -119,6 +134,14 @@ public:
}
}
+ // @brief Handler invoked when test timeout is hit.
+ //
+ // This callback stops all running (hanging) tasks on IO service.
+ void testTimeoutHandler() {
+ service_.stop();
+ FAIL() << "Test timeout hit.";
+ }
+
// @brief Handler invoked when test request is received.
//
// This callback handler is installed when performing async read on a
@@ -164,8 +187,61 @@ public:
// callback object is NULL.
void runConstructorTest() {
D2UpdateMessagePtr null_response;
- EXPECT_THROW(DNSClient(null_response, this), isc::BadValue);
- EXPECT_NO_THROW(DNSClient(response_, NULL));
+ EXPECT_THROW(DNSClient(null_response, this, DNSClient::UDP),
+ isc::BadValue);
+ EXPECT_NO_THROW(DNSClient(response_, NULL, DNSClient::UDP));
+
+ // The TCP Transport is not supported right now. So, we return exception
+ // if caller specified TCP as a preferred protocol. This test will be
+ // removed once TCP is supported.
+ EXPECT_THROW(DNSClient(response_, NULL, DNSClient::TCP),
+ isc::NotImplemented);
+ }
+
+ // This test verifies that it accepted timeout values belong to the range of
+ // <0, DNSClient::getMaxTimeout()>.
+ void runInvalidTimeoutTest() {
+
+ expect_response_ = false;
+
+ // Create outgoing message. Simply set the required message fields:
+ // error code and Zone section. This is enough to create on-wire format
+ // of this message and send it.
+ D2UpdateMessage message(D2UpdateMessage::OUTBOUND);
+ ASSERT_NO_THROW(message.setRcode(Rcode(Rcode::NOERROR_CODE)));
+ ASSERT_NO_THROW(message.setZone(Name("example.com"), RRClass::IN()));
+
+ // Start with a valid timeout equal to maximal allowed. This why we will
+ // ensure that doUpdate doesn't throw an exception for valid timeouts.
+ unsigned int timeout = DNSClient::getMaxTimeout();
+ EXPECT_NO_THROW(dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS),
+ TEST_PORT, message, timeout));
+
+ // Cross the limit and expect that exception is thrown this time.
+ timeout = DNSClient::getMaxTimeout() + 1;
+ EXPECT_THROW(dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS),
+ TEST_PORT, message, timeout),
+ isc::BadValue);
+ }
+
+ // This test verifies that isc::NotImplemented exception is thrown when
+ // attempt to send DNS Update message with TSIG is attempted.
+ void runTSIGTest() {
+ // Create outgoing message. Simply set the required message fields:
+ // error code and Zone section. This is enough to create on-wire format
+ // of this message and send it.
+ D2UpdateMessage message(D2UpdateMessage::OUTBOUND);
+ ASSERT_NO_THROW(message.setRcode(Rcode(Rcode::NOERROR_CODE)));
+ ASSERT_NO_THROW(message.setZone(Name("example.com"), RRClass::IN()));
+
+ const int timeout = 0;
+ // Try to send DNS Update with TSIG key. Currently TSIG is not supported
+ // and therefore we expect an exception.
+ TSIGKey tsig_key("key.example:MSG6Ng==");
+ EXPECT_THROW(dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS),
+ TEST_PORT, message, timeout,
+ tsig_key),
+ isc::NotImplemented);
}
// This test verifies the DNSClient behavior when a server does not respond
@@ -201,7 +277,7 @@ public:
// This test verifies that DNSClient can send DNS Update and receive a
// corresponding response from a server.
void runSendReceiveTest(const bool corrupt_response,
- const bool two_sends = false) {
+ const bool two_sends) {
corrupt_response_ = corrupt_response;
// Create a request DNS Update message.
@@ -270,23 +346,41 @@ TEST_F(DNSClientTest, constructor) {
runConstructorTest();
}
+// This test verifies that the maximal allowed timeout value is maximal int
+// value.
+TEST_F(DNSClientTest, getMaxTimeout) {
+ EXPECT_EQ(std::numeric_limits<int>::max(), DNSClient::getMaxTimeout());
+}
+
// Verify that timeout is reported when no response is received from DNS.
TEST_F(DNSClientTest, timeout) {
runSendNoReceiveTest();
}
+// Verify that exception is thrown when invalid (too high) timeout value is
+// specified for asynchronous DNS Update.
+TEST_F(DNSClientTest, invalidTimeout) {
+ runInvalidTimeoutTest();
+}
+
+// Verify that exception is thrown when an attempt to send DNS Update with TSIG
+// is made. This test will be removed/changed once TSIG support is added.
+TEST_F(DNSClientTest, runTSIGTest) {
+ runTSIGTest();
+}
+
// Verify that the DNSClient receives the response from DNS and the received
// buffer can be decoded as DNS Update Response.
TEST_F(DNSClientTest, sendReceive) {
// false means that server response is not corrupted.
- runSendReceiveTest(false);
+ runSendReceiveTest(false, false);
}
// Verify that the DNSClient reports an error when the response is received from
// a DNS and this response is corrupted.
TEST_F(DNSClientTest, sendReceiveCurrupted) {
// true means that server's response is corrupted.
- runSendReceiveTest(true);
+ runSendReceiveTest(true, false);
}
// Verify that it is possible to use the same DNSClient instance to
@@ -296,8 +390,8 @@ TEST_F(DNSClientTest, sendReceiveCurrupted) {
// 3. send
// 4. receive
TEST_F(DNSClientTest, sendReceiveTwice) {
- runSendReceiveTest(false);
- runSendReceiveTest(false);
+ runSendReceiveTest(false, false);
+ runSendReceiveTest(false, false);
}
// Verify that it is possible to use the DNSClient instance to perform the
More information about the bind10-changes
mailing list