From 6861ed9c2a69dcfbcd21af88ab8b6ce00b17e40f Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Sat, 1 Mar 2025 01:29:14 +0100 Subject: [PATCH] [#3683] Addressed more comments --- doc/sphinx/arm/dhcp6-srv.rst | 7 + src/bin/admin/tests/mysql_tests.sh.in | 4 +- src/bin/admin/tests/pgsql_tests.sh.in | 4 +- src/bin/dhcp6/dhcp6_srv.cc | 2 +- src/bin/dhcp6/tests/addr_reg_unittest.cc | 120 +++++++++++++++++- src/bin/dhcp6/tests/dhcp6_srv_unittest.cc | 41 ++++++ .../run_script/tests/run_script_unittests.cc | 1 + src/lib/dhcpsrv/alloc_engine.cc | 12 +- .../dhcpsrv/tests/alloc_engine6_unittest.cc | 119 +++++++++++++++++ .../scripts/mysql/dhcpdb_create.mysql | 2 +- .../scripts/mysql/upgrade_028_to_029.sh.in | 2 +- .../scripts/pgsql/dhcpdb_create.pgsql | 2 +- .../scripts/pgsql/upgrade_028_to_029.sh.in | 2 +- 13 files changed, 299 insertions(+), 19 deletions(-) diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index ae9f1c90c3..17b06270bc 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/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 diff --git a/src/bin/admin/tests/mysql_tests.sh.in b/src/bin/admin/tests/mysql_tests.sh.in index 21c2fe027b..7910291f03 100755 --- a/src/bin/admin/tests/mysql_tests.sh.in +++ b/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 diff --git a/src/bin/admin/tests/pgsql_tests.sh.in b/src/bin/admin/tests/pgsql_tests.sh.in index 7acc404b65..b9475e7e30 100755 --- a/src/bin/admin/tests/pgsql_tests.sh.in +++ b/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 diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index ff9a273ccb..84608ad231 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/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. diff --git a/src/bin/dhcp6/tests/addr_reg_unittest.cc b/src/bin/dhcp6/tests/addr_reg_unittest.cc index 193e3a0f17..e633b845d3 100644 --- a/src/bin/dhcp6/tests/addr_reg_unittest.cc +++ b/src/bin/dhcp6/tests/addr_reg_unittest.cc @@ -181,7 +181,11 @@ public: /// @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(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_)); - testAnother(); + 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_)); + + testAnotherSubnet(); } // Test that the FQDN option is handled. @@ -1357,7 +1424,7 @@ 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_)); @@ -1365,7 +1432,7 @@ TEST_F(AddrRegTest, calloutAnother) { // 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(3)); + StatsMgr::instance().setValue(registered_nas_name_, + static_cast(5)); + string cumulative_registered_nas_name2 = + StatsMgr::generateName("subnet", 2, "cumulative-registered-nas"); + StatsMgr::instance().setValue(cumulative_registered_nas_name2, + static_cast(8)); + StatsMgr::instance().setValue(cumulative_registered_nas_name_, + static_cast(10)); + StatsMgr::instance().setValue("cumulative-registered-nas", + static_cast(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 diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index b8ced23c46..12d4a8a0a6 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/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 ia = boost::dynamic_pointer_cast(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); diff --git a/src/hooks/dhcp/run_script/tests/run_script_unittests.cc b/src/hooks/dhcp/run_script/tests/run_script_unittests.cc index e2c3b068eb..66a700da62 100644 --- a/src/hooks/dhcp/run_script/tests/run_script_unittests.cc +++ b/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); diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index ed5d80ced9..b6c6301bcd 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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_); } diff --git a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc index b7c8779fbe..24bd92174d 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc @@ -737,6 +737,49 @@ TEST_F(AllocEngine6Test, solicitReuseExpiredRegisteredLease6) { EXPECT_FALSE(lease); } +// This test checks a registered lease can't be reused in SOLICT. +TEST_F(AllocEngine6Test, solicitReuseRegisteredLease6) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(100))); + ASSERT_TRUE(engine); + + IOAddress addr("2001:db8:1::ad"); + + // Create one subnet with a pool holding one address. + initSubnet(IOAddress("2001:db8:1::"), addr, addr); + + // Initialize FQDN data for the lease. + initFqdn("myhost.example.com", true, true); + + Lease6Ptr lease(new Lease6(Lease::TYPE_NA, addr, duid_, iaid_, + 501, 502, subnet_->getID(), HWAddrPtr())); + lease->state_ = Lease::STATE_REGISTERED; + lease->cltt_ = time(0) - 200; // Registered 200 seconds ago + lease->valid_lft_ = 400; // Lease was valid for 400 seconds + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); + + // CASE 1: Asking for any address + AllocEngine::ClientContext6 ctx1(subnet_, duid_, fqdn_fwd_, fqdn_rev_, hostname_, + true, Pkt6Ptr(new Pkt6(DHCPV6_SOLICIT, 1234))); + ctx1.currentIA().iaid_ = iaid_; + + EXPECT_NO_THROW(lease = expectOneLease(engine->allocateLeases6(ctx1))); + + // Check that we did not get this single lease. + EXPECT_FALSE(lease); + + // CASE 2: Asking specifically for this address + AllocEngine::ClientContext6 ctx2(subnet_, duid_, false, false, "", true, + Pkt6Ptr(new Pkt6(DHCPV6_REQUEST, 1234))); + ctx2.currentIA().iaid_ = iaid_; + ctx2.currentIA().addHint(addr); + + EXPECT_NO_THROW(lease = expectOneLease(engine->allocateLeases6(ctx2))); + + // Check that we did not get this single lease. + EXPECT_FALSE(lease); +} + // This test checks if an expired lease can be reused in REQUEST (actual allocation) TEST_F(AllocEngine6Test, requestReuseExpiredLease6) { boost::scoped_ptr engine; @@ -872,6 +915,51 @@ TEST_F(AllocEngine6Test, requestReuseExpiredRegisteredLease6) { EXPECT_FALSE(old_lease); } +// This test checks a registered lease can't be reused in REQUEST. +TEST_F(AllocEngine6Test, requestReuseRegisteredLease6) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(100))); + ASSERT_TRUE(engine); + + IOAddress addr("2001:db8:1::ad"); + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.clear(); // Get rid of the default test configuration + + // Create configuration similar to other tests, but with a single address pool + subnet_ = Subnet6::create(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4, SubnetID(10)); + pool_ = Pool6Ptr(new Pool6(Lease::TYPE_NA, addr, addr)); // just a single address + subnet_->addPool(pool_); + cfg_mgr.getStagingCfg()->getCfgSubnets6()->add(subnet_); + cfg_mgr.commit(); + + // Let's create a registered lease + const SubnetID other_subnetid = 999; + Lease6Ptr lease(new Lease6(Lease::TYPE_NA, addr, duid_, iaid_, + 501, 502, other_subnetid, HWAddrPtr())); + lease->state_ = Lease::STATE_REGISTERED; + lease->cltt_ = time(0) - 200; // Registered 200 seconds ago + lease->valid_lft_ = 400; // Lease was valid for 400 seconds + lease->fqdn_fwd_ = true; + lease->fqdn_rev_ = true; + lease->hostname_ = "myhost.example.com."; + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); + + // A client comes along, asking specifically for this address + AllocEngine::ClientContext6 ctx(subnet_, duid_, false, false, "", false, + Pkt6Ptr(new Pkt6(DHCPV6_REQUEST, 1234))); + ctx.currentIA().iaid_ = iaid_; + ctx.currentIA().addHint(addr); + + EXPECT_NO_THROW(lease = expectOneLease(engine->allocateLeases6(ctx))); + + // Check that we did not get this single lease. + EXPECT_FALSE(lease); + + // Check that no old lease has been returned. + Lease6Ptr old_lease = expectOneLease(ctx.currentIA().old_leases_); + EXPECT_FALSE(old_lease); +} + // This test checks if a released lease can be reused in REQUEST (actual allocation) TEST_F(AllocEngine6Test, requestReuseReleasedLease6) { boost::scoped_ptr engine; @@ -1042,6 +1130,37 @@ TEST_F(AllocEngine6Test, renewExtendLeaseLifetime) { << "Lease lifetime was not extended, but it should"; } +// Checks a registered lease can't be renewed. +TEST_F(AllocEngine6Test, renewRegisteredLease6) { + // Create a registered lease for the client. + IOAddress addr("2001:db8:1::15"); + Lease6Ptr lease(new Lease6(Lease::TYPE_NA, addr, + duid_, iaid_, 300, 400, + subnet_->getID(), HWAddrPtr())); + + lease->state_ = Lease::STATE_REGISTERED; + // Allocated 200 seconds ago - half of the lifetime. + time_t lease_cltt = time(0) - 200; + lease->cltt_ = lease_cltt; + + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); + + AllocEngine engine(100); + + // This is what the client will send in his renew message. + AllocEngine::HintContainer hints; + hints.push_back(AllocEngine::Resource(addr, 128)); + + // Get renewed leases. + Lease6Collection renewed = renewTest(engine, pool_, hints, IN_SUBNET, IN_POOL); + for (auto const& l : renewed) { + // Not registered. + EXPECT_EQ(0, l->state_); + // Another address. + EXPECT_NE(addr, l->addr_); + } +} + // Checks that a renewed lease uses default lifetimes. TEST_F(AllocEngine6Test, defaultRenewLeaseLifetime) { // Create a lease for the client. diff --git a/src/share/database/scripts/mysql/dhcpdb_create.mysql b/src/share/database/scripts/mysql/dhcpdb_create.mysql index bcab22d65a..0f001bc9b9 100644 --- a/src/share/database/scripts/mysql/dhcpdb_create.mysql +++ b/src/share/database/scripts/mysql/dhcpdb_create.mysql @@ -6169,7 +6169,7 @@ UPDATE schema_version INSERT INTO lease_state VALUES (4, 'registered'); -- Update *_lease6_stat stored procedures to count registered leases. --- Not *_lease6_pool_stat because a registered address in not from a pool +-- Not *_lease6_pool_stat because a registered address is not from a pool -- Not *_lease6_stat_by_client_class because it is for state 0 only DROP PROCEDURE IF EXISTS lease6_AINS_lease6_stat; diff --git a/src/share/database/scripts/mysql/upgrade_028_to_029.sh.in b/src/share/database/scripts/mysql/upgrade_028_to_029.sh.in index 35fefc06e2..c8f0b0af14 100755 --- a/src/share/database/scripts/mysql/upgrade_028_to_029.sh.in +++ b/src/share/database/scripts/mysql/upgrade_028_to_029.sh.in @@ -60,7 +60,7 @@ mysql "$@" <