[3694] Cleanup in the failing DHCPv4 tests after update to AllocEngine.

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -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>
@@ -310,47 +311,23 @@ TEST_F(DirectClientTest, renew) {
     // 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
@@ -370,54 +347,31 @@ TEST_F(DirectClientTest, rebind) {
     // 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);
+    // 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());
 
-    // 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);
-
-    // 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/lib/dhcpsrv/alloc_engine.cc

@@ -1293,7 +1293,11 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) {
         }
     }
 
-    if (ctx.requested_address_.isV4Zero() && !addressReserved(ctx.requested_address_, ctx)) {
+    if (client_lease && !addressReserved(ctx.requested_address_, ctx)) {
+        return (renewLease4(client_lease, ctx));
+    }
+
+    if (!ctx.requested_address_.isV4Zero() && !addressReserved(ctx.requested_address_, ctx)) {
         if (ctx.subnet_->inPool(Lease::TYPE_V4, ctx.requested_address_)) {
             new_lease = allocateOrReuseLease(ctx.requested_address_, ctx);
             if (new_lease) {
@@ -1303,13 +1307,6 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) {
                 return (new_lease);
             }
         }
-
-        if (client_lease) {
-            if ((client_lease->addr_ == ctx.requested_address_) ||
-                ctx.requested_address_.isV4Zero()) {
-                return (renewLease4(client_lease, ctx));
-            }
-        }
     }
 
     AllocatorPtr allocator = getAllocator(Lease::TYPE_V4);
@@ -1499,23 +1496,6 @@ AllocEngine::renewLease4(const Lease4Ptr& lease,
         isc_throw(BadValue, "null lease specified for renewLease4");
     }
 
-    // The ctx.host_ possibly contains a reservation for the client for which
-    // we are renewing a lease. If this reservation exists, we assume that
-    // there is no conflict in assigning the address to this client. Note
-    // that the reallocateClientLease checks if the address reserved for
-    // the client matches the address in the lease we're renewing here.
-    if (!ctx.host_) {
-        // Do not renew the lease if:
-        // - If address is reserved for someone else or ...
-        // - renewed address doesn't belong to a pool.
-        if (addressReserved(lease->addr_, ctx) ||
-            (!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
     // (the lease returned points directly to the lease4 object in the database)
     // We'll need it if we want to skip update (i.e. roll back renewal)