[#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 (file)
index 0000000..8795969
--- /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 d3061b61d0bf53e4d4da3167abc9252df07f72af..709f5158ff3b45f7297377f6ae9bd68c961a44da 100644 (file)
--- 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 f34bd61fbf7228dc219a25171bac3b221c83f0d3..adffc3107e985b7e763eb4593105f86f168260b8 100644 (file)
--- 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 e6b7fa32990a4d88677b1a4de785ce8d5203ec44..1cb910f6912d271c4e130a1552f52a3afc3441ef 100644 (file)
--- 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 335f2b762507d6e2d968da2601fb2215d54d7924..6f212b6678938f1dd184cfda3727031e28d0f20d 100644 (file)
--- 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<int>(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 504b6ec4216a824d7c77a2b8279fe482bbf8d362..ff1662943671e6edeb8b50eb25938eb68c823538 100644 (file)
--- 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 ce10fac29cbbe9a88838583a2e35b57ad8de9085..8f328d0a7505d96d306e6e0049f5385f444583a7 100644 (file)
--- 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<int64_t>(10));
+
+    std::string name = StatsMgr::generateName("subnet", subnet_->getID(),
+                                              type == Lease::TYPE_NA ?
+                                              "assigned-nas" : "assigned-pds");
+    StatsMgr::instance().setValue(name, static_cast<int64_t>(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<Option6IA> ia = boost::dynamic_pointer_cast<Option6IA>(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 2ae5849570a588934dc9abe55d8c73346a562547..cf3b20e7c68d278bb51f8079cba2f3e66968b82a 100644 (file)
--- 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.
     ///