]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[#1147] Checkpoint: finished v6 same client
authorFrancis Dupont <fdupont@isc.org>
Tue, 5 May 2020 21:24:48 +0000 (23:24 +0200)
committerFrancis Dupont <fdupont@isc.org>
Tue, 26 May 2020 09:51:57 +0000 (11:51 +0200)
src/bin/dhcp6/client_handler.cc
src/bin/dhcp6/client_handler.h
src/bin/dhcp6/dhcp6_messages.mes
src/bin/dhcp6/tests/Makefile.am
src/bin/dhcp6/tests/client_handler_unittest.cc [new file with mode: 0644]

index 9ca18e530661d65409cab3cc08b29276aff0beb3..964633b242f1e507fb5d082ebef723d5721f3ba0 100644 (file)
@@ -7,7 +7,9 @@
 #include <config.h>
 
 #include <dhcp6/client_handler.h>
+#include <dhcp6/dhcp6_log.h>
 #include <exceptions/exceptions.h>
+#include <stats/stats_mgr.h>
 
 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<mutex> 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<mutex> 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<int64_t>(1));
+    return (true);
 }
 
 }  // namespace dhcp
index 2d1396a34349047cc40f2355b3917159858a5886..e43f589cc1dfca9cc52dfa41da5307cc04923dae 100644 (file)
@@ -10,7 +10,6 @@
 #include <dhcp/pkt6.h>
 #include <boost/noncopyable.hpp>
 #include <boost/multi_index_container.hpp>
-#include <boost/multi_index/mem_fun.hpp>
 #include <boost/multi_index/member.hpp>
 #include <boost/multi_index/hashed_index.hpp>
 #include <mutex>
@@ -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<uint8_t> 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<uint8_t>& 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<Client> 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<uint8_t>&, &Client::getClientId
+                // Client ID binary content as a member of the Client object.
+                boost::multi_index::member<
+                    Client, std::vector<uint8_t>, &Client::duid_
                 >
             >
         >
index bc912a1b920da691cabc2e475ce28d065b2853cf..8a3142e8d02005923c23fe995faea41bf9192301 100644 (file)
@@ -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.
index df5e48421f72f853999958076400fcf6b0260533..b26deb82705166e929099e6db995c81c77ada945 100644 (file)
@@ -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 (file)
index 0000000..469e5c6
--- /dev/null
@@ -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 <config.h>
+#include <dhcp/option.h>
+#include <dhcp6/client_handler.h>
+#include <dhcp6/tests/dhcp6_test_utils.h>
+#include <stats/stats_mgr.h>
+
+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<int64_t>(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