[#4143] Checkpoint: more UTs and doc todo

diff --git a/src/hooks/dhcp/flex_option/flex_option.cc b/src/hooks/dhcp/flex_option/flex_option.cc
index e1a1647583488816b11b4bf3089b0e5ee531db17..b7cb445b28125432c3ca6bd999f53d846e95b597 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.cc
+++ b/src/hooks/dhcp/flex_option/flex_option.cc
@@ -320,12 +320,16 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
     // Not working as expected: the destination is the query and classes
     // are used.
     if (!opt_cfg->getDestination() && !opt_cfg->getClass().empty()) {
-        // LOG
+        LOG_WARN(flex_option_logger, FLEX_OPTION_CONFIG_USELESS_CLASS)
+            .arg(code)
+            .arg(opt_cfg->getClass());
     }
     if (!opt_cfg->getDestination() && opt_cfg->getExpr()) {
         for (auto const& tok : *opt_cfg->getExpr()) {
             if (boost::dynamic_pointer_cast<TokenMember>(tok)) {
-                // LOG
+                LOG_WARN(flex_option_logger, FLEX_OPTION_CONFIG_USELESS_MEMBER)
+                    .arg(code)
+                    .arg(opt_cfg->getText());
                 break;
             }
         }

diff --git a/src/hooks/dhcp/flex_option/flex_option.h b/src/hooks/dhcp/flex_option/flex_option.h
index f8d52ed4a67ba3e6ccc91f6603912f74582b6730..5a95dce2d4d8fd47a6c58d87807b5d58fbdd8536 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.h
+++ b/src/hooks/dhcp/flex_option/flex_option.h
@@ -325,9 +325,7 @@ public:
         // Handle the case where the source is the response and
         // there is a TokenMember in an expression.
         if (query && response && need_copy_classes_to_response_) {
-            for (auto const& cclass : query->getClasses()) {
-                response->addClass(cclass);
-            }
+            response->copyClasses(*query);
         }
         for (auto const& pair : getOptionConfigMap()) {
             for (const OptionConfigPtr& opt_cfg : pair.second) {

diff --git a/src/hooks/dhcp/flex_option/flex_option_messages.cc b/src/hooks/dhcp/flex_option/flex_option_messages.cc
index e1bd676cec8c1986602f3b0fd5d02d60629818a4..0f5cc6a69b2332bcb7663e8f1cea24a6c80d7d7e 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option_messages.cc
+++ b/src/hooks/dhcp/flex_option/flex_option_messages.cc
@@ -4,6 +4,8 @@
 #include <log/message_types.h>
 #include <log/message_initializer.h>
 
+extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_CLASS = "FLEX_OPTION_CONFIG_USELESS_CLASS";
+extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_MEMBER = "FLEX_OPTION_CONFIG_USELESS_MEMBER";
 extern const isc::log::MessageID FLEX_OPTION_LOAD_ERROR = "FLEX_OPTION_LOAD_ERROR";
 extern const isc::log::MessageID FLEX_OPTION_PROCESS_ADD = "FLEX_OPTION_PROCESS_ADD";
 extern const isc::log::MessageID FLEX_OPTION_PROCESS_CLIENT_CLASS = "FLEX_OPTION_PROCESS_CLIENT_CLASS";
@@ -20,6 +22,8 @@ extern const isc::log::MessageID FLEX_OPTION_UNLOAD = "FLEX_OPTION_UNLOAD";
 namespace {
 
 const char* values[] = {
+    "FLEX_OPTION_CONFIG_USELESS_CLASS", "For the option '%1' the client class '%2' is required before classification for a 'query' destination",
+    "FLEX_OPTION_CONFIG_USELESS_MEMBER", "For the option '%1' the  member expression '%2' is evaluated before classification for a 'query' destination",
     "FLEX_OPTION_LOAD_ERROR", "loading Flex Option hooks library failed: %1",
     "FLEX_OPTION_PROCESS_ADD", "Added the option code %1 with value %2",
     "FLEX_OPTION_PROCESS_CLIENT_CLASS", "Skip processing of the option code %1 for class '%2'",

diff --git a/src/hooks/dhcp/flex_option/flex_option_messages.h b/src/hooks/dhcp/flex_option/flex_option_messages.h
index 7ffe7d81bebf9c3f3d12bda3d3fd2a4752172d91..67950a2cb3889fc7946c1f6c4f3fb08f0f4aa5ed 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option_messages.h
+++ b/src/hooks/dhcp/flex_option/flex_option_messages.h
@@ -5,6 +5,8 @@
 
 #include <log/message_types.h>
 
+extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_CLASS;
+extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_MEMBER;
 extern const isc::log::MessageID FLEX_OPTION_LOAD_ERROR;
 extern const isc::log::MessageID FLEX_OPTION_PROCESS_ADD;
 extern const isc::log::MessageID FLEX_OPTION_PROCESS_CLIENT_CLASS;

diff --git a/src/hooks/dhcp/flex_option/flex_option_messages.mes b/src/hooks/dhcp/flex_option/flex_option_messages.mes
index 0c83e1cbaa476c353b816f32e7dd1d8b6709c0bf..18da2940d444d22caaacf1758d5e9147ad311c40 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option_messages.mes
+++ b/src/hooks/dhcp/flex_option/flex_option_messages.mes
@@ -9,6 +9,19 @@ This error message indicates an error during loading the Flex Option
 hooks library. The details of the error are provided as argument of
 the log message.
 
+% FLEX_OPTION_CONFIG_USELESS_CLASS For the option '%1' the client class '%2' is required before classification for a 'query' destination
+This warning message indicates the config specifies a class requirement
+when the destination is the query but the callout point for patching
+queries is before the classification so it will very likely not work
+as expected. The code of the option and the client class are displayed.
+
+% FLEX_OPTION_CONFIG_USELESS_MEMBER For the option '%1' the  member expression '%2' is evaluated before classification for a 'query' destination
+This warning message indicates the config specifies an expression
+with a member clause when the destination is the query but the callout point
+for patching queries is before the classification so it will very likely
+not work as expected. The code of the option and the expression are
+displayed.
+
 % FLEX_OPTION_PROCESS_ADD Added the option code %1 with value %2
 Logged at debug log level 40.
 This debug message is printed when an option was added into the response

diff --git a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc
index 20a991c4f9e781c655cb181fd2d000354e584c9e..92d56f0f2e6f05190790ac2167bf7b02dad86149 100644 (file)
--- a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc
+++ b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc
@@ -889,6 +889,32 @@ TEST_F(FlexOptionTest, processEmpty) {
     EXPECT_EQ(response_txt, response->toText());
 }
 
+// Verify that response processing does nothing with no response.
+TEST_F(FlexOptionTest, processNoResponse) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+
+    option = Element::createMap();
+    options->add(option);
+    code = Element::create(DHO_DOMAIN_SEARCH);
+    option->set("code", code);
+    add = Element::create(string("'example.com'"));
+    option->set("add", add);
+    // fqdn option data is parsed using option definition in csv format.
+    option->set("csv-format", Element::create(true));
+
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 12345));
+    EXPECT_NO_THROW(impl_->process<Pkt4Ptr>(Option::V4, query, Pkt4Ptr()));
+}
+
 // Verify that NONE action really does nothing.
 TEST_F(FlexOptionTest, processNone) {
     CfgMgr::instance().setFamily(AF_INET6);
@@ -1583,4 +1609,276 @@ TEST_F(FlexOptionTest, optionConfigGuardMatch) {
     EXPECT_FALSE(response->getOption(D6O_BOOTFILE_URL));
 }
 
+// Verify that an unknown source keyword is rejected.
+TEST_F(FlexOptionTest, unknownSource) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'ab'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(123);
+    option->set("code", code);
+    ElementPtr source = Element::create(string("foo"));
+    option->set("source", source);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    string expected = "unknown source 'foo', ";
+    expected += "valid values are 'query' and 'response'";
+    EXPECT_EQ(expected, impl_->getErrMsg());
+}
+
+// Verify that the default source is 'query'.
+TEST_F(FlexOptionTest, defaultSource) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    auto map = impl_->getOptionConfigMap();
+    FlexOptionImpl::OptionConfigList opt_lst;
+    ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME));
+    ASSERT_FALSE(opt_lst.empty());
+    EXPECT_EQ(1U, opt_lst.size());
+    FlexOptionImpl::OptionConfigPtr opt_cfg;
+    ASSERT_NO_THROW(opt_cfg = opt_lst.front());
+
+    ASSERT_TRUE(opt_cfg);
+    EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode());
+    EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction());
+    EXPECT_EQ("'abc'", opt_cfg->getText());
+    EXPECT_TRUE(opt_cfg->getSource());
+}
+
+// Verify that the source can be set to 'query'.
+TEST_F(FlexOptionTest, querySource) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    ElementPtr source = Element::create(string("query"));
+    option->set("source", source);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    auto map = impl_->getOptionConfigMap();
+    FlexOptionImpl::OptionConfigList opt_lst;
+    ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME));
+    ASSERT_FALSE(opt_lst.empty());
+    EXPECT_EQ(1U, opt_lst.size());
+    FlexOptionImpl::OptionConfigPtr opt_cfg;
+    ASSERT_NO_THROW(opt_cfg = opt_lst.front());
+
+    ASSERT_TRUE(opt_cfg);
+    EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode());
+    EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction());
+    EXPECT_EQ("'abc'", opt_cfg->getText());
+    EXPECT_TRUE(opt_cfg->getSource());
+}
+
+// Verify that the source can be set to 'response'.
+TEST_F(FlexOptionTest, responseSource) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    ElementPtr source = Element::create(string("response"));
+    option->set("source", source);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    auto map = impl_->getOptionConfigMap();
+    FlexOptionImpl::OptionConfigList opt_lst;
+    ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME));
+    ASSERT_FALSE(opt_lst.empty());
+    EXPECT_EQ(1U, opt_lst.size());
+    FlexOptionImpl::OptionConfigPtr opt_cfg;
+    ASSERT_NO_THROW(opt_cfg = opt_lst.front());
+
+    ASSERT_TRUE(opt_cfg);
+    EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode());
+    EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction());
+    EXPECT_EQ("'abc'", opt_cfg->getText());
+    EXPECT_FALSE(opt_cfg->getSource());
+}
+
+// Verify that an unknown destination keyword is rejected.
+TEST_F(FlexOptionTest, unknownDestination) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'ab'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(123);
+    option->set("code", code);
+    ElementPtr dest = Element::create(string("foo"));
+    option->set("destination", dest);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    string expected = "unknown destination 'foo', ";
+    expected += "valid values are 'response' and 'query'";
+    EXPECT_EQ(expected, impl_->getErrMsg());
+}
+
+// Verify that the default destination is 'response'.
+TEST_F(FlexOptionTest, defaultDestination) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    auto map = impl_->getOptionConfigMap();
+    FlexOptionImpl::OptionConfigList opt_lst;
+    ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME));
+    ASSERT_FALSE(opt_lst.empty());
+    EXPECT_EQ(1U, opt_lst.size());
+    FlexOptionImpl::OptionConfigPtr opt_cfg;
+    ASSERT_NO_THROW(opt_cfg = opt_lst.front());
+
+    ASSERT_TRUE(opt_cfg);
+    EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode());
+    EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction());
+    EXPECT_EQ("'abc'", opt_cfg->getText());
+    EXPECT_TRUE(opt_cfg->getDestination());
+}
+
+// Verify that the destination can be set to 'response'.
+TEST_F(FlexOptionTest, responseDestination) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    ElementPtr dest = Element::create(string("response"));
+    option->set("destination", dest);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    auto map = impl_->getOptionConfigMap();
+    FlexOptionImpl::OptionConfigList opt_lst;
+    ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME));
+    ASSERT_FALSE(opt_lst.empty());
+    EXPECT_EQ(1U, opt_lst.size());
+    FlexOptionImpl::OptionConfigPtr opt_cfg;
+    ASSERT_NO_THROW(opt_cfg = opt_lst.front());
+
+    ASSERT_TRUE(opt_cfg);
+    EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode());
+    EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction());
+    EXPECT_EQ("'abc'", opt_cfg->getText());
+    EXPECT_TRUE(opt_cfg->getDestination());
+}
+
+// Verify that the destination can be set to 'query'.
+TEST_F(FlexOptionTest, queryDestination) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    ElementPtr dest = Element::create(string("query"));
+    option->set("destination", dest);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    auto map = impl_->getOptionConfigMap();
+    FlexOptionImpl::OptionConfigList opt_lst;
+    ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME));
+    ASSERT_FALSE(opt_lst.empty());
+    EXPECT_EQ(1U, opt_lst.size());
+    FlexOptionImpl::OptionConfigPtr opt_cfg;
+    ASSERT_NO_THROW(opt_cfg = opt_lst.front());
+
+    ASSERT_TRUE(opt_cfg);
+    EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode());
+    EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction());
+    EXPECT_EQ("'abc'", opt_cfg->getText());
+    EXPECT_FALSE(opt_cfg->getDestination());
+}
+
+// Verify that destination query and source response combo is rejected.
+TEST_F(FlexOptionTest, badSourceDestination) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'ab'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(123);
+    option->set("code", code);
+    ElementPtr source = Element::create(string("response"));
+    option->set("source", source);
+    ElementPtr dest = Element::create(string("query"));
+    option->set("destination", dest);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    string expected = "destination 'query' requires source 'query'";
+    EXPECT_EQ(expected, impl_->getErrMsg());
+}
+
+// Verify that the query can be processed.
+TEST_F(FlexOptionTest, processQuery) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr dest = Element::create(string("query"));
+    option->set("destination", dest);
+
+    option = Element::createMap();
+    options->add(option);
+    code = Element::create(DHO_DOMAIN_SEARCH);
+    option->set("code", code);
+    add = Element::create(string("'example.com'"));
+    option->set("add", add);
+    // fqdn option data is parsed using option definition in csv format.
+    option->set("csv-format", Element::create(true));
+    option->set("destination", dest);
+
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg();
+
+    Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 12345));
+    EXPECT_FALSE(query->getOption(DHO_HOST_NAME));
+    EXPECT_FALSE(query->getOption(DHO_DOMAIN_SEARCH));
+
+    EXPECT_NO_THROW(impl_->process<Pkt4Ptr>(Option::V4, query, Pkt4Ptr()));
+
+    OptionPtr opt = query->getOption(DHO_HOST_NAME);
+    ASSERT_TRUE(opt);
+    EXPECT_EQ(DHO_HOST_NAME, opt->getType());
+    const OptionBuffer& buffer = opt->getData();
+    ASSERT_EQ(3U, buffer.size());
+    EXPECT_EQ(0, memcmp(&buffer[0], "abc", 3));
+
+    opt = query->getOption(DHO_DOMAIN_SEARCH);
+    ASSERT_TRUE(opt);
+    EXPECT_EQ(DHO_DOMAIN_SEARCH, opt->getType());
+    const OptionBuffer& buffer_fqdn = opt->getData();
+    ASSERT_EQ(13U, buffer_fqdn.size());
+    EXPECT_EQ(7U, buffer_fqdn[0]);
+    EXPECT_EQ(0, memcmp(&buffer_fqdn[1], "example", 7));
+    EXPECT_EQ(3U, buffer_fqdn[8]);
+    EXPECT_EQ(0, memcmp(&buffer_fqdn[9], "com", 3));
+    EXPECT_EQ(0U, buffer_fqdn[12]);
+}
+
 } // end of anonymous namespace

diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc
index ec22ea5894a5518b18b76c829b639f5972c5122a..87d1d06deb6e1d2cecedcab48cb6ef8a0adb2939 100644 (file)
--- a/src/lib/dhcp/pkt.cc
+++ b/src/lib/dhcp/pkt.cc
@@ -150,6 +150,15 @@ Pkt::addSubClass(const ClientClass& class_def, const ClientClass& subclass) {
     }
 }
 
+void
+Pkt::copyClasses(const Pkt& other) {
+    for (auto const& cclass : other.classes_) {
+        if (!classes_.contains(cclass)) {
+            classes_.insert(cclass);
+        }
+    }
+}
+
 void
 Pkt::updateTimestamp() {
     timestamp_ = boost::posix_time::microsec_clock::universal_time();

diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h
index ad690d64615e320ba77413987d2fc512757efcc8..5182591bda2575efbc7c103cd3a15169d0e52631 100644 (file)
--- a/src/lib/dhcp/pkt.h
+++ b/src/lib/dhcp/pkt.h
@@ -404,6 +404,12 @@ public:
         return (subclasses_);
     }
 
+    /// @brief Simple copy of classes from another packet.
+    ///
+    /// @note To be used by the flex option hook.
+    /// @param other the other packet.
+    void copyClasses(const Pkt& other);
+
     /// @brief Unparsed data (in received packets).
     ///
     /// @warning This public member is accessed by derived