[3677] alloc_new_leases_in_renewals_ added in AllocEngine

src/bin/lfc/tests/lfc_controller_unittests.cc

@@ -112,7 +112,7 @@ std::string
 LFCControllerTest::readFile(const std::string& filename) const {
     std::ifstream fs;
 
-    fs.open(filename, std::ifstream::in);
+    fs.open(filename.c_str(), std::ifstream::in);
     std::string contents((std::istreambuf_iterator<char>(fs)),
                          std::istreambuf_iterator<char>());
     fs.close();
@@ -122,7 +122,7 @@ LFCControllerTest::readFile(const std::string& filename) const {
 void
 LFCControllerTest::writeFile(const std::string& filename,
                              const std::string& contents) const {
-    std::ofstream fs(filename, std::ofstream::out);
+    std::ofstream fs(filename.c_str(), std::ofstream::out);
 
     if (fs.is_open()) {
         fs << contents;

src/lib/dhcpsrv/alloc_engine.cc

@@ -687,6 +687,17 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, Lease6Collection& exis
                     .arg(addr.toText()).arg(static_cast<int>(prefix_len))
                     .arg(ctx.duid_->toText());
             }
+
+            // We found a lease for this client and this IA. Let's return.
+            // Returning after the first lease was assigned is useful if we
+            // have multiple reservations for the same client. If the client
+            // sends 2 IAs, the first time we call allocateReservedLeases6 will
+            // use the first reservation and return. The second time, we'll
+            // go over the first reservation, but will discover that there's
+            // a lease corresponding to it and will skip it and then pick
+            // the second reservation and turn it into the lease. This approach
+            // would work for any number of reservations.
+            return;
         }
     }
 }
@@ -721,11 +732,25 @@ AllocEngine::removeNonmatchingReservedLeases6(ClientContext6& ctx,
 
         // Ok, we have a problem. This host has a lease that is reserved
         // for someone else. We need to recover from this.
+        if (ctx.type_ == Lease::TYPE_NA) {
+            LOG_INFO(dhcpsrv_logger, DHCPSRV_HR_REVOKED_ADDR6_LEASE)
+                .arg((*candidate)->addr_.toText()).arg(ctx.duid_->toText())
+                .arg(host->getIdentifierAsText());
+        } else {
+            LOG_INFO(dhcpsrv_logger, DHCPSRV_HR_REVOKED_PREFIX6_LEASE)
+                .arg((*candidate)->addr_.toText())
+                .arg(static_cast<int>((*candidate)->prefixlen_))
+                .arg(ctx.duid_->toText())
+                .arg(host->getIdentifierAsText());
+        }
 
         // Remove this lease from LeaseMgr
         LeaseMgrFactory::instance().deleteLease((*candidate)->addr_);
 
-        /// @todo: Probably trigger a hook here
+        // In principle, we could trigger a hook here, but we will do this
+        // only if we get serious complaints from actual users. We want the
+        // conflict resolution procedure to really work and user libraries
+        // should not interfere with it.
 
         // Add this to the list of removed leases.
         ctx.old_leases_.push_back(*candidate);
@@ -1673,7 +1698,10 @@ AllocEngine::renewLeases6(ClientContext6& ctx) {
         }
 
         // If we happen to removed all leases, get something new for this guy.
-        if (leases.empty()) {
+        // Depending on the configuration, we may enable or disable granting
+        // new leases during renewals. This is controlled with the
+        // allow_new_leases_in_renewals_ field.
+        if (leases.empty() && ctx.allow_new_leases_in_renewals_) {
             leases = allocateUnreservedLeases6(ctx);
         }
 
@@ -1687,7 +1715,7 @@ AllocEngine::renewLeases6(ClientContext6& ctx) {
     } catch (const isc::Exception& e) {
 
         // Some other error, return an empty lease.
-        LOG_ERROR(dhcpsrv_logger, DHCPSRV_ADDRESS6_ALLOC_ERROR).arg(e.what());
+        LOG_ERROR(dhcpsrv_logger, DHCPSRV_RENEW6_ERROR).arg(e.what());
     }
 
     return (Lease6Collection());

src/lib/dhcpsrv/alloc_engine.h

@@ -418,12 +418,24 @@ protected:
         /// @brief A pointer to the IA_NA/IA_PD option to be sent in response
         Option6IAPtr ia_rsp_;
 
+
+        /// @brief Specifies whether new leases in Renew/Rebind are allowed
+        ///
+        /// This field controls what to do when renewing or rebinding client
+        /// does not have any leases. RFC3315 and stateful-issues draft does not
+        /// specify it and it is left up to the server configuration policy.
+        /// False (the default) means that the client will not get any new
+        /// unreserved leases if his existing leases are no longer suitable.
+        /// True means that the allocation engine will do its best to assign
+        /// something.
+        bool allow_new_leases_in_renewals_;
+
         /// @brief Default constructor.
         ClientContext6()
            : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(),
              hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""),
              callout_handle_(), fake_allocation_(false), old_leases_(), host_(),
-             query_(), ia_rsp_() {
+             query_(), ia_rsp_(), allow_new_leases_in_renewals_(false) {
         }
 
         /// @brief Constructor with parameters.
@@ -454,7 +466,8 @@ protected:
             subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(),
             hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns),
             hostname_(hostname), fake_allocation_(fake_allocation),
-            old_leases_(), host_(), query_(), ia_rsp_() {
+            old_leases_(), host_(), query_(), ia_rsp_(),
+            allow_new_leases_in_renewals_(false){
 
             static asiolink::IOAddress any("::");
 

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -225,6 +225,24 @@ This informational message singals that there is a reservation for this client
 and this client got this specific prefix. This is normal operation for
 host reservation.
 
+% DHCPSRV_HR_REVOKED_ADDR6_LEASE Address %1 was revoked from client %2 as it is reserved for client %3
+This informational message is an indication that the specified IPv6 address used
+to be used by client A, but it is now reserved for client B. As such, client
+A was told to stop using it, so it could be leased to client B. This is
+normal operation when doing conflict resolution, i.e. system administrator
+added reservation for an address that is currently in use by someone other
+than the reservation is for. The server will fully recover from this situation,
+but clients will experience address change.
+
+% DHCPSRV_HR_REVOKED_PREFIX6_LEASE Prefix %1/%2 was revoked from client %3 as it is reserved for client %4
+This informational message is an indication that the specified IPv6 address used
+to be used by client A, but it is now reserved for client B. As such, client
+A was told to stop using it, so it could be leased to client B. This is
+normal operation when doing conflict resolution, i.e. system administrator
+added reservation for an address that is currently in use by someone other
+than the reservation is for. The server will fully recover from this situation,
+but clients will experience address change.
+
 % DHCPSRV_INVALID_ACCESS invalid database access string: %1
 This is logged when an attempt has been made to parse a database access string
 and the attempt ended in error.  The access string in question - which
@@ -519,6 +537,13 @@ lease from the PostgreSQL database for the specified address.
 A debug message issued when the server is attempting to update IPv6
 lease from the PostgreSQL database for the specified address.
 
+% DHCPSRV_RENEW6_ERROR allocation engine experienced error with attempting to renew a lease: %1
+This error message indicates that an error was experienced during Renew or Rebind
+processing. Additional explanation is provided with this message. Depending
+on its nature, manual intervention may be required to continue processing
+messages from this particular client. The server will handle other clients
+normally.
+
 % DHCPSRV_UNEXPECTED_NAME database access parameters passed through '%1', expected 'lease-database'
 The parameters for access the lease database were passed to the server through
 the named configuration parameter, but the code was expecting them to be

src/lib/dhcpsrv/host.cc

@@ -230,5 +230,22 @@ Host::addClientClassInternal(ClientClasses& classes,
     }
 }
 
+std::string
+Host::getIdentifierAsText() const {
+    std::string txt;
+    if (hw_address_) {
+        txt = "hwaddr=" + hw_address_->toText(false);
+    } else {
+        txt = "duid=";
+        if (duid_) {
+            txt += duid_->toText();
+        } else {
+            txt += "(none)";
+        }
+    }
+
+    return (txt);
+}
+
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcpsrv/host.h

@@ -295,10 +295,18 @@ public:
         return (duid_);
     }
 
+    /// @brief Returns the identifier (MAC or DUID) in binary form.
+    /// @return const reference to MAC or DUID in vector<uint8_t> form
     const std::vector<uint8_t>& getIdentifier() const;
 
+    /// @brief Returns the identifier type.
+    /// @return the identifier type
     IdentifierType getIdentifierType() const;
 
+    /// @brief Returns host identifier (mac or DUID) in printer friendly form.
+    /// @return text form of the identifier, including (duid= or mac=).
+    std::string getIdentifierAsText() const;
+
     /// @brief Sets new IPv4 subnet identifier.
     ///
     /// @param ipv4_subnet_id New subnet identifier.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -299,10 +299,12 @@ public:
         return (lease);
     }
 
-    /// @brief Checks if the simple allocation can succeed
+    /// @brief Checks if the allocation can succeed.
     ///
-    /// The type of lease is determined by pool type (pool->getType()
+    /// The type of lease is determined by pool type (pool->getType()).
+    /// This test is particularly useful in connection with @ref renewTest.
     ///
+    /// @param engine a reference to Allocation Engine
     /// @param pool pool from which the lease will be allocated from
     /// @param hint address to be used as a hint
     /// @param fake true - this is fake allocation (SOLICIT)
@@ -353,8 +355,21 @@ public:
         return (leases);
     }
 
+    /// @brief Checks if the allocation can be renewed.
+    ///
+    /// The type of lease is determined by pool type (pool->getType()).
+    /// This test is particularly useful as a follow up to @ref allocateTest.
+    ///
+    /// @param engine a reference to Allocation Engine
+    /// @param pool pool from which the lease will be allocated from
+    /// @param hint address to be used as a hint
+    /// @param fake true - this is fake allocation (SOLICIT)
+    /// @param in_pool specifies whether the lease is expected to be in pool
+    /// @return allocated lease(s) (may be empty)
     Lease6Collection renewTest(AllocEngine& engine, const Pool6Ptr& pool,
-                               AllocEngine::HintContainer& hints, bool in_pool = true) {
+                               AllocEngine::HintContainer& hints,
+                               bool allow_new_leases_in_renewal,
+                               bool in_pool = true) {
 
         Lease::Type type = pool->getType();
         uint8_t expected_len = pool->getLength();
@@ -363,6 +378,7 @@ public:
                                         type, false, false, "", false);
         ctx.hints_ = hints;
         ctx.query_.reset(new Pkt6(DHCPV6_RENEW, 123));
+        ctx.allow_new_leases_in_renewals_ = allow_new_leases_in_renewal;
 
         Lease6Collection leases = engine.renewLeases6(ctx);
 
@@ -1632,7 +1648,7 @@ TEST_F(AllocEngine6Test, addressRenewal) {
     AllocEngine::HintContainer hints;
     hints.push_back(make_pair(leases[0]->addr_, 128));
 
-    Lease6Collection renewed = renewTest(engine, pool_, hints);
+    Lease6Collection renewed = renewTest(engine, pool_, hints, true);
     ASSERT_EQ(1, renewed.size());
 
     // Check that the lease was indeed renewed and hasn't changed
@@ -1663,7 +1679,7 @@ TEST_F(AllocEngine6Test, reservedAddressRenewal) {
     AllocEngine::HintContainer hints;
     hints.push_back(make_pair(leases[0]->addr_, 128));
 
-    Lease6Collection renewed = renewTest(engine, pool_, hints);
+    Lease6Collection renewed = renewTest(engine, pool_, hints, true);
     ASSERT_EQ(1, renewed.size());
     ASSERT_EQ("2001:db8:1::1c", leases[0]->addr_.toText());
 }
@@ -1688,7 +1704,7 @@ TEST_F(AllocEngine6Test, reservedAddressRenewChange) {
     // as the pool is 2001:db8:1::10 - 2001:db8:1::20.
     createHost6(true, IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::1c"), 128);
 
-    Lease6Collection renewed = renewTest(engine, pool_, hints);
+    Lease6Collection renewed = renewTest(engine, pool_, hints, true);
     ASSERT_EQ(1, renewed.size());
     ASSERT_EQ("2001:db8:1::1c", renewed[0]->addr_.toText());
 }
@@ -1719,7 +1735,7 @@ TEST_F(AllocEngine6Test, reservedAddressRenewReserved) {
     CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host);
     CfgMgr::instance().commit();
 
-    Lease6Collection renewed = renewTest(engine, pool_, hints);
+    Lease6Collection renewed = renewTest(engine, pool_, hints, true);
     ASSERT_EQ(1, renewed.size());
 
     // Check that we no longer have the reserved address.

src/lib/dhcpsrv/tests/host_unittest.cc

@@ -500,4 +500,17 @@ TEST(HostTest, addClientClasses) {
     EXPECT_TRUE(host->getClientClasses6().contains("bar"));
 }
 
+TEST(HostTest, getIdentifierAsText) {
+    Host host1("01:02:03:04:05:06", "hw-address",
+               SubnetID(1), SubnetID(2),
+               IOAddress("192.0.2.3"));
+    EXPECT_EQ("hwaddr=01:02:03:04:05:06", host1.getIdentifierAsText());
+
+    Host host2("0a:0b:0c:0d:0e:0f:ab:cd:ef", "duid",
+               SubnetID(1), SubnetID(2),
+               IOAddress("192.0.2.3"));
+    EXPECT_EQ("duid=0a:0b:0c:0d:0e:0f:ab:cd:ef",
+              host2.getIdentifierAsText());
+}
+
 } // end of anonymous namespace