[#3683] Addressed more comments

Author: fxdupont

doc/sphinx/arm/dhcp6-srv.rst

@@ -6869,6 +6869,13 @@ added or updated and a ADDR-REG-REPLY (37) is sent back to the client.
    Kea accepts and handles the Option Request option in the ADDR-REG-INFORM
    message (the RFC specifies the client MUST NOT put such option in it).
 
+.. note::
+
+   Kea currently only accepts addresses which are "appropriate to the link"
+   but not yet "within a prefix delegated to the client" which is not
+   compatible with the way the topology is described in the Kea
+   configuration.
+
 .. _dhcp6-stats:
 
 Statistics in the DHCPv6 Server

src/bin/admin/tests/mysql_tests.sh.in

@@ -2200,7 +2200,7 @@ mysql_lease6_stat_per_type() {
 # insert a lease, update it, and delete checking the
 # lease stat counts along the way.  It assumes the
 # database has been created but is empty.
-# param addr - address to use to add to subnet 1
+# param addr, addr1, addr2 - addresses to use to add to subnet 1
 mysql_lease6_stat_registered() {
     addr=$1;shift
     addr1=$1;shift
@@ -2288,7 +2288,7 @@ mysql_lease6_stat_test() {
     # Test for address ::22, PD lease type
     mysql_lease6_stat_per_type "::22" "::23" "::24" "1"
 
-    # Test for registered address ::33
+    # Test for registered addresses ::33, ::34 and ::35
     mysql_lease6_stat_registered "::33" "::34" "::35"
 
     # Let's wipe the whole database

src/bin/admin/tests/pgsql_tests.sh.in

@@ -1753,7 +1753,7 @@ pgsql_lease6_stat_per_type() {
 # insert a lease, update it, and delete checking the
 # lease stat counts along the way.  It assumes the
 # database has been created but is empty.
-# param addr - address to use to add to subnet 1
+# param addr, addr1, addr2 - addresses to use to add to subnet 1
 pgsql_lease6_stat_registered() {
     addr=$1;shift
     addr1=$1;shift
@@ -1841,7 +1841,7 @@ pgsql_lease6_stat_test() {
     # Test for address 222, PD lease type
     pgsql_lease6_stat_per_type "::22" "::23" "::24" "1"
 
-    # Test for registered address ::33
+    # Test for registered addresses ::33, ::34 and ::35
     pgsql_lease6_stat_registered "::33" "::34" "::35"
 
     # Let's wipe the whole database

src/bin/dhcp6/dhcp6_srv.cc

@@ -3384,7 +3384,7 @@ Dhcpv6Srv::releaseIA_NA(const DuidPtr& duid, const Pkt6Ptr& query,
     Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
                                                             release_addr->getAddress());
 
-    if (!lease) {
+    if (!lease || (lease->state_ == Lease::STATE_REGISTERED)) {
         // client releasing a lease that we don't know about.
 
         // Insert status code NoBinding.

src/bin/dhcp6/tests/addr_reg_unittest.cc

@@ -181,7 +181,11 @@ class AddrRegTest : public Dhcpv6MessageTest {
 
     /// @brief Test that the registered lease is updated even it
     /// belongs to another client.
-    void testAnother();
+    void testAnotherClient();
+
+    /// @brief Test that the registered lease is updated even it
+    /// belongs to another subnet.
+    void testAnotherSubnet();
 
     /// @brief Common part of skip/drop callout.
     void commonSkipOrDrop();
@@ -904,7 +908,7 @@ TEST_F(AddrRegTest, renew) {
 
 // Test that the registered lease is updated even it belongs to another client.
 void
-AddrRegTest::testAnother() {
+AddrRegTest::testAnotherClient() {
     // Create and add a lease for another client.
     IOAddress addr("2001:db8:1::1");
     DuidPtr duid(new DUID(vector<uint8_t>(8, 0x44)));
@@ -961,12 +965,75 @@ AddrRegTest::testAnother() {
 }
 
 // Test that the registered lease is updated even it belongs to another client.
-TEST_F(AddrRegTest, another) {
+TEST_F(AddrRegTest, anotherClient) {
+    IfaceMgrTestConfig test_config(true);
+
+    ASSERT_NO_THROW(configure(config_));
+
+    testAnotherClient();
+}
+
+// Test that the registratered lease is updated even it belongs to
+// another subnet.
+void
+AddrRegTest::testAnotherSubnet() {
+    // Create and add a lease.
+    IOAddress addr("2001:db8:1::1");
+    OptionPtr clientid = generateClientId();
+    Lease6Ptr lease(new Lease6(Lease::TYPE_NA, addr, duid_, 2345, 200, 300, 2));
+    lease->state_ = Lease::STATE_REGISTERED;
+    lease->cltt_ = 1234;
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
+
+    addr_reg_inf_ = Pkt6Ptr(new Pkt6(DHCPV6_ADDR_REG_INFORM, 1234));
+    addr_reg_inf_->setRemoteAddr(addr);
+    addr_reg_inf_->setIface("eth0");
+    addr_reg_inf_->setIndex(ETH0_INDEX);
+    addr_reg_inf_->addOption(clientid);
+    auto ia = generateIA(D6O_IA_NA, 234, 1500, 3000);
+    ia->addOption(generateIAAddr(addr, 3000, 4000));
+    addr_reg_inf_->addOption(ia);
+
+    // Pass it to the server.
+    AllocEngine::ClientContext6 ctx;
+    bool drop = !srv_.earlyGHRLookup(addr_reg_inf_, ctx);
+    ASSERT_FALSE(drop);
+    ctx.subnet_ = srv_.selectSubnet(addr_reg_inf_, drop);
+    ASSERT_FALSE(drop);
+    srv_.initContext(ctx, drop);
+    ASSERT_FALSE(drop);
+    ASSERT_TRUE(ctx.subnet_);
+
+    // Verify the response.
+    Pkt6Ptr response = srv_.processAddrRegInform(ctx);
+    ASSERT_TRUE(response);
+
+    // Verify the updated lease.
+    Lease6Ptr l = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, addr);
+    ASSERT_TRUE(l);
+    EXPECT_EQ(Lease::STATE_REGISTERED, l->state_);
+    EXPECT_EQ(addr, l->addr_);
+    ASSERT_TRUE(l->duid_);
+    EXPECT_TRUE(*l->duid_ == *duid_);
+    EXPECT_EQ(ia->getIAID(), l->iaid_);
+    EXPECT_EQ(1, l->subnet_id_);
+    EXPECT_FALSE(l->fqdn_fwd_);
+    EXPECT_FALSE(l->fqdn_rev_);
+
+    string expected = "DHCPSRV_MEMFILE_UPDATE_ADDR6 ";
+    expected += "updating IPv6 lease for address 2001:db8:1::1";
+    EXPECT_EQ(1, countFile(expected));
+    expected = "DHCP6_ADDR_REG_INFORM_CLIENT_CHANGE";
+    EXPECT_EQ(0, countFile(expected));
+}
+
+// Test that the registered lease is updated even it belongs to another subnet.
+TEST_F(AddrRegTest, anotherSubnet) {
     IfaceMgrTestConfig test_config(true);
 
     ASSERT_NO_THROW(configure(config_));
 
-    testAnother();
+    testAnotherSubnet();
 }
 
 // Test that the FQDN option is handled.
@@ -1357,15 +1424,15 @@ TEST_F(AddrRegTest, calloutRenew) {
 }
 
 // Test the callout in the another scenario.
-TEST_F(AddrRegTest, calloutAnother) {
+TEST_F(AddrRegTest, calloutAnotherClient) {
     IfaceMgrTestConfig test_config(true);
 
     ASSERT_NO_THROW(configure(config_));
 
     // Install addr6_register_callout.
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "addr6_register", addr6_register_callout));
-    testAnother();
+    testAnotherClient();
     EXPECT_TRUE(callout_errmsg_.empty()) << callout_errmsg_;
     checkCalloutHandleReset();
 
@@ -1535,4 +1602,45 @@ TEST_F(AddrRegTest, statsRenew) {
     EXPECT_EQ(20, stat->getInteger().first);
 }
 
+// Check the statictics for the another subnet scenario.
+TEST_F(AddrRegTest, statsAnotherSubnet) {
+    IfaceMgrTestConfig test_config(true);
+
+    ASSERT_NO_THROW(configure(config_));
+
+    string registered_nas_name2 =
+        StatsMgr::generateName("subnet", 2, "registered-nas");
+    StatsMgr::instance().setValue(registered_nas_name2,
+                                  static_cast<int64_t>(3));
+    StatsMgr::instance().setValue(registered_nas_name_,
+                                  static_cast<int64_t>(5));
+    string cumulative_registered_nas_name2 =
+        StatsMgr::generateName("subnet", 2, "cumulative-registered-nas");
+    StatsMgr::instance().setValue(cumulative_registered_nas_name2,
+                                  static_cast<int64_t>(8));
+    StatsMgr::instance().setValue(cumulative_registered_nas_name_,
+                                  static_cast<int64_t>(10));
+    StatsMgr::instance().setValue("cumulative-registered-nas",
+                                  static_cast<int64_t>(20));
+
+    testAnotherSubnet();
+
+    // Statistics should have been not touched.
+    ObservationPtr stat;
+    stat = StatsMgr::instance().getObservation(registered_nas_name2);
+    ASSERT_TRUE(stat);
+    EXPECT_EQ(2, stat->getInteger().first);
+    stat = StatsMgr::instance().getObservation(registered_nas_name_);
+    ASSERT_TRUE(stat);
+    EXPECT_EQ(6, stat->getInteger().first);
+    stat = StatsMgr::instance().getObservation(cumulative_registered_nas_name2);
+    ASSERT_TRUE(stat);
+    EXPECT_EQ(8, stat->getInteger().first);
+    stat = StatsMgr::instance().getObservation(cumulative_registered_nas_name_);
+    ASSERT_TRUE(stat);
+    EXPECT_EQ(11, stat->getInteger().first);
+    stat = StatsMgr::instance().getObservation("cumulative-registered-nas");
+    EXPECT_EQ(20, stat->getInteger().first);
+}
+
 } // end of anonymous namespace

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -2313,6 +2313,47 @@ TEST_F(Dhcpv6SrvTest, pdReleaseReject) {
     testReleaseReject(Lease::TYPE_PD, IOAddress("2001:db8:1:2::"));
 }
 
+// This test verifies that RELEASE doesn't work on registered leases.
+TEST_F(Dhcpv6SrvTest, ReleaseRegistered) {
+    NakedDhcpv6Srv srv(0);
+
+    const IOAddress addr("2001:db8:1:2::");
+    const uint32_t transid = 1234;
+    const uint32_t iaid = 234;
+
+    // GenerateClientId() also sets duid_
+    OptionPtr clientid = generateClientId();
+
+    // Create a registered lease.
+    Lease6Ptr lease(new Lease6(Lease::TYPE_NA, addr, duid_, iaid, 501, 502,
+                               subnet_->getID(), HWAddrPtr()));
+    lease->state_ = Lease::STATE_REGISTERED;
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
+
+    // Let's create a RELEASE
+    Pkt6Ptr rel = createMessage(DHCPV6_RELEASE, Lease::TYPE_NA, addr, 128, 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, transid);
+    OptionPtr tmp = reply->getOption(D6O_IA_NA);
+    ASSERT_TRUE(tmp);
+    // Check that IA_NA/IA_PD was returned and that there's status code in it
+    boost::shared_ptr<Option6IA> ia = boost::dynamic_pointer_cast<Option6IA>(tmp);
+    ASSERT_TRUE(ia);
+    checkIA_NAStatusCode(ia, STATUS_NoBinding, 0, 0);
+    checkMsgStatusCode(reply, STATUS_NoBinding);
+
+    // Check that the lease is not there
+    Lease6Ptr l = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, addr);
+    ASSERT_TRUE(l);
+    ASSERT_EQ(Lease::STATE_REGISTERED, l->state_);
+}
+
 // This test verifies if the sanityCheck() really checks options presence.
 TEST_F(Dhcpv6SrvTest, sanityCheck) {
     NakedDhcpv6Srv srv(0);

src/hooks/dhcp/run_script/tests/run_script_unittests.cc

@@ -1076,6 +1076,7 @@ TEST_F(RunScriptTest, lease6Decline) {
     checkScriptResult();
 }
 
+// Check the addr6_register callout.
 TEST_F(RunScriptTest, add6Register) {
     impl.reset(new RunScriptImpl());
     impl->setName(RUN_SCRIPT_TEST_SH);

src/lib/dhcpsrv/alloc_engine.cc

@@ -477,11 +477,12 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) {
                                                    ctx.currentIA().iaid_);
 
         // Iterate over the leases and eliminate those that are outside of
-        // our shared network.
+        // our shared network or registered.
         Lease6Collection leases;
         while (subnet) {
             for (auto const& l : all_leases) {
-                if ((l)->subnet_id_ == subnet->getID()) {
+                if (((l)->state_ != Lease::STATE_REGISTERED) &&
+                    ((l)->subnet_id_ == subnet->getID())) {
                     leases.push_back(l);
                 }
             }
@@ -2107,8 +2108,11 @@ AllocEngine::renewLeases6(ClientContext6& ctx) {
                                                        *ctx.duid_,
                                                        ctx.currentIA().iaid_,
                                                        subnet->getID());
-            leases.insert(leases.end(), leases_subnet.begin(), leases_subnet.end());
-
+            for (auto const& l : leases_subnet) {
+                if (l->state_ != Lease::STATE_REGISTERED) {
+                    leases.push_back(l);
+                }
+            }
             subnet = subnet->getNextSubnet(ctx.subnet_);
         }