[#4437] Avoid orphaning PTR records

/src/bin/d2/check_exists_remove.cc
    CheckExistsRemoveTransaction::removingFwdRRsHandler()
    - treat YXRRSET as success

/src/bin/d2/nc_remove.cc
    NameRemoveTransaction::removingFwdRRsHandler()
    - treat YXRRSET as success

/src/bin/d2/tests/check_exists_remove_unittests.cc
    TEST_F(CheckExistsRemoveTransactionTest, removingFwdRRsHandler_FQDNInUse)
    - new test

/src/bin/d2/tests/nc_remove_unittests.cc
    TEST_F(NameRemoveTransactionTest, removingFwdRRsHandler_FQDNInUse)
    - new test

diff --git a/changelog_unreleased/4437-d2-leaves-orphaned-ptr-records-when-a-client-changes-subnets-with-an-active-lease b/changelog_unreleased/4437-d2-leaves-orphaned-ptr-records-when-a-client-changes-subnets-with-an-active-lease
new file mode 100644 (file)
index 0000000..39d349e
--- /dev/null
+++ b/changelog_unreleased/4437-d2-leaves-orphaned-ptr-records-when-a-client-changes-subnets-with-an-active-lease
@@ -0,0 +1,7 @@
+[func]         tmark
+       Modified D2 logic to avoid orphaning
+       PTR records when clients have changed
+       subnets or there are forward RR records
+       of the other protocol in a dual-stack
+       environment.
+       (Gitlab #4437)

diff --git a/src/bin/d2/check_exists_remove.cc b/src/bin/d2/check_exists_remove.cc
index 8ae529605d8f504e3cb028d26cb8e7b8a15c9016..be3294dc2ec1fd5941c2a7c20e4b0d86fb5e95a6 100644 (file)
--- a/src/bin/d2/check_exists_remove.cc
+++ b/src/bin/d2/check_exists_remove.cc
@@ -326,10 +326,15 @@ CheckExistsRemoveTransaction::removingFwdRRsHandler() {
             // see RFC 2136 section 3.2.3/3.2.4.
             const dns::Rcode& rcode = getDnsUpdateResponse()->getRcode();
             if ((rcode == dns::Rcode::NOERROR()) ||
-                (rcode == dns::Rcode::NXRRSET())) {
+                (rcode == dns::Rcode::NXRRSET()) ||
+                (rcode == dns::Rcode::YXRRSET())) {
                 // We were able to remove them or they were not there (
                 // Rcode of NXRRSET means there are no matching RRsets).
                 // In either case, we consider it success and mark it as done.
+                // YXRRSET means either a FWD RR with a different IP (client
+                // change subnets) exists OR a FWD RR of the other protocols
+                // exists, either way we want the DHCID left intact so we
+                // treat this as success too.
                 setForwardChangeCompleted(true);
 
                 // If request calls for reverse update then do that next,

diff --git a/src/bin/d2/nc_remove.cc b/src/bin/d2/nc_remove.cc
index 874e43b041891cd3dace643dda085e51bf960876..174b69ddf4bdebc4217c20ed1877888b9276d5eb 100644 (file)
--- a/src/bin/d2/nc_remove.cc
+++ b/src/bin/d2/nc_remove.cc
@@ -326,10 +326,15 @@ NameRemoveTransaction::removingFwdRRsHandler() {
             // see RFC 2136 section 3.2.3/3.2.4.
             const dns::Rcode& rcode = getDnsUpdateResponse()->getRcode();
             if ((rcode == dns::Rcode::NOERROR()) ||
-                (rcode == dns::Rcode::NXRRSET())) {
+                (rcode == dns::Rcode::NXRRSET()) ||
+                (rcode == dns::Rcode::YXRRSET())) {
                 // We were able to remove them or they were not there (
                 // Rcode of NXRRSET means there are no matching RRsets).
                 // In either case, we consider it success and mark it as done.
+                // YXRRSET means either a FWD RR with a different IP (client
+                // change subnets) exists OR a FWD RR of the other protocols
+                // exists, either way we want the DHCID left intact so we
+                // treat this as success.
                 setForwardChangeCompleted(true);
 
                 // If request calls for reverse update then do that next,
@@ -669,7 +674,7 @@ NameRemoveTransaction::buildRemoveFwdRRsRequest() {
                                 dns::RRType::A(), dns::RRTTL(0)));
     request->addRRset(D2UpdateMessage::SECTION_PREREQUISITE, prereq);
 
-    // Create an assertion that there are no A RRs for the FQDN.
+    // Create an assertion that there are no AAAA RRs for the FQDN.
     // Add it to the pre-reqs.
     // Based on RFC 2136, section 2.4.3.
     prereq.reset(new dns::RRset(fqdn, dns::RRClass::NONE(),

diff --git a/src/bin/d2/tests/check_exists_remove_unittests.cc b/src/bin/d2/tests/check_exists_remove_unittests.cc
index fac36a8b2f832fc58590dca8e6e0acd264322cba..98238c5b4e48d9f849caa206e031eb3bb1c3283f 100644 (file)
--- a/src/bin/d2/tests/check_exists_remove_unittests.cc
+++ b/src/bin/d2/tests/check_exists_remove_unittests.cc
@@ -1150,6 +1150,43 @@ TEST_F(CheckExistsRemoveTransactionTest, removingFwdRRsHandler_InvalidResponse)
     }
 }
 
+// Tests removingFwdRRsHandler with the following scenario:
+//
+//  The request includes a forward and reverse change.
+//  Initial posted event is UPDATE_OK_EVT.
+//  The update request is sent without error.
+//  A server response is received which indicates FQDN is in use.
+//  This should be treated as success.
+//
+TEST_F(CheckExistsRemoveTransactionTest, removingFwdRRsHandler_FQDNInUse) {
+    CheckExistsRemoveStubPtr name_remove;
+    // Create and prep a transaction, poised to run the handler.
+    ASSERT_NO_THROW(name_remove =
+                    prepHandlerTest(CheckExistsRemoveTransaction::
+                                    REMOVING_FWD_ADDRS_ST,
+                                    NameChangeTransaction::
+                                    UPDATE_OK_EVT, FWD_AND_REV_CHG));
+
+    // Run removingFwdRRsHandler to construct and send the request.
+    EXPECT_NO_THROW(name_remove->removingFwdRRsHandler());
+
+    // Simulate receiving a FQDN in use update response.
+    name_remove->fakeResponse(DNSClient::SUCCESS, dns::Rcode::YXRRSET());
+
+    // Run removingFwdRRsHandler again  to process the response.
+    EXPECT_NO_THROW(name_remove->removingFwdRRsHandler());
+
+    // Forward change completion should be true, reverse flag should be false.
+    EXPECT_TRUE(name_remove->getForwardChangeCompleted());
+    EXPECT_FALSE(name_remove->getReverseChangeCompleted());
+
+    // Since the request also includes a reverse change we should
+    // be poised to start it. Verify that we transitioned correctly.
+    EXPECT_EQ(NameChangeTransaction::SELECTING_REV_SERVER_ST,
+              name_remove->getCurrState());
+    EXPECT_EQ(NameChangeTransaction::SELECT_SERVER_EVT,
+              name_remove->getNextEvent());
+}
 
 // Tests the selectingRevServerHandler functionality.
 // It verifies behavior for the following scenarios:

diff --git a/src/bin/d2/tests/nc_remove_unittests.cc b/src/bin/d2/tests/nc_remove_unittests.cc
index 4d5c246da390ae768518cee110f8b76db653da9b..1ba439c6c57d793f7fbff98e40390ff8573be8df 100644 (file)
--- a/src/bin/d2/tests/nc_remove_unittests.cc
+++ b/src/bin/d2/tests/nc_remove_unittests.cc
@@ -1152,6 +1152,43 @@ TEST_F(NameRemoveTransactionTest, removingFwdRRsHandler_InvalidResponse) {
     }
 }
 
+// Tests removingFwdRRsHandler with the following scenario:
+//
+//  The request includes a forward and reverse change.
+//  Initial posted event is UPDATE_OK_EVT.
+//  The update request is sent without error.
+//  A server response is received which indicates FQDN is in use.
+//  This should be treated as success.
+//
+TEST_F(NameRemoveTransactionTest, removingFwdRRsHandler_FQDNInUse) {
+    NameRemoveStubPtr name_remove;
+    // Create and prep a transaction, poised to run the handler.
+    ASSERT_NO_THROW(name_remove =
+                    prepHandlerTest(NameRemoveTransaction::
+                                    REMOVING_FWD_ADDRS_ST,
+                                    NameChangeTransaction::
+                                    UPDATE_OK_EVT, FWD_AND_REV_CHG));
+
+    // Run removingFwdRRsHandler to construct and send the request.
+    EXPECT_NO_THROW(name_remove->removingFwdRRsHandler());
+
+    // Simulate receiving an FQDN in use update response.
+    name_remove->fakeResponse(DNSClient::SUCCESS, dns::Rcode::YXRRSET());
+
+    // Run removingFwdRRsHandler again to process the response.
+    EXPECT_NO_THROW(name_remove->removingFwdRRsHandler());
+
+    // Forward change completion should be true, reverse flag should be false.
+    EXPECT_TRUE(name_remove->getForwardChangeCompleted());
+    EXPECT_FALSE(name_remove->getReverseChangeCompleted());
+
+    // Since the request also includes a reverse change we should
+    // be poised to start it. Verify that we transitioned correctly.
+    EXPECT_EQ(NameChangeTransaction::SELECTING_REV_SERVER_ST,
+              name_remove->getCurrState());
+    EXPECT_EQ(NameChangeTransaction::SELECT_SERVER_EVT,
+              name_remove->getNextEvent());
+}
 
 // Tests the selectingRevServerHandler functionality.
 // It verifies behavior for the following scenarios: