[2977] Addressed review comments.

In particular, added a doUpdate version which supports TSIG. It is also
possible to specify Transport layer protocol preferred.

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
 

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.

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