[3241] Addressed review comments.

Changed dhcp_ddns::NameChangeRequest to store lease address as
IOAddress only.  Corrected logic that handles request build
exceptions and added unit tests for same.  Other minor changes.

src/bin/d2/d2_messages.mes

@@ -325,19 +325,19 @@ message but the attempt to send it suffered a unexpected error. This is most
 likely a programmatic error, rather than a communications issue. Some or all
 of the DNS updates requested as part of this request did not succeed.
 
-% DHCP_DDNS_FORWARD_ADD_BUILD_FAILURE A DNS udpate message to add a forward DNS entry could not be constructed for this request: %1  reason: %2
+% DHCP_DDNS_FORWARD_ADD_BUILD_FAILURE A DNS udpate message to add a forward DNS entry could not be constructed for this request: %1, reason: %2
 This is an error message issued when an error occurs attempting to construct
 the server bound packet requesting a forward address addition.  This is due
 to invalid data contained in the NameChangeRequest. The request will be aborted.
 This is most likely a configuration issue.
 
-% DHCP_DDNS_FORWARD_REPLACE_BUILD_FAILURE A DNS update message to replace a foward DNS entry could not be constructed from this request: %1  reason: %2
+% DHCP_DDNS_FORWARD_REPLACE_BUILD_FAILURE A DNS update message to replace a foward DNS entry could not be constructed from this request: %1, reason: %2
 This is an error message issued when an error occurs attempting to construct
 the server bound packet requesting a forward address replacement.  This is
 due to invalid data contained in the NameChangeRequest. The request will be
 aborted.  This is most likely a configuration issue.
 
-% DHCP_DDNS_REVERSE_REPLACE_BUILD_FAILURE A DNS update message to replace a reverse DNS entry could not be constructed from this request: %1  reason: %2
+% DHCP_DDNS_REVERSE_REPLACE_BUILD_FAILURE A DNS update message to replace a reverse DNS entry could not be constructed from this request: %1, reason: %2
 This is an error message issued when an error occurs attempting to construct
 the server bound packet requesting a reverse PTR replacement.  This is
 due to invalid data contained in the NameChangeRequest. The request will be
@@ -349,5 +349,5 @@ additions which were received and accepted by an appropriate DNS server.
 
 % DHCP_DDNS_ADD_FAILED DHCP_DDNS failed attempting to make DNS mapping additions for this request: %1
 This is an error message issued after DHCP_DDNS attempts to submit DNS mapping
-entry additions have failed.  There precise reason for the failure should be
+entry additions have failed.  The precise reason for the failure should be
 documented in preceding log entries.

src/bin/d2/d2_update_message.cc

@@ -31,7 +31,7 @@ D2UpdateMessage::D2UpdateMessage(const Direction direction)
     if (direction == OUTBOUND) {
         message_.setOpcode(Opcode(Opcode::UPDATE_CODE));
         message_.setHeaderFlag(dns::Message::HEADERFLAG_QR, false);
-        message_.setRcode(Rcode(Rcode::NOERROR_CODE));
+        message_.setRcode(Rcode::NOERROR());
     }
 }
 

src/bin/d2/nc_add.cc

@@ -182,6 +182,7 @@ NameAddTransaction::addingFwdAddrsHandler() {
                           .arg(getNcr()->toText())
                           .arg(ex.what());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
+                break;
             }
         }
 
@@ -292,6 +293,7 @@ NameAddTransaction::replacingFwdAddrsHandler() {
                           .arg(getNcr()->toText())
                           .arg(ex.what());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
+                break;
             }
         }
 
@@ -437,6 +439,7 @@ NameAddTransaction::replacingRevPtrsHandler() {
                           .arg(getNcr()->toText())
                           .arg(ex.what());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
+                break;
             }
         }
 

src/bin/d2/nc_trans.h

@@ -368,11 +368,12 @@ protected:
     ///
     /// Constructs a new "empty", OUTBOUND, request with the message id set
     /// and zone section populated based on the given domain.
+    /// It is declared virtual for test purposes.
     ///
     /// @return A D2UpdateMessagePtr to the new request.
     ///
     /// @throw NameChangeTransactionError if request cannot be constructed.
-    D2UpdateMessagePtr prepNewRequest(DdnsDomainPtr domain);
+    virtual D2UpdateMessagePtr prepNewRequest(DdnsDomainPtr domain);
 
     /// @brief Adds an RData for the lease address to the given RRset.
     ///

src/bin/d2/tests/d_test_stubs.cc

@@ -32,7 +32,7 @@ const char* valid_d2_config = "{ "
                         "} ],"
                         "\"forward_ddns\" : {"
                         "\"ddns_domains\": [ "
-                        "{ \"name\": \"tmark.org\" , "
+                        "{ \"name\": \"tmark.org.\" , "
                         "  \"key_name\": \"d2_key.tmark.org\" , "
                         "  \"dns_servers\" : [ "
                         "  { \"hostname\": \"one.tmark\" } "

src/bin/d2/tests/nc_add_unittests.cc

@@ -37,7 +37,8 @@ public:
                 DdnsDomainPtr& forward_domain,
                 DdnsDomainPtr& reverse_domain)
         : NameAddTransaction(io_service, ncr, forward_domain, reverse_domain),
-          simulate_send_exception_(false) {
+          simulate_send_exception_(false),
+          simulate_build_request_exception_(false) {
     }
 
     virtual ~NameAddStub() {
@@ -70,11 +71,31 @@ public:
         postNextEvent(StateModel::NOP_EVT);
     }
 
+    /// @brief Prepares the initial D2UpdateMessage
+    ///
+    /// This method overrides the NameChangeTransactio implementation to
+    /// provide the ability to simulate an exception throw in the build
+    /// request logic.
+    /// If the one-shot flag, simulate_build_request_exception_ is true,
+    /// this method will throw an exception, otherwise it will invoke the
+    /// base class method, providing normal functionality.
+    ///
+    /// For parameter description see the NameChangeTransaction implementation.
+    virtual D2UpdateMessagePtr prepNewRequest(DdnsDomainPtr domain) {
+        if (simulate_build_request_exception_) {
+            simulate_build_request_exception_ = false;
+            isc_throw (NameAddTransactionError,
+                       "Simulated build requests exception");
+        }
+
+        return (NameChangeTransaction::prepNewRequest(domain));
+    }
+
     /// @brief Simulates receiving a response
     ///
     /// This method simulates the completion of a DNSClient send.  This allows
     /// the state handler logic devoted to dealing with IO completion to be
-    /// fully exercise without requiring any actual IO.  The two primary
+    /// fully exercised without requiring any actual IO.  The two primary
     /// pieces of information gleaned from IO completion are the DNSClient
     /// status which indicates whether or not the IO exchange was successful
     /// and the rcode, which indicates the server's reaction to the request.
@@ -134,6 +155,10 @@ public:
     /// @brief One-shot flag which will simulate sendUpdate failure if true.
     bool simulate_send_exception_;
 
+    /// @brief One-shot flag which will simulate an exception when sendUpdate
+    /// failure if true.
+    bool simulate_build_request_exception_;
+
     using StateModel::postNextEvent;
     using StateModel::setState;
     using StateModel::initDictionaries;
@@ -1626,7 +1651,7 @@ TEST_F(NameAddTransactionTest, addingFwdAddrsHandler_sendUpdateException) {
     name_add->simulate_send_exception_ = true;
 
     // Run replacingFwdAddrsHandler to construct and send the request.
-    EXPECT_NO_THROW(name_add->addingFwdAddrsHandler());
+    ASSERT_NO_THROW(name_add->addingFwdAddrsHandler());
 
     // Completion flags should be false.
     EXPECT_FALSE(name_add->getForwardChangeCompleted());
@@ -1658,7 +1683,7 @@ TEST_F(NameAddTransactionTest, replacingFwdAddrsHandler_SendUpdateException) {
     name_add->simulate_send_exception_ = true;
 
     // Run replacingFwdAddrsHandler to construct and send the request.
-    EXPECT_NO_THROW(name_add->replacingFwdAddrsHandler());
+    ASSERT_NO_THROW(name_add->replacingFwdAddrsHandler());
 
     // Completion flags should be false.
     EXPECT_FALSE(name_add->getForwardChangeCompleted());
@@ -1690,7 +1715,45 @@ TEST_F(NameAddTransactionTest, replacingRevPtrsHandler_SendUpdateException) {
     name_add->simulate_send_exception_ = true;
 
     // Run replacingRevPtrsHandler to construct and send the request.
-    EXPECT_NO_THROW(name_add->replacingRevPtrsHandler());
+    ASSERT_NO_THROW(name_add->replacingRevPtrsHandler());
+
+    // Completion flags should be false.
+    EXPECT_FALSE(name_add->getForwardChangeCompleted());
+    EXPECT_FALSE(name_add->getReverseChangeCompleted());
+
+    // Since IO exceptions should be gracefully handled, any that occur
+    // are unanticipated, and deemed unrecoverable, so the transaction should
+    // be transitioned to failure.
+    EXPECT_EQ(NameChangeTransaction::PROCESS_TRANS_FAILED_ST,
+              name_add->getCurrState());
+    EXPECT_EQ(NameChangeTransaction::UPDATE_FAILED_EVT,
+              name_add->getNextEvent());
+}
+
+// Tests addingFwdAddrsHandler with the following scenario:
+//
+//  The request includes only a forward change.
+//  Initial posted event is SERVER_SELECTED_EVT.
+//  The request build fails due to an unexpected exception.
+//
+TEST_F(NameAddTransactionTest, addingFwdAddrsHandler_BuildRequestException) {
+    NameAddStubPtr name_add;
+    // Create and prep a transaction, poised to run the handler.
+    ASSERT_NO_THROW(name_add =
+                    prepHandlerTest(NameAddTransaction::ADDING_FWD_ADDRS_ST,
+                                    NameChangeTransaction::
+                                    SERVER_SELECTED_EVT, FORWARD_CHG));
+
+    // Set the one-shot exception simulation flag.
+    name_add->simulate_build_request_exception_ = true;
+
+    // Run replacingRevPtrsHandler to construct and send the request.
+    // This should fail with a build request throw which should be caught
+    // in the state handler.
+    ASSERT_NO_THROW(name_add->addingFwdAddrsHandler());
+
+    // Verify we did not attempt to send anything.
+    EXPECT_EQ(0, name_add->getUpdateAttempts());
 
     // Completion flags should be false.
     EXPECT_FALSE(name_add->getForwardChangeCompleted());
@@ -1705,4 +1768,82 @@ TEST_F(NameAddTransactionTest, replacingRevPtrsHandler_SendUpdateException) {
               name_add->getNextEvent());
 }
 
+// Tests replacingFwdAddrsHandler with the following scenario:
+//
+//  The request includes only a forward change.
+//  Initial posted event is SERVER_SELECTED_EVT.
+//  The request build fails due to an unexpected exception.
+//
+TEST_F(NameAddTransactionTest, replacingFwdAddrsHandler_BuildRequestException) {
+    NameAddStubPtr name_add;
+    // Create and prep a transaction, poised to run the handler.
+    ASSERT_NO_THROW(name_add =
+                    prepHandlerTest(NameAddTransaction::REPLACING_FWD_ADDRS_ST,
+                                    NameChangeTransaction::
+                                    SERVER_SELECTED_EVT, FORWARD_CHG));
+
+    // Set the one-shot exception simulation flag.
+    name_add->simulate_build_request_exception_ = true;
+
+    // Run replacingFwdAddrsHandler to construct and send the request.
+    // This should fail with a build request throw which should be caught
+    // in the state handler.
+    ASSERT_NO_THROW(name_add->replacingFwdAddrsHandler());
+
+    // Verify we did not attempt to send anything.
+    EXPECT_EQ(0, name_add->getUpdateAttempts());
+
+    // Completion flags should be false.
+    EXPECT_FALSE(name_add->getForwardChangeCompleted());
+    EXPECT_FALSE(name_add->getReverseChangeCompleted());
+
+    // Since IO exceptions should be gracefully handled, any that occur
+    // are unanticipated, and deemed unrecoverable, so the transaction should
+    // be transitioned to failure.
+    EXPECT_EQ(NameChangeTransaction::PROCESS_TRANS_FAILED_ST,
+              name_add->getCurrState());
+    EXPECT_EQ(NameChangeTransaction::UPDATE_FAILED_EVT,
+              name_add->getNextEvent());
+}
+
+
+// Tests replacingRevPtrHandler with the following scenario:
+//
+//  The request includes only a reverse change.
+//  Initial posted event is SERVER_SELECTED_EVT.
+//  The request build fails due to an unexpected exception.
+//
+TEST_F(NameAddTransactionTest, replacingRevPtrsHandler_BuildRequestException) {
+    NameAddStubPtr name_add;
+    // Create and prep a transaction, poised to run the handler.
+    ASSERT_NO_THROW(name_add =
+                    prepHandlerTest(NameAddTransaction::REPLACING_REV_PTRS_ST,
+                                    NameChangeTransaction::
+                                    SERVER_SELECTED_EVT, REVERSE_CHG));
+
+    // Set the one-shot exception simulation flag.
+    name_add->simulate_build_request_exception_ = true;
+
+    // Run replacingRevPtrsHandler to construct and send the request.
+    // This should fail with a build request throw which should be caught
+    // in the state handler.
+    ASSERT_NO_THROW(name_add->replacingRevPtrsHandler());
+
+    // Verify we did not attempt to send anything.
+    EXPECT_EQ(0, name_add->getUpdateAttempts());
+
+    // Completion flags should be false.
+    EXPECT_FALSE(name_add->getForwardChangeCompleted());
+    EXPECT_FALSE(name_add->getReverseChangeCompleted());
+
+    // Since IO exceptions should be gracefully handled, any that occur
+    // are unanticipated, and deemed unrecoverable, so the transaction should
+    // be transitioned to failure.
+    EXPECT_EQ(NameChangeTransaction::PROCESS_TRANS_FAILED_ST,
+              name_add->getCurrState());
+    EXPECT_EQ(NameChangeTransaction::UPDATE_FAILED_EVT,
+              name_add->getNextEvent());
+}
+
+
 }

src/bin/d2/tests/nc_trans_unittests.cc

@@ -979,7 +979,7 @@ TEST_F(NameChangeTransactionTest, prepNewRequest) {
         NameChangeTransactionError);
 
     // Verify that prepNewRequest fails on invalid zone name.
-    // @todo This test becomes obsolete if/when DdsnDomain enforces valid
+    // @todo This test becomes obsolete if/when DdnsDomain enforces valid
     // names as is done in dns::Name.
     DdnsDomainPtr bsDomain = makeDomain(".badname","");
     ASSERT_THROW(request = name_change->prepNewRequest(bsDomain),

src/lib/dhcp_ddns/ncr_msg.cc

@@ -13,6 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <dhcp_ddns/ncr_msg.h>
+#include <dns/name.h>
 #include <asiolink/io_address.h>
 #include <asiolink/io_error.h>
 #include <cryptolink/cryptolink.h>
@@ -160,10 +161,10 @@ D2Dhcid::createDigest(const uint8_t identifier_type,
     }
 
     // The DHCID RDATA has the following structure:
-    // 
+    //
     //    < identifier-type > < digest-type > < digest >
     //
-    // where identifier type 
+    // where identifier type
 
     // Let's allocate the space for the identifier-type (2 bytes) and
     // digest-type (1 byte). This is 3 bytes all together.
@@ -189,8 +190,7 @@ operator<<(std::ostream& os, const D2Dhcid& dhcid) {
 
 NameChangeRequest::NameChangeRequest()
     : change_type_(CHG_ADD), forward_change_(false),
-    reverse_change_(false), fqdn_(""), ip_address_(""),
-    ip_io_address_("0.0.0.0"),
+    reverse_change_(false), fqdn_(""), ip_io_address_("0.0.0.0"),
     dhcid_(), lease_expires_on_(), lease_length_(0), status_(ST_NEW) {
 }
 
@@ -201,13 +201,12 @@ NameChangeRequest::NameChangeRequest(const NameChangeType change_type,
             const uint64_t lease_expires_on,
             const uint32_t lease_length)
     : change_type_(change_type), forward_change_(forward_change),
-    reverse_change_(reverse_change), fqdn_(fqdn), ip_address_(ip_address),
-    ip_io_address_("0.0.0.0"),
+    reverse_change_(reverse_change), fqdn_(fqdn), ip_io_address_("0.0.0.0"),
     dhcid_(dhcid), lease_expires_on_(lease_expires_on),
     lease_length_(lease_length), status_(ST_NEW) {
 
     // User setter to validate address.
-    setIpAddress(ip_address_);
+    setIpAddress(ip_address);
 
     // Validate the contents. This will throw a NcrMessageError if anything
     // is invalid.
@@ -475,18 +474,23 @@ NameChangeRequest::setFqdn(isc::data::ConstElementPtr element) {
 
 void
 NameChangeRequest::setFqdn(const std::string& value) {
-    fqdn_ = value;
+    try {
+        dns::Name tmp(value);
+        fqdn_ = tmp.toText();
+    } catch (const std::exception& ex) {
+        isc_throw(NcrMessageError,
+                  "Invalid FQDN value: " << value << ", reason:" << ex.what());
+    }
 }
 
 void
 NameChangeRequest::setIpAddress(const std::string& value) {
     // Validate IP Address.
     try {
-        ip_address_ = value;
-        ip_io_address_ = isc::asiolink::IOAddress(ip_address_);
+        ip_io_address_ = isc::asiolink::IOAddress(value);
     } catch (const isc::asiolink::IOError& ex) {
         isc_throw(NcrMessageError,
-                  "Invalid ip address string for ip_address: " << ip_address_);
+                  "Invalid ip address string for ip_address: " << value);
     }
 }
 
@@ -586,7 +590,7 @@ NameChangeRequest::toText() const {
            << "Reverse Change: " << (reverse_change_ ? "yes" : "no")
            << std::endl
            << "FQDN: [" << fqdn_ << "]" << std::endl
-           << "IP Address: [" << ip_address_  << "]" << std::endl
+           << "IP Address: [" << ip_io_address_.toText()  << "]" << std::endl
            << "DHCID: [" << dhcid_.toStr() << "]" << std::endl
            << "Lease Expires On: " << getLeaseExpiresOnStr() << std::endl
            << "Lease Length: " << lease_length_ << std::endl;
@@ -600,7 +604,7 @@ NameChangeRequest::operator == (const NameChangeRequest& other) {
             (forward_change_ == other.forward_change_) &&
             (reverse_change_ == other.reverse_change_) &&
             (fqdn_ == other.fqdn_) &&
-            (ip_address_ == other.ip_address_) &&
+            (ip_io_address_ == other.ip_io_address_) &&
             (dhcid_ == other.dhcid_) &&
             (lease_expires_on_ == other.lease_expires_on_) &&
             (lease_length_ == other.lease_length_));

src/lib/dhcp_ddns/ncr_msg.h

@@ -23,6 +23,7 @@
 #include <cc/data.h>
 #include <dhcp/duid.h>
 #include <dhcp/hwaddr.h>
+#include <dns/name.h>
 #include <exceptions/exceptions.h>
 #include <util/buffer.h>
 #include <util/encode/hex.h>
@@ -414,8 +415,8 @@ public:
     /// @brief Fetches the request IP address string.
     ///
     /// @return a string containing the IP address
-    const std::string& getIpAddress() const {
-        return (ip_address_);
+    std::string getIpAddress() const {
+        return (ip_io_address_.toText());
     }
 
     /// @brief Fetches the request IP address as an IOAddress.
@@ -428,14 +429,14 @@ public:
     /// @brief Returns true if the lease address is a IPv4 lease.
     ///
     /// @return boolean true if the lease address family is AF_INET.
-    bool isV4 () {
+    bool isV4 () const {
         return (ip_io_address_.isV4());
     }
 
     /// @brief Returns true if the lease address is a IPv6 lease.
     ///
     /// @return boolean true if the lease address family is AF_INET6.
-    bool isV6 () {
+    bool isV6 () const {
         return (ip_io_address_.isV6());
     }
 
@@ -588,9 +589,6 @@ private:
     /// manipulation.
     std::string fqdn_;
 
-    /// @brief The ip address leased to the FQDN.
-    std::string ip_address_;
-
     /// @brief The ip address leased to the FQDN as an IOAddress.
     ///
     /// The lease address is used in many places, sometimes as a string

src/lib/dhcp_ddns/tests/ncr_unittests.cc

@@ -125,6 +125,17 @@ const char *invalid_msgs[] =
      " \"lease_expires_on\" : \"20130121132405\" , "
      " \"lease_length\" : 1300 "
      "}",
+    // Malformed FQDN
+     "{"
+     " \"change_type\" : 0 , "
+     " \"forward_change\" : true , "
+     " \"reverse_change\" : false , "
+     " \"fqdn\" : \".bad_name\" , "
+     " \"ip_address\" : \"192.168.2.1\" , "
+     " \"dhcid\" : \"010203040A7F8E3D\" , "
+     " \"lease_expires_on\" : \"20130121132405\" , "
+     " \"lease_length\" : 1300 "
+     "}",
     // Bad IP address
      "{"
      " \"change_type\" : 0 , "
@@ -462,7 +473,7 @@ TEST(NameChangeRequestTest, basicJsonTest) {
                             "\"change_type\":1,"
                             "\"forward_change\":true,"
                             "\"reverse_change\":false,"
-                            "\"fqdn\":\"walah.walah.com\","
+                            "\"fqdn\":\"walah.walah.com.\","
                             "\"ip_address\":\"192.168.2.1\","
                             "\"dhcid\":\"010203040A7F8E3D\","
                             "\"lease_expires_on\":\"20130121132405\","
@@ -543,7 +554,7 @@ TEST(NameChangeRequestTest, toFromBufferTest) {
                             "\"change_type\":1,"
                             "\"forward_change\":true,"
                             "\"reverse_change\":false,"
-                            "\"fqdn\":\"walah.walah.com\","
+                            "\"fqdn\":\"walah.walah.com.\","
                             "\"ip_address\":\"192.168.2.1\","
                             "\"dhcid\":\"010203040A7F8E3D\","
                             "\"lease_expires_on\":\"20130121132405\","
@@ -593,7 +604,7 @@ TEST(NameChangeRequestTest, ipAddresses) {
     EXPECT_FALSE(ncr.isV4());
     EXPECT_TRUE(ncr.isV6());
 
-    // Verify that an valid address fails.
+    // Verify that an invalid address fails.
     ASSERT_THROW(ncr.setIpAddress("x001:1::f3"),NcrMessageError);
 }