[#3110] Clean up and more tests

Added a ChangeLog entry

src/hooks/dhcp/high_availability/ha_impl.cc
    HAImpl::lease4ServerDecline() - always return NEXT_STEP_CONTINUE,
    and set peers_to_update argument.

src/hooks/dhcp/high_availability/ha_service.*
    Renamed HAService::asyncSendLeaseUpdate() to asyncSendSingleLeaseUpdate()

src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc
    TEST_F(HAImplTest, lease4ServerDecline) - new test

src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
    TEST_F(HAServiceTest, successfulSendSingleLeaseUpdateWithoutParking)
    TEST_F(HAServiceTest, successfulSendSingleLeaseUpdateWithParking) - new tests

doc/sphinx/arm/hooks-ping-check.rst
    Removed note about HA updates

doc/sphinx/arm/hooks.rst
    Updated ping-check description in table of hooks

doc/sphinx/arm/logging.rst
    Added ping-check-hooks logger to table of loggers

ChangeLog

@@ -1,3 +1,9 @@
+2195.	[func]		tmark
+	Added a new hook point to kea-dhcp4, "lease4_server_decline".
+	DHCPv4 leases declined by ping-check hook library are now
+	propagated to HA peers.
+	(Gitlab #3110)
+
 Kea 2.5.4 (development) released on November 29, 2023
 
 2194.	[build]		razvan

doc/sphinx/arm/hooks-ping-check.rst

@@ -171,9 +171,3 @@ The following excerpt demonstrates subnet level configuration:
 .. note::
 
     Ping checking is currently only supported when Kea is configured for multi-threaded operation.
-
-.. note::
-
-    Ping checking is not yet fully integrated with High Availability (HA). When ping checking
-    concludes that an address is not available and a declined lease is created, no corresponding
-    lease update is sent to HA peer(s).

doc/sphinx/arm/hooks.rst

@@ -533,7 +533,6 @@ loaded by the correct process per the table below.
    | :ref:`Ping Check <hooks-ping-check>`                      | ISC support  | With this hook library, :iscman:`kea-dhcp4` server can       |
    |                                                           | customers    | perform ping checks of candidate lease addresses before      |
    |                                                           |              | offering them to clients.                                    |
-   |                                                           |              | This library is under development and not yet functional.    |
    +-----------------------------------------------------------+--------------+--------------------------------------------------------------+
    | :ref:`PostgreSQL Configuration Backend <hooks-cb-pgsql>`  | Kea open     | This hook library is an implementation of the Kea            |
    |                                                           | source       | Configuration Backend for PostgreSQL. It uses a PostgreSQL   |

doc/sphinx/arm/logging.rst

@@ -427,6 +427,11 @@ libraries), or hook libraries (open source or premium).
    |                                  |                                       | message carried in             |
    |                                  |                                       | the packet is parsed.          |
    +----------------------------------+---------------------------------------+--------------------------------+
+   | ``kea-dhcp4.ping-check-hooks``   | :ischooklib:`libdhcp_ping_check.so`   | Used                           |
+   |                                  | subscription hook library             | to log messages related to     |
+   |                                  |                                       | carrying out pre-offer ping    |
+   |                                  |                                       | checks of candidate leases.    |
+   +----------------------------------+---------------------------------------+--------------------------------+
    | ``kea-dhcp4.pgsql-cb-hooks``,    | :ischooklib:`libdhcp_pgsql_cb.so`     | Used                           |
    | ``kea-dhcp6.pgsql-cb-hooks``     | open-source hook library              | to log messages                |
    |                                  |                                       | related to the                 |

src/hooks/dhcp/high_availability/ha_impl.cc

@@ -187,39 +187,37 @@ HAImpl::leases4Committed(CalloutHandle& callout_handle) {
 
 void
 HAImpl::lease4ServerDecline(CalloutHandle& callout_handle) {
+    // Always return CONTINUE.
+    callout_handle.setStatus(CalloutHandle::NEXT_STEP_CONTINUE);
+    size_t peers_to_update = 0;
+
     // If the hook library is configured to not send lease updates to the
     // partner, there is nothing to do because this whole callout is
     // currently about sending lease updates.
     if (!config_->amSendingLeaseUpdates()) {
         // No need to log it, because it was already logged when configuration
         // was applied.
+        callout_handle.setArgument("peers_to_update", peers_to_update);
         return;
     }
 
-    Pkt4Ptr query4;
-    Lease4Ptr lease4;
-
     // Get all arguments available for the lease4_server_decline hook point.
     // If any of these arguments is not available this is a programmatic
     // error. An exception will be thrown which will be caught by the
     // caller and logged.
+    Pkt4Ptr query4;
     callout_handle.getArgument("query4", query4);
+
+    Lease4Ptr lease4;
     callout_handle.getArgument("lease4", lease4);
 
-    // Get the parking lot for this hook point. We're going to remember this
+    // Asynchronously send the lease update. In some cases no updates will be sent,
-    // pointer until we unpark the packet.
-    ParkingLotHandlePtr parking_lot = callout_handle.getParkingLotHandlePtr();
-
-    // Asynchronously send lease updates. In some cases no updates will be sent,
     // e.g. when this server is in the partner-down state and there are no backup
-    // servers. In those cases we simply return without parking the DHCP query.
+    // servers.
-    // The response will be sent to the client immediately.
+    peers_to_update = service_->asyncSendSingleLeaseUpdate(query4, lease4, 0);
-    service_->asyncSendLeaseUpdate(query4, lease4, parking_lot);
+    callout_handle.setArgument("peers_to_update", peers_to_update);
-
-    callout_handle.setStatus(CalloutHandle::NEXT_STEP_CONTINUE);
 }
 
-
 void
 HAImpl::buffer6Receive(hooks::CalloutHandle& callout_handle) {
     Pkt6Ptr query6;

src/hooks/dhcp/high_availability/ha_service.cc

@@ -1244,7 +1244,7 @@ HAService::asyncSendLeaseUpdates(const dhcp::Pkt4Ptr& query,
 }
 
 size_t
-HAService::asyncSendLeaseUpdate(const dhcp::Pkt4Ptr& query,
+HAService::asyncSendSingleLeaseUpdate(const dhcp::Pkt4Ptr& query,
                                 const dhcp::Lease4Ptr& lease,
                                 const hooks::ParkingLotHandlePtr& parking_lot) {
 

src/hooks/dhcp/high_availability/ha_service.h

@@ -567,7 +567,7 @@ public:
     /// @param leases Pointer to a collection of the newly allocated or
     /// updated leases.
     /// @param deleted_leases Pointer to a collection of the released leases.
-    /// @param [out] parking_lot Pointer to the parking lot handle available
+    /// @param parking_lot Pointer to the parking lot handle available
     /// for the "leases4_committed" hook point. This is where the DHCP client
     /// message is parked. This method calls @c unpark() on this object when
     /// the asynchronous updates are completed.
@@ -580,9 +580,38 @@ public:
                                  const dhcp::Lease4CollectionPtr& deleted_leases,
                                  const hooks::ParkingLotHandlePtr& parking_lot);
 
-    size_t asyncSendLeaseUpdate(const dhcp::Pkt4Ptr& query,
+    /// @brief Schedules an asynchronous IPv4 lease update.
-                                const dhcp::Lease4Ptr& lease,
+    ///
-                                const hooks::ParkingLotHandlePtr& parking_lot);
+    /// This method schedules an asynchronous lease update for a single lease.
+    /// It is currently only used for "lease4_server_decline" callout.
+    /// The lease update is transmitted over HTTP to the peers specified in
+    /// the configuration (except self).
+    /// If the server is in the partner-down state the lease update is not
+    /// sent to the partner but is sent to all backup servers.
+    /// In other states in which the server responds to DHCP queries, the
+    /// lease update is sent to all servers. The scheduled lease update
+    /// is performed after the callouts return. The server may or may not
+    /// parks the processed DHCP packet and runs IO service shared between
+    /// the server and the hook library.
+    ////
+    /// If the lease update to the partner (primary, secondary or standby)
+    /// fails, the packet, if parked, is dropped. If the lease update to
+    /// any of the backup server fails, an error message is logged but the DHCP
+    /// packet is not dropped.
+    ///
+    /// @param query Pointer to the processed DHCP client message.
+    /// @param lease Pointer to the updated lease
+    /// @param parking_lot Pointer to the parking lot handle available
+    /// for the hook point if one.  This is where the DHCP client message is
+    /// parked if it is parked. This method calls @c unpark() on this object
+    /// when the asynchronous updates are completed.
+    ///
+    /// @return Number of peers to whom the lease update has been scheduled
+    /// to be sent and from which we expect a response prior to unparking
+    /// the packet and sending a response to the DHCP client.
+    size_t asyncSendSingleLeaseUpdate(const dhcp::Pkt4Ptr& query,
+                                      const dhcp::Lease4Ptr& lease,
+                                      const hooks::ParkingLotHandlePtr& parking_lot);
 
     /// @brief Schedules asynchronous IPv6 lease updates.
     ///

src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc

@@ -42,6 +42,9 @@ struct TestHooks {
     /// @brief Index of leases6_committed callout.
     int hook_index_leases6_committed_;
 
+    /// @brief Index of lease4_server_decline callout.
+    int hook_index_lease4_server_decline_;
+
     /// @brief Constructor
     ///
     /// The constructor registers hook points for callout tests.
@@ -50,6 +53,8 @@ struct TestHooks {
             HooksManager::registerHook("leases4_committed");
         hook_index_leases6_committed_ =
             HooksManager::registerHook("leases6_committed");
+        hook_index_lease4_server_decline_ =
+            HooksManager::registerHook("lease4_server_decline");
     }
 };
 
@@ -979,6 +984,7 @@ TEST_F(HAImplTest, haResetNoServerName) {
 // Test ha-reset command handler with a wrong server name.
 TEST_F(HAImplTest, haResetBadServerName) {
     HAImpl ha_impl;
+
     ASSERT_NO_THROW(ha_impl.configure(createValidJsonConfiguration()));
 
     // Starting the service is required prior to running any callouts.
@@ -1284,5 +1290,65 @@ TEST_F(HAImplTest, haScopesBadServerName) {
     checkAnswer(response, CONTROL_RESULT_ERROR, "server5 matches no configured 'server-name'");
 }
 
+// Tests lease4_server_decline callout implementation.
+TEST_F(HAImplTest, lease4ServerDecline) {
+    // Create implementation object and configure it.
+    TestHAImpl ha_impl;
+    ASSERT_NO_THROW(ha_impl.startService(io_service_, network_state,
+                                         HAServerType::DHCPv4));
+
+    // Make sure we wait for the acks from the backup server to be able to
+    // test the case of sending lease updates even though the service is
+    // in the state in which the lease updates are normally not sent.
+    ha_impl.config_->setWaitBackupAck(true);
+
+    // Create callout handle to be used for passing arguments to the
+    // callout.
+    CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
+    ASSERT_TRUE(callout_handle);
+
+    // Set the hook index so we can park packets.
+    callout_handle->setCurrentHook(test_hooks.hook_index_lease4_server_decline_);
+
+    // query4
+    Pkt4Ptr query4 = createMessage4(DHCPDISCOVER, 1, 0, 0);
+    callout_handle->setArgument("query4", query4);
+
+    // Create a lease and pass it to the callout.
+    HWAddrPtr hwaddr(new HWAddr(std::vector<uint8_t>(6, 1), HTYPE_ETHER));
+    Lease4Ptr lease4(new Lease4(IOAddress("192.1.2.3"), hwaddr,
+                                static_cast<const uint8_t*>(0), 0,
+                                60, 0, 1));
+
+    callout_handle->setArgument("lease4", lease4);
+
+    // Set initial status.
+    callout_handle->setStatus(CalloutHandle::NEXT_STEP_CONTINUE);
+
+    ha_impl.config_->setSendLeaseUpdates(false);
+
+    // Run the callout.
+    ASSERT_NO_THROW(ha_impl.lease4ServerDecline(*callout_handle));
+
+    // Status should be continue.
+    EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus());
+
+    size_t peers_to_update;
+    ASSERT_NO_THROW_LOG(callout_handle->getArgument("peers_to_update", peers_to_update));
+    EXPECT_EQ(peers_to_update, 0);
+
+    // Enable updates and retry.
+    ha_impl.config_->setSendLeaseUpdates(true);
+    callout_handle->setArgument("lease4", lease4);
+
+    // Run the callout again.
+    ASSERT_NO_THROW(ha_impl.lease4ServerDecline(*callout_handle));
+
+    // Status should be continue.
+    EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus());
+
+    ASSERT_NO_THROW_LOG(callout_handle->getArgument("peers_to_update", peers_to_update));
+    EXPECT_EQ(peers_to_update, 1);
+}
 
 }

src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc

@@ -2126,6 +2126,156 @@ public:
         io_service_->poll();
     }
 
+    /// @brief Tests scenarios when a single lease update is sent to a partner while
+    /// the partner is online or offline, using asyncSendLeaseUpdate().
+    ///
+    /// @param unpark_handler a function called when packet is unparked.  If empty,
+    /// test assumes packet parking is not in use.
+    /// @param should_fail indicates if the update is expected to be unsuccessful.
+    /// @param num_updates expected number of servers to which lease updates are
+    /// sent.
+    /// @param my_state state of the server while lease updates are sent.
+    /// @param wait_backup_ack indicates if the server should wait for the acknowledgment
+    /// from the backup servers.
+    /// @param create_service a boolean flag indicating whether the test should
+    /// re-create HA service and communication state.
+    void testSendLeaseUpdate(std::function<void()> unpark_handler,
+                             const bool should_fail,
+                             const size_t num_updates,
+                             const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
+                             const bool wait_backup_ack = false,
+                             const bool create_service = true) {
+        // Create parking lot where query is going to be parked and unparked.
+        ParkingLotPtr parking_lot;
+        ParkingLotHandlePtr parking_lot_handle;
+        if (unpark_handler) {
+            parking_lot.reset(new ParkingLot());
+            parking_lot_handle.reset(new ParkingLotHandle(parking_lot));
+        }
+
+        // Create query.
+        Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 1234));
+
+        // Create the update lease.
+        HWAddrPtr hwaddr(new HWAddr(std::vector<uint8_t>(6, 1), HTYPE_ETHER));
+        Lease4Ptr lease4(new Lease4(IOAddress("192.1.2.3"), hwaddr,
+                                    static_cast<const uint8_t*>(0), 0,
+                                    60, 0, 1));
+        if (create_service) {
+            // Create HA configuration for 3 servers. This server is
+            // server 1.
+            HAConfigPtr config_storage = createValidConfiguration();
+            config_storage->setWaitBackupAck(wait_backup_ack);
+            // Let's override the default value. The lower value makes it easier
+            // for some unit tests to simulate the server's overflow. Simulating it
+            // requires appending leases to the backlog. It is easier to add 10
+            // than 100.
+            config_storage->setDelayedUpdatesLimit(10);
+            setBasicAuth(config_storage);
+
+            // The communication state is the member of the HAServce object. We have to
+            // replace this object with our own implementation to have an ability to
+            // modify its poke time.
+            NakedCommunicationState4Ptr state(new NakedCommunicationState4(io_service_,
+                                                                       config_storage));
+            // Set poke time 30s in the past. If the state is poked it will be reset
+            // to the current time. This allows for testing whether the object has been
+            // poked by the HA service.
+            state->modifyPokeTime(-30);
+
+            // Create HA service.
+            createSTService(network_state_, config_storage);
+            service_->communication_state_ = state;
+        }
+
+        service_->transition(my_state.state_, HAService::NOP_EVT);
+
+        // Schedule lease updates.
+        EXPECT_EQ(num_updates,
+                  service_->asyncSendSingleLeaseUpdate(query, lease4, parking_lot_handle));
+
+        // Verify we have the right number of updates pending.
+        EXPECT_EQ(num_updates, service_->getPendingRequest(query));
+
+        if (parking_lot) {
+            // Let's park the packet and associate it with the callback function which
+            // simply records the fact that it has been called. We expect that it wasn't
+            // because the parked packet should be dropped as a result of lease updates
+            // failures.
+            ASSERT_NO_THROW(parking_lot->park(query, unpark_handler));
+            ASSERT_NO_THROW(parking_lot->reference(query));
+        }
+
+        // Actually perform the lease updates.
+        ASSERT_NO_THROW(runIOService(TEST_TIMEOUT, [this]() {
+            // Finish running IO service when there are no more pending requests.
+            return (service_->pendingRequestSize() == 0);
+        }));
+
+        // Only if we wait for lease updates to complete it makes sense to test
+        // that the packet was either dropped or unparked.
+        if (parking_lot && num_updates > 0) {
+            // Try to drop the packet. We expect that the packet has been already
+            // dropped so this should return false.
+            EXPECT_FALSE(parking_lot_handle->drop(query));
+        }
+
+        // The updates should not be sent to this server.
+        EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty());
+
+        if (should_fail) {
+            EXPECT_EQ(HA_UNAVAILABLE_ST, service_->communication_state_->getPartnerState());
+        } else {
+            EXPECT_NE(service_->communication_state_->getPartnerState(), HA_UNAVAILABLE_ST);
+        }
+    }
+
+    /// @brief Tests successful scenarios when a single lease update is done using
+    /// sendLeaseUpdate().
+    ///
+    /// @param with_parking True if packet parking should be used, false is not.
+    void testSuccessSendSingleLeaseUpdate(bool with_parking) {
+        // Start HTTP servers.
+        ASSERT_NO_THROW({
+                listener_->start();
+                listener2_->start();
+                listener3_->start();
+        });
+
+        if (with_parking) {
+            // This flag will be set to true if unpark is called.
+            bool unpark_called = false;
+            testSendLeaseUpdate([&unpark_called] { unpark_called = true; },
+                                false, 1);
+            // Expecting that the packet was unparked because lease
+            // updates are expected to be successful.
+            EXPECT_TRUE(unpark_called);
+        } else {
+            testSendLeaseUpdate(0, false, 1);
+        }
+
+        // Updates have been sent so this counter should remain 0.
+        EXPECT_EQ(0, service_->communication_state_->getUnsentUpdateCount());
+
+        // The server 2 should have received two commands.
+        EXPECT_EQ(1, factory2_->getResponseCreator()->getReceivedRequests().size());
+
+        // Check that the server 2 has received lease4-update command.
+        auto update_request2 =
+            factory2_->getResponseCreator()->findRequest("lease4-update",
+                                                         "192.1.2.3");
+        EXPECT_TRUE(update_request2);
+
+        // Lease update should be successfully sent to server3.
+        EXPECT_EQ(1, factory3_->getResponseCreator()->getReceivedRequests().size());
+
+        // Check that the server 3 has received lease4-update command.
+        auto update_request3 =
+            factory3_->getResponseCreator()->findRequest("lease4-update",
+                                                         "192.1.2.3");
+        EXPECT_TRUE(update_request3);
+    }
+
     /// @brief Pointer to the HA service under test.
     TestHAServicePtr service_;
 
@@ -8041,4 +8191,16 @@ TEST_F(HAServiceStateMachineTest, doNotTerminateWhenPartnerUnavailable) {
     EXPECT_EQ(HA_COMMUNICATION_RECOVERY_ST, service_->getCurrState());
 }
 
+// Test scenario when a single lease4 update is sent successfully, parking is not
+// employed.
+TEST_F(HAServiceTest, successfulSendSingleLeaseUpdateWithoutParking) {
+    testSuccessSendSingleLeaseUpdate(false);
+}
+
+// Test scenario when a single lease4 update is sent successfully, parkin is
+// employed.
+TEST_F(HAServiceTest, successfulSendSingleLeaseUpdateWithParking) {
+    testSuccessSendSingleLeaseUpdate(true);
+}
+
 } // end of anonymous namespace