[master] Merge branch 'trac3694'

Conflicts:
	src/lib/dhcpsrv/alloc_engine.cc
	src/lib/dhcpsrv/alloc_engine.h
	src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc

doc/guide/dhcp4-srv.xml

@@ -2046,70 +2046,73 @@ temporarily override a list of interface names and listen on all interfaces.
 
     <section id="reservation4-conflict">
       <title>Conflicts in DHCPv4 reservations</title>
-      <para>As reservations and lease information are stored separately,
-      conflicts may arrise. Consider the following series of events. The server
-      has configured dynamic pool of addresses from the range of 192.0.2.10 to
-      192.0.2.20. Host A requests an address and gets 19.0.2.10. Now the system
-      administrator decides to reserve an address for host B. He decides to
+      <para>As the reservations and lease information are stored separately,
+      conflicts may arise. Consider the following series of events. The server
+      has configured the dynamic pool of addresses from the range of 192.0.2.10 to
+      192.0.2.20. The Host A requests an address and gets 19.0.2.10. Now the system
+      administrator decides to reserve the address for the Host B. He decides to
       reserve 192.0.2.10 for that purpose. In general, reserving an address that
       is currently assigned to someone else is not recommended, but there are
       valid use cases where such an operation is warranted.</para>
 
       <para>The server now has a conflict to resolve. Let's analyze the
-      situation here. If the host B boots up and requests an address, the server is
-      not able to assign the reserved address 192.0.2.10. A naive approach would
-      to be immediately remove the lease for host A and create a new one for
-      host B. That would not solve the problem, though, because as soon as host
-      B get the address, it will detect that the address is already in use by
-      someone else (host A) and would send Decline. Therefore in this situation,
-      the server has to temporarily assign a different address (not matching
-      what has been reserved) to host B.</para>
+      situation here. If the Host B boots up and requests an address, the server is
+      not able to assign the reserved address 192.0.2.10 for the Host B. A naive
+      approach would to be immediately remove the existing lease for the Host A
+      and create a new one for the Host B. That would not solve the problem,
+      though, because as soon as the Host B gets the address, it will detect
+      that the address is already in use by the Host A and would send
+      the DHCPDECLINE message. Therefore, in this situation, the server has
+      to temporarily assign a different address (not matching what has been
+      reserved) to the Host B.</para>
 
       <!-- let's keep this text around. It describes how that is working in v6
-      <para>When the host A renews its address, the server will discover that
+      <para>When the Host A renews its address, the server will discover that
       the address being renewed is now reserved for someone else (host
-      B). Therefore the server will remove the lease and will inform the host A
-      that it is no longer allowed to use it by sending NAK message. Host A
+      B). Therefore the server will remove the lease and will inform the Host A
+      that it is no longer allowed to use it by sending DHCPNAK message. Host A
       will then revert to server discovery and will eventually get a different
       address.  The address 192.0.2.10 is now no longer used. When host B tries
-      to renew its temporary address, the server will detect that it has a valid
-      lease, but there is a reservation for a different address. The server will
-      send NAK to inform host B that its address is no longer usable. The
-      server will also remove its temporary lease. It will revert to the server
-      discovery phase and will eventually send a REQUEST message. This time the
-      server will find out that there is a reservation for that host and the
-      reserved address 192.0.2.10 is not used, so it will be granted.</para> -->
-
-      <para>When the host A renews its address, the server will discover that
-      the address being renewed is now reserved for someone else (host
-      B). Therefore the server will inform the host A that it is no longer
-      allowed to use it by sending NAK message. The server will remove the
-      lease, though, as there's small chance that the NAK may be lost if the
+      to renew its temporarily assigned  address, the server will detect that
+      it has a valid lease, but there is a reservation for a different address.
+      The server will send DHCPNAK to inform host B that its address is no
+      longer usable. The server will also remove its temporary lease. It will
+      revert to the server discovery phase and will eventually send a
+      DHCPREQUEST message. This time the server will find out that there is a
+      reservation for that host and the reserved address 192.0.2.10 is not used,
+      so it will be granted.</para> -->
+
+      <para>When the Host A renews its address, the server will discover that
+      the address being renewed is now reserved for another host - the Host
+      B. Therefore the server will inform the Host A that it is no longer
+      allowed to use it by sending DHCPNAK message. The server will not remove the
+      lease, though, as there's small chance that the DHCPNAK may be lost if the
       network is lossy. If that happens, the client will not receive any
-      responses, so it will retransmit its Request packet. Once the NAK is
-      received by the host A, it will then revert to the server discovery and
-      will eventually get a different address. Besides allocating a new lease,
-      the server will also remove the old one. As a result the address
-      192.0.2.10 will be no longer used. When host B tries to renew its
-      temporary address, the server will detect that it has a valid lease, but
-      there is a reservation for a different address. The server will send NAK
-      to inform host B that its address is no longer usable, but will keep its
-      least (again, the NAK may be lost, so the server will keep it, until the
-      client gets back for a new address). The host B will revert to the server
-      discovery phase and will eventually send a REQUEST message. This time the
-      server will find out that there is a reservation for that host and the
-      reserved address 192.0.2.10 is not used, so it will be granted. It will
-      also remove the lease for the temporary address that the host B previously
-      had.</para>
+      responses, so it will retransmit its DHCPREQUEST packet. Once the
+      DHCPNAK is received by the Host A, it will then revert to the server
+      discovery and will eventually get a different address. Besides
+      allocating a new lease, the server will also remove the old one. As
+      a result, the address 192.0.2.10 will be no longer used. When Host B
+      tries to renew its temporarily assigned address, the server will detect
+      that it has a valid lease, but there is a reservation for a different
+      address. The server will send DHCPNAK to inform Host B that its address
+      is no longer usable, but will keep its lease (again, the DHCPNAK may be
+      lost, so the server will keep it, until the client returns for a new
+      address). The Host B will revert to the server discovery phase and will
+      eventually send a DHCPREQUEST message. This time the server will find
+      out that there is a reservation for that host and the reserved address
+      192.0.2.10 is not used, so it will be granted. It will also remove the
+      lease for the temporarily assigned address that the Host B previously
+      obtained.</para>
 
       <para>This recovery will succeed, even if other hosts will attempt to get
-      the reserved address. Had the host C requested address 192.0.2.10 after
-      the reservation was made, the server will either propose a different
-      address (when responding to DISCOVER) or would send NAK (when responding
-      to REQUEST).</para>
+      the reserved address. Had the Host C requested address 192.0.2.10 after
+      the reservation was made, the server will either offer a different
+      address (when responding to DHCPDISCOVER) or would send DHCPNAK
+      (when responding to DHCPREQUEST).</para>
 
       <para>This recovery mechanism allows the server to fully recover from a
-      case where reservations conflict with existing leases. This procedure
+      case where reservations conflict with the existing leases. This procedure
       takes time and will roughly take as long as renew-timer value specified.
       The best way to avoid such recovery is to not define new reservations that
       conflict with existing leases. Another recommendation is to use

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -49,6 +49,7 @@ Dhcp4Client::Dhcp4Client(const Dhcp4Client::State& state) :
     curr_transid_(0),
     dest_addr_("255.255.255.255"),
     hwaddr_(generateHWAddr()),
+    iface_name_("eth0"),
     relay_addr_("192.0.2.2"),
     requested_options_(),
     server_facing_relay_addr_("10.0.0.2"),
@@ -64,6 +65,7 @@ Dhcp4Client::Dhcp4Client(boost::shared_ptr<NakedDhcpv4Srv> srv,
     curr_transid_(0),
     dest_addr_("255.255.255.255"),
     hwaddr_(generateHWAddr()),
+    iface_name_("eth0"),
     relay_addr_("192.0.2.2"),
     requested_options_(),
     server_facing_relay_addr_("10.0.0.2"),
@@ -344,7 +346,7 @@ Dhcp4Client::sendMsg(const Pkt4Ptr& msg) {
                               msg->getBuffer().getLength()));
     msg_copy->setRemoteAddr(msg->getLocalAddr());
     msg_copy->setLocalAddr(dest_addr_);
-    msg_copy->setIface("eth0");
+    msg_copy->setIface(iface_name_);
     srv_->fakeReceive(msg_copy);
     srv_->run();
 }

src/bin/dhcp4/tests/dhcp4_client.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -273,6 +273,14 @@ public:
     /// @param hwaddr_str String representation of the HW address.
     void setHWAddress(const std::string& hwaddr_str);
 
+    /// @brief Sets the interface over which the messages should be sent.
+    ///
+    /// @param iface_name Name of the interface over which the messages should
+    /// be sent.
+    void setIfaceName(const std::string& iface_name) {
+        iface_name_ = iface_name;
+    }
+
     /// @brief Sets client state.
     ///
     /// Depending on the current state the client's behavior is different
@@ -371,6 +379,9 @@ private:
     /// @brief Current hardware address of the client.
     HWAddrPtr hwaddr_;
 
+    /// @brief Interface to be used to send the messages.
+    std::string iface_name_;
+
     /// @brief Relay address to use.
     asiolink::IOAddress relay_addr_;
 

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -845,6 +845,7 @@ TEST_F(Dhcpv4SrvTest, RenewBasic) {
     req->setYiaddr(addr);
     req->setCiaddr(addr); // client's address
     req->setIface("eth0");
+    req->setHWAddr(hwaddr2);
 
     req->addOption(clientid);
     req->addOption(srv->getServerID());
@@ -2782,6 +2783,7 @@ TEST_F(HooksDhcpv4SrvTest, lease4RenewSimple) {
     req->setYiaddr(addr);
     req->setCiaddr(addr); // client's address
     req->setIface("eth0");
+    req->setHWAddr(hwaddr2);
 
     req->addOption(clientid);
     req->addOption(srv_->getServerID());
@@ -2877,6 +2879,7 @@ TEST_F(HooksDhcpv4SrvTest, lease4RenewSkip) {
     req->setYiaddr(addr);
     req->setCiaddr(addr); // client's address
     req->setIface("eth0");
+    req->setHWAddr(hwaddr2);
 
     req->addOption(clientid);
     req->addOption(srv_->getServerID());

src/bin/dhcp4/tests/direct_client_unittest.cc

@@ -21,6 +21,7 @@
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcp4/json_config_parser.h>
+#include <dhcp4/tests/dhcp4_client.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
 #include <gtest/gtest.h>
 #include <string>
@@ -306,51 +307,23 @@ TEST_F(DirectClientTest, renew) {
     ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
     // Add a subnet.
     ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
-    // Make sure that the subnet has been really added. Also, the subnet
-    // will be needed to create a lease for a client.
-    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(IOAddress("10.0.0.10"));
-    // Create a lease for a client that we will later renewed. By explicitly
-    // creating a lease we will get to know the lease parameters, such as
-    // leased address etc.
-    const uint8_t hwaddr_data[] = { 1, 2, 3, 4, 5, 6 };
-    HWAddrPtr hwaddr(new HWAddr(hwaddr_data, sizeof(hwaddr_data), HTYPE_ETHER));
-    Lease4Ptr lease(new Lease4(IOAddress("10.0.0.10"), hwaddr,
-                               &generateClientId()->getData()[0],
-                               generateClientId()->getData().size(),
-                               100, 50, 75, time(NULL),
-                               subnet->getID()));
-    LeaseMgrFactory::instance().addLease(lease);
-
-    // Create a Request to renew client's lease. The renew request is unicast
-    // through eth1. Note, that in case of renewal the client unicasts its
-    // Request and sets the ciaddr. The server is supposed to use ciaddr to
-    // pick the subnet for the client. In order to make sure that the server
-    // uses ciaddr, we simulate reception of the packet through eth1, for which
-    // there is no subnet for directly connected clients.
-    Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
-    req->setCiaddr(IOAddress("10.0.0.10"));
-    req = createClientMessage(req, "eth1");
-    req->setLocalAddr(IOAddress("10.0.0.1"));
-    req->setRemoteAddr(req->getCiaddr());
 
-    srv_.fakeReceive(req);
+    // Create the DHCPv4 client.
+    Dhcp4Client client;
+    client.useRelay(false);
 
-    // Process clients' messages.
-    srv_.run();
+    // Obtain the lease using the 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<IOAddress>(new IOAddress("10.0.0.10"))));
+    ASSERT_EQ("10.0.0.10", client.config_.lease_.addr_.toText());
 
-    // Check that the server did send response.
-    ASSERT_EQ(1, srv_.fake_sent_.size());
-    Pkt4Ptr response = srv_.fake_sent_.front();
-    ASSERT_TRUE(response);
-
-    ASSERT_EQ(DHCPACK, response->getType());
-    // Check that the offered address belongs to the suitable subnet.
-    subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(response->getYiaddr());
-    ASSERT_TRUE(subnet);
-    EXPECT_EQ("10.0.0.0", subnet->get().first.toText());
+    // Put the client into the renewing state.
+    client.setState(Dhcp4Client::RENEWING);
 
+    // Renew, and make sure we have obtained the same address.
+    ASSERT_NO_THROW(client.doRequest());
+    ASSERT_TRUE(client.getContext().response_);
+    EXPECT_EQ(DHCPACK, static_cast<int>(client.getContext().response_->getType()));
+    EXPECT_EQ("10.0.0.10", client.config_.lease_.addr_.toText());
 }
 
 // This test verifies that when a client in the Rebinding state broadcasts
@@ -366,58 +339,31 @@ TEST_F(DirectClientTest, rebind) {
     ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
     // Add a subnet.
     ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
-    // Make sure that the subnet has been really added. Also, the subnet
-    // will be needed to create a lease for a client.
-    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(IOAddress("10.0.0.10"));
-    // Create a lease, which will be later renewed. By explicitly creating a
-    // lease we will know the lease parameters, such as leased address etc.
-    const uint8_t hwaddr_data[] = { 1, 2, 3, 4, 5, 6 };
-    HWAddrPtr hwaddr(new HWAddr(hwaddr_data, sizeof(hwaddr_data), HTYPE_ETHER));
-    Lease4Ptr lease(new Lease4(IOAddress("10.0.0.10"), hwaddr,
-                               &generateClientId()->getData()[0],
-                               generateClientId()->getData().size(),
-                               100, 50, 75, time(NULL),
-                               subnet->getID()));
-    LeaseMgrFactory::instance().addLease(lease);
-
-    // Broadcast Request through an interface for which there is no subnet
-    // configured. This message should be discarded by the server.
-    Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
-     req->setCiaddr(IOAddress("10.0.0.10"));
-    req = createClientMessage(req, "eth1");
-    req->setRemoteAddr(req->getCiaddr());
 
-    srv_.fakeReceive(req);
-
-    // Broadcast another Request through an interface for which there is
-    // a subnet configured. The server should generate a response.
-    req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 5678));
-    req->setCiaddr(IOAddress("10.0.0.10"));
-    req = createClientMessage(req, "eth0");
-    req->setRemoteAddr(req->getCiaddr());
+    // Create the DHCPv4 client.
+    Dhcp4Client client;
+    client.useRelay(false);
 
-    srv_.fakeReceive(req);
-
-    // Process clients' messages.
-    srv_.run();
-
-    // Check that the server did send exactly one response.
-    ASSERT_EQ(1, srv_.fake_sent_.size());
-    Pkt4Ptr response = srv_.fake_sent_.front();
-    ASSERT_TRUE(response);
+    // Obtain the lease using the 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<IOAddress>(new IOAddress("10.0.0.10"))));
+    ASSERT_EQ("10.0.0.10", client.config_.lease_.addr_.toText());
 
-    // Make sure that the server responded with ACK, not NAK.
-    ASSERT_EQ(DHCPACK, response->getType());
-    // Make sure that the response is generated for the second Request
-    // (transmitted over eth0).
-    EXPECT_EQ(5678, response->getTransid());
-    // Check that the offered address belongs to the suitable subnet.
-    subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(response->getYiaddr());
-    ASSERT_TRUE(subnet);
-    EXPECT_EQ("10.0.0.0", subnet->get().first.toText());
+    // Put the client into the rebinding state.
+    client.setState(Dhcp4Client::REBINDING);
 
+    // Broadcast Request through an interface for which there is no subnet
+    // configured. This message should be discarded by the server.
+    client.setIfaceName("eth1");
+    ASSERT_NO_THROW(client.doRequest());
+    EXPECT_FALSE(client.getContext().response_);
+
+    // Send Rebind over the correct interface, and make sure we have obtained
+    // the same address.
+    client.setIfaceName("eth0");
+    ASSERT_NO_THROW(client.doRequest());
+    ASSERT_TRUE(client.getContext().response_);
+    EXPECT_EQ(DHCPACK, static_cast<int>(client.getContext().response_->getType()));
+    EXPECT_EQ("10.0.0.10", client.config_.lease_.addr_.toText());
 }
 
 }

src/bin/dhcp4/tests/dora_unittest.cc

@@ -508,19 +508,21 @@ TEST_F(DORATest, reservation) {
 //     Client A still holds this address.
 // 13. Client B uses 4-way exchange to obtain a new lease.
 // 14. The server determines that the Client B has a reservation for the
-//     address which is in use by Client A. The server drops the client's
-//     DHCPDISCOVER message.
-// 15. Client A renews the lease.
-// 16. The server determines that the address that Client A is using is reserved
+//     address which is in use by Client A and offers an address different
+//     than reserved.
+// 15. Client B requests the allocation of the offered address and the server
+//     allocates this address.
+// 16. Client A renews the lease.
+// 17. The server determines that the address that Client A is using is reserved
 //     for Client B. The server returns DHCPNAK to the Client A.
-// 17. Client B uses 4-way exchange to obtain the reserved lease but the lease
-//     for the Client A hasn't been removed yet. Client B's DHCPDISCOVER
-//     message is dropped again.
-// 18. Client A uses 4-way exchange to allocate a new lease.
-// 19. The server allocates a new lease from the dynamic pool but it avoids
+// 18. Client B uses 4-way exchange to obtain the reserved lease but the lease
+//     for the Client A hasn't been removed yet. Client B is assigned the same
+//     address it has been using.
+// 19. Client A uses 4-way exchange to allocate a new lease.
+// 20. The server allocates a new lease from the dynamic pool but it avoids
 //     allocating the address reserved for the Client B.
-// 20. Client B uses 4-way exchange to obtain a new lease.
-// 21. The server finally allocates a reserved address to the Client B.
+// 21. Client B uses 4-way exchange to obtain a new lease.
+// 22. The server finally allocates a reserved address to the Client B.
 TEST_F(DORATest, reservationsWithConflicts) {
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Configure DHCP server.
@@ -629,10 +631,13 @@ TEST_F(DORATest, reservationsWithConflicts) {
     // Client B performs a DHCPDISCOVER.
     clientB.setState(Dhcp4Client::SELECTING);
     // The server determines that the address reserved for Client B is
-    // in use by Client A so it drops the message from the Client B.
-    ASSERT_NO_THROW(clientB.doDiscover(boost::shared_ptr<
-                                       IOAddress>(new IOAddress("0.0.0.0"))));
-    ASSERT_FALSE(clientB.getContext().response_);
+    // in use by Client A so it offers a different address.
+    ASSERT_NO_THROW(clientB.doDORA(boost::shared_ptr<
+                                   IOAddress>(new IOAddress("0.0.0.0"))));
+    ASSERT_TRUE(clientB.getContext().response_);
+    ASSERT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
+    IOAddress client_b_addr = clientB.config_.lease_.addr_;
+    ASSERT_NE(client_b_addr, in_pool_addr);
 
     // Client A renews the lease.
     client.setState(Dhcp4Client::RENEWING);
@@ -644,15 +649,29 @@ TEST_F(DORATest, reservationsWithConflicts) {
     resp = client.getContext().response_;
     ASSERT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
 
-    // Client B performs 4-way exchange but still doesn't get an address
-    // because Client A hasn't obtained a new lease, so it is still using
-    // an address reserved for Client B.
+    // Client B performs 4-way exchange but still gets an address from the
+    // dynamic pool, because Client A hasn't obtained a new lease, so it is
+    // still using an address reserved for Client B.
     clientB.setState(Dhcp4Client::SELECTING);
     // Obtain a lease from the server using the 4-way exchange.
-    ASSERT_NO_THROW(clientB.doDiscover(boost::shared_ptr<
-                                       IOAddress>(new IOAddress("0.0.0.0"))));
+    ASSERT_NO_THROW(clientB.doDORA(boost::shared_ptr<
+                                   IOAddress>(new IOAddress("0.0.0.0"))));
     // Make sure that the server responded.
-    ASSERT_FALSE(clientB.getContext().response_);
+    ASSERT_TRUE(clientB.getContext().response_);
+    ASSERT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
+    ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    ASSERT_EQ(client_b_addr, clientB.config_.lease_.addr_);
+
+    // Client B renews its lease.
+    clientB.setState(Dhcp4Client::RENEWING);
+    clientB.setDestAddress(IOAddress("10.0.0.1"));
+    ASSERT_NO_THROW(clientB.doRequest());
+    // The server should renew the client's B lease because the address
+    // reserved for client B is still in use by the client A.
+    ASSERT_TRUE(clientB.getContext().response_);
+    EXPECT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
+    ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    ASSERT_EQ(client_b_addr, clientB.config_.lease_.addr_);
 
     // Client A performs 4-way exchange.
     client.setState(Dhcp4Client::SELECTING);
@@ -667,12 +686,20 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
     // The server should have assigned a different address than the one
     // reserved for the Client B.
-    ASSERT_NE(client.config_.lease_.addr_.toText(), in_pool_addr.toText());
+    ASSERT_NE(client.config_.lease_.addr_, in_pool_addr);
     subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->
         selectSubnet(client.config_.lease_.addr_);
     ASSERT_TRUE(subnet);
     ASSERT_TRUE(subnet->inPool(Lease::TYPE_V4, client.config_.lease_.addr_));
 
+    // Client B renews again.
+    ASSERT_NO_THROW(clientB.doRequest());
+    // The client B should now receive the DHCPNAK from the server because
+    // the reserved address is now available and the client should
+    // revert to the DHCPDISCOVER to obtain it.
+    ASSERT_TRUE(clientB.getContext().response_);
+    EXPECT_EQ(DHCPNAK, static_cast<int>(clientB.getContext().response_->getType()));
+
     // Client B performs 4-way exchange and obtains a lease for the
     // reserved address.
     clientB.setState(Dhcp4Client::SELECTING);

src/lib/asiolink/io_address.h

@@ -103,6 +103,21 @@ public:
         return (asio_address_.is_v4());
     }
 
+    /// \brief Convenience function to check if it is an IPv4 zero address.
+    ///
+    /// \return true if the address is the zero IPv4 address.
+    bool isV4Zero() const {
+        return (equals(IPV4_ZERO_ADDRESS()));
+    }
+
+    /// \brief Convenience function to check if it is an IPv4 broadcast
+    ///        address.
+    ///
+    /// \return true if the address is the broadcast IPv4 address.
+    bool isV4Bcast() const {
+        return (equals(IPV4_BCAST_ADDRESS()));
+    }
+
     /// \brief Convenience function to check for an IPv6 address
     ///
     /// \return true if the address is a V6 address
@@ -110,6 +125,13 @@ public:
         return (asio_address_.is_v6());
     }
 
+    /// \brief Convenience function to check if it is an IPv4 zero address.
+    ///
+    /// \return true if the address is the zero IPv4 address.
+    bool isV6Zero() const {
+        return (equals(IPV6_ZERO_ADDRESS()));
+    }
+
     /// \brief checks whether and address is IPv6 and is link-local
     ///
     /// \return true if the address is IPv6 link-local, false otherwise

src/lib/asiolink/tests/io_address_unittest.cc

@@ -119,6 +119,40 @@ TEST(IOAddressTest, isV4) {
     EXPECT_FALSE(address6.isV4());
 }
 
+TEST(IOAddressTest, isV4Zero) {
+    // 0.0.0.0
+    const IOAddress address_zero("0.0.0.0");
+    EXPECT_TRUE(address_zero.isV4Zero());
+    // :: (v6 zero address)
+    const IOAddress address_zero_v6("::");
+    EXPECT_FALSE(address_zero_v6.isV4Zero());
+    // 192.0.2.3
+    const IOAddress address_non_zero("192.0.2.3");
+    EXPECT_FALSE(address_non_zero.isV4Zero());
+    // 0.0.0.100
+    const IOAddress address_non_zero1("0.0.0.100");
+    EXPECT_FALSE(address_non_zero1.isV4Zero());
+    // 64.0.0.0
+    const IOAddress address_non_zero2("64.0.0.0");
+    EXPECT_FALSE(address_non_zero2.isV4Zero());
+}
+
+TEST(IOAddressTest, isV4Bcast) {
+    // 255.255.255.255
+    const IOAddress address_bcast("255.255.255.255");
+    EXPECT_TRUE(address_bcast.isV4Bcast());
+    // 10.2.3.4
+    const IOAddress address_non_bcast("10.2.3.4");
+    EXPECT_FALSE(address_non_bcast.isV4Bcast());
+    // 255.255.255.23
+    const IOAddress address_non_bcast1("255.255.255.23");
+    EXPECT_FALSE(address_non_bcast1.isV4Bcast());
+    // 123.255.255.255
+    const IOAddress address_non_bcast2("123.255.255.255");
+    EXPECT_FALSE(address_non_bcast2.isV4Bcast());
+
+}
+
 TEST(IOAddressTest, isV6) {
     const IOAddress address4("192.0.2.1");
     const IOAddress address6("2001:db8:1::dead:beef");
@@ -127,6 +161,21 @@ TEST(IOAddressTest, isV6) {
     EXPECT_TRUE(address6.isV6());
 }
 
+TEST(IOAddressTest, isV6Zero) {
+    // ::
+    const IOAddress address_zero("::");
+    EXPECT_TRUE(address_zero.isV6Zero());
+    // 0.0.0.0
+    const IOAddress address_non_zero("0.0.0.0");
+    EXPECT_FALSE(address_non_zero.isV6Zero());
+    // ::ff
+    const IOAddress address_non_zero1("::ff");
+    EXPECT_FALSE(address_non_zero1.isV6Zero());
+    // ff::
+    const IOAddress address_non_zero2("ff::");
+    EXPECT_FALSE(address_non_zero2.isV6Zero());
+}
+
 TEST(IOAddressTest, uint32) {
     IOAddress addr1("192.0.2.5");
 

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -179,6 +179,12 @@ the database access parameters are changed: in the latter case, the
 server closes the currently open database, and opens a database using
 the new parameters.
 
+% DHCPSRV_DISCOVER_ADDRESS_CONFLICT conflicting reservation for address %1 with existing lease %2
+This warning message is issued when the DHCP server finds that the
+address reserved for the client can't be offered because this address
+is currently allocated to another client. The server will try to allocate
+a different address to the client to use until the conflict is resolved.
+
 % DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION error handler for DHCP_DDNS IO generated an expected exception: %1
 This is an error message that occurs when an attempt to send a request to
 kea-dhcp-ddns fails there registered error handler threw an uncaught exception.

src/lib/dhcpsrv/lease_mgr.h

@@ -198,7 +198,7 @@ public:
     ///        and a subnet
     ///
     /// There can be at most one lease for a given HW address in a single
-    /// pool, so this method with either return a single lease or NULL.
+    /// pool, so this method will either return a single lease or NULL.
     ///
     /// @param hwaddr hardware address of the client
     /// @param subnet_id identifier of the subnet that lease must belong to
@@ -235,8 +235,8 @@ public:
 
     /// @brief Returns existing IPv4 lease for specified client-id
     ///
-    /// There can be at most one lease for a given HW address in a single
-    /// pool, so this method with either return a single lease or NULL.
+    /// There can be at most one lease for a given client-id in a single
+    /// pool, so this method will either return a single lease or NULL.
     ///
     /// @param clientid client identifier
     /// @param subnet_id identifier of the subnet that lease must belong to

src/lib/dhcpsrv/memfile_lease_storage.h

@@ -88,7 +88,7 @@ typedef boost::multi_index_container<
         >,
 
         // Specification of the second index starts here.
-        boost::multi_index::ordered_unique<
+        boost::multi_index::ordered_non_unique<
             // This is a composite index that combines two attributes of the
             // Lease4 object: hardware address and subnet id.
             boost::multi_index::composite_key<

src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc

@@ -141,7 +141,7 @@ TEST_F(AllocEngine4Test, allocWithValidHint4) {
 
 
 // This test checks if the allocation with a hint that is in range,
-// in pool, but is currently used) can succeed
+// in pool, but is currently used can succeed
 TEST_F(AllocEngine4Test, allocWithUsedHint4) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE,
@@ -162,7 +162,7 @@ TEST_F(AllocEngine4Test, allocWithUsedHint4) {
     // twice.
     AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_,
                                     IOAddress("192.0.2.106"), false, false,
-                                    "", false);
+                                    "", true);
     Lease4Ptr lease = engine->allocateLease4(ctx);
 
     // New lease has been allocated, so the old lease should not exist.
@@ -180,16 +180,13 @@ TEST_F(AllocEngine4Test, allocWithUsedHint4) {
     // Do all checks on the lease
     checkLease4(lease);
 
-    // Check that the lease is indeed in LeaseMgr
+    // The lease should not be in the LeaseMgr because it was a failed allocation.
     Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
-    ASSERT_TRUE(from_mgr);
-
-    // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease(lease, from_mgr);
+    ASSERT_FALSE(from_mgr);
 }
 
 
-// This test checks if the allocation with a hint that is out the blue
+// This test checks if an allocation with a hint that is out of the blue
 // can succeed. The invalid hint should be ignored completely.
 TEST_F(AllocEngine4Test, allocBogusHint4) {
     boost::scoped_ptr<AllocEngine> engine;
@@ -197,12 +194,12 @@ TEST_F(AllocEngine4Test, allocBogusHint4) {
                                                  100, false)));
     ASSERT_TRUE(engine);
 
-    // Client would like to get a 3000::abc lease, which does not belong to any
+    // Client would like to get a 10.1.1.1 lease, which does not belong to any
     // supported lease. Allocation engine should ignore it and carry on
     // with the normal allocation
     AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_,
                                     IOAddress("10.1.1.1"), false, false,
-                                    "", false);
+                                    "", true);
     Lease4Ptr lease = engine->allocateLease4(ctx);
     // Check that we got a lease
     ASSERT_TRUE(lease);
@@ -216,15 +213,11 @@ TEST_F(AllocEngine4Test, allocBogusHint4) {
     // Do all checks on the lease
     checkLease4(lease);
 
-    // Check that the lease is indeed in LeaseMgr
+    // Check that the lease is not in the LeaseMgr as it is a fake allocation.
     Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
-    ASSERT_TRUE(from_mgr);
-
-    // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease(lease, from_mgr);
+    EXPECT_FALSE(from_mgr);
 }
 
-
 // This test checks that NULL values are handled properly
 TEST_F(AllocEngine4Test, allocateLease4Nulls) {
     boost::scoped_ptr<AllocEngine> engine;
@@ -543,54 +536,39 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
     EXPECT_TRUE(*ctx.old_lease_ == original_lease);
 }
 
-/// @todo write renewLease6
-
-// This test checks if a lease is really renewed when renewLease4 method is
-// called
-TEST_F(AllocEngine4Test, renewLease4) {
-    boost::scoped_ptr<AllocEngine> engine;
-
-    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE,
-                                                 100, false)));
-    ASSERT_TRUE(engine);
-
-    IOAddress addr("192.0.2.102");
-    const uint32_t old_lifetime = 100;
-    const uint32_t old_t1 = 50;
-    const uint32_t old_t2 = 75;
-    const time_t old_timestamp = time(NULL) - 45; // Allocated 45 seconds ago
-
-    // Just a different hw/client-id for the second client
-    const uint8_t hwaddr2_data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
-    HWAddrPtr hwaddr2(new HWAddr(hwaddr2_data, sizeof(hwaddr2_data), HTYPE_ETHER));
-    const uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
-    Lease4Ptr lease(new Lease4(addr, hwaddr2, clientid2, sizeof(clientid2),
-                               old_lifetime, old_t1, old_t2,
-                               old_timestamp, subnet_->getID()));
-    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
-
-    // Lease was assigned 45 seconds ago and is valid for 100 seconds. Let's
-    // renew it.
-    ASSERT_FALSE(lease->expired());
-    ctx_.fwd_dns_update_ = true;
-    ctx_.rev_dns_update_ = true;
-    ctx_.hostname_ = "host.example.com.";
-    ctx_.fake_allocation_ = false;
-    lease = engine->renewLease4(lease, ctx_);
-
-    // Check that he got that single lease
-    ASSERT_TRUE(lease);
-    EXPECT_EQ(addr, lease->addr_);
-
-    // Check that the lease matches subnet_, hwaddr_,clientid_ parameters
-    checkLease4(lease);
+// This test checks that when the client requests the address which belongs
+// to another client, the allocation engine returns NULL (for the
+// DHCPREQUEST case) or a lease for the address which belongs to this
+// client (DHCPDISCOVER case).
+TEST_F(AllocEngine4Test, requestOtherClientLease) {
+    // Create the first lease.
+    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
+                               100, 30, 60, time(NULL), subnet_->getID(),
+                               false, false, ""));
+    // Create the second lease.
+    Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_, 0, 0,
+                               100, 30, 60, time(NULL), subnet_->getID(),
+                               false, false, ""));
+    // Add leases for both clients to the Lease Manager.
+    LeaseMgrFactory::instance().addLease(lease);
+    LeaseMgrFactory::instance().addLease(lease2);
 
-    // Check that the lease is indeed updated in LeaseMgr
-    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(addr);
-    ASSERT_TRUE(from_mgr);
+    AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false);
 
-    // Now check that the lease in LeaseMgr has the same parameters
-    detailCompareLease(lease, from_mgr);
+    // First client requests the lease which belongs to the second client.
+    AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_, IOAddress("192.0.2.102"),
+                                    false, false, "", false);
+    Lease4Ptr new_lease = engine.allocateLease4(ctx);
+    // Allocation engine should return NULL.
+    ASSERT_FALSE(new_lease);
+
+    // Now simulate the DHCPDISCOVER case when the provided address is
+    // treated as a hint. The engine should return a lease for a
+    // different address than requested.
+    ctx.fake_allocation_ = true;
+    new_lease = engine.allocateLease4(ctx);
+    ASSERT_TRUE(new_lease);
+    EXPECT_EQ("192.0.2.101", new_lease->addr_.toText());
 }
 
 // This test checks the behavior of the allocation engine in the following
@@ -790,8 +768,7 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLease) {
     EXPECT_EQ("192.0.2.123", allocated_lease->addr_.toText());
 
     // Make sure that the lease has been committed to the lease database.
-    Lease4Ptr from_mgr =
-        LeaseMgrFactory::instance().getLease4(allocated_lease->addr_);
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(allocated_lease->addr_);
     ASSERT_TRUE(from_mgr);
     detailCompareLease(allocated_lease, from_mgr);
 
@@ -863,13 +840,7 @@ TEST_F(AllocEngine4Test, reservedAddressHijacked) {
 // - Client B has a reservation for the address in use by client A.
 // - Client B sends a DHCPDISCOVER.
 // - Server determines that the reserved address is in use by a different client
-// and that it can't allocate a lease to the client B.
-//
-// In the scenario presented here, the allocation engine should return a
-// NULL lease to the server. When the server receives NULL pointer from the
-// allocation engine the proper action for the server will be to not
-// respond to the client. Instead it should report to the administrator
-// that it was unable to allocate the (reserved) lease.
+//   so it offers an address from the dynamic pool.
 TEST_F(AllocEngine4Test, reservedAddressHijackedFakeAllocation) {
     // Create a reservation for the client B.
     HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(),
@@ -894,9 +865,13 @@ TEST_F(AllocEngine4Test, reservedAddressHijackedFakeAllocation) {
                                     "", true);
     Lease4Ptr allocated_lease = engine.allocateLease4(ctx1);
 
-    // The allocation engine should return no lease.
-    ASSERT_FALSE(allocated_lease);
+    // The allocation engine should return a lease but for a different address
+    // than requested because this address is in use.
+    ASSERT_TRUE(allocated_lease);
     EXPECT_FALSE(ctx1.old_lease_);
+    EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.123");
+    EXPECT_TRUE(subnet_->inPool(Lease::TYPE_V4, allocated_lease->addr_));
+
 
     // Do the same test. But, this time do not specify any address to be
     // allocated.
@@ -905,7 +880,9 @@ TEST_F(AllocEngine4Test, reservedAddressHijackedFakeAllocation) {
                                     "", true);
     allocated_lease = engine.allocateLease4(ctx2);
 
-    EXPECT_FALSE(allocated_lease);
+    ASSERT_TRUE(allocated_lease);
+    EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.123");
+    EXPECT_TRUE(subnet_->inPool(Lease::TYPE_V4, allocated_lease->addr_));
     EXPECT_FALSE(ctx2.old_lease_);
 }
 
@@ -968,7 +945,7 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseInvalidHint) {
 // - Client has a reservation for a different address than the one for which it
 // has a lease.
 // - Client sends a DHCPDISCOVER and asks for a different address than reserved
-// and different from which it has a lease for.
+//   and different from which it has a lease for.
 // - Server ignores the client's hint and offers a reserved address.
 TEST_F(AllocEngine4Test, reservedAddressExistingLeaseFakeAllocation) {
     // Create a reservation for the client.
@@ -1075,8 +1052,8 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHint) {
 // - Client has a lease for a different address than reserved.
 // - Client sends a DHCPDISCOVER with no hint.
 // - Server determines that there is a reservation for the client and that
-// the current lease should be removed and the reserved address should be
-// allocated.
+//   the reserved address should be offered when the client sends a
+//   DHCPDISCOVER.
 TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
     // Create a reservation.
     HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(),
@@ -1113,6 +1090,8 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
     Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
     ASSERT_TRUE(from_mgr);
     detailCompareLease(lease, from_mgr);
+
+
 }
 
 // This test checks that the behavior of the allocation engine in the following
@@ -1153,12 +1132,13 @@ TEST_F(AllocEngine4Test, reservedAddressConflictResolution) {
 
 
     // Client B sends a DHCPREQUEST to allocate a reserved lease. The
-    // allocation engine declines allocation of the address for the
-    // client because Client A has a lease for it.
+    // allocation engine can't allocate a reserved lease for this client
+    // because this specific address is in use by the Client A.
     AllocEngine::ClientContext4 ctx1(subnet_, ClientIdPtr(), hwaddr2_,
                                     IOAddress("192.0.2.101"), false, false,
                                     "", false);
-    ASSERT_FALSE(engine.allocateLease4(ctx1));
+    Lease4Ptr offered_lease = engine.allocateLease4(ctx1);
+    ASSERT_FALSE(offered_lease);
 
     // Client A tries to renew the lease. The renewal should fail because
     // server detects that Client A doesn't have reservation for this
@@ -1168,8 +1148,7 @@ TEST_F(AllocEngine4Test, reservedAddressConflictResolution) {
                                     "", false);
     ASSERT_FALSE(engine.allocateLease4(ctx2));
 
-    ASSERT_TRUE(ctx2.old_lease_);
-    EXPECT_EQ("192.0.2.101", ctx2.old_lease_->addr_.toText());
+    ASSERT_FALSE(ctx2.old_lease_);
 
     // Client A returns to DHCPDISCOVER and should be offered a lease.
     // The offered lease address must be different than the one the
@@ -1177,11 +1156,11 @@ TEST_F(AllocEngine4Test, reservedAddressConflictResolution) {
     AllocEngine::ClientContext4 ctx3(subnet_, clientid_, hwaddr_,
                                     IOAddress("192.0.2.101"), false, false,
                                     "", true);
-    Lease4Ptr offered_lease = engine.allocateLease4(ctx3);
+    offered_lease = engine.allocateLease4(ctx3);
     ASSERT_TRUE(offered_lease);
     EXPECT_NE(offered_lease->addr_.toText(), "192.0.2.101");
 
-    // Client A tried to acquire the lease. It should succeed. At this point
+    // Client A tries to acquire the lease. It should succeed. At this point
     // the previous lease should be released and become available for the
     // Client B.
     AllocEngine::ClientContext4 ctx4(subnet_, clientid_, hwaddr_,
@@ -1235,6 +1214,10 @@ TEST_F(AllocEngine4Test, reservedAddressVsDynamicPool) {
 
     ASSERT_TRUE(allocated_lease);
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.100");
+
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(allocated_lease->addr_);
+    ASSERT_TRUE(from_mgr);
+    detailCompareLease(allocated_lease, from_mgr);
 }
 
 // This test checks that the client requesting an address which is
@@ -1272,7 +1255,7 @@ TEST_F(AllocEngine4Test, reservedAddressHintUsedByOtherClient) {
 }
 
 // This test checks that the allocation engine refuses to allocate an
-// address when the pool is exhausted, and the only one available
+// address when the pool is exhausted, and the only available
 // address is reserved for a different client.
 TEST_F(AllocEngine4Test, reservedAddressShortPool) {
     AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false);