From 8b8d69fe910b96caac7794616e8d427f60d5d3c4 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 6 Feb 2015 12:27:20 +0100 Subject: [PATCH] [3690] Added unit test to cover change in allocation engine. Also, further fixes in allocation engine were needed to correctly handle the case when a client is requesting an address which belongs to someone else. --- src/lib/dhcpsrv/alloc_engine.cc | 30 +++++++++++----- .../dhcpsrv/tests/alloc_engine_unittest.cc | 36 +++++++++++++++++++ 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index e306eb76fb..88d4c243d6 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -606,14 +606,28 @@ 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_; - - if (ctx.host_ || - (!ctx.host_ && !HostMgr::instance().get4(ctx.subnet_->getID(), - candidate))) { + // 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(); + + } else { + 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); diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc index 55ffe109cc..9a98fcfc7e 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc @@ -2263,6 +2263,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"); +} + /// @brief helper class used in Hooks testing in AllocEngine6 /// /// It features a couple of callout functions and buffers to store -- 2.47.3