From: Thomas Markwalder Date: Wed, 29 Jul 2026 17:17:09 +0000 (-0400) Subject: [#4437] Avoid orphaning PTR records X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=75ea2bbb1e4bf7a99d7e30bd73f16c4d32391826;p=thirdparty%2Fkea.git [#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 index 0000000000..39d349e1b3 --- /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 8ae529605d..be3294dc2e 100644 --- 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 874e43b041..174b69ddf4 100644 --- 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 fac36a8b2f..98238c5b4e 100644 --- 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 4d5c246da3..1ba439c6c5 100644 --- 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: