From aa0ae8388d42cffa372ee94bc0941e6050e4e311 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Mon, 9 Feb 2015 15:46:09 +0100 Subject: [PATCH] [3677] alloc_new_leases_in_renewals_ added in AllocEngine --- src/bin/lfc/tests/lfc_controller_unittests.cc | 4 +-- src/lib/dhcpsrv/alloc_engine.cc | 34 +++++++++++++++++-- src/lib/dhcpsrv/alloc_engine.h | 17 ++++++++-- src/lib/dhcpsrv/dhcpsrv_messages.mes | 25 ++++++++++++++ src/lib/dhcpsrv/host.cc | 17 ++++++++++ src/lib/dhcpsrv/host.h | 8 +++++ .../dhcpsrv/tests/alloc_engine_unittest.cc | 30 ++++++++++++---- src/lib/dhcpsrv/tests/host_unittest.cc | 13 +++++++ 8 files changed, 134 insertions(+), 14 deletions(-) diff --git a/src/bin/lfc/tests/lfc_controller_unittests.cc b/src/bin/lfc/tests/lfc_controller_unittests.cc index 69de34dac4..d0cc808e3a 100644 --- a/src/bin/lfc/tests/lfc_controller_unittests.cc +++ b/src/bin/lfc/tests/lfc_controller_unittests.cc @@ -112,7 +112,7 @@ std::string LFCControllerTest::readFile(const std::string& filename) const { std::ifstream fs; - fs.open(filename, std::ifstream::in); + fs.open(filename.c_str(), std::ifstream::in); std::string contents((std::istreambuf_iterator(fs)), std::istreambuf_iterator()); fs.close(); @@ -122,7 +122,7 @@ LFCControllerTest::readFile(const std::string& filename) const { void LFCControllerTest::writeFile(const std::string& filename, const std::string& contents) const { - std::ofstream fs(filename, std::ofstream::out); + std::ofstream fs(filename.c_str(), std::ofstream::out); if (fs.is_open()) { fs << contents; diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 46f6f13354..6ade1237ec 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -687,6 +687,17 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, Lease6Collection& exis .arg(addr.toText()).arg(static_cast(prefix_len)) .arg(ctx.duid_->toText()); } + + // We found a lease for this client and this IA. Let's return. + // Returning after the first lease was assigned is useful if we + // have multiple reservations for the same client. If the client + // sends 2 IAs, the first time we call allocateReservedLeases6 will + // use the first reservation and return. The second time, we'll + // go over the first reservation, but will discover that there's + // a lease corresponding to it and will skip it and then pick + // the second reservation and turn it into the lease. This approach + // would work for any number of reservations. + return; } } } @@ -721,11 +732,25 @@ AllocEngine::removeNonmatchingReservedLeases6(ClientContext6& ctx, // Ok, we have a problem. This host has a lease that is reserved // for someone else. We need to recover from this. + if (ctx.type_ == Lease::TYPE_NA) { + LOG_INFO(dhcpsrv_logger, DHCPSRV_HR_REVOKED_ADDR6_LEASE) + .arg((*candidate)->addr_.toText()).arg(ctx.duid_->toText()) + .arg(host->getIdentifierAsText()); + } else { + LOG_INFO(dhcpsrv_logger, DHCPSRV_HR_REVOKED_PREFIX6_LEASE) + .arg((*candidate)->addr_.toText()) + .arg(static_cast((*candidate)->prefixlen_)) + .arg(ctx.duid_->toText()) + .arg(host->getIdentifierAsText()); + } // Remove this lease from LeaseMgr LeaseMgrFactory::instance().deleteLease((*candidate)->addr_); - /// @todo: Probably trigger a hook here + // In principle, we could trigger a hook here, but we will do this + // only if we get serious complaints from actual users. We want the + // conflict resolution procedure to really work and user libraries + // should not interfere with it. // Add this to the list of removed leases. ctx.old_leases_.push_back(*candidate); @@ -1673,7 +1698,10 @@ AllocEngine::renewLeases6(ClientContext6& ctx) { } // If we happen to removed all leases, get something new for this guy. - if (leases.empty()) { + // Depending on the configuration, we may enable or disable granting + // new leases during renewals. This is controlled with the + // allow_new_leases_in_renewals_ field. + if (leases.empty() && ctx.allow_new_leases_in_renewals_) { leases = allocateUnreservedLeases6(ctx); } @@ -1687,7 +1715,7 @@ AllocEngine::renewLeases6(ClientContext6& ctx) { } catch (const isc::Exception& e) { // Some other error, return an empty lease. - LOG_ERROR(dhcpsrv_logger, DHCPSRV_ADDRESS6_ALLOC_ERROR).arg(e.what()); + LOG_ERROR(dhcpsrv_logger, DHCPSRV_RENEW6_ERROR).arg(e.what()); } return (Lease6Collection()); diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index d6b0549283..c9a65055b7 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -418,12 +418,24 @@ protected: /// @brief A pointer to the IA_NA/IA_PD option to be sent in response Option6IAPtr ia_rsp_; + + /// @brief Specifies whether new leases in Renew/Rebind are allowed + /// + /// This field controls what to do when renewing or rebinding client + /// does not have any leases. RFC3315 and stateful-issues draft does not + /// specify it and it is left up to the server configuration policy. + /// False (the default) means that the client will not get any new + /// unreserved leases if his existing leases are no longer suitable. + /// True means that the allocation engine will do its best to assign + /// something. + bool allow_new_leases_in_renewals_; + /// @brief Default constructor. ClientContext6() : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(), hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""), callout_handle_(), fake_allocation_(false), old_leases_(), host_(), - query_(), ia_rsp_() { + query_(), ia_rsp_(), allow_new_leases_in_renewals_(false) { } /// @brief Constructor with parameters. @@ -454,7 +466,8 @@ protected: subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(), hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns), hostname_(hostname), fake_allocation_(fake_allocation), - old_leases_(), host_(), query_(), ia_rsp_() { + old_leases_(), host_(), query_(), ia_rsp_(), + allow_new_leases_in_renewals_(false){ static asiolink::IOAddress any("::"); diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index ae160c4138..5089f07624 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -225,6 +225,24 @@ This informational message singals that there is a reservation for this client and this client got this specific prefix. This is normal operation for host reservation. +% DHCPSRV_HR_REVOKED_ADDR6_LEASE Address %1 was revoked from client %2 as it is reserved for client %3 +This informational message is an indication that the specified IPv6 address used +to be used by client A, but it is now reserved for client B. As such, client +A was told to stop using it, so it could be leased to client B. This is +normal operation when doing conflict resolution, i.e. system administrator +added reservation for an address that is currently in use by someone other +than the reservation is for. The server will fully recover from this situation, +but clients will experience address change. + +% DHCPSRV_HR_REVOKED_PREFIX6_LEASE Prefix %1/%2 was revoked from client %3 as it is reserved for client %4 +This informational message is an indication that the specified IPv6 address used +to be used by client A, but it is now reserved for client B. As such, client +A was told to stop using it, so it could be leased to client B. This is +normal operation when doing conflict resolution, i.e. system administrator +added reservation for an address that is currently in use by someone other +than the reservation is for. The server will fully recover from this situation, +but clients will experience address change. + % DHCPSRV_INVALID_ACCESS invalid database access string: %1 This is logged when an attempt has been made to parse a database access string and the attempt ended in error. The access string in question - which @@ -519,6 +537,13 @@ lease from the PostgreSQL database for the specified address. A debug message issued when the server is attempting to update IPv6 lease from the PostgreSQL database for the specified address. +% DHCPSRV_RENEW6_ERROR allocation engine experienced error with attempting to renew a lease: %1 +This error message indicates that an error was experienced during Renew or Rebind +processing. Additional explanation is provided with this message. Depending +on its nature, manual intervention may be required to continue processing +messages from this particular client. The server will handle other clients +normally. + % DHCPSRV_UNEXPECTED_NAME database access parameters passed through '%1', expected 'lease-database' The parameters for access the lease database were passed to the server through the named configuration parameter, but the code was expecting them to be diff --git a/src/lib/dhcpsrv/host.cc b/src/lib/dhcpsrv/host.cc index 75977d9b47..6f5a0a4926 100644 --- a/src/lib/dhcpsrv/host.cc +++ b/src/lib/dhcpsrv/host.cc @@ -230,5 +230,22 @@ Host::addClientClassInternal(ClientClasses& classes, } } +std::string +Host::getIdentifierAsText() const { + std::string txt; + if (hw_address_) { + txt = "hwaddr=" + hw_address_->toText(false); + } else { + txt = "duid="; + if (duid_) { + txt += duid_->toText(); + } else { + txt += "(none)"; + } + } + + return (txt); +} + } // end of namespace isc::dhcp } // end of namespace isc diff --git a/src/lib/dhcpsrv/host.h b/src/lib/dhcpsrv/host.h index 8d597148e9..3b1b606a08 100644 --- a/src/lib/dhcpsrv/host.h +++ b/src/lib/dhcpsrv/host.h @@ -295,10 +295,18 @@ public: return (duid_); } + /// @brief Returns the identifier (MAC or DUID) in binary form. + /// @return const reference to MAC or DUID in vector form const std::vector& getIdentifier() const; + /// @brief Returns the identifier type. + /// @return the identifier type IdentifierType getIdentifierType() const; + /// @brief Returns host identifier (mac or DUID) in printer friendly form. + /// @return text form of the identifier, including (duid= or mac=). + std::string getIdentifierAsText() const; + /// @brief Sets new IPv4 subnet identifier. /// /// @param ipv4_subnet_id New subnet identifier. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc index ce75253439..b4bdd0616a 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc @@ -299,10 +299,12 @@ public: return (lease); } - /// @brief Checks if the simple allocation can succeed + /// @brief Checks if the allocation can succeed. /// - /// The type of lease is determined by pool type (pool->getType() + /// The type of lease is determined by pool type (pool->getType()). + /// This test is particularly useful in connection with @ref renewTest. /// + /// @param engine a reference to Allocation Engine /// @param pool pool from which the lease will be allocated from /// @param hint address to be used as a hint /// @param fake true - this is fake allocation (SOLICIT) @@ -353,8 +355,21 @@ public: return (leases); } + /// @brief Checks if the allocation can be renewed. + /// + /// The type of lease is determined by pool type (pool->getType()). + /// This test is particularly useful as a follow up to @ref allocateTest. + /// + /// @param engine a reference to Allocation Engine + /// @param pool pool from which the lease will be allocated from + /// @param hint address to be used as a hint + /// @param fake true - this is fake allocation (SOLICIT) + /// @param in_pool specifies whether the lease is expected to be in pool + /// @return allocated lease(s) (may be empty) Lease6Collection renewTest(AllocEngine& engine, const Pool6Ptr& pool, - AllocEngine::HintContainer& hints, bool in_pool = true) { + AllocEngine::HintContainer& hints, + bool allow_new_leases_in_renewal, + bool in_pool = true) { Lease::Type type = pool->getType(); uint8_t expected_len = pool->getLength(); @@ -363,6 +378,7 @@ public: type, false, false, "", false); ctx.hints_ = hints; ctx.query_.reset(new Pkt6(DHCPV6_RENEW, 123)); + ctx.allow_new_leases_in_renewals_ = allow_new_leases_in_renewal; Lease6Collection leases = engine.renewLeases6(ctx); @@ -1632,7 +1648,7 @@ TEST_F(AllocEngine6Test, addressRenewal) { AllocEngine::HintContainer hints; hints.push_back(make_pair(leases[0]->addr_, 128)); - Lease6Collection renewed = renewTest(engine, pool_, hints); + Lease6Collection renewed = renewTest(engine, pool_, hints, true); ASSERT_EQ(1, renewed.size()); // Check that the lease was indeed renewed and hasn't changed @@ -1663,7 +1679,7 @@ TEST_F(AllocEngine6Test, reservedAddressRenewal) { AllocEngine::HintContainer hints; hints.push_back(make_pair(leases[0]->addr_, 128)); - Lease6Collection renewed = renewTest(engine, pool_, hints); + Lease6Collection renewed = renewTest(engine, pool_, hints, true); ASSERT_EQ(1, renewed.size()); ASSERT_EQ("2001:db8:1::1c", leases[0]->addr_.toText()); } @@ -1688,7 +1704,7 @@ TEST_F(AllocEngine6Test, reservedAddressRenewChange) { // as the pool is 2001:db8:1::10 - 2001:db8:1::20. createHost6(true, IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::1c"), 128); - Lease6Collection renewed = renewTest(engine, pool_, hints); + Lease6Collection renewed = renewTest(engine, pool_, hints, true); ASSERT_EQ(1, renewed.size()); ASSERT_EQ("2001:db8:1::1c", renewed[0]->addr_.toText()); } @@ -1719,7 +1735,7 @@ TEST_F(AllocEngine6Test, reservedAddressRenewReserved) { CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host); CfgMgr::instance().commit(); - Lease6Collection renewed = renewTest(engine, pool_, hints); + Lease6Collection renewed = renewTest(engine, pool_, hints, true); ASSERT_EQ(1, renewed.size()); // Check that we no longer have the reserved address. diff --git a/src/lib/dhcpsrv/tests/host_unittest.cc b/src/lib/dhcpsrv/tests/host_unittest.cc index e80e46dac9..0102785032 100644 --- a/src/lib/dhcpsrv/tests/host_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_unittest.cc @@ -500,4 +500,17 @@ TEST(HostTest, addClientClasses) { EXPECT_TRUE(host->getClientClasses6().contains("bar")); } +TEST(HostTest, getIdentifierAsText) { + Host host1("01:02:03:04:05:06", "hw-address", + SubnetID(1), SubnetID(2), + IOAddress("192.0.2.3")); + EXPECT_EQ("hwaddr=01:02:03:04:05:06", host1.getIdentifierAsText()); + + Host host2("0a:0b:0c:0d:0e:0f:ab:cd:ef", "duid", + SubnetID(1), SubnetID(2), + IOAddress("192.0.2.3")); + EXPECT_EQ("duid=0a:0b:0c:0d:0e:0f:ab:cd:ef", + host2.getIdentifierAsText()); +} + } // end of anonymous namespace -- 2.47.3