From 8bd26523b5940bc1af5a0dd94a66561cc89acf47 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 31 Jan 2025 11:29:20 -0500 Subject: [PATCH] [#3463] Implementd leases4_committed callout /src/hooks/dhcp/lease_cmds/binding_variables.cc BindingVariableMgr::evaluateVariables() - Add a throw if query,response, or lease are empty /src/hooks/dhcp/lease_cmds/lease_cmds.* LeaseCmdsImpl::leases4Committed() LeaseCmds::leases4Committed() - implemented callout handler /src/hooks/dhcp/lease_cmds/lease_cmds_callouts.cc leases4_committed(CalloutHandle& handle) - new callout /src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE LEASE_CMDS_LEASES4_COMMITTED_FAILED processing - new messages /src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc void Lease4CmdsTest::testValidLeases4Committed() TEST_F(Lease4CmdsTest, validLeases4Committed) TEST_F(Lease4CmdsTest, validLeases4CommittedMultiThreading) - new tests --- .../dhcp/lease_cmds/binding_variables.cc | 8 +- src/hooks/dhcp/lease_cmds/binding_variables.h | 3 + src/hooks/dhcp/lease_cmds/lease_cmds.cc | 57 +++++++ src/hooks/dhcp/lease_cmds/lease_cmds.h | 14 ++ .../dhcp/lease_cmds/lease_cmds_callouts.cc | 22 +++ .../dhcp/lease_cmds/lease_cmds_messages.cc | 4 + .../dhcp/lease_cmds/lease_cmds_messages.h | 2 + .../dhcp/lease_cmds/lease_cmds_messages.mes | 9 ++ .../tests/binding_variables_unittest.cc | 3 +- .../lease_cmds/tests/lease_cmds4_unittest.cc | 151 ++++++++++++++++++ 10 files changed, 270 insertions(+), 3 deletions(-) diff --git a/src/hooks/dhcp/lease_cmds/binding_variables.cc b/src/hooks/dhcp/lease_cmds/binding_variables.cc index a0e7b5fb78..09e45a4976 100644 --- a/src/hooks/dhcp/lease_cmds/binding_variables.cc +++ b/src/hooks/dhcp/lease_cmds/binding_variables.cc @@ -29,7 +29,7 @@ BindingVariable::BindingVariable(const std::string& name, isc_throw(BadValue, "BindingVariable - name cannot be empty"); } - /// @todo If we add socpes we may wish to allow higher order + /// @todo If we add scopes we may wish to allow higher order /// scopes to override lower scopes with empty expressions. if (expression_str_.empty()) { isc_throw(BadValue, "BindingVariable - '" << name_ @@ -223,10 +223,14 @@ BindingVariableMgr::configure(data::ConstElementPtr config) { bool BindingVariableMgr::evaluateVariables(PktPtr query, PktPtr response, LeasePtr lease) { + if (!query || !response || !lease) { + isc_throw(BadValue, "evaluateVariables - missing query, response, and/or lease"); + } + if (!cache_->size()) { /// @todo If the lease has binding-variables in its context from a prior /// update, but config changed and now there are none defined, should we - /// removed them from the lease? + /// removed them from the lease? For now, we'll leave them. return(false); } diff --git a/src/hooks/dhcp/lease_cmds/binding_variables.h b/src/hooks/dhcp/lease_cmds/binding_variables.h index 786743f4d1..e7f7af1322 100644 --- a/src/hooks/dhcp/lease_cmds/binding_variables.h +++ b/src/hooks/dhcp/lease_cmds/binding_variables.h @@ -289,6 +289,9 @@ public: /// @brief Evaluates the binding variables for a given lease and packet pair. /// + /// Iiterates over the variables in the cache evaluating each on and added its + /// name and value to the context. + /// /// @param query Client packet which produced the lease. Variables whose source /// is "query" will be evaluated against this packet. /// @param response Server response conveying the lease. Variables whose source diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index b0baf99079..43a940c3dd 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -43,6 +43,7 @@ using namespace isc::asiolink; using namespace isc::hooks; using namespace isc::stats; using namespace isc::util; +using namespace isc::log; using namespace std; namespace isc { @@ -471,6 +472,19 @@ public: } return (isc->get("relay-info")); } + + /// @brief leases4-committed hookpoint handler. + /// + /// Evaluates the binding variables (if any), and updates the given lease's + /// user-context accordingly. This includes updating the lease in the lease + /// back end. + /// + /// @param handle Callout context - which is expected to contain the query4, response4, + /// and leases4 argumeents. + /// @param mgr Pointer to the BindingVariableMgr singleton. + /// @throw + static void leases4Committed(CalloutHandle& callout_handle, + BindingVariableMgrPtr mgr); }; void @@ -2698,6 +2712,42 @@ LeaseCmdsImpl::leaseWriteHandler(CalloutHandle& handle) { return (0); } +void +LeaseCmdsImpl::leases4Committed(CalloutHandle& callout_handle, + BindingVariableMgrPtr mgr) { + Pkt4Ptr query; + Pkt4Ptr response; + Lease4CollectionPtr leases; + + // Get the necessary arguments. + callout_handle.getArgument("query4", query); + callout_handle.getArgument("response4", response); + callout_handle.getArgument("leases4", leases); + + // In some cases we may have no leases, e.g. DHCPNAK. + if (leases->empty()) { + LOG_DEBUG(lease_cmds_logger, DBGLVL_TRACE_BASIC, + LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE) + .arg(query->getLabel()); + return; + } + + Lease4Ptr lease = (*leases)[0]; + try { + if (mgr->evaluateVariables(query, response, lease)) { + LeaseMgrFactory::instance().updateLease4(lease); + } + } catch (const NoSuchLease&) { + isc_throw(LeaseCmdsConflict, "failed to update" + " the lease with address " << lease->addr_ << + " either because the lease has been" + " deleted or it has changed in the database"); + } catch (const std::exception& ex) { + isc_throw(Unexpected, "evaluating binding variables failed for " + << query->getLabel() << ", :" << ex.what()); + } +} + int LeaseCmds::leaseAddHandler(CalloutHandle& handle) { return (impl_->leaseAddHandler(handle)); @@ -2793,5 +2843,12 @@ LeaseCmds::leaseWriteHandler(CalloutHandle& handle) { LeaseCmds::LeaseCmds() : impl_(new LeaseCmdsImpl()) { } + +void +LeaseCmds::leases4Committed(CalloutHandle& callout_handle, + BindingVariableMgrPtr mgr) { + impl_->leases4Committed(callout_handle, mgr); +} + } // end of namespace lease_cmds } // end of namespace isc diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.h b/src/hooks/dhcp/lease_cmds/lease_cmds.h index 1ea9e534de..b0ed786ab7 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.h +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.h @@ -8,6 +8,7 @@ #define LEASE_CMDS_H #include +#include #include #include @@ -605,6 +606,19 @@ public: int leaseWriteHandler(hooks::CalloutHandle& handle); + /// @brief leases4_committed hookpoint handler. + /// + /// Evaluates the binding variables (if any), and updates the given lease's + /// user-context accordingly. This includes updating the lease in the lease + /// back end. + /// + /// @param handle Callout context - which is expected to contain the query4, response4, + /// and leases4 argumeents. + /// @param mgr Pointer to the BindingVariableMgr singleton. + void + leases4Committed(hooks::CalloutHandle& callout_handle, + BindingVariableMgrPtr mgr); + private: /// Pointer to the actual implementation boost::shared_ptr impl_; diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_callouts.cc b/src/hooks/dhcp/lease_cmds/lease_cmds_callouts.cc index 5c09e978cc..c3c8a99b23 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_callouts.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_callouts.cc @@ -378,4 +378,26 @@ int multi_threading_compatible() { return (1); } +/// @brief leases4_committed callout implementation. +/// +/// @param handle callout handle. +int leases4_committed(CalloutHandle& handle) { + CalloutHandle::CalloutNextStep status = handle.getStatus(); + if (status == CalloutHandle::NEXT_STEP_DROP || + status == CalloutHandle::NEXT_STEP_SKIP) { + return (0); + } + + try { + LeaseCmds lease_cmds; + lease_cmds.leases4Committed(handle, binding_var_mgr); + } catch (const std::exception& ex) { + LOG_ERROR(lease_cmds_logger, LEASE_CMDS_LEASES4_COMMITTED_FAILED) + .arg(ex.what()); + return (1); + } + + return (0); +} + } // end extern "C" diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc index 2732de09f2..c13da0ca19 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc @@ -20,6 +20,8 @@ extern const isc::log::MessageID LEASE_CMDS_DEL6_FAILED = "LEASE_CMDS_DEL6_FAILE extern const isc::log::MessageID LEASE_CMDS_GET4_FAILED = "LEASE_CMDS_GET4_FAILED"; extern const isc::log::MessageID LEASE_CMDS_GET6_FAILED = "LEASE_CMDS_GET6_FAILED"; extern const isc::log::MessageID LEASE_CMDS_INIT_OK = "LEASE_CMDS_INIT_OK"; +extern const isc::log::MessageID LEASE_CMDS_LEASES4_COMMITTED_FAILED = "LEASE_CMDS_LEASES4_COMMITTED_FAILED"; +extern const isc::log::MessageID LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE = "LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE"; extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4 = "LEASE_CMDS_RESEND_DDNS4"; extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4_FAILED = "LEASE_CMDS_RESEND_DDNS4_FAILED"; extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS6 = "LEASE_CMDS_RESEND_DDNS6"; @@ -56,6 +58,8 @@ const char* values[] = { "LEASE_CMDS_GET4_FAILED", "lease4-get command failed (parameters: %1, reason: %2)", "LEASE_CMDS_GET6_FAILED", "lease6-get command failed (parameters: %1, reason: %2)", "LEASE_CMDS_INIT_OK", "loading Lease Commands hooks library successful", + "LEASE_CMDS_LEASES4_COMMITTED_FAILED", "processing error occured evaluating binding variables for %1, %2", + "LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE", "query %1, has no active lease.", "LEASE_CMDS_RESEND_DDNS4", "lease4-resend-ddns command successful: %1", "LEASE_CMDS_RESEND_DDNS4_FAILED", "lease4-resend-ddns command failed: %1", "LEASE_CMDS_RESEND_DDNS6", "lease6-resend-ddns command successful: %1", diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h index 90efb4596f..056f935b35 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h @@ -21,6 +21,8 @@ extern const isc::log::MessageID LEASE_CMDS_DEL6_FAILED; extern const isc::log::MessageID LEASE_CMDS_GET4_FAILED; extern const isc::log::MessageID LEASE_CMDS_GET6_FAILED; extern const isc::log::MessageID LEASE_CMDS_INIT_OK; +extern const isc::log::MessageID LEASE_CMDS_LEASES4_COMMITTED_FAILED; +extern const isc::log::MessageID LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE; extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4; extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4_FAILED; extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS6; diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes index f83e259d2c..44985368da 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes @@ -138,3 +138,12 @@ The lease6-wipe command is deprecated and it will be removed in the future. % LEASE_CMDS_WIPE6_FAILED lease6-wipe command failed (parameters: %1, reason: %2) The lease6-wipe command has failed. Both the reason as well as the parameters passed are logged. + +% LEASE_CMDS_LEASES4_COMMITTED_NOTHING_TO_UPDATE query %1, has no active lease. +This debug log is emitted when the leaes4-committed handler is invoked and +there is no active lease associated with the query. + +% LEASE_CMDS_LEASES4_COMMITTED_FAILED processing error occured evaluating binding variables for %1, %2 +This debug log is emitted when an error occurs in the leaes4-committed +handler is invoked. The arguemntes detail the query and the error. This +error means binding-variable values were not added/updated for the lease. diff --git a/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc index 9c949305a8..184cd47e47 100644 --- a/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc @@ -6,7 +6,6 @@ #include - #include #include #include @@ -14,6 +13,7 @@ #include #include #include + #include #include #include @@ -26,6 +26,7 @@ using namespace isc::dhcp; using namespace isc::data; using namespace isc::test; using namespace isc::asiolink; +using namespace isc::hooks; using namespace isc::lease_cmds; diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc index 4dcc6efc86..aff427c228 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc @@ -16,10 +16,12 @@ #include #include #include +#include #include #include #include #include +#include #include @@ -36,6 +38,7 @@ using namespace isc::dhcp_ddns; using namespace isc::asiolink; using namespace isc::stats; using namespace isc::test; +using namespace isc::lease_cmds; namespace { @@ -444,6 +447,10 @@ public: /// @brief Check that lease4-write works as expected. void testLease4Write(); + + /// @brief Check that lease4-committed handler works as expected with + /// valid inputs. + void testValidLeases4Committed(); }; void Lease4CmdsTest::testLease4AddMissingParams() { @@ -3482,6 +3489,141 @@ void Lease4CmdsTest::testLease4Write() { testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); } +#define SCOPED_LINE(line) \ + std::stringstream ss; \ + ss << "Scenario at line: " << line; \ + SCOPED_TRACE(ss.str()); + +void Lease4CmdsTest::testValidLeases4Committed() { + // Initialize lease manager (false = v4, true = add leases) + initLeaseMgr(false, true); + + struct Scenario { + uint32_t line_; + std::string config_; + std::string orig_context_; + std::string exp_context_; + }; + + std::list scenarios = { + { + // No variables configured, nothing in lease context. + __LINE__, + R"({})", + R"({})", + R"({})" + },{ + // lease context has no binding-variables, two configured + __LINE__, + R"^({"binding-variables":[ + { + "name": "hwaddr", + "expression": "hexstring(pkt4.mac,':')", + "source": "query" + }, + { + "name": "yiaddr", + "expression": "addrtotext(pkt4.yiaddr)", + "source": "response" + }]})^", + R"({})", + R"({"ISC":{ + "binding-variables":{ + "hwaddr": "01:02:03:04:05:06", + "yiaddr": "192.0.2.1" + } + }})", + }, + { + // lease context has binding-variables, none configured + // Current logic leaves lease untouched. + __LINE__, + R"({})", + R"({"ISC":{ + "binding-variables":{ + "hwaddr": "01:02:03:04:05:06", + "yiaddr": "192.0.2.1" + } + }})", + R"({"ISC":{ + "binding-variables":{ + "hwaddr": "01:02:03:04:05:06", + "yiaddr": "192.0.2.1" + } + }})", + }, + { + // Evaluated variable value is an empty string. + __LINE__, + R"^({"binding-variables":[ + { + "name": "hwaddr", + "expression": "''", + "source": "query" + }]})^", + R"({"ISC":{ + "binding-variables":{ + "hwaddr": "01:02:03:04:05:06" + } + }})", + R"({"ISC":{ + "binding-variables":{ + "hwaddr": "" + } + }})", + }}; + + // Create packet pair and lease. + Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 1234)); + query->setHWAddr(1, 6, { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 }); + + Pkt4Ptr response(new Pkt4(DHCPOFFER, 1234)); + IOAddress yiaddr("192.0.2.1"); + response->setYiaddr(yiaddr); + + // Iterater over scenarios. + for (auto const& scenario : scenarios) { + SCOPED_LINE(scenario.line_); + + // Create and configure the manager. + BindingVariableMgrPtr mgr; + ASSERT_NO_THROW_LOG(mgr.reset(new BindingVariableMgr(AF_INET))); + ConstElementPtr config; + ASSERT_NO_THROW_LOG(config = Element::fromJSON(scenario.config_)); + ASSERT_NO_THROW_LOG(mgr->configure(config)); + + // Fetch the lease and set its user-context to the original content. + Lease4Ptr orig_lease = lmptr_->getLease4(yiaddr); + ASSERT_TRUE(orig_lease); + ConstElementPtr orig_context; + ASSERT_NO_THROW_LOG(orig_context = Element::fromJSON(scenario.orig_context_)); + orig_lease->setContext(orig_context); + ASSERT_NO_THROW_LOG(lmptr_->updateLease4(orig_lease)); + + Lease4CollectionPtr leases(new Lease4Collection()); + leases->push_back(orig_lease); + + // Create a callout handle and add the expected arguments. + CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle(); + callout_handle->setArgument("query4", query); + callout_handle->setArgument("response4", response); + callout_handle->setArgument("leases4", leases); + + // Invoke the leases4Committed handler. + LeaseCmds cmds; + ASSERT_NO_THROW_LOG(cmds.leases4Committed(*callout_handle, mgr)); + + // Fetch the lease. + Lease4Ptr after_lease = lmptr_->getLease4(yiaddr); + ASSERT_TRUE(after_lease); + + // Context contents should match the expected context content. + ConstElementPtr exp_context; + ASSERT_NO_THROW_LOG(exp_context = Element::fromJSON(scenario.exp_context_)); + ASSERT_EQ(*(after_lease->getContext()), *exp_context); + } +} + TEST_F(Lease4CmdsTest, lease4AddMissingParams) { testLease4AddMissingParams(); } @@ -4210,4 +4352,13 @@ TEST_F(Lease4CmdsTest, lease4WriteMultiThreading) { testLease4Write(); } +TEST_F(Lease4CmdsTest, validLeases4Committed) { + testValidLeases4Committed(); +} + +TEST_F(Lease4CmdsTest, validLeases4CommittedMultiThreading) { + MultiThreadingTest mt(true); + testValidLeases4Committed(); +} + } // end of anonymous namespace -- 2.47.3