[master] Merge branch 'trac3690'

Conflicts:
	src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

src/lib/dhcpsrv/alloc_engine.cc

@@ -827,7 +827,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         /// API which would in turn require lots of changes in unit tests.
         /// The ticket introducing a context and host reservation in the
         /// allocation engine is complex enough by itself to warrant that
-        /// the API change is done with a separate ticket.
+        /// the API change is done with a separate ticket (#3709).
         ClientContext4 ctx;
         ctx.subnet_ = subnet;
         ctx.clientid_ = clientid;
@@ -918,52 +918,70 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
                 return (Lease4Ptr());
             }
 
-            // The reserved address always takes precedence over an address
-            // supplied by the client (renewed address or requested).
-            const IOAddress& candidate = ctx.host_ ?
-                ctx.host_->getIPv4Reservation() : ctx.requested_address_;
+            // Now let's pick an address to be allocated to the client. The
+            // candidate address may either be a reserved one or the one that
+            // the client requests.
+            IOAddress candidate = 0;
+            ConstHostPtr other_host;
+            if (ctx.host_) {
+                candidate = ctx.host_->getIPv4Reservation();
 
-            // Once we picked an address we want to allocate, we have to check
-            // if this address is available.
-            existing = LeaseMgrFactory::instance().getLease4(candidate);
-            if (!existing) {
-                // The candidate address is currently unused. Let's create a
-                // lease for it.
-                Lease4Ptr lease = createLease4(subnet, clientid, hwaddr,
-                                               candidate, fwd_dns_update,
-                                               rev_dns_update,
-                                               hostname, callout_handle,
-                                               fake_allocation);
-
-                // If we have allocated the lease let's return it. Also,
-                // always return when tried to allocate reserved address,
-                // regardless if allocation was successful or not. If it
-                // was not successful, we will return a NULL pointer which
-                // indicates to the server that it should send NAK to the
-                // client.
-                if (lease || ctx.host_) {
-                    return (lease);
-                }
-
-            // There is a lease for this address in the lease database but
-            // it is possible that the lease has expired, in which case
-            // we will be able to reuse it.
             } else {
-                if (existing->expired()) {
-                    // Save the old lease, before reusing it.
-                    old_lease.reset(new Lease4(*existing));
-                    return (reuseExpiredLease(existing, ctx));
-
-                // The existing lease is not expired (is in use by some
-                // other client). If we are trying to get this lease because
-                // the address has been reserved for the client we have no
-                // choice but to return a NULL lease to indicate that the
-                // allocation has failed.
-                } else if (ctx.host_) {
+                candidate = ctx.requested_address_;
+                // If client is requesting an address we have to check if this address
+                // is not reserved for someone else. Note that for DHCPDISCOVER we
+                // treat the requested address as a hint and we don't return an empty
+                // lease.
+                other_host = HostMgr::instance().get4(ctx.subnet_->getID(), candidate);
+                if (!ctx.fake_allocation_ && other_host) {
                     return (Lease4Ptr());
-
                 }
+            }
+
+            // If address is not reserved for another client, let's try allocate it.
+            if (!other_host) {
+                // Once we picked an address we want to allocate, we have to check
+                // if this address is available.
+                existing = LeaseMgrFactory::instance().getLease4(candidate);
+                if (!existing) {
+                    // The candidate address is currently unused. Let's create a
+                    // lease for it.
+                    Lease4Ptr lease = createLease4(subnet, clientid, hwaddr,
+                                                   candidate, fwd_dns_update,
+                                                   rev_dns_update,
+                                                   hostname, callout_handle,
+                                                   fake_allocation);
+
+                    // If we have allocated the lease let's return it. Also,
+                    // always return when tried to allocate reserved address,
+                    // regardless if allocation was successful or not. If it
+                    // was not successful, we will return a NULL pointer which
+                    // indicates to the server that it should send NAK to the
+                    // client.
+                    if (lease || ctx.host_) {
+                        return (lease);
+                    }
+
+                    // There is a lease for this address in the lease database but
+                    // it is possible that the lease has expired, in which case
+                    // we will be able to reuse it.
+                } else {
+                    if (existing->expired()) {
+                        // Save the old lease, before reusing it.
+                        old_lease.reset(new Lease4(*existing));
+                        return (reuseExpiredLease(existing, ctx));
+
+                        // The existing lease is not expired (is in use by some
+                        // other client). If we are trying to get this lease because
+                        // the address has been reserved for the client we have no
+                        // choice but to return a NULL lease to indicate that the
+                        // allocation has failed.
+                    } else if (ctx.host_) {
+                        return (Lease4Ptr());
+
+                    }
 
+                }
             }
         }
 

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -2780,6 +2780,42 @@ TEST_F(AllocEngine4Test, reservedAddressVsDynamicPool) {
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.100");
 }
 
+// This test checks that the client requesting an address which is
+// reserved for another client will get no lease or a different
+// address will be assigned if the client is sending a DHCPDISCOVER.
+TEST_F(AllocEngine4Test, reservedAddressHintUsedByOtherClient) {
+    // Create a reservation for the client.
+    HostPtr host(new Host(&hwaddr2_->hwaddr_[0], hwaddr_->hwaddr_.size(),
+                          Host::IDENT_HWADDR, subnet_->getID(),
+                          SubnetID(0), IOAddress("192.0.2.100")));
+    CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host);
+    CfgMgr::instance().commit();
+
+    AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false);
+
+    // Different client is requesting this address.
+    Lease4Ptr allocated_lease = engine.allocateLease4(subnet_, ClientIdPtr(),
+                                                      hwaddr_,
+                                                      IOAddress("192.0.2.100"),
+                                                      false, false, "", false,
+                                                      CalloutHandlePtr(),
+                                                      old_lease_);
+    // The client should get no lease (DHCPNAK).
+    ASSERT_FALSE(allocated_lease);
+
+    // The same client should get a different lease than requested if
+    // if is sending a DHCPDISCOVER (fake allocation is true).
+    allocated_lease = engine.allocateLease4(subnet_, ClientIdPtr(),
+                                            hwaddr_,
+                                            IOAddress("192.0.2.100"),
+                                            false, false, "", true,
+                                            CalloutHandlePtr(),
+                                            old_lease_);
+    ASSERT_TRUE(allocated_lease);
+    // Make sure the lease obtained is for a different address.
+    EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.100");
+}
+
 // This test checks that the allocation engine refuses to allocate an
 // address when the pool is exhausted, and the only one available
 // address is reserved for a different client.