diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes index e08c283b24..62ffd08b37 100644 --- a/src/bin/d2/d2_messages.mes +++ b/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. diff --git a/src/bin/d2/d2_update_message.cc b/src/bin/d2/d2_update_message.cc index 5d1639275b..71ea5b2aa4 100644 --- a/src/bin/d2/d2_update_message.cc +++ b/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()); } } diff --git a/src/bin/d2/nc_add.cc b/src/bin/d2/nc_add.cc index 968bcbe308..b92ec564e6 100644 --- a/src/bin/d2/nc_add.cc +++ b/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; } } diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index 8f94124d06..b0f7c4a747 100644 --- a/src/bin/d2/nc_trans.h +++ b/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. /// diff --git a/src/bin/d2/tests/d_test_stubs.cc b/src/bin/d2/tests/d_test_stubs.cc index ab061ef97b..319dcd74e4 100644 --- a/src/bin/d2/tests/d_test_stubs.cc +++ b/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\" } " diff --git a/src/bin/d2/tests/nc_add_unittests.cc b/src/bin/d2/tests/nc_add_unittests.cc index 6e3fefbd50..c291a54944 100644 --- a/src/bin/d2/tests/nc_add_unittests.cc +++ b/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,7 @@ 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()); @@ -1705,4 +1730,120 @@ TEST_F(NameAddTransactionTest, replacingRevPtrsHandler_SendUpdateException) { 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()); + 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 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()); +} + + } diff --git a/src/bin/d2/tests/nc_trans_unittests.cc b/src/bin/d2/tests/nc_trans_unittests.cc index cc08a360ab..6e1c3fc69a 100644 --- a/src/bin/d2/tests/nc_trans_unittests.cc +++ b/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), diff --git a/src/lib/dhcp_ddns/ncr_msg.cc b/src/lib/dhcp_ddns/ncr_msg.cc index 739eac4c86..418eaa0a1f 100644 --- a/src/lib/dhcp_ddns/ncr_msg.cc +++ b/src/lib/dhcp_ddns/ncr_msg.cc @@ -13,6 +13,7 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include #include #include #include @@ -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_)); diff --git a/src/lib/dhcp_ddns/ncr_msg.h b/src/lib/dhcp_ddns/ncr_msg.h index c2ad86c0ba..47a7be642e 100644 --- a/src/lib/dhcp_ddns/ncr_msg.h +++ b/src/lib/dhcp_ddns/ncr_msg.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -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 diff --git a/src/lib/dhcp_ddns/tests/ncr_unittests.cc b/src/lib/dhcp_ddns/tests/ncr_unittests.cc index 5d72350dc2..9386a5fcbf 100644 --- a/src/lib/dhcp_ddns/tests/ncr_unittests.cc +++ b/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); }