From: Marcin Siodelski Date: Sun, 18 Sep 2022 18:22:39 +0000 (+0200) Subject: [#2408] HA interprets conflict status code X-Git-Tag: Kea-2.3.1~45 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2a9fbfc5955ff41d22846ad11609cfc8fd336939;p=thirdparty%2Fkea.git [#2408] HA interprets conflict status code --- diff --git a/src/hooks/dhcp/high_availability/communication_state.cc b/src/hooks/dhcp/high_availability/communication_state.cc index 60bccca80d..cb47b02ddf 100644 --- a/src/hooks/dhcp/high_availability/communication_state.cc +++ b/src/hooks/dhcp/high_availability/communication_state.cc @@ -701,6 +701,26 @@ CommunicationState4::reportRejectedLeaseUpdate(const boost::shared_ptr& message) { + Pkt4Ptr msg = boost::dynamic_pointer_cast(message); + if (!msg) { + isc_throw(BadValue, "DHCP message for which the lease update was successful is not a DHCPv4 message"); + } + std::vector client_id; + OptionPtr opt_client_id = msg->getOption(DHO_DHCP_CLIENT_IDENTIFIER); + if (opt_client_id) { + client_id = opt_client_id->getData(); + } + auto& idx = rejected_clients_.get<0>(); + auto existing_client = idx.find(boost::make_tuple(msg->getHWAddr()->hwaddr_, client_id)); + if (existing_client != idx.end()) { + idx.erase(existing_client); + return (true); + } + return (false); +} + void CommunicationState4::clearRejectedLeaseUpdates() { rejected_clients_.clear(); @@ -858,6 +878,25 @@ CommunicationState6::reportRejectedLeaseUpdate(const boost::shared_ptr& message) { + Pkt6Ptr msg = boost::dynamic_pointer_cast(message); + if (!msg) { + isc_throw(BadValue, "DHCP message for which the lease update was successful is not a DHCPv6 message"); + } + OptionPtr duid = msg->getOption(D6O_CLIENTID); + if (!duid) { + return (false); + } + auto& idx = rejected_clients_.get<0>(); + auto existing_client = idx.find(duid->getData()); + if (existing_client != idx.end()) { + idx.erase(existing_client); + return (true); + } + return (false); +} + void CommunicationState6::clearRejectedLeaseUpdates() { rejected_clients_.clear(); diff --git a/src/hooks/dhcp/high_availability/communication_state.h b/src/hooks/dhcp/high_availability/communication_state.h index 3229d1aba6..912e412ed9 100644 --- a/src/hooks/dhcp/high_availability/communication_state.h +++ b/src/hooks/dhcp/high_availability/communication_state.h @@ -306,6 +306,17 @@ public: /// otherwise. virtual bool reportRejectedLeaseUpdate(const boost::shared_ptr& message) = 0; + /// @brief Marks the lease update successful. + /// + /// If the lease update was previously marked "in conflict", it is + /// now cleared, effectively lowering the number of conflicted leases. + /// + /// @param message DHCP message for which the lease update was + /// successful. + /// @return true when the lease was marked "in conflict" and it is + /// now cleared. + virtual bool reportSuccessfulLeaseUpdate(const boost::shared_ptr& message) = 0; + /// @brief Clears rejected client leases. virtual void clearRejectedLeaseUpdates() = 0; @@ -690,6 +701,17 @@ public: /// otherwise. virtual bool reportRejectedLeaseUpdate(const boost::shared_ptr& message); + /// @brief Marks the lease update successful. + /// + /// If the lease update was previously marked "in conflict", it is + /// now cleared, effectively lowering the number of conflicted leases. + /// + /// @param message DHCP message for which the lease update was + /// successful. + /// @return true when the lease was marked "in conflict" and it is + /// now cleared. + virtual bool reportSuccessfulLeaseUpdate(const boost::shared_ptr& message); + /// @brief Clears rejected client leases. virtual void clearRejectedLeaseUpdates(); @@ -869,6 +891,17 @@ public: /// otherwise. virtual bool reportRejectedLeaseUpdate(const boost::shared_ptr& message); + /// @brief Marks the lease update successful. + /// + /// If the lease update was previously marked "in conflict", it is + /// now cleared, effectively lowering the number of conflicted leases. + /// + /// @param message DHCP message for which the lease update was + /// successful. + /// @return true when the lease was marked "in conflict" and it is + /// now cleared. + virtual bool reportSuccessfulLeaseUpdate(const boost::shared_ptr& message); + /// @brief Clears rejected client leases. virtual void clearRejectedLeaseUpdates(); diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index ff99aba466..9aa4d276c0 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -1436,10 +1436,22 @@ HAService::asyncSendLeaseUpdate(const QueryPtrType& query, // We don't care about the result of the lease update to the backup server. // It is a best effort update. - if ((config->getRole() != HAConfig::PeerConfig::BACKUP) && !lease_update_conflict && !lease_update_success) { - // If we were unable to communicate with the partner we set partner's + if (config->getRole() != HAConfig::PeerConfig::BACKUP) { + // If the lease update was unsuccessful we may need to set the partner // state as unavailable. - communication_state_->setPartnerState("unavailable"); + if (!lease_update_success) { + // Do not set it as unavailable if it was a conflict because the + // partner actually responded. + if (!lease_update_conflict) { + // If we were unable to communicate with the partner we set partner's + // state as unavailable. + communication_state_->setPartnerState("unavailable"); + } + } else if (communication_state_->getRejectedLeaseUpdatesCount() > 0) { + // Lease update successful and we may need to clear some previously + // rejected lease updates. + communication_state_->reportSuccessfulLeaseUpdate(query); + } } // It is possible to configure the server to not wait for a response from diff --git a/src/hooks/dhcp/high_availability/tests/communication_state_unittest.cc b/src/hooks/dhcp/high_availability/tests/communication_state_unittest.cc index 4ae8cd4d79..0bc626d4e7 100644 --- a/src/hooks/dhcp/high_availability/tests/communication_state_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/communication_state_unittest.cc @@ -114,6 +114,10 @@ public: /// @brief Test that gathering rejected leases works fine in DHCPv4 case. void reportRejectedLeasesV4Test(); + /// @brief Test that rejected leases are cleared after reporting respective + /// successful leases. + void reportSuccessfulLeasesV4Test(); + /// @brief Test checking that invalid values not accepted when reporting /// rejected leases. void reportRejectedLeasesV4InvalidValuesTest(); @@ -121,6 +125,10 @@ public: /// @brief Test that gathering rejected leases works fine in DHCPv4 case. void reportRejectedLeasesV6Test(); + /// @brief Test that rejected leases are cleared after reporting respective + /// successful leases. + void reportSuccessfulLeasesV6Test(); + /// @brief Test checking that invalid values not accepted when reporting /// rejected leases. void reportRejectedLeasesV6InvalidValuesTest(); @@ -711,20 +719,43 @@ CommunicationStateTest::reportRejectedLeasesV4Test() { state_.reportRejectedLeaseUpdate(msg); EXPECT_EQ(2, state_.getRejectedLeaseUpdatesCount()); - msg = createMessage4(DHCPREQUEST, 2, 0, 0); + msg = createMessage4(DHCPREQUEST, 2, 1, 0); state_.reportRejectedLeaseUpdate(msg); - EXPECT_EQ(2, state_.getRejectedLeaseUpdatesCount()); + EXPECT_EQ(3, state_.getRejectedLeaseUpdatesCount()); state_.clearRejectedLeaseUpdates(); EXPECT_EQ(0, state_.getRejectedLeaseUpdatesCount()); } +void +CommunicationStateTest::reportSuccessfulLeasesV4Test() { + EXPECT_EQ(0, state_.getRejectedLeaseUpdatesCount()); + auto msg0 = createMessage4(DHCPREQUEST, 1, 0, 0); + state_.reportRejectedLeaseUpdate(msg0); + EXPECT_EQ(1, state_.getRejectedLeaseUpdatesCount()); + + auto msg1 = createMessage4(DHCPREQUEST, 2, 0, 0); + state_.reportRejectedLeaseUpdate(msg1); + EXPECT_EQ(2, state_.getRejectedLeaseUpdatesCount()); + + EXPECT_TRUE(state_.reportSuccessfulLeaseUpdate(msg0)); + EXPECT_EQ(1, state_.getRejectedLeaseUpdatesCount()); + + auto msg2 = createMessage4(DHCPREQUEST, 1, 1, 0); + EXPECT_FALSE(state_.reportSuccessfulLeaseUpdate(msg2)); + EXPECT_EQ(1, state_.getRejectedLeaseUpdatesCount()); + + EXPECT_TRUE(state_.reportSuccessfulLeaseUpdate(msg1)); + EXPECT_EQ(0, state_.getRejectedLeaseUpdatesCount()); +} + void CommunicationStateTest::reportRejectedLeasesV4InvalidValuesTest() { // Using DHCPv6 message in the DHCPv4 context is a programming // error and deserves an exception. auto msg = createMessage6(DHCPV6_REQUEST, 1, 0); EXPECT_THROW(state_.reportRejectedLeaseUpdate(msg), BadValue); + EXPECT_THROW(state_.reportSuccessfulLeaseUpdate(msg), BadValue); } void @@ -746,16 +777,40 @@ CommunicationStateTest::reportRejectedLeasesV6Test() { EXPECT_EQ(0, state6_.getRejectedLeaseUpdatesCount()); } +void +CommunicationStateTest::reportSuccessfulLeasesV6Test() { + EXPECT_EQ(0, state6_.getRejectedLeaseUpdatesCount()); + auto msg0 = createMessage6(DHCPV6_SOLICIT, 1, 0); + EXPECT_TRUE(state6_.reportRejectedLeaseUpdate(msg0)); + EXPECT_EQ(1, state6_.getRejectedLeaseUpdatesCount()); + + auto msg1 = createMessage6(DHCPV6_SOLICIT, 2, 0); + EXPECT_TRUE(state6_.reportRejectedLeaseUpdate(msg1)); + EXPECT_EQ(2, state6_.getRejectedLeaseUpdatesCount()); + + EXPECT_TRUE(state6_.reportSuccessfulLeaseUpdate(msg0)); + EXPECT_EQ(1, state6_.getRejectedLeaseUpdatesCount()); + + auto msg2 = createMessage6(DHCPV6_SOLICIT, 3, 0); + EXPECT_FALSE(state6_.reportSuccessfulLeaseUpdate(msg2)); + EXPECT_EQ(1, state6_.getRejectedLeaseUpdatesCount()); + + EXPECT_TRUE(state6_.reportSuccessfulLeaseUpdate(msg1)); + EXPECT_EQ(0, state6_.getRejectedLeaseUpdatesCount()); +} + void CommunicationStateTest::reportRejectedLeasesV6InvalidValuesTest() { // Using DHCPv4 message in the DHCPv6 context is a programming // error and deserves an exception. auto msg0 = createMessage4(DHCPREQUEST, 1, 1, 0); EXPECT_THROW(state6_.reportRejectedLeaseUpdate(msg0), BadValue); + EXPECT_THROW(state6_.reportSuccessfulLeaseUpdate(msg0), BadValue); auto msg1 = createMessage6(DHCPV6_SOLICIT, 1, 0); msg1->delOption(D6O_CLIENTID); EXPECT_FALSE(state6_.reportRejectedLeaseUpdate(msg1)); + EXPECT_FALSE(state6_.reportSuccessfulLeaseUpdate(msg1)); } TEST_F(CommunicationStateTest, partnerStateTest) { @@ -911,6 +966,15 @@ TEST_F(CommunicationStateTest, reportRejectedLeasesV4TestMultiThreading) { reportRejectedLeasesV4Test(); } +TEST_F(CommunicationStateTest, reportSuccessfulLeasesV4Test) { + reportSuccessfulLeasesV4Test(); +} + +TEST_F(CommunicationStateTest, reportSuccessfulLeasesV4TestMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + reportSuccessfulLeasesV4Test(); +} + TEST_F(CommunicationStateTest, reportRejectedLeasesV4InvalidValuesTest) { reportRejectedLeasesV4InvalidValuesTest(); } @@ -929,6 +993,15 @@ TEST_F(CommunicationStateTest, reportRejectedLeasesV6TestMultiThreading) { reportRejectedLeasesV6Test(); } +TEST_F(CommunicationStateTest, reportSuccessfulLeasesV6Test) { + reportSuccessfulLeasesV6Test(); +} + +TEST_F(CommunicationStateTest, reportSuccessfulLeasesV6TestMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + reportSuccessfulLeasesV6Test(); +} + TEST_F(CommunicationStateTest, reportRejectedLeasesV6InvalidValuesTest) { reportRejectedLeasesV6InvalidValuesTest(); } diff --git a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc index fb53204293..70cf320b2a 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc @@ -802,22 +802,14 @@ public: /// @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 testSendLeaseUpdates(std::function 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) { - // 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); - + 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(new ParkingLot()); ParkingLotHandlePtr parking_lot_handle(new ParkingLotHandle(parking_lot)); @@ -840,22 +832,36 @@ public: 60, 0, 1)); deleted_leases4->push_back(deleted_lease4); - // 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_, + 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 and schedule lease updates. - createSTService(network_state_, config_storage); - service_->communication_state_ = state; + // 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_->asyncSendLeaseUpdates(query, leases4, deleted_leases4, parking_lot_handle)); @@ -892,9 +898,9 @@ public: EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty()); if (should_fail) { - EXPECT_EQ(HA_UNAVAILABLE_ST, state->getPartnerState()); + EXPECT_EQ(HA_UNAVAILABLE_ST, service_->communication_state_->getPartnerState()); } else { - EXPECT_NE(state->getPartnerState(), HA_UNAVAILABLE_ST); + EXPECT_NE(service_->communication_state_->getPartnerState(), HA_UNAVAILABLE_ST); } } @@ -908,17 +914,14 @@ public: /// @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 testSendLeaseUpdates6(std::function 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) { - // Create HA configuration for 3 servers. This server is - // server 1. - HAConfigPtr config_storage = createValidConfiguration(); - config_storage->setDelayedUpdatesLimit(10); - config_storage->setWaitBackupAck(wait_backup_ack); - setBasicAuth(config_storage); + 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(new ParkingLot()); @@ -940,22 +943,32 @@ public: duid, 1234, 50, 60, 1)); deleted_leases6->push_back(deleted_lease6); - // 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. - NakedCommunicationState6Ptr state(new NakedCommunicationState6(io_service_, + if (create_service) { + // Create HA configuration for 3 servers. This server is + // server 1. + HAConfigPtr config_storage = createValidConfiguration(); + config_storage->setDelayedUpdatesLimit(10); + config_storage->setWaitBackupAck(wait_backup_ack); + 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. + NakedCommunicationState6Ptr state(new NakedCommunicationState6(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 and schedule lease updates. - createSTService(network_state_, config_storage, HAServerType::DHCPv6); - service_->communication_state_ = state; + // 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, HAServerType::DHCPv6); + service_->communication_state_ = state; + } service_->transition(my_state.state_, HAService::NOP_EVT); + // Schedule lease updates. EXPECT_EQ(num_updates, service_->asyncSendLeaseUpdates(query, leases6, deleted_leases6, parking_lot_handle)); @@ -965,7 +978,8 @@ public: // and the deletions in a single bulk update command. EXPECT_EQ(num_updates, service_->getPendingRequest(query)); - EXPECT_FALSE(state->isPoked()); + EXPECT_FALSE(boost::dynamic_pointer_cast + (service_->communication_state_)->isPoked()); // 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 @@ -993,7 +1007,7 @@ public: EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty()); if (should_fail) { - EXPECT_EQ(HA_UNAVAILABLE_ST, state->getPartnerState()); + EXPECT_EQ(HA_UNAVAILABLE_ST, service_->communication_state_->getPartnerState()); } } @@ -1451,8 +1465,8 @@ public: EXPECT_FALSE(delete_request3); } - /// @brief Tests scenarios when one of the servers to which a - /// lease update is sent returns an error. + /// @brief Test the scenario when the servers receiving a lease update + /// return the conflict status code. void testSendUpdatesControlResultConflict() { // Instruct the server 2 to return an error as a result of receiving a command. factory2_->getResponseCreator()->setControlResult(CONTROL_RESULT_CONFLICT); @@ -1473,6 +1487,16 @@ public: // Ensure that the server has recorded a lease update conflict. The conflict // reported by the backup server should not count. EXPECT_EQ(1, service_->communication_state_->getRejectedLeaseUpdatesCount()); + + factory2_->getResponseCreator()->setControlResult(CONTROL_RESULT_SUCCESS); + factory3_->getResponseCreator()->setControlResult(CONTROL_RESULT_CONFLICT); + + bool unpark_called = false; + testSendLeaseUpdates([&unpark_called] { + unpark_called = true; + }, false, 1, MyState(HA_LOAD_BALANCING_ST), true, false); + EXPECT_TRUE(unpark_called); + EXPECT_EQ(0, service_->communication_state_->getRejectedLeaseUpdatesCount()); } /// @brief Tests scenarios when all lease updates are sent successfully.