From c05631f3978625fe82837cee765e23ea9c220d77 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 24 Feb 2015 18:53:32 +0100 Subject: [PATCH] [3694] Fixed the unit test for the conflict resolution in DHCPv4. --- src/bin/dhcp4/tests/dora_unittest.cc | 42 +++++++++++++++++++++------- src/lib/dhcpsrv/alloc_engine.cc | 22 +++++++++------ 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 8fa177ef83..591cab61e1 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -629,10 +629,12 @@ 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(clientB.getContext().response_->getType())); + ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr); // Client A renews the lease. client.setState(Dhcp4Client::RENEWING); @@ -644,15 +646,27 @@ TEST_F(DORATest, reservationsWithConflicts) { resp = client.getContext().response_; ASSERT_EQ(DHCPNAK, static_cast(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(clientB.getContext().response_->getType())); + ASSERT_NE(clientB.config_.lease_.addr_, in_pool_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(clientB.getContext().response_->getType())); + ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr); // Client A performs 4-way exchange. client.setState(Dhcp4Client::SELECTING); @@ -673,6 +687,14 @@ TEST_F(DORATest, reservationsWithConflicts) { 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(clientB.getContext().response_->getType())); + // Client B performs 4-way exchange and obtains a lease for the // reserved address. clientB.setState(Dhcp4Client::SELECTING); diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 1b6fd5263a..37fd7ae44e 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1293,7 +1293,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { } } - if (client_lease && !addressReserved(ctx.requested_address_, ctx)) { + if (client_lease && !addressReserved(ctx.requested_address_, ctx) && + ctx.subnet_->inPool(Lease::TYPE_V4, ctx.requested_address_)) { return (renewLease4(client_lease, ctx)); } @@ -1349,15 +1350,13 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { return (Lease4Ptr()); } - } else if (ctx.host_ && (ctx.host_->getIPv4Reservation() != ctx.requested_address_)) { - return (Lease4Ptr()); } - } - if (client_lease) { - if ((client_lease->addr_ == ctx.requested_address_) || - ctx.requested_address_.isV4Zero()) { - return (renewLease4(client_lease, ctx)); + if (ctx.host_ && (ctx.host_->getIPv4Reservation() != ctx.requested_address_)) { + existing = LeaseMgrFactory::instance().getLease4(ctx.host_->getIPv4Reservation()); + if (!existing || existing->expired()) { + return (Lease4Ptr()); + } } } @@ -1368,6 +1367,13 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { } } + if (client_lease) { + if ((client_lease->addr_ == ctx.requested_address_) || + ctx.requested_address_.isV4Zero()) { + return (renewLease4(client_lease, ctx)); + } + } + Lease4Ptr new_lease; if (!ctx.requested_address_.isV4Zero()) { new_lease = allocateOrReuseLease(ctx.requested_address_, ctx); -- 2.47.2