From: Francis Dupont Date: Sun, 5 Nov 2017 21:53:49 +0000 (+0100) Subject: [5425] Fixed renew after pool class change X-Git-Tag: trac5374_base~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=29c00734a010dc09b5d30aaa82b3b7926deace77;p=thirdparty%2Fkea.git [5425] Fixed renew after pool class change --- diff --git a/src/bin/dhcp6/tests/shared_network_unittest.cc b/src/bin/dhcp6/tests/shared_network_unittest.cc index 2e869443d9..bca8a1e08f 100644 --- a/src/bin/dhcp6/tests/shared_network_unittest.cc +++ b/src/bin/dhcp6/tests/shared_network_unittest.cc @@ -2356,9 +2356,7 @@ TEST_F(Dhcpv6SharedNetworkTest, poolInSharedNetworkSelectedByClass) { testAssigned([this, &client2] { ASSERT_NO_THROW(client2.doRenew()); }); -#if 0 - EXPECT_EQ(0, client2.getLeaseNum()); -#endif + EXPECT_EQ(0, client2.getLeasesWithNonZeroLifetime().size()); // If we add option 1234 with a value matching this class, the lease should // get renewed. @@ -2368,6 +2366,7 @@ TEST_F(Dhcpv6SharedNetworkTest, poolInSharedNetworkSelectedByClass) { ASSERT_NO_THROW(client2.doRenew()); }); EXPECT_EQ(1, client2.getLeaseNum()); + EXPECT_EQ(1, client2.getLeasesWithNonZeroLifetime().size()); } // Pool is selected based on the client class specified using a plain subnet. @@ -2420,9 +2419,7 @@ TEST_F(Dhcpv6SharedNetworkTest, poolInSubnetSelectedByClass) { testAssigned([this, &client2] { ASSERT_NO_THROW(client2.doRenew()); }); -#if 0 - EXPECT_EQ(0, client2.getLeaseNum()); -#endif + EXPECT_EQ(0, client2.getLeasesWithNonZeroLifetime().size()); // If we add option 1234 with a value matching this class, the lease should // get renewed. @@ -2432,6 +2429,7 @@ TEST_F(Dhcpv6SharedNetworkTest, poolInSubnetSelectedByClass) { ASSERT_NO_THROW(client2.doRenew()); }); EXPECT_EQ(1, client2.getLeaseNum()); + EXPECT_EQ(1, client2.getLeasesWithNonZeroLifetime().size()); } } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index b4f93d2733..d608de1b28 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -427,20 +427,29 @@ namespace { /// function when it belongs to a shared network. /// @param lease_type Type of the lease. /// @param address IPv6 address or prefix to be checked. +/// @param check_subnet if true only subnets are checked else both subnets +/// and pools are checked /// /// @return true if address belongs to a pool in a selected subnet or in /// a pool within any of the subnets belonging to the current shared network. bool inAllowedPool(AllocEngine::ClientContext6& ctx, const Lease::Type& lease_type, - const IOAddress& address) { + const IOAddress& address, bool check_subnet) { // If the subnet belongs to a shared network we will be iterating // over the subnets that belong to this shared network. Subnet6Ptr current_subnet = ctx.subnet_; while (current_subnet) { if (current_subnet->clientSupported(ctx.query_->getClasses())) { - if (current_subnet->inPool(lease_type, address)) { - return (true); + if (check_subnet) { + if (current_subnet->inPool(lease_type, address)) { + return (true); + } + } else { + if (current_subnet->inPool(lease_type, address, + ctx.query_->getClasses())) { + return (true); + } } } @@ -1114,11 +1123,14 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, void AllocEngine::removeNonmatchingReservedLeases6(ClientContext6& ctx, Lease6Collection& existing_leases) { - // If there are no leases (so nothing to remove) or - // host reservation is disabled (so there are no reserved leases), - // just return. - if (existing_leases.empty() || !ctx.subnet_ || - (ctx.subnet_->getHostReservationMode() == Network::HR_DISABLED) ) { + // If there are no leases (so nothing to remove) just return. + if (existing_leases.empty() || !ctx.subnet_) { + return; + } + // If host reservation is disabled (so there are no reserved leases) + // use the simplified version. + if (ctx.subnet_->getHostReservationMode() == Network::HR_DISABLED) { + removeNonmatchingReservedNoHostLeases6(ctx, existing_leases); return; } @@ -1155,7 +1167,8 @@ AllocEngine::removeNonmatchingReservedLeases6(ClientContext6& ctx, // be allocated to us from a dynamic pool, but we must check if // this lease belongs to any pool. If it does, we can proceed to // checking the next lease. - if (!host && inAllowedPool(ctx, candidate->type_, candidate->addr_)) { + if (!host && inAllowedPool(ctx, candidate->type_, + candidate->addr_, false)) { continue; } @@ -1202,6 +1215,44 @@ AllocEngine::removeNonmatchingReservedLeases6(ClientContext6& ctx, } } +void +AllocEngine::removeNonmatchingReservedNoHostLeases6(ClientContext6& ctx, + Lease6Collection& existing_leases) { + // We need a copy, so we won't be iterating over a container and + // removing from it at the same time. It's only a copy of pointers, + // so the operation shouldn't be that expensive. + Lease6Collection copy = existing_leases; + + BOOST_FOREACH(const Lease6Ptr& candidate, copy) { + // Lease can be allocated to us from a dynamic pool, but we must + // check if this lease belongs to any allowed pool. If it does, + // we can proceed to checking the next lease. + if (inAllowedPool(ctx, candidate->type_, + candidate->addr_, false)) { + continue; + } + + // Remove this lease from LeaseMgr as it doesn't belong to a pool. + LeaseMgrFactory::instance().deleteLease(candidate->addr_); + + // Update DNS if needed. + queueNCR(CHG_REMOVE, candidate); + + // Need to decrease statistic for assigned addresses. + StatsMgr::instance().addValue( + StatsMgr::generateName("subnet", candidate->subnet_id_, + ctx.currentIA().type_ == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"), + static_cast(-1)); + + // Add this to the list of removed leases. + ctx.currentIA().old_leases_.push_back(candidate); + + // Let's remove this candidate from existing leases + removeLeases(existing_leases, candidate->addr_); + } +} + bool AllocEngine::removeLeases(Lease6Collection& container, const asiolink::IOAddress& addr) { @@ -1755,7 +1806,8 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases // If the lease is in the current subnet we need to account // for the re-assignment of The lease. - if (inAllowedPool(ctx, ctx.currentIA().type_, lease->addr_)) { + if (inAllowedPool(ctx, ctx.currentIA().type_, + lease->addr_, true)) { StatsMgr::instance().addValue( StatsMgr::generateName("subnet", lease->subnet_id_, ctx.currentIA().type_ == Lease::TYPE_NA ? @@ -2558,6 +2610,7 @@ void findClientLease(AllocEngine::ClientContext4& ctx, Lease4Ptr& client_lease) /// within a shared network. /// /// @todo Update this function to take client classification into account. +/// @note client classification in pools (vs subnets) is checked /// /// @param ctx Client context. Current subnet may be modified by this /// function when it belongs to a shared network. @@ -2897,9 +2950,13 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // If the client is requesting an address which is assigned to the client // let's just renew this address. Also, renew this address if the client // doesn't request any specific address. + // Added extra checks: the address is reserved or belongs to the dynamic + // pool for the case the pool class has changed before the request. if (client_lease) { - if ((client_lease->addr_ == ctx.requested_address_) || - ctx.requested_address_.isV4Zero()) { + if (((client_lease->addr_ == ctx.requested_address_) || + ctx.requested_address_.isV4Zero()) && + (hasAddressReservation(ctx) || + inAllowedPool(ctx, client_lease->addr_))) { LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, ALLOC_ENGINE_V4_REQUEST_EXTEND_LEASE) diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index d30e96008c..af5c44da21 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -831,8 +831,8 @@ private: /// @brief Removes leases that are reserved for someone else. /// /// Goes through the list specified in existing_leases and removes those that - /// are reserved by someone else. The removed leases are added to the - /// ctx.removed_leases_ collection. + /// are reserved by someone else or do not belong to an allowed pool. + /// The removed leases are added to the ctx.removed_leases_ collection. /// /// @param ctx client context that contains all details (subnet, client-id, etc.) /// @param existing_leases [in/out] leases that should be checked @@ -840,6 +840,17 @@ private: removeNonmatchingReservedLeases6(ClientContext6& ctx, Lease6Collection& existing_leases); + /// @brief Removes leases that are reserved for someone else. + /// + /// Simplified version of removeNonmatchingReservedLeases6 to be + /// used when host reservations are disabled. + /// + /// @param ctx client context that contains all details (subnet, client-id, etc.) + /// @param existing_leases [in/out] leases that should be checked + void + removeNonmatchingReservedNoHostLeases6(ClientContext6& ctx, + Lease6Collection& existing_leases); + /// @brief Removed leases that are not reserved for this client /// /// This method iterates over existing_leases and will remove leases that are diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 7abfd5cc87..3e1a370580 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -999,10 +999,7 @@ TEST_F(SharedNetworkAlloc4Test, requestSharedNetworkPoolClassification) { ctx.subnet_ = subnet1_; lease = engine_.allocateLease4(ctx); ASSERT_TRUE(lease); -#if 0 - // It should work but currently it does not... EXPECT_TRUE(subnet2_->inPool(Lease::TYPE_V4, lease->addr_)); -#endif } // Test that reservations within shared network take precedence over the