diff --git a/ChangeLog b/ChangeLog index c4ae5dac73..2b71b7dea7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +1820. [bug] razvan + Fixed atomic lease update when using hooks with database back-end. + (Gitlab #1434) + 1819. [func] fdupont Improved error messages for bad escapes in JSON strings. (Gitlab #151) diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index 041bc34470..596d21f775 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -1699,8 +1699,8 @@ HAService::asyncSyncLeasesInternal(http::HttpClient& http_client, } else if (existing_lease->cltt_ < lease->cltt_) { // If the existing lease is older than the fetched lease, update // the lease in our local database. - lease->old_cltt_ = existing_lease->old_cltt_; - lease->old_valid_lft_ = existing_lease->old_valid_lft_; + // Update lease information with values received from the database. + Lease::syncInternalTimestamp(*lease, *existing_lease); LeaseMgrFactory::instance().updateLease4(lease); } else { @@ -1730,8 +1730,8 @@ HAService::asyncSyncLeasesInternal(http::HttpClient& http_client, } else if (existing_lease->cltt_ < lease->cltt_) { // If the existing lease is older than the fetched lease, update // the lease in our local database. - lease->old_cltt_ = existing_lease->old_cltt_; - lease->old_valid_lft_ = existing_lease->old_valid_lft_; + // Update lease information with values received from the database. + Lease::syncInternalTimestamp(*lease, *existing_lease); LeaseMgrFactory::instance().updateLease6(lease); } else { diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index 261265032e..322142c237 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -655,10 +655,18 @@ LeaseCmdsImpl::addOrUpdate4(Lease4Ptr lease, bool force_create) { return (true); } if (existing) { - lease->old_cltt_ = existing->old_cltt_; - lease->old_valid_lft_ = existing->old_valid_lft_; + // Update lease information with values received from the database. + Lease::syncInternalTimestamp(*lease, *existing); } - LeaseMgrFactory::instance().updateLease4(lease); + try { + LeaseMgrFactory::instance().updateLease4(lease); + } catch (const NoSuchLease&) { + isc_throw(InvalidOperation, "failed to update the lease with address " + << lease->addr_ << " either because the lease has been " + "deleted or it has changed in the database, in both cases a " + "retry might succeed"); + } + LeaseCmdsImpl::updateStatsOnUpdate(existing, lease); return (false); } @@ -677,10 +685,18 @@ LeaseCmdsImpl::addOrUpdate6(Lease6Ptr lease, bool force_create) { return (true); } if (existing) { - lease->old_cltt_ = existing->old_cltt_; - lease->old_valid_lft_ = existing->old_valid_lft_; + // Update lease information with values received from the database. + Lease::syncInternalTimestamp(*lease, *existing); } - LeaseMgrFactory::instance().updateLease6(lease); + try { + LeaseMgrFactory::instance().updateLease6(lease); + } catch (const NoSuchLease&) { + isc_throw(InvalidOperation, "failed to update the lease with address " + << lease->addr_ << " either because the lease has been " + "deleted or it has changed in the database, in both cases a " + "retry might succeed"); + } + LeaseCmdsImpl::updateStatsOnUpdate(existing, lease); return (false); } diff --git a/src/lib/dhcpsrv/cql_lease_mgr.cc b/src/lib/dhcpsrv/cql_lease_mgr.cc index d85587f494..ec3910dfd7 100644 --- a/src/lib/dhcpsrv/cql_lease_mgr.cc +++ b/src/lib/dhcpsrv/cql_lease_mgr.cc @@ -1596,6 +1596,7 @@ CqlLease6Exchange::retrieve() { time_t cltt = 0; CqlExchange::convertFromDatabaseTime(expire_, valid_lifetime_, cltt); + // Update cltt_ and old_cltt_ explicitly. result->cltt_ = cltt; result->old_cltt_ = cltt; @@ -2145,8 +2146,10 @@ CqlLeaseMgr::addLease(const Lease4Ptr &lease) { return false; } - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values (allows update of the + // internal state between the creation of the Lease up to the point of + // insertion in the database). + lease->updateInternalTimestamp(); return true; } @@ -2168,8 +2171,10 @@ CqlLeaseMgr::addLease(const Lease6Ptr &lease) { return false; } - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values (allows update of the + // internal state between the creation of the Lease up to the point of + // insertion in the database). + lease->updateInternalTimestamp(); return true; } @@ -2618,8 +2623,8 @@ CqlLeaseMgr::updateLease4(const Lease4Ptr &lease) { isc_throw(NoSuchLease, exception.what()); } - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values. + lease->updateInternalTimestamp(); } void @@ -2637,8 +2642,8 @@ CqlLeaseMgr::updateLease6(const Lease6Ptr &lease) { isc_throw(NoSuchLease, exception.what()); } - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values. + lease->updateInternalTimestamp(); } bool diff --git a/src/lib/dhcpsrv/cql_lease_mgr.h b/src/lib/dhcpsrv/cql_lease_mgr.h index b01d040035..d96e90734c 100644 --- a/src/lib/dhcpsrv/cql_lease_mgr.h +++ b/src/lib/dhcpsrv/cql_lease_mgr.h @@ -410,6 +410,15 @@ public: /// exist. /// @throw isc::db::DbOperationError An operation on the open database has /// failed. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The UPDATE query uses IF expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and UPDATE. virtual void updateLease4(const Lease4Ptr& lease4) override; /// @brief Updates IPv6 lease. @@ -423,6 +432,15 @@ public: /// @throw isc::db::DbOperationError An operation on the open database has /// failed. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The UPDATE query uses IF expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and UPDATE. virtual void updateLease6(const Lease6Ptr& lease6) override; /// @brief Deletes an IPv4 lease. @@ -430,6 +448,15 @@ public: /// @param lease IPv4 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The DELETE query uses IF expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and DELETE. bool deleteLease(const Lease4Ptr& lease) override final; /// @brief Deletes an IPv6 lease. @@ -437,6 +464,15 @@ public: /// @param lease IPv6 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The DELETE query uses IF expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and DELETE. bool deleteLease(const Lease6Ptr& lease) override final; /// @brief Deletes all expired and reclaimed DHCPv4 leases. diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc index c9c8ba9699..f41d3d8701 100644 --- a/src/lib/dhcpsrv/lease.cc +++ b/src/lib/dhcpsrv/lease.cc @@ -276,6 +276,17 @@ Lease::fromElementCommon(const LeasePtr& lease, const data::ConstElementPtr& ele } } +void +Lease::updateInternalTimestamp() { + Lease::syncInternalTimestamp(*this, *this); +} + +void +Lease::syncInternalTimestamp(Lease& old_lease, const Lease& new_lease) { + old_lease.old_cltt_ = new_lease.cltt_; + old_lease.old_valid_lft_ = new_lease.valid_lft_; +} + Lease4::Lease4(const Lease4& other) : Lease(other.addr_, other.valid_lft_, other.subnet_id_, other.cltt_, other.fqdn_fwd_, diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index a9e27fd023..226920e9d3 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -97,6 +97,13 @@ struct Lease : public isc::data::UserContext, public isc::data::CfgToElement { /// @param fqdn_rev If true, reverse DNS update is performed for a lease. /// @param hostname FQDN of the client which gets the lease. /// @param hwaddr Hardware/MAC address + /// + /// @note When creating a new Lease object, old_cltt_ matches cltt_ and + /// old_valid_lft_ matches valid_lft_. Any update operation that changes + /// cltt_ or valid_lft_ in the database must also update the old_cltt_ and + /// old_valid_lft_ after the database response so that additional operations + /// can be performed on the same object. Failing to do so will result in + /// the new actions to be rejected by the database. Lease(const isc::asiolink::IOAddress& addr, uint32_t valid_lft, SubnetID subnet_id, time_t cltt, const bool fqdn_fwd, const bool fqdn_rev, @@ -119,6 +126,12 @@ struct Lease : public isc::data::UserContext, public isc::data::CfgToElement { /// @brief Old valid lifetime /// /// Expressed as number of seconds since cltt before update. + /// + /// @note This is used in unittests and in actual database implementations + /// to ensure that the operation (update or delete) on the lease is atomic + /// relative to the matching select operation. + /// @note This value should not be explicitly updated from outside + /// @ref Lease or @ref LeaseMgr code. uint32_t old_valid_lft_; /// @brief Client last transmission time @@ -131,6 +144,12 @@ struct Lease : public isc::data::UserContext, public isc::data::CfgToElement { /// /// Specifies a timestamp giving the time when the last transmission from a /// client was received before update. + /// + /// @note This is used in unittests and in actual database implementations + /// to ensure that the operation (update or delete) on the lease is atomic + /// relative to the matching select operation. + /// @note This value should not be explicitly updated from outside + /// @ref Lease or @ref LeaseMgr code. time_t old_cltt_; /// @brief Subnet identifier @@ -230,6 +249,20 @@ struct Lease : public isc::data::UserContext, public isc::data::CfgToElement { /// Avoid a clang spurious error using isc::data::CfgToElement::toElement; + /// Sync internal timestamp with value of a newer version of the lease, so + /// that additional operations can be done without performing extra read + /// from the database. + /// + /// @param [out] old_lease The old version of the lease that needs to be + /// updated. + /// @param new_lease The new version of the lease used to update old_lease + /// internal timestamp. + static void syncInternalTimestamp(Lease& old_lease, const Lease& new_lease); + + /// Update internal timestamp with new value, so that additional operations + /// can be done without performing extra read from the database. + void updateInternalTimestamp(); + protected: /// @brief Sets common (for v4 and v6) properties of the lease object. diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index 801d4fe82f..ddbc9b4b41 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -1409,9 +1409,19 @@ Memfile_LeaseMgr::updateLease4Internal(const Lease4Ptr& lease) { // Obtain 'by address' index. Lease4StorageAddressIndex& index = storage4_.get(); + bool persist = persistLeases(V4); + bool valid = true; + // Lease must exist if it is to be updated. Lease4StorageAddressIndex::const_iterator lease_it = index.find(lease->addr_); if (lease_it == index.end()) { + valid = false; + } else if ((!persist) && (((*lease_it)->old_cltt_ != lease->old_cltt_) || + ((*lease_it)->old_valid_lft_ != lease->old_valid_lft_))) { + valid = false; + } + + if (!valid) { isc_throw(NoSuchLease, "failed to update the lease with address " << lease->addr_ << " - no such lease"); } @@ -1419,7 +1429,7 @@ Memfile_LeaseMgr::updateLease4Internal(const Lease4Ptr& lease) { // Try to write a lease to disk first. If this fails, the lease will // not be inserted to the memory and the disk and in-memory data will // remain consistent. - if (persistLeases(V4)) { + if (persist) { lease_file4_->append(*lease); } @@ -1445,9 +1455,19 @@ Memfile_LeaseMgr::updateLease6Internal(const Lease6Ptr& lease) { // Obtain 'by address' index. Lease6StorageAddressIndex& index = storage6_.get(); + bool persist = persistLeases(V6); + bool valid = true; + // Lease must exist if it is to be updated. Lease6StorageAddressIndex::const_iterator lease_it = index.find(lease->addr_); if (lease_it == index.end()) { + valid = false; + } else if ((!persist) && (((*lease_it)->old_cltt_ != lease->old_cltt_) || + ((*lease_it)->old_valid_lft_ != lease->old_valid_lft_))) { + valid = false; + } + + if (!valid) { isc_throw(NoSuchLease, "failed to update the lease with address " << lease->addr_ << " - no such lease"); } @@ -1455,7 +1475,7 @@ Memfile_LeaseMgr::updateLease6Internal(const Lease6Ptr& lease) { // Try to write a lease to disk first. If this fails, the lease will // not be inserted to the memory and the disk and in-memory data will // remain consistent. - if (persistLeases(V6)) { + if (persist) { lease_file6_->append(*lease); } @@ -1492,6 +1512,13 @@ Memfile_LeaseMgr::deleteLeaseInternal(const Lease4Ptr& lease) { // removed. lease_copy.valid_lft_ = 0; lease_file4_->append(lease_copy); + } else { + // for test purpose only to check that an actual database + // implementation action is atomic + if ((*l)->old_cltt_ != lease->old_cltt_ || + (*l)->old_valid_lft_ != lease->old_valid_lft_) { + return false; + } } storage4_.erase(l); return (true); @@ -1527,6 +1554,13 @@ Memfile_LeaseMgr::deleteLeaseInternal(const Lease6Ptr& lease) { lease_copy.valid_lft_ = 0; lease_copy.preferred_lft_ = 0; lease_file6_->append(lease_copy); + } else { + // for test purpose only to check that an actual database + // implementation action is atomic + if ((*l)->old_cltt_ != lease->old_cltt_ || + (*l)->old_valid_lft_ != lease->old_valid_lft_) { + return false; + } } storage6_.erase(l); return (true); diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index aa996ca782..cc234dbae8 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -403,7 +403,15 @@ public: /// /// @param lease4 The lease to be updated. /// - /// If no such lease is present, an exception will be thrown. + /// @throw NoSuchLease if there is no such lease to be updated. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. virtual void updateLease4(const Lease4Ptr& lease4); /// @brief Updates IPv6 lease. @@ -413,7 +421,15 @@ public: /// /// @param lease6 The lease to be updated. /// - /// If no such lease is present, an exception will be thrown. + /// @throw NoSuchLease if there is no such lease to be updated. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. virtual void updateLease6(const Lease6Ptr& lease6); /// @brief Deletes an IPv4 lease. @@ -421,6 +437,14 @@ public: /// @param lease IPv4 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. virtual bool deleteLease(const Lease4Ptr& lease); /// @brief Deletes an IPv6 lease. @@ -428,6 +452,14 @@ public: /// @param lease IPv6 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. virtual bool deleteLease(const Lease6Ptr& lease); /// @brief Deletes all expired-reclaimed DHCPv4 leases. @@ -660,11 +692,31 @@ private: /// @brief Updates IPv4 lease. /// /// @param lease4 The lease to be updated. + /// + /// @throw NoSuchLease if there is no such lease to be updated. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. void updateLease4Internal(const Lease4Ptr& lease4); /// @brief Updates IPv6 lease. /// /// @param lease6 The lease to be updated. + /// + /// @throw NoSuchLease if there is no such lease to be updated. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. void updateLease6Internal(const Lease6Ptr& lease6); /// @brief Deletes an IPv4 lease. @@ -672,6 +724,14 @@ private: /// @param lease IPv4 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. bool deleteLeaseInternal(const Lease4Ptr& addr); /// @brief Deletes an IPv6 lease. @@ -679,6 +739,14 @@ private: /// @param lease IPv6 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease only if persistence is + /// disabled. This is the most common setting for all unittests. + /// This is useful to test the validity of the code where this is actually + /// required, in database code where multiple processes can update + /// database entries, and old_cltt_ and old_valid_lft_ are used to force + /// atomic action on respective lease. bool deleteLeaseInternal(const Lease6Ptr& addr); /// @brief Removes specified IPv4 leases. diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 6d35896345..3e14714dde 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -1425,6 +1425,7 @@ public: valid_lft = 0; } MySqlConnection::convertFromDatabaseTime(expire_, valid_lft, cltt); + // Update cltt_ and old_cltt_ explicitly. result->cltt_ = cltt; result->old_cltt_ = cltt; @@ -1871,8 +1872,10 @@ MySqlLeaseMgr::addLease(const Lease4Ptr& lease) { // ... and drop to common code. auto result = addLeaseCommon(ctx, INSERT_LEASE4, bind); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values (allows update of the + // internal state between the creation of the Lease up to the point of + // insertion in the database). + lease->updateInternalTimestamp(); return (result); } @@ -1893,8 +1896,10 @@ MySqlLeaseMgr::addLease(const Lease6Ptr& lease) { // ... and drop to common code. auto result = addLeaseCommon(ctx, INSERT_LEASE6, bind); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values (allows update of the + // internal state between the creation of the Lease up to the point of + // insertion in the database). + lease->updateInternalTimestamp(); return (result); } @@ -2769,8 +2774,8 @@ MySqlLeaseMgr::updateLease4(const Lease4Ptr& lease) { // Drop to common update code updateLeaseCommon(ctx, stindex, &bind[0], lease); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values. + lease->updateInternalTimestamp(); } void @@ -2816,8 +2821,8 @@ MySqlLeaseMgr::updateLease6(const Lease6Ptr& lease) { // Drop to common update code updateLeaseCommon(ctx, stindex, &bind[0], lease); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values. + lease->updateInternalTimestamp(); } // Delete lease methods. Similar to other groups of methods, these comprise diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.h b/src/lib/dhcpsrv/mysql_lease_mgr.h index 29471bb0d5..6aad283a6d 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.h +++ b/src/lib/dhcpsrv/mysql_lease_mgr.h @@ -453,6 +453,15 @@ public: /// exist. /// @throw isc::db::DbOperationError An operation on the open database has /// failed. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The UPDATE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and UPDATE. virtual void updateLease4(const Lease4Ptr& lease4); /// @brief Updates IPv6 lease. @@ -466,6 +475,15 @@ public: /// exist. /// @throw isc::db::DbOperationError An operation on the open database has /// failed. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The UPDATE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and UPDATE. virtual void updateLease6(const Lease6Ptr& lease6); /// @brief Deletes an IPv4 lease. @@ -473,6 +491,15 @@ public: /// @param lease IPv4 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The DELETE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and DELETE. virtual bool deleteLease(const Lease4Ptr& lease); /// @brief Deletes an IPv6 lease. @@ -480,6 +507,15 @@ public: /// @param lease IPv6 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The DELETE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and DELETE. virtual bool deleteLease(const Lease6Ptr& lease); /// @brief Deletes all expired-reclaimed DHCPv4 leases. diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index fd6ab7765d..782d442598 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -911,6 +911,7 @@ public: subnet_id_, fqdn_fwd_, fqdn_rev_, hostname_, hwaddr, prefix_len_)); + // Update cltt_ and old_cltt_ explicitly. result->cltt_ = cltt_; result->old_cltt_ = cltt_; @@ -1306,8 +1307,10 @@ PgSqlLeaseMgr::addLease(const Lease4Ptr& lease) { ctx->exchange4_->createBindForSend(lease, bind_array); auto result = addLeaseCommon(ctx, INSERT_LEASE4, bind_array); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values (allows update of the + // internal state between the creation of the Lease up to the point of + // insertion in the database). + lease->updateInternalTimestamp(); return (result); } @@ -1327,8 +1330,10 @@ PgSqlLeaseMgr::addLease(const Lease6Ptr& lease) { auto result = addLeaseCommon(ctx, INSERT_LEASE6, bind_array); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values (allows update of the + // internal state between the creation of the Lease up to the point of + // insertion in the database). + lease->updateInternalTimestamp(); return (result); } @@ -1989,8 +1994,8 @@ PgSqlLeaseMgr::updateLease4(const Lease4Ptr& lease) { // Drop to common update code updateLeaseCommon(ctx, stindex, bind_array, lease); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values. + lease->updateInternalTimestamp(); } void @@ -2020,8 +2025,8 @@ PgSqlLeaseMgr::updateLease6(const Lease6Ptr& lease) { // Drop to common update code updateLeaseCommon(ctx, stindex, bind_array, lease); - lease->old_cltt_ = lease->cltt_; - lease->old_valid_lft_ = lease->valid_lft_; + // Update lease internal information with new values. + lease->updateInternalTimestamp(); } uint64_t diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.h b/src/lib/dhcpsrv/pgsql_lease_mgr.h index 6339592b09..20ff9348fb 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.h +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.h @@ -428,6 +428,15 @@ public: /// exist. /// @throw isc::db::DbOperationError An operation on the open database has /// failed. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The UPDATE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and UPDATE. virtual void updateLease4(const Lease4Ptr& lease4); /// @brief Updates IPv6 lease. @@ -441,6 +450,15 @@ public: /// exist. /// @throw isc::db::DbOperationError An operation on the open database has /// failed. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The UPDATE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and UPDATE. virtual void updateLease6(const Lease6Ptr& lease6); /// @brief Deletes an IPv4 lease. @@ -448,6 +466,15 @@ public: /// @param lease IPv4 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The DELETE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and DELETE. virtual bool deleteLease(const Lease4Ptr& lease); /// @brief Deletes an IPv6 lease. @@ -455,6 +482,15 @@ public: /// @param lease IPv6 lease being deleted. /// /// @return true if deletion was successful, false if no such lease exists. + /// + /// @note This function checks that old_cltt_ and old_valid_lft_ values of + /// the stored lease match values of the new lease. + /// The old_cltt_ and old_valid_lft_ are used to ensure automatic action on + /// respective lease. + /// @note The DELETE query uses WHERE expire = ? to delete the lease only if + /// the value matches the one received on the SELECT query, effectively + /// forcing that no other process or thread updates the lease between SELECT + /// and DELETE. virtual bool deleteLease(const Lease6Ptr& lease); /// @brief Deletes all expired-reclaimed DHCPv4 leases.