[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.

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<Lease6> 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<Lease6> leases = getLeasesByAddress(prefix);
     BOOST_FOREACH(const Lease6& lease, leases) {
         if ((lease.prefixlen_ == prefix_len) &&

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.
     ///

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<typename ValueType>
+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<uint32_t> {
+    /// @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<bool> {
+    /// @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

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<typename ValueType>
-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<uint32_t> {
-    /// @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<bool> {
-    /// @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<Lease6> 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

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.