From: Marcin Siodelski Date: Tue, 24 Feb 2015 16:25:56 +0000 (+0100) Subject: [3694] Cleanup in the failing DHCPv4 tests after update to AllocEngine. X-Git-Tag: trac3713_base~1^2~14 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=26a639e025d8cf7cb945d9fbe7e2d094dc91e65a;p=thirdparty%2Fkea.git [3694] Cleanup in the failing DHCPv4 tests after update to AllocEngine. --- diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc index 4fa3f2dfaf..232277223d 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.cc +++ b/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 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(); } diff --git a/src/bin/dhcp4/tests/dhcp4_client.h b/src/bin/dhcp4/tests/dhcp4_client.h index 4a3114a2da..8ba097ad9a 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.h +++ b/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_; diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 67be0d3501..6344774e04 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/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()); diff --git a/src/bin/dhcp4/tests/direct_client_unittest.cc b/src/bin/dhcp4/tests/direct_client_unittest.cc index f1af89a4a3..812846367c 100644 --- a/src/bin/dhcp4/tests/direct_client_unittest.cc +++ b/src/bin/dhcp4/tests/direct_client_unittest.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -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(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(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(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(client.getContext().response_->getType())); + EXPECT_EQ("10.0.0.10", client.config_.lease_.addr_.toText()); } } diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 64dcdb6b76..1b6fd5263a 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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)