From: Francis Dupont Date: Sun, 2 Aug 2026 09:03:52 +0000 (+0200) Subject: [#4670] Fixed v6 equivalent of #4547 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b5a29410f2e44bb6566209beef87dfb6623c2c5d;p=thirdparty%2Fkea.git [#4670] Fixed v6 equivalent of #4547 --- diff --git a/changelog_unreleased/4670-duplicated-dhcpv6_release-messes-up-stats b/changelog_unreleased/4670-duplicated-dhcpv6_release-messes-up-stats new file mode 100644 index 0000000000..87959696b0 --- /dev/null +++ b/changelog_unreleased/4670-duplicated-dhcpv6_release-messes-up-stats @@ -0,0 +1,5 @@ +[bug] fdupont + Fixed incorrect handling of double DHCPV6_RELEASE + when the lease affinity feature is enabled, e.g. + leading to incorrect assigned statistics. + (Gitlab #4670) diff --git a/src/bin/dhcp6/dhcp6_messages.cc b/src/bin/dhcp6/dhcp6_messages.cc index d3061b61d0..709f5158ff 100644 --- a/src/bin/dhcp6/dhcp6_messages.cc +++ b/src/bin/dhcp6/dhcp6_messages.cc @@ -158,12 +158,14 @@ extern const isc::log::MessageID DHCP6_RELEASE_NA = "DHCP6_RELEASE_NA"; extern const isc::log::MessageID DHCP6_RELEASE_NA_DELETED = "DHCP6_RELEASE_NA_DELETED"; extern const isc::log::MessageID DHCP6_RELEASE_NA_EXPIRED = "DHCP6_RELEASE_NA_EXPIRED"; extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL = "DHCP6_RELEASE_NA_FAIL"; +extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED = "DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED"; extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL_WRONG_DUID = "DHCP6_RELEASE_NA_FAIL_WRONG_DUID"; extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL_WRONG_IAID = "DHCP6_RELEASE_NA_FAIL_WRONG_IAID"; extern const isc::log::MessageID DHCP6_RELEASE_PD = "DHCP6_RELEASE_PD"; extern const isc::log::MessageID DHCP6_RELEASE_PD_DELETED = "DHCP6_RELEASE_PD_DELETED"; extern const isc::log::MessageID DHCP6_RELEASE_PD_EXPIRED = "DHCP6_RELEASE_PD_EXPIRED"; extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL = "DHCP6_RELEASE_PD_FAIL"; +extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED = "DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED"; extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL_WRONG_DUID = "DHCP6_RELEASE_PD_FAIL_WRONG_DUID"; extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL_WRONG_IAID = "DHCP6_RELEASE_PD_FAIL_WRONG_IAID"; extern const isc::log::MessageID DHCP6_REQUIRED_OPTIONS_CHECK_FAIL = "DHCP6_REQUIRED_OPTIONS_CHECK_FAIL"; @@ -344,12 +346,14 @@ const char* values[] = { "DHCP6_RELEASE_NA_DELETED", "%1: binding for address %2 and iaid=%3 was deleted on release", "DHCP6_RELEASE_NA_EXPIRED", "%1: binding for address %2 and iaid=%3 expired on release", "DHCP6_RELEASE_NA_FAIL", "%1: failed to remove address lease for address %2 and iaid=%3", + "DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED", "%1: client tried to release address %2 which is not assigned", "DHCP6_RELEASE_NA_FAIL_WRONG_DUID", "%1: client tried to release address %2, but it belongs to another client using duid=%3", "DHCP6_RELEASE_NA_FAIL_WRONG_IAID", "%1: client tried to release address %2, but it used wrong IAID (expected %3, but got %4)", "DHCP6_RELEASE_PD", "%1: prefix %2/%3 for iaid=%4 was released properly", "DHCP6_RELEASE_PD_DELETED", "%1: prefix %2/%3 for iaid=%4 was deleted on release", "DHCP6_RELEASE_PD_EXPIRED", "%1: prefix %2/%3 for iaid=%4 expired on release", "DHCP6_RELEASE_PD_FAIL", "%1: failed to release prefix %2/%3 for iaid=%4", + "DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED", "%1: client tried to release prefix %2/%3 which is not assigned", "DHCP6_RELEASE_PD_FAIL_WRONG_DUID", "%1: client tried to release prefix %2/%3, but it belongs to another client (duid=%4)", "DHCP6_RELEASE_PD_FAIL_WRONG_IAID", "%1: client tried to release prefix %2/%3, but it used wrong IAID (expected %4, but got %5)", "DHCP6_REQUIRED_OPTIONS_CHECK_FAIL", "%1: %2 message received from %3 failed the following check: %4", diff --git a/src/bin/dhcp6/dhcp6_messages.h b/src/bin/dhcp6/dhcp6_messages.h index f34bd61fbf..adffc3107e 100644 --- a/src/bin/dhcp6/dhcp6_messages.h +++ b/src/bin/dhcp6/dhcp6_messages.h @@ -159,12 +159,14 @@ extern const isc::log::MessageID DHCP6_RELEASE_NA; extern const isc::log::MessageID DHCP6_RELEASE_NA_DELETED; extern const isc::log::MessageID DHCP6_RELEASE_NA_EXPIRED; extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL; +extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED; extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL_WRONG_DUID; extern const isc::log::MessageID DHCP6_RELEASE_NA_FAIL_WRONG_IAID; extern const isc::log::MessageID DHCP6_RELEASE_PD; extern const isc::log::MessageID DHCP6_RELEASE_PD_DELETED; extern const isc::log::MessageID DHCP6_RELEASE_PD_EXPIRED; extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL; +extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED; extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL_WRONG_DUID; extern const isc::log::MessageID DHCP6_RELEASE_PD_FAIL_WRONG_IAID; extern const isc::log::MessageID DHCP6_REQUIRED_OPTIONS_CHECK_FAIL; diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index e6b7fa3299..1cb910f691 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -1031,6 +1031,14 @@ argument holds the client and transaction identification information. The second and third argument hold the released address and IAID respectively. +% DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED %1: client tried to release address %2 which is not assigned +Logged at debug log level 40. +This debug message is issued when a client is trying to release an address +which is not assigned (no lease or a lease not in default / assigned state +e.g. kept for the client by the lease affinity feature in the released state). +The first argument includes the client and the transaction identification +information. The second argument specifies the address. + % DHCP6_RELEASE_NA_FAIL_WRONG_DUID %1: client tried to release address %2, but it belongs to another client using duid=%3 This warning message indicates that a client tried to release an address that belongs to a different client. This should not happen in normal @@ -1073,6 +1081,14 @@ first argument hold the client and transaction identification information. The second and third argument define the prefix and its length. The fourth argument holds the IAID. +% DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED %1: client tried to release prefix %2/%3 which is not assigned +Logged at debug log level 40. +This debug message is issued when a client is trying to release a prefix +which is not assigned (no lease or a lease not in default / assigned state +e.g. kept for the client by the lease affinity feature in the released state). +The first argument includes the client and the transaction identification +information. The second and third arguments specify the prefix. + % DHCP6_RELEASE_PD_FAIL_WRONG_DUID %1: client tried to release prefix %2/%3, but it belongs to another client (duid=%4) This warning message indicates that client tried to release a prefix that belongs to a different client. This should not happen in normal diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 335f2b7625..6f212b6678 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -3438,12 +3438,17 @@ Dhcpv6Srv::releaseIA_NA(const DuidPtr& duid, const Pkt6Ptr& query, Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, release_addr->getAddress()); - if (!lease || (lease->state_ == Lease::STATE_REGISTERED)) { - // client releasing a lease that we don't know about. + if (!lease || (lease->state_ != Lease::STATE_DEFAULT)) { + // client releasing an address which is not assigned. + + LOG_DEBUG(packet6_logger, DBG_DHCP6_BASIC, + DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED) + .arg(query->getLabel()) + .arg(release_addr->getAddress().toText()); // Insert status code NoBinding. ia_rsp->addOption(createStatusCode(*query, *ia_rsp, STATUS_NoBinding, - "Sorry, no known leases for this duid/iaid, can't release.")); + "Sorry, this address isn't assigned, can't release.")); general_status = STATUS_NoBinding; return (ia_rsp); @@ -3653,12 +3658,18 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_PD, release_prefix->getAddress()); - if (!lease) { - // Client releasing a lease that we don't know about. + if (!lease || (lease->state_ != Lease::STATE_DEFAULT)) { + // Client releasing a prefix which is not assigned. + + LOG_DEBUG(packet6_logger, DBG_DHCP6_BASIC, + DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED) + .arg(query->getLabel()) + .arg(release_prefix->getAddress().toText()) + .arg(static_cast(release_prefix->getLength())); // Insert status code NoBinding. ia_rsp->addOption(createStatusCode(*query, *ia_rsp, STATUS_NoBinding, - "Sorry, no known leases for this duid/iaid, can't release.")); + "Sorry, this prefix isn't assigned, can't release.")); general_status = STATUS_NoBinding; return (ia_rsp); diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 504b6ec421..ff16629436 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2145,6 +2145,12 @@ TEST_F(Dhcpv6SrvTest, ReleaseAndReclaim) { testReleaseAndReclaim(Lease::TYPE_NA); } +// This test checks that to release a second time just logs when lease +// affinity is enabled. +TEST_F(Dhcpv6SrvTest, ReleaseAlreadyReleased) { + testReleaseAlreadyReleased(Lease::TYPE_NA); +} + // This test verifies that incoming (positive) RELEASE with address can be // handled properly, that a REPLY is generated, that the response has status // code and that the lease is expired and not removed from the database. @@ -2209,6 +2215,12 @@ TEST_F(Dhcpv6SrvTest, pdReleaseAndReclaim) { testReleaseAndReclaim(Lease::TYPE_PD); } +// This test checks that to release a second time just logs when lease +// affinity is enabled. +TEST_F(Dhcpv6SrvTest, pdReleaseAlreadyReleased) { + testReleaseAlreadyReleased(Lease::TYPE_PD); +} + // This test verifies that incoming (positive) RELEASE with prefix can be // handled properly, that a REPLY is generated, that the response has // status code and that the lease is expired and not removed from the database. diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index ce10fac29c..8f328d0a75 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -32,6 +32,7 @@ using namespace isc::asiolink; using namespace isc::config; using namespace isc::data; using namespace isc::dhcp; +using namespace isc::dhcp::test; using namespace isc::process; using namespace isc::stats; using namespace isc::util; @@ -906,6 +907,119 @@ Dhcpv6SrvTest::testReleaseAndReclaim(Lease::Type type) { ASSERT_EQ(count, subnet_after); } +void +Dhcpv6SrvTest::testReleaseAlreadyReleased(Lease::Type type) { + ASSERT_TRUE((type == Lease::TYPE_NA) || (type == Lease::TYPE_PD)); + + const uint32_t iaid = 234; + + uint32_t code; // option code of the container (IA_NA or IA_PD) + IOAddress addr = IOAddress::IPV6_ZERO_ADDRESS(); + uint8_t prefix_len; + if (type == Lease::TYPE_NA) { + code = D6O_IA_NA; + addr = IOAddress("2001:db8:1:1::cafe:babe"); + prefix_len = 128; + } else if (type == Lease::TYPE_PD) { + code = D6O_IA_PD; + addr = IOAddress("2001:db8:1:2::"); + prefix_len = pd_pool_->getLength(); + } + + // Generate client-id also duid_ + OptionPtr clientid = generateClientId(); + + // Check that the address we are about to use is indeed in pool + ASSERT_TRUE(subnet_->inPool(type, addr)); + + // Let's prepopulate the database with a released lease. + Lease6Ptr lease(new Lease6(type, addr, duid_, iaid, + 0, 0, subnet_->getID(), + HWAddrPtr(), prefix_len)); + lease->state_ = Lease6::STATE_RELEASED; + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); + + // Check that the lease is really in the database + Lease6Ptr l = LeaseMgrFactory::instance().getLease6(type, addr); + ASSERT_TRUE(l); + + // And prepopulate the stats counter + StatsMgr::instance().setValue(type == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds", + static_cast(10)); + + std::string name = StatsMgr::generateName("subnet", subnet_->getID(), + type == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"); + StatsMgr::instance().setValue(name, static_cast(10)); + + ObservationPtr stat = StatsMgr::instance().getObservation(name); + ASSERT_TRUE(stat); + + // Let's create a RELEASE + Pkt6Ptr rel = createMessage(DHCPV6_RELEASE, type, addr, prefix_len, iaid); + rel->addOption(clientid); + rel->addOption(srv_->getServerID()); + + // Pass it to the server and hope for a REPLY + Pkt6Ptr reply = srv_->processRelease(rel); + + // Check if we get response at all + checkResponse(reply, DHCPV6_REPLY, 1234); + + OptionPtr tmp = reply->getOption(code); + ASSERT_TRUE(tmp); + + // Check that IA was returned, + boost::shared_ptr ia = boost::dynamic_pointer_cast(tmp); + checkIA_NAStatusCode(ia, STATUS_NoBinding, 0, 0); + checkMsgStatusCode(reply, STATUS_NoBinding); + + // There should be no address returned in RELEASE (see RFC 9915, 18.3.7) + // There should be no prefix + EXPECT_FALSE(tmp->getOption(D6O_IAADDR)); + EXPECT_FALSE(tmp->getOption(D6O_IAPREFIX)); + + // Check DUIDs + checkServerId(reply, srv_->getServerID()); + checkClientId(reply, clientid); + + // Check the lease. + l = LeaseMgrFactory::instance().getLease6(type, addr); + ASSERT_TRUE(l); + + EXPECT_EQ(l->valid_lft_, 0U); + EXPECT_EQ(l->preferred_lft_, 0U); + + EXPECT_EQ(Lease6::STATE_RELEASED, l->state_); + + // get lease by subnetid/duid/iaid combination + l = LeaseMgrFactory::instance().getLease6(type, *duid_, iaid, + subnet_->getID()); + ASSERT_TRUE(l); + + EXPECT_EQ(l->valid_lft_, 0U); + EXPECT_EQ(l->preferred_lft_, 0U); + EXPECT_EQ(Lease::STATE_RELEASED, l->state_); + + // We should not have decremented the address counter + stat = StatsMgr::instance().getObservation(type == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"); + ASSERT_TRUE(stat); + EXPECT_EQ(10, stat->getInteger().first); + + stat = StatsMgr::instance().getObservation(name); + ASSERT_TRUE(stat); + EXPECT_EQ(10, stat->getInteger().first); + + // Expect log. + if (type == Lease::TYPE_NA) { + EXPECT_EQ(1U, countFile("DHCP6_RELEASE_NA_FAIL_NOT_ASSIGNED")); + } else { + EXPECT_EQ(1U, countFile("DHCP6_RELEASE_PD_FAIL_NOT_ASSIGNED")); + } +} + void Dhcpv6SrvTest::testReleaseNoDelete(Lease::Type type, const IOAddress& addr, uint8_t qtype) { diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h index 2ae5849570..cf3b20e7c6 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.h +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h @@ -964,6 +964,15 @@ public: void testReleaseAndReclaim(isc::dhcp::Lease::Type type); + /// @brief Performs double RELEASE test for an address within a subnet + /// and does not cause counters to decrease below 0. + /// + /// This method does not throw, but uses gtest macros to signify failures. + /// + /// @param type type (TYPE_NA or TYPE_PD) + void + testReleaseAlreadyReleased(isc::dhcp::Lease::Type type); + /// @brief Checks that reassignment of a released-expired lease /// does not lead to zero lifetimes. ///