[3564] Implemented tests for address conflicts resolution.

The new tests are implemented using the Dhcp4Client class. It has been
found that the server returns DHCPNAK in response to DHCPDISCOVER which
was wrong. Now, the server doesn't respond to DHCPDISCOVER if it can't
find an address. As a result, a number of tests had to be fixed which
relied on this invalid behavior.

src/bin/dhcp4/dhcp4_srv.cc

@@ -1312,6 +1312,11 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
         // include in the response. If client did not request
         // them we append them for him.
         appendBasicOptions(discover, offer);
+
+    } else {
+        // If the server can't offer an address, it drops the packet.
+        return (Pkt4Ptr());
+
     }
 
     // Set the src/dest IP address, port and interface for the outgoing

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -57,7 +57,7 @@ Dhcp4Client::Dhcp4Client(const Dhcp4Client::State& state) :
     use_relay_(false) {
 }
 
-Dhcp4Client::Dhcp4Client(boost::shared_ptr<NakedDhcpv4Srv>& srv,
+Dhcp4Client::Dhcp4Client(boost::shared_ptr<NakedDhcpv4Srv> srv,
                          const Dhcp4Client::State& state) :
     config_(),
     ciaddr_(IOAddress("0.0.0.0")),

src/bin/dhcp4/tests/dhcp4_client.h

@@ -103,7 +103,7 @@ public:
     ///
     /// @param srv An instance of the DHCPv4 server to be used.
     /// @param state Initial client's state.
-    Dhcp4Client(boost::shared_ptr<NakedDhcpv4Srv>& srv,
+    Dhcp4Client(boost::shared_ptr<NakedDhcpv4Srv> srv,
                 const State& state = SELECTING);
 
     /// @brief Creates a lease for the client using the specified address
@@ -203,7 +203,9 @@ public:
     HWAddrPtr generateHWAddr(const uint8_t htype = HTYPE_ETHER) const;
 
     /// @brief Returns HW address used by the client.
-    HWAddrPtr getHWAddress() const;
+    HWAddrPtr getHWAddress() const {
+        return (hwaddr_);
+    }
 
     /// @brief Returns current context.
     const Context& getContext() const {

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -1136,6 +1136,25 @@ TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho) {
 
     NakedDhcpv4Srv srv(0);
 
+    // Use of the captured DHCPDISCOVER packet requires that
+    // subnet 10.254.226.0/24 is in use, because this packet
+    // contains the giaddr which belongs to this subnet and
+    // this giaddr is used to select the subnet
+    std::string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pools\": [ { \"pool\": \"10.254.226.0/25\" } ],"
+        "    \"subnet\": \"10.254.226.0/24\", "
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"valid-lifetime\": 4000,"
+        "    \"interface\": \"eth0\" "
+        " } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    configure(config);
+
     // Let's create a relayed DISCOVER. This particular relayed DISCOVER has
     // added option 82 (relay agent info) with 3 suboptions. The server
     // is supposed to echo it back in its response.
@@ -1610,7 +1629,7 @@ public:
             192, 0, 2, 1,           // ciaddr
             1, 2, 3, 4,             // yiaddr
             192, 0, 2, 255,         // siaddr
-            255, 255, 255, 255,     // giaddr
+            192, 0, 2, 50,          // giaddr
         };
 
         // Initialize the vector with the header fields defined above.
@@ -2041,7 +2060,6 @@ TEST_F(HooksDhcpv4SrvTest, buffer4ReceiveValueChange) {
     IfaceMgrTestConfig test_config(true);
     IfaceMgr::instance().openSockets4();
 
-
     // Install callback that modifies MAC addr of incoming packet
     EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
                         "buffer4_receive", buffer4_receive_change_hwaddr));

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -545,31 +545,33 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
 
     if (msg_type == DHCPDISCOVER) {
         ASSERT_NO_THROW(rsp = srv->processDiscover(received));
-        // Should return non-NULL packet.
-        ASSERT_TRUE(rsp);
+        // Should return NULL packet.
+        ASSERT_FALSE(rsp);
+
     } else {
         ASSERT_NO_THROW(rsp = srv->processRequest(received));
         // Should return non-NULL packet.
         ASSERT_TRUE(rsp);
+        // We should get the NAK packet with yiaddr set to 0.
+        EXPECT_EQ(DHCPNAK, rsp->getType());
+        ASSERT_EQ("0.0.0.0", rsp->getYiaddr().toText());
+
+        // Make sure that none of the requested options is returned in NAK.
+        // Also options such as Routers or Subnet Mask should not be there,
+        // because lease hasn't been acquired.
+        EXPECT_TRUE(noRequestedOptions(rsp));
+        EXPECT_TRUE(noBasicOptions(rsp));
     }
-    // We should get the NAK packet with yiaddr set to 0.
-    EXPECT_EQ(DHCPNAK, rsp->getType());
-    ASSERT_EQ("0.0.0.0", rsp->getYiaddr().toText());
-
-    // Make sure that none of the requested options is returned in NAK.
-    // Also options such as Routers or Subnet Mask should not be there,
-    // because lease hasn't been acquired.
-    EXPECT_TRUE(noRequestedOptions(rsp));
-    EXPECT_TRUE(noBasicOptions(rsp));
 }
 
 void
-Dhcpv4SrvTest::configure(const std::string& config) {
-    configure(config, srv_);
+Dhcpv4SrvTest::configure(const std::string& config, const bool commit) {
+    configure(config, srv_, commit);
 }
 
 void
-Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv) {
+Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv,
+                         const bool commit) {
     ElementPtr json = Element::fromJSON(config);
     ConstElementPtr status;
 
@@ -580,7 +582,9 @@ Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv) {
     ConstElementPtr comment = config::parseAnswer(rcode, status);
     ASSERT_EQ(0, rcode);
 
-    CfgMgr::instance().commit();
+    if (commit) {
+        CfgMgr::instance().commit();
+    }
  }
 
 

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -405,13 +405,18 @@ public:
     /// @brief Runs DHCPv4 configuration from the JSON string.
     ///
     /// @param config String holding server configuration in JSON format.
-    void configure(const std::string& config);
+    /// @param commit A boolean flag indicating if the new configuration
+    /// should be committed (if true), or not (if false).
+    void configure(const std::string& config, const bool commit = true);
 
     /// @brief Configure specified DHCP server using JSON string.
     ///
     /// @param config String holding server configuration in JSON format.
     /// @param srv Instance of the server to be configured.
-    void configure(const std::string& config, NakedDhcpv4Srv& srv);
+    /// @param commit A boolean flag indicating if the new configuration
+    /// should be committed (if true), or not (if false).
+    void configure(const std::string& config, NakedDhcpv4Srv& srv,
+                   const bool commit = true);
 
     /// @brief This function cleans up after the test.
     virtual void TearDown();

src/bin/dhcp4/tests/dora_unittest.cc

@@ -17,6 +17,10 @@
 #include <cc/data.h>
 #include <dhcp/dhcp4.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
+#include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/host.h>
+#include <dhcpsrv/host_mgr.h>
+#include <dhcpsrv/subnet_id.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
 #include <dhcp4/tests/dhcp4_client.h>
 #include <boost/shared_ptr.hpp>
@@ -53,6 +57,7 @@ const char* DORA_CONFIGS[] = {
         "\"valid-lifetime\": 600,"
         "\"subnet4\": [ { "
         "    \"subnet\": \"10.0.0.0/24\", "
+        "    \"id\": 1,"
         "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
         "    \"option-data\": [ {"
         "        \"name\": \"routers\","
@@ -402,4 +407,206 @@ TEST_F(DORATest, ciaddr) {
     EXPECT_EQ("0.0.0.0", resp->getCiaddr().toText());
 }
 
+// This test checks the following scenario:
+// - Client A performs 4-way exchange and obrains a lease from the dynamic pool.
+// - Reservation is created for the client A using an address out of the dynamic
+//   pool.
+// - Client A renews the lease.
+// - Server responds with DHCPNAK to indicate that the client should stop using
+//   an address for which it has a lease. Server doesn't want to renew an
+//   address for which the client doesn't have a reservation, while it has
+//   a reservation for a different address.
+// - Client A receives a DHCPNAK and returns to the DHCP server discovery.
+// - Client A performs a 4-way exchange with a server and the server allocates
+//   a reserved address to the Client A.
+// - Client A renews the allocated address and the server returns a DHCPACK.
+// - Reservation for the Client A is removed.
+// - Client A renews the (previously reserved) lease and the server returns
+//   DHCPNAK because the address in use is neither reserved nor belongs to
+//   the dynamic pool.
+// - Client A returns to the DHCP server discovery.
+// - Client A uses 4-way exchange to obtain a lease from the dynamic pool.
+// - The new address that the Client A is using is reserved for Client B.
+//   Client A still holds this address.
+// - Client B uses 4-way exchange to obtain a new lease.
+// - 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.
+// - Client A renews the lease.
+// - The server determines that the address that Client A is using is reserved
+//   for Client B. The server returns DHCPNAK to the Client A.
+// - Client B uses 4-way echange 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.
+// - Client A uses 4-way exchange to allocate a new lease.
+// - The server allocates a new lease from the dynamic pool but it avoids
+//   allocating the address reserved for the Client B.
+// - Client B uses 4-way exchange to obtain a new lease.
+// - The server finally allocates a reserved address to the Client B.
+TEST_F(DORATest, reservations) {
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[0], *client.getServer());
+    // Client A performs 4-way exchange and obrains a lease from the
+    // dynamic pool.
+    ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("10.0.0.50"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    // Response must not be relayed.
+    EXPECT_FALSE(resp->isRelayed());
+    // Make sure that the server id is present.
+    EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText());
+    // Make sure that the client has got the lease with the requested address.
+    ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText());
+
+    configure(DORA_CONFIGS[0], false);
+    // Reservation is created for the client A using an address out of the
+    // dynamic pool.
+    HostPtr host(new Host(&client.getHWAddress()->hwaddr_[0],
+                          client.getHWAddress()->hwaddr_.size(),
+                          Host::IDENT_HWADDR, SubnetID(1),
+                          SubnetID(0), IOAddress("10.0.0.9")));
+    CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host);
+    CfgMgr::instance().commit();
+
+    // Let's transition the client to Renewing state.
+    client.setState(Dhcp4Client::RENEWING);
+
+    // Set the unicast destination address to indicate that it is a renewal.
+    client.setDestAddress(IOAddress("10.0.0.1"));
+    ASSERT_NO_THROW(client.doRequest());
+
+    // Client should get the DHCPNAK from the server because the client has
+    // a reservation for a different address that it is trying to renew.
+    resp = client.getContext().response_;
+    ASSERT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
+
+    // A conforming client would go back to the server discovery.
+    client.setState(Dhcp4Client::SELECTING);
+    // Obtain a lease from the server using the 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("0.0.0.0"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK with a reserved
+    // address
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    ASSERT_EQ("10.0.0.9", client.config_.lease_.addr_.toText());
+
+    // Client A renews the allocated address.
+    client.setState(Dhcp4Client::RENEWING);
+    // Set the unicast destination address to indicate that it is a renewal.
+    client.setDestAddress(IOAddress("10.0.0.1"));
+    ASSERT_NO_THROW(client.doRequest());
+    // Make sure the server responded and renewed the client's address.
+    resp = client.getContext().response_;
+    ASSERT_EQ("10.0.0.9", client.config_.lease_.addr_.toText());
+
+    // By reconfiguring the server, we remove the existing reservations.
+    configure(DORA_CONFIGS[0]);
+
+    // Try to renew the existing lease again.
+    ASSERT_NO_THROW(client.doRequest());
+    // The reservation has been removed, so the server should respond with
+    // a DHCPNAK because the address that the client is using doesn't belong
+    // to a dynamic pool.
+    resp = client.getContext().response_;
+    ASSERT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
+
+    // A conforming client would go back to the server discovery.
+    client.setState(Dhcp4Client::SELECTING);
+    // Obtain a lease from the server using the 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("0.0.0.0"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    // Obtain the subnet to which the returned address belongs.
+    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->
+        selectSubnet(client.config_.lease_.addr_);
+    ASSERT_TRUE(subnet);
+    // Make sure that the address has been allocated from the dynamic pool.
+    ASSERT_TRUE(subnet->inPool(Lease::TYPE_V4, client.config_.lease_.addr_));
+
+    // Remember the address allocated in the dynamic pool.
+    IOAddress in_pool_addr = client.config_.lease_.addr_;
+
+    // Create Client B.
+    Dhcp4Client clientB(client.getServer());
+    clientB.modifyHWAddr();
+
+    // Create reservation for the Client B, for the address that the
+    // Client A is using.
+    configure(DORA_CONFIGS[0], false);
+    host.reset(new Host(&clientB.getHWAddress()->hwaddr_[0],
+                        clientB.getHWAddress()->hwaddr_.size(),
+                        Host::IDENT_HWADDR, SubnetID(1),
+                        SubnetID(0), in_pool_addr));
+    CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host);
+    CfgMgr::instance().commit();
+
+    // 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_);
+
+    // Client A renews the lease.
+    client.setState(Dhcp4Client::RENEWING);
+    // Set the unicast destination address to indicate that it is a renewal.
+    client.setDestAddress(IOAddress(in_pool_addr));
+    ASSERT_NO_THROW(client.doRequest());
+    // Client A should get a DHCPNAK because it is using an address reserved
+    // for Client B.
+    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.
+    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"))));
+    // Make sure that the server responded.
+    ASSERT_FALSE(clientB.getContext().response_);
+
+    // Client A performs 4-way exchange.
+    client.setState(Dhcp4Client::SELECTING);
+    // Obtain a lease from the server using the 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("0.0.0.0"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    resp = client.getContext().response_;
+    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());
+    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 performs 4-way exchange and obtains a lease for the
+    // reserved address.
+    clientB.setState(Dhcp4Client::SELECTING);
+    ASSERT_NO_THROW(clientB.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("0.0.0.0"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(clientB.getContext().response_);
+    resp = clientB.getContext().response_;
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    ASSERT_EQ(in_pool_addr, clientB.config_.lease_.addr_);
+}
+
 } // end of anonymous namespace

src/lib/dhcpsrv/alloc_engine.cc

@@ -733,10 +733,19 @@ AllocEngine::renewLease4(const Lease4Ptr& lease,
     if (!ctx.host_) {
         ConstHostPtr host = HostMgr::instance().get4(ctx.subnet_->getID(),
                                                      lease->addr_);
-        if (host && ctx.hwaddr_ && (*host->getHWAddress() != *ctx.hwaddr_)) {
+/*        if ((host && ctx.hwaddr_ && (*host->getHWAddress() != *ctx.hwaddr_)) ||
+            ((ctx.requested_address_ != IOAddress("0.0.0.0")) &&
+             !ctx.subnet_->inPool(Lease::TYPE_V4, requested_address_))) {
+            ctx.interrupt_processing_ = !ctx.fake_allocation_;
+            return (Lease4Ptr());
+        } */
+
+        if ((host && ctx.hwaddr_ && (*host->getHWAddress() != *ctx.hwaddr_)) ||
+            (!ctx.subnet_->inPool(Lease::TYPE_V4, lease->addr_))) {
             ctx.interrupt_processing_ = !ctx.fake_allocation_;
             return (Lease4Ptr());
         }
+
     }
 
     // Let's keep the old data. This is essential if we are using memfile
@@ -967,6 +976,10 @@ AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
     }
 
     IOAddress prev_address = lease->addr_;
+    if (!ctx.fake_allocation_) {
+        LeaseMgrFactory::instance().deleteLease(prev_address);
+    }
+
     if (!ctx.host_) {
         ConstHostPtr host = HostMgr::instance().get4(ctx.subnet_->getID(),
                                                      ctx.requested_address_);
@@ -978,10 +991,8 @@ AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
 
     } else {
         lease->addr_ = ctx.host_->getIPv4Reservation();
-
     }
 
-
     updateLease4Information(lease, ctx);
 
     // Execute callouts registered for lease4_select.
@@ -1014,7 +1025,6 @@ AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
     }
 
     if (!ctx.fake_allocation_) {
-        LeaseMgrFactory::instance().deleteLease(prev_address);
         LeaseMgrFactory::instance().addLease(lease);
     }