From: Francis Dupont Date: Tue, 5 May 2020 21:24:48 +0000 (+0200) Subject: [#1147] Checkpoint: finished v6 same client X-Git-Tag: Kea-1.7.9~143 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f99f8da70cbab96e2dc0ed4b1dd3b797af0f900d;p=thirdparty%2Fkea.git [#1147] Checkpoint: finished v6 same client --- diff --git a/src/bin/dhcp6/client_handler.cc b/src/bin/dhcp6/client_handler.cc index 9ca18e5306..964633b242 100644 --- a/src/bin/dhcp6/client_handler.cc +++ b/src/bin/dhcp6/client_handler.cc @@ -7,7 +7,9 @@ #include #include +#include #include +#include using namespace std; @@ -29,35 +31,36 @@ ClientHandler::~ClientHandler() { locked_.reset(); } -ClientHandler::Client::Client(Pkt6Ptr query) +ClientHandler::Client::Client(Pkt6Ptr query, DuidPtr client_id) : query_(query), thread_(this_thread::get_id()) { if (!query) { isc_throw(InvalidParameter, "null query in ClientHandler"); } - if (!query->getClientId()) { - isc_throw(InvalidParameter, "query has no client Id in ClientHandler"); + if (!client_id) { + isc_throw(InvalidParameter, "null client-id in ClientHandler"); } + duid_ = client_id->getDuid(); } -Pkt6Ptr +ClientHandler::ClientPtr ClientHandler::lookup(const DuidPtr& duid) { if (!duid) { isc_throw(InvalidParameter, "duid is null in ClientHandler::lookup"); } auto it = clients_.find(duid->getDuid()); if (it == clients_.end()) { - return (Pkt6Ptr()); + return (0); } - return (it->query_); + return (ClientPtr(new Client(*it))); } void -ClientHandler::lock() { +ClientHandler::lock(Client client) { if (!locked_) { isc_throw(Unexpected, "nothing to lock in ClientHandler::lock"); } - Client client(locked_); - clients_.insert(Client(locked_)); + // Assume insert will never fail so not checking its result. + clients_.insert(client); } void @@ -65,16 +68,8 @@ ClientHandler::unLock() { if (!locked_) { isc_throw(Unexpected, "nothing to unlock in ClientHandler::unLock"); } - const DuidPtr& duid = locked_->getClientId(); - if (!duid) { - isc_throw(Unexpected, "no duid unlock in ClientHandler::unLock"); - } - auto it = clients_.find(duid->getDuid()); - if (it == clients_.end()) { - // Should not happen - return; - } - clients_.erase(it); + // Assume erase will never fail so not checking its result. + clients_.erase(locked_->getDuid()); } bool @@ -94,15 +89,28 @@ ClientHandler::tryLock(Pkt6Ptr query) { // A lot of code assumes this will never happen... isc_throw(Unexpected, "empty DUID in ClientHandler::tryLock"); } - lock_guard lock_(mutex_); - const Pkt6Ptr& duplicate = lookup(duid); - if (duplicate) { - // Should log. - return (true); + ClientPtr holder = 0; + { + // Try to acquire the lock and return the holder when it failed. + lock_guard lock_(mutex_); + holder = lookup(duid); + if (!holder) { + locked_ = duid; + lock(Client(query, duid)); + return (false); + } } - locked_ = query; - lock(); - return (false); + // This query is a duplicate: currently it is simply dropped. + // Logging a warning as it is supposed to be a rare event + // with well behaving clients... + LOG_WARN(bad_packet6_logger, DHCP6_PACKET_DROP_DUPLICATE) + .arg(query->toText()) + .arg(this_thread::get_id()) + .arg(holder->query_->toText()) + .arg(holder->thread_); + stats::StatsMgr::instance().addValue("pkt6-receive-drop", + static_cast(1)); + return (true); } } // namespace dhcp diff --git a/src/bin/dhcp6/client_handler.h b/src/bin/dhcp6/client_handler.h index 2d1396a343..e43f589cc1 100644 --- a/src/bin/dhcp6/client_handler.h +++ b/src/bin/dhcp6/client_handler.h @@ -10,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -46,25 +45,25 @@ private: /// @brief Constructor. /// /// @param query The query. + /// @param client_id The client ID. /// @throw if the query is null or has empty client ID. - Client(Pkt6Ptr query); + Client(Pkt6Ptr query, DuidPtr client_id); /// @brief The query being processed. Pkt6Ptr query_; + /// @brief Cached binary client ID. + std::vector duid_; + /// @brief The ID of the thread processing the query. std::thread::id thread_; - - /// @brief Key extractor. - /// - /// Returns the content of the Duid aka client ID. - const std::vector& getClientId() const { - return (query_->getClientId()->getDuid()); - } }; - /// @brief Query locked by this handler. - Pkt6Ptr locked_; + /// @brief The type of unique pointers to clients. + typedef std::unique_ptr ClientPtr; + + /// @brief Client ID locked by this handler. + DuidPtr locked_; /// @brief Mutex to protect the client container. static std::mutex mutex_; @@ -74,13 +73,15 @@ private: /// The mutex must be held by the caller. /// /// @param duid The duid of the query from the client. - /// @return The query holding the client or null. - static Pkt6Ptr lookup(const DuidPtr& duid); + /// @return The held client or null. + static ClientPtr lookup(const DuidPtr& duid); /// @brief Acquire a client. /// /// The mutex must be held by the caller. - void lock(); + /// + /// @param client The filled client object. + void lock(Client client); /// @brief Release a client. /// @@ -90,7 +91,7 @@ private: /// @brief The type of the client container. typedef boost::multi_index_container< - // This container stores clients. + // This container stores client objects. Client, // Start specification of indexes here. @@ -99,9 +100,9 @@ private: // First index is used to search by Duid. boost::multi_index::hashed_unique< - // Duid content is extracted by calling Client::getClientId() - boost::multi_index::const_mem_fun< - Client, const std::vector&, &Client::getClientId + // Client ID binary content as a member of the Client object. + boost::multi_index::member< + Client, std::vector, &Client::duid_ > > > diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index bc912a1b92..8a3142e8d0 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -526,6 +526,12 @@ after a specified amount of time since receiving "dhcp-disable" command. This debug message is emitted when an incoming packet was classified into the special class 'DROP' and dropped. The packet details are displayed. +% DHCP6_PACKET_DROP_DUPLICATE dropped as sent by the same client than a packet being processed by another thread: dropped %1 by thread %2 as duplicate of %3 processed by %4 +Currently multi-threading processing avoids races between packets sent by +the same client by dropping new packets until processing is finished. +Packet details and thread identifiers are included for both packets in +this warning message. + % DHCP6_PACKET_DROP_PARSE_FAIL failed to parse packet from %1 to %2, received over interface %3, reason: %4 The DHCPv6 server has received a packet that it is unable to interpret. The reason why the packet is invalid is included in the message. diff --git a/src/bin/dhcp6/tests/Makefile.am b/src/bin/dhcp6/tests/Makefile.am index df5e48421f..b26deb8270 100644 --- a/src/bin/dhcp6/tests/Makefile.am +++ b/src/bin/dhcp6/tests/Makefile.am @@ -82,6 +82,7 @@ TESTS += dhcp6_unittests # This list is ordered alphabetically. When adding new files, please maintain # this order. dhcp6_unittests_SOURCES = classify_unittests.cc +dhcp6_unittests_SOURCES += client_handler_unittest.cc dhcp6_unittests_SOURCES += config_parser_unittest.cc dhcp6_unittests_SOURCES += config_backend_unittest.cc dhcp6_unittests_SOURCES += confirm_unittest.cc diff --git a/src/bin/dhcp6/tests/client_handler_unittest.cc b/src/bin/dhcp6/tests/client_handler_unittest.cc new file mode 100644 index 0000000000..469e5c6508 --- /dev/null +++ b/src/bin/dhcp6/tests/client_handler_unittest.cc @@ -0,0 +1,286 @@ +// Copyright (C) 2020 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include +#include +#include +#include +#include + +using namespace isc; +using namespace isc::dhcp; +using namespace isc::dhcp::test; +using namespace isc::stats; +using namespace isc::util; + +namespace { + +/// @brief Test fixture class for testing client handler. +class ClientHandleTest : public ::testing::Test { +public: + + /// @brief Constructor. + /// + /// Creates the pkt6-receive-drop statistic. + ClientHandleTest() { + StatsMgr::instance().setValue("pkt6-receive-drop", static_cast(0)); + } + + /// @brief Destructor. + /// + /// Removes statistics. + ~ClientHandleTest() { + StatsMgr::instance().removeAll(); + } + + /// @brief Generates a client-id option. + /// + /// (from dhcp6_test_utils.h) + /// + /// @return A random client-id option. + OptionPtr generateClientId(uint8_t base = 100) { + const size_t len = 32; + OptionBuffer duid(len); + for (size_t i = 0; i < len; ++i) { + duid[i] = base + i; + } + return (OptionPtr(new Option(Option::V6, D6O_CLIENTID, duid))); + } + + /// @brief Check statistics. + /// + /// @param bumped True if pkt6-receive-drop shoud have been bumped by one, + /// false otherwise. + void checkStat(bool bumped) { + ObservationPtr obs = StatsMgr::instance().getObservation("pkt6-receive-drop"); + ASSERT_TRUE(obs); + if (bumped) { + EXPECT_EQ(1, obs->getInteger().first); + } else { + EXPECT_EQ(0, obs->getInteger().first); + } + } +}; + +// Verifies behavior with empty block. +TEST_F(ClientHandleTest, empty) { + try { + // Get a client handler. + ClientHandler client_handler; + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } +} + +// Verifies behavior with one query. +TEST_F(ClientHandleTest, oneQuery) { + // Get a query. + Pkt6Ptr sol(new Pkt6(DHCPV6_SOLICIT, 1234)); + sol->addOption(generateClientId()); + + try { + // Get a client handler. + ClientHandler client_handler; + + // Try to lock it. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } + checkStat(false); +} + +// Verifies behavior with two queries for the same client. +TEST_F(ClientHandleTest, sharedQueries) { + // Get two queries. + Pkt6Ptr sol(new Pkt6(DHCPV6_SOLICIT, 1234)); + Pkt6Ptr req(new Pkt6(DHCPV6_REQUEST, 2345)); + OptionPtr client_id = generateClientId(); + // Same client ID: same client. + sol->addOption(client_id); + req->addOption(client_id); + + try { + // Get a client handler. + ClientHandler client_handler; + + // Try to lock it with the solicit. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + + // Get a second client handler. + ClientHandler client_handler2; + + // Try to lock it with a request. + EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } + checkStat(true); +} + +// Verifies behavior with a sequence of two queries. +TEST_F(ClientHandleTest, sequence) { + // Get two queries. + Pkt6Ptr sol(new Pkt6(DHCPV6_SOLICIT, 1234)); + Pkt6Ptr req(new Pkt6(DHCPV6_REQUEST, 2345)); + OptionPtr client_id = generateClientId(); + // Same client ID: same client. + sol->addOption(client_id); + req->addOption(client_id); + + try { + // Get a client handler. + ClientHandler client_handler; + + // Try to lock it with the solicit. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } + + // As it is a different block the lock was released. + + try { + // Get a second client handler. + ClientHandler client_handler2; + + // Try to lock it with a request. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } + checkStat(false); +} + +// Verifies behavior with different clients. +TEST_F(ClientHandleTest, notSharedQueries) { + // Get two queries. + Pkt6Ptr sol(new Pkt6(DHCPV6_SOLICIT, 1234)); + Pkt6Ptr req(new Pkt6(DHCPV6_REQUEST, 2345)); + OptionPtr client_id = generateClientId(); + OptionPtr client_id2 = generateClientId(111); + // Different client ID: different client. + sol->addOption(client_id); + req->addOption(client_id2); + + try { + // Get a client handler. + ClientHandler client_handler; + + // Try to lock it with the solicit. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + + // Get a second client handler. + ClientHandler client_handler2; + + // Try to lock it with a request. + EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } + checkStat(false); +} + +// Verifies behavior without client ID. +TEST_F(ClientHandleTest, noClientId) { + // Get two queries. + Pkt6Ptr sol(new Pkt6(DHCPV6_SOLICIT, 1234)); + Pkt6Ptr req(new Pkt6(DHCPV6_REQUEST, 2345)); + // No client id: nothing to recognize the client. + + try { + // Get a client handler. + ClientHandler client_handler; + + // Try to lock it with the solicit. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + + // Get a second client handler. + ClientHandler client_handler2; + + // Try to lock it with a request. + EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } + checkStat(false); +} + +// Verifies the query is required. +TEST_F(ClientHandleTest, noQuery) { + Pkt6Ptr no_pkt; + + try { + // Get a client handler. + ClientHandler client_handler; + + EXPECT_THROW(client_handler.tryLock(no_pkt), InvalidParameter); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } +} + +// Verifies that double tryLock call fails. +TEST_F(ClientHandleTest, doubleTryLock) { + // Get a query. + Pkt6Ptr sol(new Pkt6(DHCPV6_SOLICIT, 1234)); + sol->addOption(generateClientId()); + + try { + // Get a client handler. + ClientHandler client_handler; + + // Try to lock it. + bool duplicate = false; + EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + + // Should return false (no duplicate). + EXPECT_FALSE(duplicate); + + // Try to lock a second time. + EXPECT_THROW(client_handler.tryLock(sol), Unexpected); + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } +} + +// Cannot verifies that empty client ID fails because getClientId() handles +// this condition and replaces it by no client ID. + +} // end of anonymous namespace