From: Marcin Siodelski Date: Fri, 17 Jun 2016 13:31:15 +0000 (+0200) Subject: [4321] Do not replace allocated reserved address in the IA. X-Git-Tag: trac4551_base~29^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9df20123466ded1646eec3df06c8360683fae1fe;p=thirdparty%2Fkea.git [4321] Do not replace allocated reserved address in the IA. If IA contains allocated lease for which there is a reservation the liftime of this lease is extended. Any newly reserved lease will be assigned to a different IA, possibly replacing dynamically allocated lease. --- diff --git a/src/bin/dhcp6/tests/dhcp6_client.cc b/src/bin/dhcp6/tests/dhcp6_client.cc index 9fa894a19e..fb8be12f49 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.cc +++ b/src/bin/dhcp6/tests/dhcp6_client.cc @@ -700,7 +700,7 @@ Dhcp6Client::hasLeaseForAddress(const asiolink::IOAddress& address) const { bool Dhcp6Client::hasLeaseForAddress(const asiolink::IOAddress& address, - const uint32_t iaid) const { + const IAID& iaid) const { std::vector leases = getLeasesByAddress(address); BOOST_FOREACH(const Lease6& lease, leases) { if (lease.iaid_ == iaid) { @@ -745,7 +745,7 @@ Dhcp6Client::hasLeaseForPrefix(const asiolink::IOAddress& prefix, bool Dhcp6Client::hasLeaseForPrefix(const asiolink::IOAddress& prefix, const uint8_t prefix_len, - const uint32_t iaid) const { + const IAID& iaid) const { std::vector leases = getLeasesByAddress(prefix); BOOST_FOREACH(const Lease6& lease, leases) { if ((lease.prefixlen_ == prefix_len) && diff --git a/src/bin/dhcp6/tests/dhcp6_client.h b/src/bin/dhcp6/tests/dhcp6_client.h index 614be2294f..7cb615480d 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.h +++ b/src/bin/dhcp6/tests/dhcp6_client.h @@ -402,7 +402,7 @@ public: /// @param address Address for which lease should be found. /// @param iaid IAID of the IA_NA in which the lease is expected. bool hasLeaseForAddress(const asiolink::IOAddress& address, - const uint32_t iaid) const; + const IAID& iaid) const; /// @brief Checks if client has a lease for an address within range. /// @@ -440,7 +440,7 @@ public: /// @param iaid IAID of the IA_PD in which the lease is expected. bool hasLeaseForPrefix(const asiolink::IOAddress& prefix, const uint8_t prefix_len, - const uint32_t iaid) const; + const IAID& iaid) const; /// @brief Checks if client has a lease belonging to a prefix pool. /// diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h index c9f619fc4d..f36ba41b42 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.h +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h @@ -34,6 +34,76 @@ namespace isc { namespace dhcp { namespace test { +/// @brief Generic wrapper to provide strongly typed values. +/// +/// In many cases, the test fixture class methods require providing many +/// paramaters, of which some ore optional. Some of the parameters may also +/// be implicitly converted to other types. Non-careful test implementer +/// may often "shift by one" or swap two values on the arguments list, which +/// will be accepted by the compiler but will result in troubles running the +/// function. Sometimes it takes non trivial amount of debugging to find out +/// why the particular function fails until we find that the arguments were +/// swapped or shifted. In addition, the use of classes wrapping simple types +/// results in better readbility of the test code. +/// +/// @tparam ValueType Type of the wrapped value. +template +struct SpecializedTypeWrapper { + + /// @brief Constructor + /// + /// @param value Wrapped value + explicit SpecializedTypeWrapper(const ValueType& value) + : value_(value) { } + + /// @brief Operator returning a wrapped value. + operator ValueType () const { + return (value_); + } + + /// @brief Wrapped value. + ValueType value_; +}; + + +/// @brief Class representing strongly typed IAID. +struct IAID : public SpecializedTypeWrapper { + /// @brief Constructor + /// + /// @param iaid IAID. + explicit IAID(const uint32_t iaid) + : SpecializedTypeWrapper(iaid) { } +}; + +/// @brief Class representing strongly typed value for strict IAID checks. +/// +/// Strict IAID checks are used to verify that the particular address has been +/// assign to a specific IA. In many cases we don't check that because it may +/// not be possible to predict to which IA the specific lease will be assigned. +struct StrictIAIDChecking : public SpecializedTypeWrapper { + /// @brief Constructor. + /// + /// @param strict_check Boolean value indicating if strict checking should + /// be performed. + explicit StrictIAIDChecking(const bool strict_check) + : SpecializedTypeWrapper(strict_check) { } + + /// @brief Convenience function returning an object indicating that strict + /// checks should be performed. + static const StrictIAIDChecking YES() { + static StrictIAIDChecking strict_check(true); + return (strict_check); + } + + /// @brief Convenience function returning an object indicating that strict + /// checks should not be performed. + static StrictIAIDChecking NO() { + static StrictIAIDChecking strict_check(false); + return (strict_check); + } +}; + + /// @brief Base class for DHCPv6 server testing. /// /// Currently it configures the test data path directory in diff --git a/src/bin/dhcp6/tests/host_unittest.cc b/src/bin/dhcp6/tests/host_unittest.cc index 44e06adfbe..06b29e8d0e 100644 --- a/src/bin/dhcp6/tests/host_unittest.cc +++ b/src/bin/dhcp6/tests/host_unittest.cc @@ -116,76 +116,6 @@ const char* CONFIGS[] = { "}" }; -/// @brief Generic wrapper to provide strongly typed values. -/// -/// In many cases, the test fixture class methods require providing many -/// paramaters, of which some ore optional. Some of the parameters may also -/// be implicitly converted to other types. Non-careful test implementer -/// may often "shift by one" or swap two values on the arguments list, which -/// will be accepted by the compiler but will result in troubles running the -/// function. Sometimes it takes non trivial amount of debugging to find out -/// why the particular function fails until we find that the arguments were -/// swapped or shifted. In addition, the use of classes wrapping simple types -/// results in better readbility of the test code. -/// -/// @tparam ValueType Type of the wrapped value. -template -struct SpecializedTypeWrapper { - - /// @brief Constructor - /// - /// @param value Wrapped value - explicit SpecializedTypeWrapper(const ValueType& value) - : value_(value) { } - - /// @brief Operator returning a wrapped value. - operator ValueType () const { - return (value_); - } - - /// @brief Wrapped value. - ValueType value_; -}; - - -/// @brief Class representing strongly typed IAID. -struct IAID : public SpecializedTypeWrapper { - /// @brief Constructor - /// - /// @param iaid IAID. - explicit IAID(const uint32_t iaid) - : SpecializedTypeWrapper(iaid) { } -}; - -/// @brief Class representing stronly typed value for strict IAID checks. -/// -/// Strict IAID checks are used to verify that the particular address has been -/// assign to a specific IA. In many cases we don't check that because it may -/// not be possible to predict to which IA the specific lease will be assigned. -struct StrictIAIDChecking : public SpecializedTypeWrapper { - /// @brief Constructor. - /// - /// @param strict_check Boolean value indicating if strict checking should - /// be performed. - explicit StrictIAIDChecking(const bool strict_check) - : SpecializedTypeWrapper(strict_check) { } - - /// @brief Convenience function returning an object indicating that strict - /// checks should be performed. - static const StrictIAIDChecking YES() { - static StrictIAIDChecking strict_check(true); - return (strict_check); - } - - /// @brief Convenience function returning an object indicating that strict - /// checks should not be performed. - static StrictIAIDChecking NO() { - static StrictIAIDChecking strict_check(false); - return (strict_check); - } - -}; - /// @brief Base class representing leases and hints coveyed within IAs. /// /// This is a base class for @ref Reservation and @ref Hint classes. @@ -310,11 +240,11 @@ public: /// @param iaid IAID of IA in which hint should be placed. /// @param resource Resource string as for @ref IAResource constructor. Hint(const IAID& iaid, const std::string& resource) - : IAResource(resource), iaid_(iaid.value_) { + : IAResource(resource), iaid_(iaid) { } - /// @brief Returns IAID as 32-bit unsigned value. - uint32_t getIAID() const; + /// @brief Returns IAID. + const IAID& getIAID() const; /// @brief Convenience function returning unspecified hint. static const Hint& UNSPEC(); @@ -322,10 +252,10 @@ public: private: /// @brief Holds IAID as 32-bit unsigned integer. - uint32_t iaid_; + IAID iaid_; }; -uint32_t +const IAID& Hint::getIAID() const { return (iaid_); } @@ -530,11 +460,14 @@ HostTest::testLeaseForIA(const Reservation& r, size_t& address_count, size_t& prefix_count) { if (r.isPrefix()) { ++prefix_count; - EXPECT_TRUE(client_.hasLeaseForPrefix(r.getPrefix(), r.getPrefixLen())); + EXPECT_TRUE(client_.hasLeaseForPrefix(r.getPrefix(), + r.getPrefixLen(), + IAID(3 + prefix_count))); } else if (!r.isEmpty()) { ++address_count; - EXPECT_TRUE(client_.hasLeaseForAddress(r.getPrefix())); + EXPECT_TRUE(client_.hasLeaseForAddress(r.getPrefix(), + IAID(address_count))); } } @@ -1104,7 +1037,7 @@ TEST_F(HostTest, multipleIAsRenew) { // allocate newly reserved address and prefix, replacing the previously // allocated dynamic leases. For both dynamically allocated leases, the // server should return IAs with zero lifetimes. -TEST_F(HostTest, additionalReservationDuringRenew) { +TEST_F(HostTest, appendReservationDuringRenew) { // 4-way exchange to acquire 4 reserved leases and 2 dynamic leases. testMultipleIAs(do_solicit_request_, Reservation("2001:db8:1:1::1"), @@ -1196,5 +1129,84 @@ TEST_F(HostTest, additionalReservationDuringRenew) { EXPECT_EQ(3, leases.size()); } +// In this test, the client performs 4-way exchange and includes 3 IA_NAs +// and 3 IA_PDs. Initially, the server has 2 address reservations and +// 2 prefix reservations for this client. The server allocates the 2 +// reserved addresses to the first 2 IA_NAs and 2 reserved prefixes to the +// first two IA_PDs. The server is reconfigured and 2 new reservations are +// inserted: new address reservation before existing address reservations +// and prefix reservation before existing prefix reservations. +// The server should detect that leases already exist for reserved addresses +// and prefixes and it should not remove existing leases. Instead, it should +// replace dynamically allocated leases with newly added reservations +TEST_F(HostTest, insertReservationDuringRenew) { + // 4-way exchange to acquire 4 reserved leases and 2 dynamic leases. + testMultipleIAs(do_solicit_request_, + Reservation("2001:db8:1:1::1"), + Reservation("2001:db8:1:1::2"), + Reservation("3000:1:1::/64"), + Reservation("3000:1:2::/64")); + + // The server must have not lease for the address and prefix for which + // we will later make reservations, because these are outside of the + // dynamic pool. + ASSERT_FALSE(client_.hasLeaseForAddress(IOAddress("2001:db8:1:1::3"))); + ASSERT_FALSE(client_.hasLeaseForPrefix(IOAddress("3000:1:3::"), 64)); + + // Retrieve leases from the dynamic pools and store them so as we can + // later check that they were returned with zero lifetimes when the + // reservations are added. + std::vector leases = + client_.getLeasesByAddressRange(IOAddress("2001:db8:1::1"), + IOAddress("2001:db8:1::10")); + ASSERT_EQ(1, leases.size()); + IOAddress dynamic_address_lease = leases[0].addr_; + + leases = client_.getLeasesByPrefixPool(IOAddress("3001::"), 32, 64); + ASSERT_EQ(1, leases.size()); + IOAddress dynamic_prefix_lease = leases[0].addr_; + + // Add two additional reservations. + std::string c = configString(*client_.getDuid(), + Reservation("2001:db8:1:1::3"), + Reservation("2001:db8:1:1::1"), + Reservation("2001:db8:1:1::2"), + Reservation("3000:1:3::/64"), + Reservation("3000:1:1::/64"), + Reservation("3000:1:2::/64")); + + ASSERT_NO_THROW(configure(c, *client_.getServer())); + + // Client renews and includes all leases it currently has in the IAs. + ASSERT_NO_THROW(client_.doRenew()); + + // The expectation is that the server allocated two new reserved leases to + // the client and removed leases allocated from the dynamic pools. The + // number if leases in the server configuration should include those that + // are returned with zero lifetimes. Hence, the total number of leases + // should be equal to 6 + 2 = 8. + ASSERT_EQ(8, client_.getLeaseNum()); + + // Even though the new reservations have been added before existing + // reservations, the server should assign them to the IAs with + // IAID = 3 (for address) and IAID = 6 (for prefix). + EXPECT_TRUE(client_.hasLeaseForAddress(IOAddress("2001:db8:1:1::1"), + IAID(1))); + EXPECT_TRUE(client_.hasLeaseForAddress(IOAddress("2001:db8:1:1::2"), + IAID(2))); + EXPECT_TRUE(client_.hasLeaseForAddress(IOAddress("2001:db8:1:1::3"), + IAID(3))); + EXPECT_TRUE(client_.hasLeaseForPrefix(IOAddress("3000:1:1::"), 64, + IAID(4))); + EXPECT_TRUE(client_.hasLeaseForPrefix(IOAddress("3000:1:2::"), 64, + IAID(5))); + EXPECT_TRUE(client_.hasLeaseForPrefix(IOAddress("3000:1:3::"), 64, + IAID(6))); + + // Make sure that the replaced leases have been returned with zero liftimes. + EXPECT_TRUE(client_.hasLeaseWithZeroLifetimeForAddress(dynamic_address_lease)); + EXPECT_TRUE(client_.hasLeaseWithZeroLifetimeForPrefix(dynamic_prefix_lease, 64)); +} + } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index c868e05e99..2d0cf66062 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -759,40 +759,46 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, IPv6Resrv::Type type = ctx.currentIA().type_ == Lease::TYPE_NA ? IPv6Resrv::TYPE_NA : IPv6Resrv::TYPE_PD; - // Get the IPv6 reservations of specified type. - const IPv6ResrvRange& reservs = ctx.host_->getIPv6Reservations(type); - for (IPv6ResrvIterator resv = reservs.first; resv != reservs.second; ++resv) { - // We do have a reservation for address or prefix. - IOAddress addr = resv->second.getPrefix(); - uint8_t prefix_len = resv->second.getPrefixLen(); - - // We have allocated this address/prefix while processing one of the - // previous IAs, so let's try another reservation. - if (ctx.isAllocated(addr, prefix_len)) { - continue; - } - - // Check if already have this lease on the existing_leases list. - for (Lease6Collection::iterator l = existing_leases.begin(); - l != existing_leases.end(); ++l) { - - // Ok, we already have a lease for this reservation and it's usable - if (((*l)->addr_ == addr) && (*l)->valid_lft_ != 0) { + // We want to avoid allocating new lease for an IA if there is already + // a valid lease for which client has reservation. So, we first check if + // we already have a lease for a reserved address or prefix. + BOOST_FOREACH(const Lease6Ptr& lease, existing_leases) { + if ((lease->valid_lft_ != 0)) { + if (ctx.host_->hasReservation(IPv6Resrv(type, lease->addr_, + lease->prefixlen_))) { + // We found existing lease for a reserved address or prefix. + // We'll simply extend the lifetime of the lease. LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, ALLOC_ENGINE_V6_ALLOC_HR_LEASE_EXISTS) .arg(ctx.query_->getLabel()) - .arg((*l)->typeToText((*l)->type_)) - .arg((*l)->addr_.toText()); + .arg(lease->typeToText(lease->type_)) + .arg(lease->addr_.toText()); // If this is a real allocation, we may need to extend the lease // lifetime. - if (!ctx.fake_allocation_ && conditionalExtendLifetime(**l)) { - LeaseMgrFactory::instance().updateLease6(*l); + if (!ctx.fake_allocation_ && conditionalExtendLifetime(*lease)) { + LeaseMgrFactory::instance().updateLease6(lease); } - return; } } + } + + // There is no lease for a reservation in this IA. So, let's now iterate + // over reservations specified and try to allocate one of them for the IA. + + // Get the IPv6 reservations of specified type. + const IPv6ResrvRange& reservs = ctx.host_->getIPv6Reservations(type); + BOOST_FOREACH(IPv6ResrvTuple type_lease_tuple, reservs) { + // We do have a reservation for address or prefix. + const IOAddress& addr = type_lease_tuple.second.getPrefix(); + uint8_t prefix_len = type_lease_tuple.second.getPrefixLen(); + + // We have allocated this address/prefix while processing one of the + // previous IAs, so let's try another reservation. + if (ctx.isAllocated(addr, prefix_len)) { + continue; + } // If there's a lease for this address, let's not create it. // It doesn't matter whether it is for this client or for someone else.