From: Marcin Siodelski Date: Fri, 29 Jun 2018 14:37:49 +0000 (+0200) Subject: [5664] Fix callout status propagation in Alloc Engine. X-Git-Tag: trac5555_base~1^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e5d7bc8b727867b24bfe34d185cbb561a00d8906;p=thirdparty%2Fkea.git [5664] Fix callout status propagation in Alloc Engine. --- diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 2ad1934927..c48f3857f5 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -768,6 +768,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { Pool6Ptr pool; + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + while (subnet) { if (!subnet->clientSupported(ctx.query_->getClasses())) { @@ -813,7 +815,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { // The hint is valid and not currently used, let's create a // lease for it - lease = createLease6(ctx, hint, pool->getLength()); + lease = createLease6(ctx, hint, pool->getLength(), callout_status); // It can happen that the lease allocation failed (we could // have lost the race condition. That means that the hint is @@ -852,7 +854,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { ctx.currentIA().old_leases_.push_back(old_lease); /// We found a lease and it is expired, so we can reuse it - lease = reuseExpiredLease(lease, ctx, pool->getLength()); + lease = reuseExpiredLease(lease, ctx, pool->getLength(), + callout_status); /// @todo: We support only one lease per ia for now leases.push_back(lease); @@ -966,7 +969,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { // free. Let's allocate it. ctx.subnet_ = subnet; - Lease6Ptr lease = createLease6(ctx, candidate, prefix_len); + Lease6Ptr lease = createLease6(ctx, candidate, prefix_len, callout_status); if (lease) { // We are allocating a new lease (not renewing). So, the // old lease should be NULL. @@ -976,8 +979,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { return (leases); } else if (ctx.callout_handle_ && - (ctx.callout_handle_->getStatus() != - CalloutHandle::NEXT_STEP_CONTINUE)) { + (callout_status != CalloutHandle::NEXT_STEP_CONTINUE)) { // Don't retry when the callout status is not continue. break; } @@ -993,7 +995,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { ctx.currentIA().old_leases_.push_back(old_lease); ctx.subnet_ = subnet; - existing = reuseExpiredLease(existing, ctx, prefix_len); + existing = reuseExpiredLease(existing, ctx, prefix_len, + callout_status); leases.push_back(existing); return (leases); @@ -1149,7 +1152,8 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, } // Ok, let's create a new lease... - Lease6Ptr lease = createLease6(ctx, addr, prefix_len); + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + Lease6Ptr lease = createLease6(ctx, addr, prefix_len, callout_status); // ... and add it to the existing leases list. existing_leases.push_back(lease); @@ -1405,7 +1409,8 @@ AllocEngine::removeNonreservedLeases6(ClientContext6& ctx, Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, - uint8_t prefix_len) { + uint8_t prefix_len, + CalloutHandle::CalloutNextStep& callout_status) { if (!expired->expired()) { isc_throw(BadValue, "Attempt to recycle lease that is still valid"); @@ -1472,10 +1477,12 @@ AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, // Call the callouts HooksManager::callCallouts(hook_index_lease6_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE6_SELECT_SKIP); return (Lease6Ptr()); } @@ -1515,7 +1522,8 @@ AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, Lease6Ptr AllocEngine::createLease6(ClientContext6& ctx, const IOAddress& addr, - uint8_t prefix_len) { + uint8_t prefix_len, + CalloutHandle::CalloutNextStep& callout_status) { if (ctx.currentIA().type_ != Lease::TYPE_PD) { prefix_len = 128; // non-PD lease types must be always /128 @@ -1559,10 +1567,12 @@ Lease6Ptr AllocEngine::createLease6(ClientContext6& ctx, // This is the first callout, so no need to clear any arguments HooksManager::callCallouts(hook_index_lease6_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE6_SELECT_SKIP); return (Lease6Ptr()); } @@ -2944,6 +2954,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // caller. Lease4Ptr new_lease; + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + // Check if there is a reservation for the client. If there is, we want to // assign the reserved address, rather than any other one. if (hasAddressReservation(ctx)) { @@ -2964,7 +2976,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // Note that we don't remove the existing client's lease at this point // because this is not a real allocation, we just offer what we can // allocate in the DHCPREQUEST time. - new_lease = allocateOrReuseLease4(ctx.currentHost()->getIPv4Reservation(), ctx); + new_lease = allocateOrReuseLease4(ctx.currentHost()->getIPv4Reservation(), ctx, + callout_status); if (!new_lease) { LOG_WARN(alloc_engine_logger, ALLOC_ENGINE_V4_DISCOVER_ADDRESS_CONFLICT) .arg(ctx.query_->getLabel()) @@ -3013,7 +3026,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { .arg(ctx.requested_address_.toText()) .arg(ctx.query_->getLabel()); - new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx); + new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx, + callout_status); } // The allocation engine failed to allocate all of the candidate @@ -3184,7 +3198,9 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // or the existing lease has expired. If the allocation fails, // e.g. because the lease is in use, we will return NULL to // indicate that we were unable to allocate the lease. - new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx); + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx, + callout_status); } else { @@ -3223,7 +3239,8 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { } Lease4Ptr -AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr) { +AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr, + CalloutHandle::CalloutNextStep& callout_status) { if (!ctx.hwaddr_) { isc_throw(BadValue, "Can't create a lease with NULL HW address"); } @@ -3283,10 +3300,12 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr) { // This is the first callout, so no need to clear any arguments HooksManager::callCallouts(hook_index_lease4_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE4_SELECT_SKIP); return (Lease4Ptr()); } @@ -3429,7 +3448,8 @@ AllocEngine::renewLease4(const Lease4Ptr& lease, Lease4Ptr AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, - AllocEngine::ClientContext4& ctx) { + AllocEngine::ClientContext4& ctx, + CalloutHandle::CalloutNextStep& callout_status) { if (!expired) { isc_throw(BadValue, "null lease specified for reuseExpiredLease"); } @@ -3487,10 +3507,12 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, // Call the callouts HooksManager::callCallouts(hook_index_lease4_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE4_SELECT_SKIP); return (Lease4Ptr()); @@ -3522,14 +3544,15 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, } Lease4Ptr -AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& ctx) { +AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& ctx, + CalloutHandle::CalloutNextStep& callout_status) { ctx.conflicting_lease_.reset(); Lease4Ptr exist_lease = LeaseMgrFactory::instance().getLease4(candidate); if (exist_lease) { if (exist_lease->expired()) { ctx.old_lease_ = Lease4Ptr(new Lease4(*exist_lease)); - return (reuseExpiredLease4(exist_lease, ctx)); + return (reuseExpiredLease4(exist_lease, ctx, callout_status)); } else { // If there is a lease and it is not expired, pass this lease back @@ -3539,7 +3562,7 @@ AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& c } } else { - return (createLease4(ctx, candidate)); + return (createLease4(ctx, candidate, callout_status)); } return (Lease4Ptr()); } @@ -3589,12 +3612,7 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) { max_attempts = 0; } - // Set the default status code in case the lease4_select callouts - // do not exist and the callout handle has a status returned by - // any of the callouts already invoked for this packet. - if (ctx.callout_handle_) { - ctx.callout_handle_->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); - } + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; for (uint64_t i = 0; i < max_attempts; ++i) { IOAddress candidate = allocator->pickAddress(subnet, @@ -3607,13 +3625,12 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) { // The call below will return the non-NULL pointer if we // successfully allocate this lease. This means that the // address is not in use by another client. - new_lease = allocateOrReuseLease4(candidate, ctx); + new_lease = allocateOrReuseLease4(candidate, ctx, callout_status); if (new_lease) { return (new_lease); } else if (ctx.callout_handle_ && - (ctx.callout_handle_->getStatus() != - CalloutHandle::NEXT_STEP_CONTINUE)) { + (callout_status != CalloutHandle::NEXT_STEP_CONTINUE)) { // Don't retry when the callout status is not continue. subnet.reset(); break; diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 4b1dce5c73..3a64eb91a4 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -761,6 +761,7 @@ private: /// available /// @param prefix_len length of the prefix (for PD only) /// should be 128 for other lease types + /// @param [out] callout_status callout returned by the lease6_select /// /// The following fields of the ctx structure are used: /// @ref ClientContext6::subnet_ subnet the lease is allocated from @@ -784,7 +785,8 @@ private: /// became unavailable) Lease6Ptr createLease6(ClientContext6& ctx, const isc::asiolink::IOAddress& addr, - const uint8_t prefix_len); + const uint8_t prefix_len, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Allocates a normal, in-pool, unreserved lease from the pool. /// @@ -859,6 +861,7 @@ private: /// @param ctx client context that contains all details. /// @param prefix_len prefix length (for PD leases) /// Should be 128 for other lease types + /// @param [out] callout_status callout returned by the lease6_select /// /// The following parameters are used from the ctx structure: /// @ref ClientContext6::subnet_ subnet the lease is allocated from @@ -879,9 +882,11 @@ private: /// /// @return refreshed lease /// @throw BadValue if trying to recycle lease that is still valid - Lease6Ptr reuseExpiredLease(Lease6Ptr& expired, - ClientContext6& ctx, - uint8_t prefix_len); + Lease6Ptr + reuseExpiredLease(Lease6Ptr& expired, + ClientContext6& ctx, + uint8_t prefix_len, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Updates FQDN and Client's Last Transmission Time /// for a collection of leases. @@ -1375,6 +1380,7 @@ private: /// /// @param ctx client context that contains additional parameters. /// @param addr An address that was selected and is confirmed to be available + /// @param [out] callout_status callout returned by the lease6_select /// /// In particular, the following fields from Client context are used: /// - @ref ClientContext4::subnet_ Subnet the lease is allocated from @@ -1395,7 +1401,8 @@ private: /// @return allocated lease (or NULL in the unlikely case of the lease just /// become unavailable) Lease4Ptr createLease4(const ClientContext4& ctx, - const isc::asiolink::IOAddress& addr); + const isc::asiolink::IOAddress& addr, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Renews a DHCPv4 lease. /// @@ -1422,11 +1429,14 @@ private: /// @param expired An old, expired lease. /// @param ctx Message processing context. It holds various information /// extracted from the client's message and required to allocate a lease. + /// @param [out] callout_status callout returned by the lease4_select /// /// @return Updated lease instance. /// @throw BadValue if trying to reuse a lease which is still valid or /// when the provided parameters are invalid. - Lease4Ptr reuseExpiredLease4(Lease4Ptr& expired, ClientContext4& ctx); + Lease4Ptr + reuseExpiredLease4(Lease4Ptr& expired, ClientContext4& ctx, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Allocates the lease by replacing an existing lease. /// @@ -1439,11 +1449,14 @@ private: /// allocated. /// @param ctx Client context holding the data extracted from the /// client's message. + /// @param [out] callout_status callout returned by the lease4_select /// /// @return A pointer to the allocated lease or NULL if the allocation /// was not successful. - Lease4Ptr allocateOrReuseLease4(const asiolink::IOAddress& address, - ClientContext4& ctx); + Lease4Ptr + allocateOrReuseLease4(const asiolink::IOAddress& address, + ClientContext4& ctx, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Allocates the lease from the dynamic pool. /// diff --git a/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc index 933f31fb27..7d4bb39212 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc @@ -49,6 +49,21 @@ public: callback_skip_ = 0; } + /// @brief Checks if the state of the callout handle associated with a query + /// was reset after the callout invocation. + /// + /// The check includes verification if the status was set to 'continue' and + /// that all arguments were deleted. + /// + /// @param query pointer to the query which callout handle is associated + /// with. + void checkCalloutHandleReset(const Pkt6Ptr& query) { + CalloutHandlePtr callout_handle = query->getCalloutHandle(); + ASSERT_TRUE(callout_handle); + EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus()); + EXPECT_TRUE(callout_handle->getArgumentNames().empty()); + } + /// callback that stores received callout name and received values static int lease6_select_callout(CalloutHandle& callout_handle) { @@ -227,6 +242,9 @@ TEST_F(HookAllocEngine6Test, lease6_select) { EXPECT_TRUE(callback_argument_names_ == expected_argument_names); EXPECT_TRUE(callback_qry_options_copy_); + + // Check if the callout handle state was reset after the callout. + checkCalloutHandleReset(ctx.query_); } // This test checks if lease6_select callout is able to override the values @@ -282,6 +300,9 @@ TEST_F(HookAllocEngine6Test, change_lease6_select) { EXPECT_EQ(t2_override_, from_mgr->t2_); EXPECT_EQ(pref_override_, from_mgr->preferred_lft_); EXPECT_EQ(valid_override_, from_mgr->valid_lft_); + + // Check if the callout handle state was reset after the callout. + checkCalloutHandleReset(ctx.query_); } // This test checks if lease6_select callout can set the status to next @@ -313,6 +334,9 @@ TEST_F(HookAllocEngine6Test, skip_lease6_select) { // Check no retry was attempted EXPECT_EQ(1, callback_skip_); + + // Check if the callout handle state was reset after the callout. + checkCalloutHandleReset(ctx.query_); } /// @brief helper class used in Hooks testing in AllocEngine4 @@ -348,6 +372,21 @@ public: callback_skip_ = 0; } + /// @brief Checks if the state of the callout handle associated with a query + /// was reset after the callout invocation. + /// + /// The check includes verification if the status was set to 'continue' and + /// that all arguments were deleted. + /// + /// @param query pointer to the query which callout handle is associated + /// with. + void checkCalloutHandleReset(const Pkt4Ptr& query) { + CalloutHandlePtr callout_handle = query->getCalloutHandle(); + ASSERT_TRUE(callout_handle); + EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus()); + EXPECT_TRUE(callout_handle->getArgumentNames().empty()); + } + /// callback that stores received callout name and received values static int lease4_select_callout(CalloutHandle& callout_handle) { @@ -522,6 +561,9 @@ TEST_F(HookAllocEngine4Test, lease4_select) { EXPECT_TRUE(callback_argument_names_ == expected_argument_names); EXPECT_TRUE(callback_qry_options_copy_); + + // Check if the callout handle state was reset after the callout. + checkCalloutHandleReset(ctx.query_); } // This test checks if lease4_select callout is able to override the values @@ -580,6 +622,9 @@ TEST_F(HookAllocEngine4Test, change_lease4_select) { EXPECT_EQ(t1_override_, from_mgr->t1_); EXPECT_EQ(t2_override_, from_mgr->t2_); EXPECT_EQ(valid_override_, from_mgr->valid_lft_); + + // Check if the callout handle state was reset after the callout. + checkCalloutHandleReset(ctx.query_); } // This test checks if lease4_select callout can set the status to next @@ -615,6 +660,9 @@ TEST_F(HookAllocEngine4Test, skip_lease4_select) { // Check no retry was attempted EXPECT_EQ(1, callback_skip_); + + // Check if the callout handle state was reset after the callout. + checkCalloutHandleReset(ctx.query_); } }; // namespace test