From: Thomas Markwalder Date: Fri, 16 Oct 2020 17:35:00 +0000 (-0400) Subject: [1386] Made SimpleRemoveTranscation explicitly deletes PTR,DHCID RRs X-Git-Tag: Kea-1.9.1~59 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=20a36247e40bbf9dbfe8c12abec2f461b0b6ed03;p=thirdparty%2Fkea.git [1386] Made SimpleRemoveTranscation explicitly deletes PTR,DHCID RRs src/bin/d2/d2_update_mgr.* Fixed copyright Updated commentary src/bin/d2/simple_add.h Updated commentary src/bin/d2/simple_remove.cc SimpleRemoveTransaction::buildRemoveRevPtrsRequest() - now explicitly deletes PTR and DHCID RRs src/bin/d2/simple_remove.h Updated commentary src/bin/d2/tests/nc_test_utils.* checkSimpleRemoveRevPtrsRequest() - new function src/bin/d2/tests/simple_remove_unittests.cc updated tests --- diff --git a/src/bin/d2/d2_update_mgr.cc b/src/bin/d2/d2_update_mgr.cc index 28bc25df39..c3505f45ea 100644 --- a/src/bin/d2/d2_update_mgr.cc +++ b/src/bin/d2/d2_update_mgr.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2020 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/d2/d2_update_mgr.h b/src/bin/d2/d2_update_mgr.h index 29a49d7e19..1717cdd31f 100644 --- a/src/bin/d2/d2_update_mgr.h +++ b/src/bin/d2/d2_update_mgr.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2020 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -155,6 +155,10 @@ protected: /// of servers is an error which will be logged and the request will be /// discarded. /// + /// Finally, If conflict resolution is enabled, it will instantiate either + /// a NameAddTranscation or a NameRemoveTransaction. If disabled it will + /// instantiate either a SimpleAddTransaction or a SimpleRemoveTranscation. + /// /// @param ncr the NameChangeRequest for which to create a transaction. /// /// @throw D2UpdateMgrError if a transaction for this DHCID already diff --git a/src/bin/d2/simple_add.h b/src/bin/d2/simple_add.h index 25948addb8..7bf7b6c605 100644 --- a/src/bin/d2/simple_add.h +++ b/src/bin/d2/simple_add.h @@ -26,20 +26,15 @@ public: /// @brief Embodies the "life-cycle" required to carry out a DDNS Add update. /// /// SimpleAddTransaction implements a state machine for adding (or replacing) a -/// forward and/or reverse DNS mapping. This state machine is based upon the -/// processing logic described in RFC 4703, Sections 5.3 and 5.4. That logic -/// may be paraphrased as follows: +/// forward and/or reverse DNS mapping. This state machine follows a basic +/// remove and replace scheme, that does not attempt to avoid conflicts +/// between updating clients. The logic may be paraphrased as follows: /// /// @code /// /// If the request includes a forward change: /// Select a forward server -/// Send the server a request to add the forward entry -/// If the server responds with already in use: -/// Send a server a request to delete and then add forward entry -/// -/// If the forward update is unsuccessful: -/// abandon the update +/// Send the server a request to delete and then add forward entry /// /// If the request includes a reverse change: /// Select a reverse server @@ -159,8 +154,8 @@ protected: /// handler simply attempts to select the next server. /// /// Transitions to: - /// - REPLACING_FWD_ADDRS_ST with next event of SERVER_SELECTED upon successful - /// server selection + /// - REPLACING_FWD_ADDRS_ST with next event of SERVER_SELECTED upon + /// successful server selection /// /// - PROCESS_TRANS_FAILED with next event of NO_MORE_SERVERS_EVT upon /// failure to select a server @@ -183,8 +178,8 @@ protected: /// handler simply attempts to select the next server. /// /// Transitions to: - /// - REPLACING_REV_PTRS_ST with next event of SERVER_SELECTED upon successful - /// server selection + /// - REPLACING_REV_PTRS_ST with next event of SERVER_SELECTED upon + /// successful server selection /// /// - PROCESS_TRANS_FAILED with next event of NO_MORE_SERVERS_EVT upon /// failure to select a server @@ -314,7 +309,8 @@ protected: /// UPDATE_FAILED_EVT void processAddFailedHandler(); - /// @brief Builds a DNS request to add/replace a forward DNS entry for an FQDN + /// @brief Builds a DNS request to add/replace a forward DNS entry for an + /// FQDN /// /// Constructs a DNS update request, based upon the NCR, for adding a /// forward DNS mapping. Once constructed, the request is stored as @@ -324,9 +320,10 @@ protected: /// - There are no prerequisites. /// /// Updates RRsets: - /// 1. A delete of all RRs for the given FQDN - /// 1. An FQDN/IP RR addition (type A for IPv4, AAAA for IPv6) - /// 2. An FQDN/DHCID RR addition (type DHCID) + /// -# A delete of any existing PTR RRs for the lease address + /// -# A delete of any existing DHCID RRs for the lease address + /// -# An FQDN/IP RR addition (type A for IPv4, AAAA for IPv6) + /// -# An FQDN/DHCID RR addition (type DHCID) /// /// @throw This method does not throw but underlying methods may. void buildReplaceFwdAddressRequest(); @@ -341,10 +338,10 @@ protected: /// - There are no prerequisites. /// /// Updates RRsets: - /// 1. A delete of any existing PTR RRs for the lease address - /// 2. A delete of any existing DHCID RRs for the lease address - /// 3. A PTR RR addition for the lease address and FQDN - /// 4. A DHCID RR addition for the lease address and lease client DHCID + /// -# A delete of any existing PTR RRs for the lease address + /// -# A delete of any existing DHCID RRs for the lease address + /// -# A PTR RR addition for the lease address and FQDN + /// -# A DHCID RR addition for the lease address and lease client DHCID /// /// @throw This method does not throw but underlying methods may. void buildReplaceRevPtrsRequest(); diff --git a/src/bin/d2/simple_remove.cc b/src/bin/d2/simple_remove.cc index d914637c38..63de0e3ab1 100644 --- a/src/bin/d2/simple_remove.cc +++ b/src/bin/d2/simple_remove.cc @@ -479,7 +479,7 @@ SimpleRemoveTransaction::buildRemoveFwdRRsRequest() { // Construct dns::Name from NCR fqdn. dns::Name fqdn(dns::Name(getNcr()->getFqdn())); - // Build the Update Section. + // Build the Update Section. // Create the FQDN/IP 'delete' RR and add it to update section. dns::RRsetPtr update(new dns::RRset(fqdn, dns::RRClass::ANY(), @@ -505,21 +505,19 @@ SimpleRemoveTransaction::buildRemoveRevPtrsRequest() { std::string rev_addr = D2CfgMgr::reverseIpAddress(getNcr()->getIpAddress()); dns::Name rev_ip(rev_addr); - /// TKM @todo - not sure we need this pre-req - // Create an assertion that the PTRDNAME in the PTR record matches the - // client's FQDN for the address that was released. - // Based on RFC 2136, section 3.2.3 - dns::RRsetPtr prereq(new dns::RRset(rev_ip, dns::RRClass::IN(), - dns::RRType::PTR(), dns::RRTTL(0))); - addPtrRdata(prereq); - request->addRRset(D2UpdateMessage::SECTION_PREREQUISITE, prereq); + // There are no pre-requisites. - // Now, build the Update section. + // Build the Update section. - // Create a delete of any RRs for the FQDN and add it to update section. - // Based on RFC 2136, section 3.4.2.3 + // Create the FQDN/IP PTR 'delete' RR for this IP and add it to + // the update section. dns::RRsetPtr update(new dns::RRset(rev_ip, dns::RRClass::ANY(), - dns::RRType::ANY(), dns::RRTTL(0))); + dns::RRType::PTR(), dns::RRTTL(0))); + request->addRRset(D2UpdateMessage::SECTION_UPDATE, update); + + // Create the DHCID 'delete' RR and add it to the update section. + update.reset(new dns::RRset(rev_ip, dns::RRClass::ANY(), + dns::RRType::DHCID(), dns::RRTTL(0))); request->addRRset(D2UpdateMessage::SECTION_UPDATE, update); // Set the transaction's update request to the new request. diff --git a/src/bin/d2/simple_remove.h b/src/bin/d2/simple_remove.h index ff432541d1..ce6d525145 100644 --- a/src/bin/d2/simple_remove.h +++ b/src/bin/d2/simple_remove.h @@ -25,24 +25,22 @@ public: /// @brief Embodies the "life-cycle" required to carry out a DDNS Remove update. /// /// SimpleRemoveTransaction implements a state machine for removing a forward -/// and/or reverse DNS mappings. This state machine is based upon the processing -/// logic described in RFC 4703, Section 5.5. That logic may be paraphrased as -/// follows: +/// and/or reverse DNS mappings. This state machine follows a basic +/// removal scheme, that does not attempt to avoid conflicts between updating +/// clients. The logic may be paraphrased as follows: /// /// @code /// /// If the request includes a forward change: /// Select a forward server -/// Send the server a request to remove client's specific forward address RR -/// If it succeeds or the server responds with name no longer in use -/// Send a server a request to delete any other RRs for that FQDN, such -/// as the DHCID RR. -/// otherwise +/// Send the server a request to remove client's specific forward address +/// RR and DHCID RR. +/// If it does not succeed /// abandon the update /// /// If the request includes a reverse change: /// Select a reverse server -/// Send a server a request to delete reverse entry (PTR RR) +/// Send a server a request to delete the matching PTR and DHCID RRs. /// /// @endcode /// @@ -313,12 +311,9 @@ protected: /// @brief Builds a DNS request to remove all forward DNS RRs for a FQDN. /// - /// Constructs a DNS update request, based upon the NCR, for removing any - /// remaining forward DNS RRs, once all A or AAAA entries for the FQDN - /// have been removed. Once constructed, the request is stored as the - /// transaction's DNS update request. - /// - /// The request content is adherent to RFC 4703 section 5.5, paragraph 5. + /// Constructs a DNS update request, based upon the NCR, to remove the + /// forward DNS (A or AAAA) RR and DHCID RR. Once constructed, the request + /// is stored as the transaction's DNS update request. /// /// Prerequisite RRsets: /// - None @@ -336,13 +331,12 @@ protected: /// reverse DNS mapping. Once constructed, the request is stored as /// the transaction's DNS update request. /// - /// The request content is adherent to RFC 4703 section 5.5, paragraph 2: - /// /// Prerequisite RRsets: - /// 1. An assertion that a PTR record matching the client's FQDN exists. + /// - None /// /// Updates RRsets: - /// 1. A delete of all RRs for the FQDN + /// -# A delete of PTR RR for the IP + /// -# A delete of DHCID RR for the IP /// /// @throw This method does not throw but underlying methods may. void buildRemoveRevPtrsRequest(); diff --git a/src/bin/d2/tests/nc_test_utils.cc b/src/bin/d2/tests/nc_test_utils.cc index 01014c5655..79b3dc54b4 100644 --- a/src/bin/d2/tests/nc_test_utils.cc +++ b/src/bin/d2/tests/nc_test_utils.cc @@ -917,6 +917,43 @@ void checkSimpleRemoveFwdRRsRequest(NameChangeTransaction& tran) { ASSERT_NO_THROW(request->toWire(renderer)); } +void checkSimpleRemoveRevPtrsRequest(NameChangeTransaction& tran) { + const D2UpdateMessagePtr& request = tran.getDnsUpdateRequest(); + ASSERT_TRUE(request); + + // Safety check. + dhcp_ddns::NameChangeRequestPtr ncr = tran.getNcr(); + ASSERT_TRUE(ncr); + + std::string exp_zone_name = tran.getReverseDomain()->getName(); + std::string exp_rev_addr = D2CfgMgr::reverseIpAddress(ncr->getIpAddress()); + const dns::RRType& exp_ip_rr_type = tran.getAddressRRType(); + + // Verify the zone section. + checkZone(request, exp_zone_name); + + // Verify there are no RRs in the PREREQUISITE Section. + checkRRCount(request, D2UpdateMessage::SECTION_PREREQUISITE, 0); + + // Verify there is 2 RRs in the UPDATE Section. + checkRRCount(request, D2UpdateMessage::SECTION_UPDATE, 2); + + // Verify the FQDN delete RR. + dns::RRsetPtr rrset; + ASSERT_TRUE(rrset = getRRFromSection(request, D2UpdateMessage:: + SECTION_UPDATE, 0)); + checkRR(rrset, exp_rev_addr, dns::RRClass::ANY(), dns::RRType::PTR(), 0, ncr); + + // Verify the DHCID delete RR. + ASSERT_TRUE(rrset = getRRFromSection(request, D2UpdateMessage:: + SECTION_UPDATE, 1)); + checkRR(rrset, exp_rev_addr, dns::RRClass::ANY(), dns::RRType::DHCID(), 0, ncr); + + // Verify that it will render toWire without throwing. + dns::MessageRenderer renderer; + ASSERT_NO_THROW(request->toWire(renderer)); +} + void checkContext(NameChangeTransactionPtr trans, const int exp_state, const int exp_evt, const std::string& file, int line) { ASSERT_TRUE(trans); diff --git a/src/bin/d2/tests/nc_test_utils.h b/src/bin/d2/tests/nc_test_utils.h index fa37bde2ed..bfb9240938 100644 --- a/src/bin/d2/tests/nc_test_utils.h +++ b/src/bin/d2/tests/nc_test_utils.h @@ -388,6 +388,14 @@ extern void checkSimpleReplaceFwdAddressRequest(NameChangeTransaction& tran); /// @param tran Transaction containing the request to be verified. extern void checkSimpleRemoveFwdRRsRequest(NameChangeTransaction& tran); +/// @brief Verifies a simple reverse RR removal DNS update request +/// +/// Tests that the DNS Update request for a given transaction, is correct for +/// removing reverse RR DNS entries when not using conflict resolution. +/// +/// @param tran Transaction containing the request to be verified. +extern void checkSimpleRemoveRevPtrsRequest(NameChangeTransaction& tran); + /// @brief Creates a NameChangeRequest from JSON string. /// /// @param ncr_str string of JSON text from which to make the request. diff --git a/src/bin/d2/tests/simple_remove_unittests.cc b/src/bin/d2/tests/simple_remove_unittests.cc index 98a70eb612..f86e3eaff5 100644 --- a/src/bin/d2/tests/simple_remove_unittests.cc +++ b/src/bin/d2/tests/simple_remove_unittests.cc @@ -344,14 +344,14 @@ TEST_F(SimpleRemoveTransactionTest, buildRemoveRevPtrsRequest) { SimpleRemoveStubPtr name_remove; ASSERT_NO_THROW(name_remove = makeTransaction4(REVERSE_CHG)); ASSERT_NO_THROW(name_remove->buildRemoveRevPtrsRequest()); - checkRemoveRevPtrsRequest(*name_remove); + checkSimpleRemoveRevPtrsRequest(*name_remove); // Create a IPv6 reverse replace transaction. // Verify the request builds without error. // and then verify the request contents. ASSERT_NO_THROW(name_remove = makeTransaction6(REVERSE_CHG)); ASSERT_NO_THROW(name_remove->buildRemoveRevPtrsRequest()); - checkRemoveRevPtrsRequest(*name_remove); + checkSimpleRemoveRevPtrsRequest(*name_remove); } // Tests the readyHandler functionality. @@ -813,7 +813,7 @@ TEST_F(SimpleRemoveTransactionTest, removingRevPtrsHandler_RevOnlyOK) { EXPECT_NO_THROW(name_remove->removingRevPtrsHandler()); // Verify that an update message was constructed properly. - checkRemoveRevPtrsRequest(*name_remove); + checkSimpleRemoveRevPtrsRequest(*name_remove); // Verify that we are still in this state and next event is NOP_EVT. // This indicates we "sent" the message and are waiting for IO completion. @@ -863,7 +863,7 @@ TEST_F(SimpleRemoveTransactionTest, removingRevPtrsHandler_FqdnNotInUse) { EXPECT_NO_THROW(name_remove->removingRevPtrsHandler()); // Verify that an update message was constructed properly. - checkRemoveRevPtrsRequest(*name_remove); + checkSimpleRemoveRevPtrsRequest(*name_remove); // Verify that we are still in this state and next event is NOP_EVT. // This indicates we "sent" the message and are waiting for IO completion.