]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[#3592] Addressed review comments
authorThomas Markwalder <tmark@isc.org>
Mon, 25 Nov 2024 14:52:20 +0000 (09:52 -0500)
committerThomas Markwalder <tmark@isc.org>
Tue, 26 Nov 2024 17:19:56 +0000 (17:19 +0000)
Addressed first round of comments.

Changes to be committed:
modified:   ChangeLog
modified:   doc/sphinx/arm/classify.rst
modified:   doc/sphinx/arm/dhcp4-srv.rst
modified:   doc/sphinx/arm/dhcp6-srv.rst
modified:   src/bin/dhcp4/tests/classify_unittest.cc
modified:   src/lib/dhcp/classify.cc
modified:   src/lib/dhcp/classify.h
modified:   src/lib/dhcpsrv/parsers/base_network_parser.cc
modified:   src/lib/dhcpsrv/parsers/base_network_parser.h
modified:   src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc

ChangeLog
doc/sphinx/arm/classify.rst
doc/sphinx/arm/dhcp4-srv.rst
doc/sphinx/arm/dhcp6-srv.rst
src/bin/dhcp4/tests/classify_unittest.cc
src/lib/dhcp/classify.cc
src/lib/dhcp/classify.h
src/lib/dhcpsrv/parsers/base_network_parser.cc
src/lib/dhcpsrv/parsers/base_network_parser.h
src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc

index ec7e411cc089be01b72bda29ea4519102ad3dd9d..2679445c859c52451740a715f034b8bcf9318f08 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
        added to HTTP responses.
        (Gitlab #3609)
 
-2304.  [func]          tmark
+2305.  [func]          tmark
        Both kea-dhcp4 and kea-dhcp6 servers will now
        log a warning message when they detect classes that
        configure lease life time parameters (e.g. 'valid-lifetime',
        'preferred-lifetime') while also setting
-       'only-in-addditiional-list' to true.
+       'only-in-additiional-list' to true.
        (Gitlab #2736)
 
-2303.  [bug]           tmark
+2304.  [bug]           tmark
        Modified both kea-dhcp4 and kea-dhcp6 to avoid
        generating DDNS update requests when leases are
        being reused due to lease caching.
index d47c1abf697ddfa54ab7089615a89a6d9c25a54f..e7a421869550f437ab52e68db46f1cbd1e3773a6 100644 (file)
@@ -1005,11 +1005,11 @@ Configuring Subnets With Class Information
     As of Kea 2.7.5, ``client-class`` (a single class name) has been replaced
     with ``client-classes`` (a list of one or more class names) and is now
     deprecated. It will still be accepted as input for a time to allow users
-    to migrate but will eventually be unsupported.
+    to migrate but will eventually be rejected.
 
 In certain cases it is beneficial to restrict access to certain subnets
 only to clients that belong to a given class, using the ``client-classes``
-parameter when defining the subnet.  This parameter may be used to sepcify
+parameter when defining the subnet.  This parameter may be used to specify
 a list of one or more classes to which clients must belong in order to
 use the subnet.
 
index 7af7ab2aee5cbd08efb9862a033038c2ba97d6ef..7ef183ff39b0aea7df24352d9693b007b0672115 100644 (file)
@@ -3499,7 +3499,7 @@ to always be evaluated to ``true``.
    As of Kea version 2.7.4, ``only-if-required`` and ``require-client-classes``
    have been renamed to ``only-in-additional-list`` and ``evaluate-additional-classes``
    respectivley.  The original names will still be accepted as input to allow
-   users to migrate but will eventually be unsupported.
+   users to migrate but will eventually be rejected.
 
 .. note::
 
@@ -6640,7 +6640,7 @@ for clients when client classification is in use, to ensure that the
 appropriate subnet is selected for a given client type.
 
 If a subnet is associated with one or more classes, only the clients belonging
-to at least one of these classes may this subnet. If there are no classes
+to at least one of these classes may use this subnet. If there are no classes
 specified for a subnet, any client connected to a given shared network can use
 this subnet. A common mistake is to assume that a subnet that includes a client
 class is preferred over subnets without client classes.
index 8088cd3ccfec77011d0625c0282fcbde720ee54f..43db93df028d7ce8b9f22d781b4ea4b8eea465ec 100644 (file)
@@ -3230,7 +3230,7 @@ to always be evaluated to ``true``.
    As of Kea version 2.7.4, ``only-if-required`` and ``require-client-classes``
    have been renamed to ``only-in-additional-list`` and ``evaluate-additional-classes``
    respectivley.  The original names will still be accepted as input to allow
-   users to migrate but will eventually be unsupported.
+   users to migrate but will eventually be rejected.
 
 .. _dhcp6-ddns-config:
 
index cd31b1d20e7677dc9e7b96bf6cabfa1fc406b184..51fc400f1b367c6ed7bc03b23fda8c21a6cf02f3 100644 (file)
@@ -2355,13 +2355,13 @@ TEST_F(ClassifyTest, networkScopeClientClasses) {
     };
 
     // Run scenarios.
-    for (const auto& scenario : scenarios) {
+    for (auto const& scenario : scenarios) {
         ClientId id(scenario.client_id_);
         SCOPED_TRACE(id.toText());
         Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 1234));
         query->addOption(OptionPtr(new Option(Option::V4,
-                                               DHO_DHCP_CLIENT_IDENTIFIER,
-                                               id.getClientId())));
+                                              DHO_DHCP_CLIENT_IDENTIFIER,
+                                              id.getClientId())));
         query->setIface("eth0");
         query->setIndex(ETH0_INDEX);
 
index 7a001ba6211cdcb980972cdf5e34f69c56925228..38943342befea55d2bbdf3533dd7bf28b1c3b10c 100644 (file)
@@ -65,8 +65,7 @@ ClientClasses::intersects(const ClientClasses& cclasses) const {
                 return (true);
             }
         }
-    }
-    else {
+    } else {
         for (const auto& cclass : cclasses) {
             if (contains(cclass)) {
                 return (true);
index 69cdffd41738b2a44414c169cdc475f697ba573c..8571cf990949f37e8c008217a8ffdeb9af0ba3ac 100644 (file)
@@ -8,6 +8,7 @@
 #define CLASSIFY_H
 
 #include <cc/data.h>
+#include <cc/cfg_to_element.h>
 
 #include <boost/multi_index_container.hpp>
 #include <boost/multi_index/member.hpp>
 /// @file   classify.h
 ///
 /// @brief Defines elements for storing the names of client classes
-/// /// This file defines common elements used to track the client classes /// that may be associated with a given packet.  In order to minimize the /// exposure of the DHCP library to server side concepts such as client
+///
+/// This file defines common elements used to track the client classes
+/// that may be associated with a given packet.  In order to minimize the
+/// exposure of the DHCP library to server side concepts such as client
 /// classification the classes herein provide a mechanism to maintain lists
 /// of class names, rather than the classes they represent.  It is the
 /// upper layers' prerogative to use these names as they see fit.
 namespace isc {
 namespace dhcp {
 
-    /// @brief Defines a single class name.
-    typedef std::string ClientClass;
-
-    /// @brief Tag for the sequence index.
-    struct ClassSequenceTag { };
-
-    /// @brief Tag for the name index.
-    struct ClassNameTag { };
-
-    /// @brief the client class multi-index.
-    typedef boost::multi_index_container<
-        ClientClass,
-        boost::multi_index::indexed_by<
-            // First index is the sequence one.
-            boost::multi_index::sequenced<
-                boost::multi_index::tag<ClassSequenceTag>
-            >,
-            // Second index is the name hash one.
-            boost::multi_index::hashed_unique<
-                boost::multi_index::tag<ClassNameTag>,
-                boost::multi_index::identity<ClientClass>
-            >
+/// @brief Defines a single class name.
+typedef std::string ClientClass;
+
+/// @brief Tag for the sequence index.
+struct ClassSequenceTag { };
+
+/// @brief Tag for the name index.
+struct ClassNameTag { };
+
+/// @brief the client class multi-index.
+typedef boost::multi_index_container<
+    ClientClass,
+    boost::multi_index::indexed_by<
+        // First index is the sequence one.
+        boost::multi_index::sequenced<
+            boost::multi_index::tag<ClassSequenceTag>
+        >,
+        // Second index is the name hash one.
+        boost::multi_index::hashed_unique<
+            boost::multi_index::tag<ClassNameTag>,
+            boost::multi_index::identity<ClientClass>
         >
-    > ClientClassContainer;
-
-    /// @brief Defines a subclass to template class relation.
-    struct SubClassRelation {
-        /// @brief Constructor.
-        SubClassRelation(const ClientClass& class_def, const ClientClass& subclass) :
-            class_def_(class_def), class_(subclass) {
-        }
-
-        /// @brief The class definition name.
-        ClientClass class_def_;
-
-        /// @brief The class or subclass name.
-        ClientClass class_;
-    };
-
-    /// @brief Tag for the sequence index.
-    struct TemplateClassSequenceTag { };
-
-    /// @brief Tag for the name index.
-    struct TemplateClassNameTag { };
-
-    /// @brief the subclass multi-index.
-    typedef boost::multi_index_container<
-        SubClassRelation,
-        boost::multi_index::indexed_by<
-            // First index is the sequence one.
-            boost::multi_index::sequenced<
-                boost::multi_index::tag<TemplateClassSequenceTag>
-            >,
-            // Second index is the name hash one.
-            boost::multi_index::hashed_unique<
-                boost::multi_index::tag<TemplateClassNameTag>,
-                boost::multi_index::member<SubClassRelation,
-                                           ClientClass,
-                                           &SubClassRelation::class_def_>
-            >
+    >
+> ClientClassContainer;
+
+/// @brief Defines a subclass to template class relation.
+struct SubClassRelation {
+    /// @brief Constructor.
+    SubClassRelation(const ClientClass& class_def, const ClientClass& subclass) :
+        class_def_(class_def), class_(subclass) {
+    }
+
+    /// @brief The class definition name.
+    ClientClass class_def_;
+
+    /// @brief The class or subclass name.
+    ClientClass class_;
+};
+
+/// @brief Tag for the sequence index.
+struct TemplateClassSequenceTag { };
+
+/// @brief Tag for the name index.
+struct TemplateClassNameTag { };
+
+/// @brief the subclass multi-index.
+typedef boost::multi_index_container<
+    SubClassRelation,
+    boost::multi_index::indexed_by<
+        // First index is the sequence one.
+        boost::multi_index::sequenced<
+            boost::multi_index::tag<TemplateClassSequenceTag>
+        >,
+        // Second index is the name hash one.
+        boost::multi_index::hashed_unique<
+            boost::multi_index::tag<TemplateClassNameTag>,
+            boost::multi_index::member<SubClassRelation,
+                                       ClientClass,
+                                       &SubClassRelation::class_def_>
         >
-    > SubClassRelationContainer;
+    >
+> SubClassRelationContainer;
+
+/// @brief Container for storing client class names
+///
+/// Both a list to iterate on it in insert order and unordered
+/// set of names for existence.
+class ClientClasses : public isc::data::CfgToElement {
+public:
+
+    /// @brief Type of iterators
+    typedef ClientClassContainer::const_iterator const_iterator;
+    typedef ClientClassContainer::iterator iterator;
+
+    /// @brief Default constructor.
+    ClientClasses() : container_() {
+    }
+
+    /// @brief Constructor from comma separated values.
+    ///
+    /// @param class_names A string containing a client classes separated
+    /// with commas. The class names are trimmed before insertion to the set.
+    ClientClasses(const std::string& class_names);
+
+    /// @brief Destructor.
+    virtual ~ClientClasses() {}
+
+    /// @brief Copy constructor.
+    ///
+    /// @param other ClientClasses object to be copied.
+    ClientClasses(const ClientClasses& other);
+
+    /// @brief Assigns the contents of on container to another.
+    ClientClasses& operator=(const ClientClasses& other);
+
+    /// @brief Compares two ClientClasses container for equality
+    ///
+    /// @return True if the two containers are equal, false otherwise.
+    bool equals(const ClientClasses& other) const;
+
+    /// @brief Compares two ClientClasses containers for equality.
+    ///
+    /// @return True if the two containers are equal, false otherwise.
+    bool operator==(const ClientClasses& other) const {
+        return(equals(other));
+    }
+
+    /// @brief Compares two ClientClasses container for inequality
+    ///
+    /// @return True if the two containers are not equal, false otherwise.
+    bool operator!=(const ClientClasses& other) const {
+        return(!equals(other));
+    }
+
+    /// @brief Insert an element.
+    ///
+    /// @param class_name The name of the class to insert
+    void insert(const ClientClass& class_name) {
+        static_cast<void>(container_.push_back(class_name));
+    }
+
+    /// @brief Erase element by name.
+    ///
+    /// @param class_name The name of the class to erase.
+    void erase(const ClientClass& class_name);
+
+    /// @brief Check if classes is empty.
+    bool empty() const {
+        return (container_.empty());
+    }
+
+    /// @brief Returns the number of classes.
+    ///
+    /// @note; in C++ 11 list size complexity is constant so
+    /// there is no advantage to use the set part.
+    size_t size() const {
+        return (container_.size());
+    }
+
+    /// @brief Iterators to the first element.
+    /// @{
+    const_iterator cbegin() const {
+        return (container_.cbegin());
+    }
+
+    const_iterator begin() const {
+        return (container_.begin());
+    }
+
+    iterator begin() {
+        return (container_.begin());
+    }
+    /// @}
+
+    /// @brief Iterators to the past the end element.
+    /// @{
+    const_iterator cend() const {
+        return (container_.cend());
+    }
+
+    const_iterator end() const {
+        return (container_.end());
+    }
+
+    iterator end() {
+        return (container_.end());
+    }
+    /// @}
+
+    /// @brief returns whether this container has at least one class
+    /// in common with a given container.
+    ///
+    /// @param cclasses list of classes to check for intersection with
+    /// @return true if this container has at least one class that is
+    /// also in cclasses, false otherwise.
+    bool intersects(const ClientClasses& cclasses) const;
+
+    /// @brief returns if class x belongs to the defined classes
+    ///
+    /// @param x client class to be checked
+    /// @return true if x belongs to the classes
+    bool contains(const ClientClass& x) const;
 
-    /// @brief Container for storing client class names
+    /// @brief Clears containers.
+    void clear() {
+        container_.clear();
+    }
+
+    /// @brief Returns all class names as text
+    ///
+    /// @param separator Separator to be used between class names. The
+    /// default separator comprises comma sign followed by space
+    /// character.
     ///
-    /// Both a list to iterate on it in insert order and unordered
-    /// set of names for existence.
-    class ClientClasses {
-    public:
-
-        /// @brief Type of iterators
-        typedef ClientClassContainer::const_iterator const_iterator;
-        typedef ClientClassContainer::iterator iterator;
-
-        /// @brief Default constructor.
-        ClientClasses() : container_() {
-        }
-
-        /// @brief Constructor from comma separated values.
-        ///
-        /// @param class_names A string containing a client classes separated
-        /// with commas. The class names are trimmed before insertion to the set.
-        ClientClasses(const std::string& class_names);
-
-        /// @brief Copy constructor.
-        ///
-        /// @param other ClientClasses object to be copied.
-        ClientClasses(const ClientClasses& other);
-
-        /// @brief Assigns the contents of on container to another.
-        ClientClasses& operator=(const ClientClasses& other);
-
-        /// @brief Compares two ClientClasses container for equality
-        ///
-        /// @return True if the two containers are equal, false otherwise.
-        bool equals(const ClientClasses& other) const;
-
-        /// @brief Compares two ClientClasses containers for equality.
-        ///
-        /// @return True if the two containers are equal, false otherwise.
-        bool operator==(const ClientClasses& other) const {
-            return(equals(other));
-        }
-
-        /// @brief Compares two ClientClasses container for inequality
-        ///
-        /// @return True if the two containers are not equal, false otherwise.
-        bool operator!=(const ClientClasses& other) const {
-            return(!equals(other));
-        }
-
-        /// @brief Insert an element.
-        ///
-        /// @param class_name The name of the class to insert
-        void insert(const ClientClass& class_name) {
-            static_cast<void>(container_.push_back(class_name));
-        }
-
-        /// @brief Erase element by name.
-        ///
-        /// @param class_name The name of the class to erase.
-        void erase(const ClientClass& class_name);
-
-        /// @brief Check if classes is empty.
-        bool empty() const {
-            return (container_.empty());
-        }
-
-        /// @brief Returns the number of classes.
-        ///
-        /// @note; in C++ 11 list size complexity is constant so
-        /// there is no advantage to use the set part.
-        size_t size() const {
-            return (container_.size());
-        }
-
-        /// @brief Iterators to the first element.
-        /// @{
-        const_iterator cbegin() const {
-            return (container_.cbegin());
-        }
-        const_iterator begin() const {
-            return (container_.begin());
-        }
-        iterator begin() {
-            return (container_.begin());
-        }
-        /// @}
-
-        /// @brief Iterators to the past the end element.
-        /// @{
-        const_iterator cend() const {
-            return (container_.cend());
-        }
-        const_iterator end() const {
-            return (container_.end());
-        }
-        iterator end() {
-            return (container_.end());
-        }
-        /// @}
-
-        /// @brief returns whether this container has at least one class
-        /// in given container.
-        ///
-        /// @param cclasses list of classes to check for intersection with
-        /// @return true if this container has at least one class that is
-        /// also in cclasses, false otherwise.
-        bool intersects(const ClientClasses& cclasses) const;
-
-        /// @brief returns if class x belongs to the defined classes
-        ///
-        /// @param x client class to be checked
-        /// @return true if x belongs to the classes
-        bool contains(const ClientClass& x) const;
-
-        /// @brief Clears containers.
-        void clear() {
-            container_.clear();
-        }
-
-        /// @brief Returns all class names as text
-        ///
-        /// @param separator Separator to be used between class names. The
-        /// default separator comprises comma sign followed by space
-        /// character.
-        ///
-        /// @return the string representation of all classes
-        std::string toText(const std::string& separator = ", ") const;
-
-        /// @brief Returns all class names as an ElementPtr of type ListElement
-        ///
-        /// @return the list
-        isc::data::ElementPtr toElement() const;
-
-        /// @brief Sets contents from a ListElement
-        ///
-        /// @param list JSON list of classes from which to populate
-        /// the container.
-        ///
-        /// @throw BadValue if the element is not a list or contents
-        /// are invalid
-        void fromElement(isc::data::ConstElementPtr list);
-
-    private:
-        /// @brief container part
-        ClientClassContainer container_;
-    };
+    /// @return the string representation of all classes
+    std::string toText(const std::string& separator = ", ") const;
+
+    /// @brief Returns all class names as an ElementPtr of type ListElement
+    ///
+    /// @return the list
+    virtual isc::data::ElementPtr toElement() const;
+
+    /// @brief Sets contents from a ListElement
+    ///
+    /// @param list JSON list of classes from which to populate
+    /// the container.
+    ///
+    /// @throw BadValue if the element is not a list or contents
+    /// are invalid
+    void fromElement(isc::data::ConstElementPtr list);
+
+private:
+    /// @brief container part
+    ClientClassContainer container_;
+};
+
 }
 }
 
index cc3bb5b45d85bbcf95cd622b13b37bda69fcd292..7cfad4b83edbbfdf646041b45bcbb6ea4de50495 100644 (file)
@@ -305,8 +305,7 @@ BaseNetworkParser::getClientClassesElem(ConstElementPtr params,
     }
 
     if (class_list) {
-        const std::vector<data::ElementPtr>& classes = class_list->listValue();
-        for (auto const& cclass : classes) {
+        for (auto const& cclass : class_list->listValue()) {
             if ((cclass->getType() != Element::string) ||
                 cclass->stringValue().empty()) {
                 isc_throw(DhcpConfigError, "invalid class name (" << cclass->getPosition() << ")");
index f61cca20020c4e18c23b890a3c04268495b19490..6a5760c240114a4f140767b32cc501f3d894ad12 100644 (file)
@@ -144,7 +144,6 @@ public:
     ///
     /// @param params configuration element tree to search.
     /// @param adder_func function to add class names to an object's client class list.
-    /// @return Element referred to or an empty pointer.
     /// @throw DhcpConfigError if both entries are present.
     static void getClientClassesElem(data::ConstElementPtr params,
                                      ClassAdderFunc adder_func);
index 8474fe3fc1ea6eaa3a46fe150fcd4270f807002d..3eba3f573a42c5f5e2a6a81627bd5ef97a4b3904 100644 (file)
@@ -1094,7 +1094,8 @@ TEST_F(SharedNetwork6ParserTest, deprecatedRequireClientClasses) {
     std::string config =
        R"^({
             "name": "foo",
-            "require-client-classes": [ "one", "two" ] })^";
+            "require-client-classes": [ "one", "two" ]
+       })^";
 
     ElementPtr config_element = Element::fromJSON(config);