From 7eebd1434d3bad1f258c03bf77ab685f47f392ff Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 25 Nov 2024 09:52:20 -0500 Subject: [PATCH] [#3592] Addressed review comments 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 | 6 +- doc/sphinx/arm/classify.rst | 4 +- doc/sphinx/arm/dhcp4-srv.rst | 4 +- doc/sphinx/arm/dhcp6-srv.rst | 2 +- src/bin/dhcp4/tests/classify_unittest.cc | 6 +- src/lib/dhcp/classify.cc | 3 +- src/lib/dhcp/classify.h | 422 +++++++++--------- .../dhcpsrv/parsers/base_network_parser.cc | 3 +- src/lib/dhcpsrv/parsers/base_network_parser.h | 1 - .../tests/shared_network_parser_unittest.cc | 3 +- 10 files changed, 232 insertions(+), 222 deletions(-) diff --git a/ChangeLog b/ChangeLog index ec7e411cc0..2679445c85 100644 --- a/ChangeLog +++ b/ChangeLog @@ -12,15 +12,15 @@ 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. diff --git a/doc/sphinx/arm/classify.rst b/doc/sphinx/arm/classify.rst index d47c1abf69..e7a4218695 100644 --- a/doc/sphinx/arm/classify.rst +++ b/doc/sphinx/arm/classify.rst @@ -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. diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index 7af7ab2aee..7ef183ff39 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -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. diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index 8088cd3ccf..43db93df02 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -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: diff --git a/src/bin/dhcp4/tests/classify_unittest.cc b/src/bin/dhcp4/tests/classify_unittest.cc index cd31b1d20e..51fc400f1b 100644 --- a/src/bin/dhcp4/tests/classify_unittest.cc +++ b/src/bin/dhcp4/tests/classify_unittest.cc @@ -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); diff --git a/src/lib/dhcp/classify.cc b/src/lib/dhcp/classify.cc index 7a001ba621..38943342be 100644 --- a/src/lib/dhcp/classify.cc +++ b/src/lib/dhcp/classify.cc @@ -65,8 +65,7 @@ ClientClasses::intersects(const ClientClasses& cclasses) const { return (true); } } - } - else { + } else { for (const auto& cclass : cclasses) { if (contains(cclass)) { return (true); diff --git a/src/lib/dhcp/classify.h b/src/lib/dhcp/classify.h index 69cdffd417..8571cf9909 100644 --- a/src/lib/dhcp/classify.h +++ b/src/lib/dhcp/classify.h @@ -8,6 +8,7 @@ #define CLASSIFY_H #include +#include #include #include @@ -21,7 +22,10 @@ /// @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. @@ -35,214 +39,222 @@ 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 - >, - // Second index is the name hash one. - boost::multi_index::hashed_unique< - boost::multi_index::tag, - boost::multi_index::identity - > +/// @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 + >, + // Second index is the name hash one. + boost::multi_index::hashed_unique< + boost::multi_index::tag, + boost::multi_index::identity > - > 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 - >, - // Second index is the name hash one. - boost::multi_index::hashed_unique< - boost::multi_index::tag, - boost::multi_index::member - > + > +> 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 + >, + // Second index is the name hash one. + boost::multi_index::hashed_unique< + boost::multi_index::tag, + boost::multi_index::member > - > 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(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(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_; +}; + } } diff --git a/src/lib/dhcpsrv/parsers/base_network_parser.cc b/src/lib/dhcpsrv/parsers/base_network_parser.cc index cc3bb5b45d..7cfad4b83e 100644 --- a/src/lib/dhcpsrv/parsers/base_network_parser.cc +++ b/src/lib/dhcpsrv/parsers/base_network_parser.cc @@ -305,8 +305,7 @@ BaseNetworkParser::getClientClassesElem(ConstElementPtr params, } if (class_list) { - const std::vector& 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() << ")"); diff --git a/src/lib/dhcpsrv/parsers/base_network_parser.h b/src/lib/dhcpsrv/parsers/base_network_parser.h index f61cca2002..6a5760c240 100644 --- a/src/lib/dhcpsrv/parsers/base_network_parser.h +++ b/src/lib/dhcpsrv/parsers/base_network_parser.h @@ -144,7 +144,6 @@ class BaseNetworkParser : public data::SimpleParser { /// /// @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); diff --git a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc index 8474fe3fc1..3eba3f573a 100644 --- a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc @@ -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);