[#3583] Addressred review comments

modified:
    doc/sphinx/arm/classify.rst
    src/bin/dhcp4/dhcp4_srv.cc
    src/bin/dhcp4/tests/config_parser_unittest.cc
    src/bin/dhcp6/tests/config_parser_unittest.cc
    src/hooks/dhcp/mysql/mysql_cb_dhcp4.cc
    src/lib/dhcp/classify.cc
    src/lib/dhcp/classify.h
    src/lib/dhcp/tests/classify_unittest.cc
    src/lib/dhcpsrv/cfg_option.cc
    src/lib/dhcpsrv/cfg_option.h
    src/lib/dhcpsrv/parsers/option_data_parser.cc
    src/lib/dhcpsrv/parsers/simple_parser4.cc
    src/lib/dhcpsrv/parsers/simple_parser6.cc
    src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
    src/lib/dhcpsrv/testutils/generic_backend_unittest.cc

Author: tmarkwalder

doc/sphinx/arm/classify.rst

@@ -1246,6 +1246,12 @@ the same value for option "foo":
         }]
     }
 
+The ``client-classes`` list is allowed in an option specification at
+any scope.  Option class-tagging is enforced at the time options are
+being added to the response which occurs after lease assignment just
+before the response is to be sent to the client.
+
+
 When ``never-send`` for an option is true at any scope, all 
 ``client-classes`` entries for that option are ignored. The
 option will not included.

src/bin/dhcp4/dhcp4_srv.cc

@@ -2342,6 +2342,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
         }
     }
 
+    const auto& cclasses = query->getClasses();
     for (uint32_t vendor_id : vendor_ids) {
 
         std::set<uint8_t> cancelled_opts;
@@ -2406,7 +2407,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
             if (!vendor_rsp->getOption(opt)) {
                 for (auto const& copts : co_list) {
                     OptionDescriptor desc = copts->get(vendor_id, opt);
-                    if (desc.option_ && desc.allowedForClientClasses(query->getClasses())) {
+                    if (desc.option_ && desc.allowedForClientClasses(cclasses)) {
                         vendor_rsp->addOption(desc.option_);
                         added = true;
                         break;
@@ -2446,6 +2447,7 @@ Dhcpv4Srv::appendBasicOptions(Dhcpv4Exchange& ex) {
     }
 
     Pkt4Ptr resp = ex.getResponse();
+    const auto& cclasses = ex.getQuery()->getClasses();
 
     // Try to find all 'required' options in the outgoing
     // message. Those that are not present will be added.
@@ -2456,7 +2458,7 @@ Dhcpv4Srv::appendBasicOptions(Dhcpv4Exchange& ex) {
             for (auto const& copts : co_list) {
                 OptionDescriptor desc = copts->get(DHCP4_OPTION_SPACE, required);
                 /// @todo TKM - not sure if otion class-tagging should be allowed here?
-                if (desc.option_ && desc.allowedForClientClasses(ex.getQuery()->getClasses())) {
+                if (desc.option_ && desc.allowedForClientClasses(cclasses)) {
                     resp->addOption(desc.option_);
                     break;
                 }
@@ -3635,7 +3637,7 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
 
     // Step 2: Try to set the values based on classes.
     // Any values defined in classes will override those from subnet level.
-    const ClientClasses classes = query->getClasses();
+    const ClientClasses& classes = query->getClasses();
     if (!classes.empty()) {
 
         // Let's get class definitions

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -8013,4 +8013,38 @@ TEST_F(Dhcp4ParserTest, storeDdnsConflictResolutionMode) {
     }
 }
 
+//This test verifies that duplicates in option-data.client-classes
+// are ignored and do not affect class order.
+TEST_F(Dhcp4ParserTest, optionClientClassesDuplicateCheck) {
+    std::string config = "{ " + genIfaceConfig() + ","
+        R"^(
+        "option-data": [{
+            "name": "domain-name",
+            "data": "example.com",
+            "client-classes": [ "foo", "bar", "foo", "bar" ]
+        }],
+        "rebind-timer": 2000,
+        "renew-timer": 1000,
+        "subnet4": [],
+        "valid-lifetime": 400
+        })^";
+
+    ConstElementPtr json;
+    ASSERT_NO_THROW(json = parseDHCP4(config));
+    extractConfig(config);
+
+    ConstElementPtr status;
+    ASSERT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+    checkResult(status, 0);
+
+    CfgOptionPtr cfg = CfgMgr::instance().getStagingCfg()->getCfgOption();
+    const auto desc = cfg->get(DHCP4_OPTION_SPACE, DHO_DOMAIN_NAME);
+    ASSERT_TRUE(desc.option_);
+    ASSERT_EQ(desc.client_classes_.size(), 2);
+    auto cclasses = desc.client_classes_.begin();
+    EXPECT_EQ(*cclasses, "foo");
+    ++cclasses;
+    EXPECT_EQ(*cclasses, "bar");
+}
+
 }  // namespace

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -8987,4 +8987,38 @@ TEST_F(Dhcp6ParserTest, storeDdnsConflictResolutionMode) {
     }
 }
 
+// This test verifies that duplicates in option-data.client-classes
+// are ignored and do not affect class order.
+TEST_F(Dhcp6ParserTest, optionClientClassesDuplicateCheck) {
+    std::string config = "{ " + genIfaceConfig() + ","
+        R"^(
+        "option-data": [{
+            "name": "domain-search",
+            "data": "example.com",
+            "client-classes": [ "foo", "bar", "foo", "bar" ]
+        }],
+        "rebind-timer": 2000,
+        "renew-timer": 1000,
+        "subnet6": [],
+        "valid-lifetime": 400
+        })^";
+
+    ConstElementPtr json;
+    ASSERT_NO_THROW(json = parseDHCP6(config));
+    extractConfig(config);
+
+    ConstElementPtr status;
+    ASSERT_NO_THROW(status = configureDhcp6Server(srv_, json));
+    checkResult(status, 0);
+
+    CfgOptionPtr cfg = CfgMgr::instance().getStagingCfg()->getCfgOption();
+    const auto desc = cfg->get(DHCP6_OPTION_SPACE, D6O_DOMAIN_SEARCH);
+    ASSERT_TRUE(desc.option_);
+    ASSERT_EQ(desc.client_classes_.size(), 2);
+    auto cclasses = desc.client_classes_.begin();
+    EXPECT_EQ(*cclasses, "foo");
+    ++cclasses;
+    EXPECT_EQ(*cclasses, "bar");
+}
+
 }  // namespace

src/hooks/dhcp/mysql/mysql_cb_dhcp4.cc

@@ -1870,7 +1870,7 @@ class MySqlConfigBackendDHCPv4Impl : public MySqlConfigBackendImpl {
             cc_binding,
             MySqlBinding::createString(tag),
             MySqlBinding::createInteger<uint8_t>(option->option_->getType()),
-            MySqlBinding::condCreateString(option->space_name_),
+            MySqlBinding::condCreateString(option->space_name_)
         };
 
         MySqlTransaction transaction(conn_);
@@ -2074,7 +2074,7 @@ class MySqlConfigBackendDHCPv4Impl : public MySqlConfigBackendImpl {
             cc_binding,
             MySqlBinding::createString(shared_network_name),
             MySqlBinding::createInteger<uint8_t>(option->option_->getType()),
-            MySqlBinding::condCreateString(option->space_name_),
+            MySqlBinding::condCreateString(option->space_name_)
         };
 
         boost::scoped_ptr<MySqlTransaction> transaction;

src/lib/dhcp/classify.cc

@@ -82,7 +82,7 @@ ClientClasses::toElement() const {
 }
 
 void
-ClientClasses::fromElement(isc::data::ElementPtr cc_list) {
+ClientClasses::fromElement(isc::data::ConstElementPtr cc_list) {
     if (cc_list) {
         clear();
         if (cc_list->getType() != Element::list) {
@@ -95,7 +95,7 @@ ClientClasses::fromElement(isc::data::ElementPtr cc_list) {
                 isc_throw(BadValue, "elements of list must be valid strings"); 
             }
 
-            insert(cclass->stringValue());
+            static_cast<void>(insert(cclass->stringValue()));
         }
     }
 }

src/lib/dhcp/classify.h

@@ -124,18 +124,18 @@ namespace dhcp {
 
         /// @brief Copy constructor.
         ///
-        /// @param ClientClasses object to be copied.
+        /// @param other ClientClasses object to be copied.
         ClientClasses(const ClientClasses& other);
 
-        /// @brief Assigns the contents of on container to another
+        /// @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 container for equality
+        /// @brief Compares two ClientClasses containers for equality.
         ///
         /// @return True if the two containers are equal, false otherwise.
         bool operator==(const ClientClasses& other) const {
@@ -227,9 +227,12 @@ namespace dhcp {
 
         /// @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::ElementPtr list);
+        void fromElement(isc::data::ConstElementPtr list);
 
     private:
         /// @brief container part

src/lib/dhcp/tests/classify_unittest.cc

@@ -253,4 +253,16 @@ TEST(ClassifyTest, ClientClassesFromElement) {
     cclasses_element = Element::createList();
     ASSERT_NO_THROW(classes.fromElement(cclasses_element));
     EXPECT_TRUE(classes.empty());
+
+    cclasses_element->add(Element::create("foo"));
+    cclasses_element->add(Element::create("bar"));
+    cclasses_element->add(Element::create("foo"));
+    cclasses_element->add(Element::create("bar"));
+
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+    ASSERT_EQ(classes.size(), 2);
+    auto cclass = classes.begin();
+    EXPECT_EQ(*cclass, "foo");
+    ++cclass;
+    EXPECT_EQ(*cclass, "bar");
 }

src/lib/dhcpsrv/cfg_option.cc

@@ -64,17 +64,25 @@ OptionDescriptor::allowedForClientClasses(const ClientClasses& cclasses) const {
     if (client_classes_.empty()) {
         return (true);
     }
-    
-    for (const auto& cclass : client_classes_) {
-        if (cclasses.contains(cclass)) {
-            return (true);
+
+    if (cclasses.size() > client_classes_.size()) {
+        for (const auto& cclass : client_classes_) {
+            if (cclasses.contains(cclass)) {
+                return (true);
+            }
+        }
+    }
+    else {
+        for (const auto& cclass : cclasses) {
+            if (client_classes_.contains(cclass)) {
+                return (true);
+            }
         }
     }
 
     return (false);
 }
 
-
 CfgOption::CfgOption()
     : encapsulated_(false) {
 }
@@ -108,7 +116,7 @@ CfgOption::add(const OptionDescriptor& desc, const std::string& option_space) {
     if (!desc.option_) {
         isc_throw(isc::BadValue, "option being configured must not be NULL");
 
-    } else  if (!OptionSpace::validateName(option_space)) {
+    } else if (!OptionSpace::validateName(option_space)) {
         isc_throw(isc::BadValue, "invalid option space name: '"
                   << option_space << "'");
     }

src/lib/dhcpsrv/cfg_option.h

@@ -817,8 +817,6 @@ class CfgOption : public isc::data::CfgToElement {
     VendorOptionSpaceCollection vendor_options_;
 };
 
-class CfgOption; // forward declaration 
-
 /// @name Pointers to the @c CfgOption objects.
 //@{