[#2314] Checkpoint: began sub-option UTs

diff --git a/src/hooks/dhcp/flex_option/flex_option.cc b/src/hooks/dhcp/flex_option/flex_option.cc
index be3b93c292098bd5f37d87a4128a11e45814a144..42de78f1a1bb6bbe432eaae71f630ed5c2bc6635 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.cc
+++ b/src/hooks/dhcp/flex_option/flex_option.cc
@@ -316,6 +316,18 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
 
     // opt_cfg initial action is NONE.
     if (sub_options) {
+        string action;
+        if (option->contains("add")) {
+            action = "add";
+        } else if (option->contains("supersede")) {
+            action = "supersede";
+        } else if (option->contains("remove")) {
+            action = "remove";
+        }
+        if (!action.empty()) {
+            isc_throw(BadValue, "'sub-options' and '" << action << "' are "
+                      << "incompatible in the same entry");
+        }
         parseSubOptions(sub_options, opt_cfg, universe);
     } else {
         parseAction(option, opt_cfg, universe,
@@ -340,7 +352,6 @@ void
 FlexOptionImpl::parseSubOptions(ConstElementPtr sub_options,
                                 OptionConfigPtr opt_cfg,
                                 Option::Universe universe) {
-    opt_cfg->setAction(FlexOptionImpl::SUB_OPTIONS);
     for (ConstElementPtr sub_option : sub_options->listValue()) {
         parseSubOption(sub_option, opt_cfg, universe);
     }
@@ -428,13 +439,13 @@ FlexOptionImpl::parseSubOption(ConstElementPtr sub_option,
             def = LibDHCP::getLastResortOptionDef(space, name);
         }
         if (!def) {
-            isc_throw(BadValue, "no known '" << name << "' option in '"
+            isc_throw(BadValue, "no known '" << name << "' sub-option in '"
                       << space << "' space");
         }
         if (code_elem && (def->getCode() != code)) {
-            isc_throw(BadValue, "option '" << name << "' is defined as code: "
-                      << def->getCode() << ", not the specified code: "
-                      << code);
+            isc_throw(BadValue, "sub-option '" << name
+                      << "' is defined as code: " << def->getCode()
+                      << ", not the specified code: " << code);
         }
         code = def->getCode();
     }
@@ -459,7 +470,7 @@ FlexOptionImpl::parseSubOption(ConstElementPtr sub_option,
             def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, code);
         }
         if (!def) {
-            isc_throw(BadValue, "no known option with code '" << code
+            isc_throw(BadValue, "no known sub-option with code '" << code
                       << "' in '" << space << "' space");
         }
     }
@@ -526,7 +537,7 @@ FlexOptionImpl::logClass(const ClientClass& client_class, uint16_t code) {
 void
 FlexOptionImpl::logAction(Action action, uint16_t code,
                           const string& value) {
-    if ((action == NONE) || (action == SUB_OPTIONS)) {
+    if (action == NONE) {
         return;
     }
     if (action == REMOVE) {
@@ -588,7 +599,7 @@ void
 FlexOptionImpl::logSubAction(Action action, uint16_t code,
                              uint16_t container_code,
                              const string& value) {
-    if ((action == NONE) || (action == SUB_OPTIONS)) {
+    if (action == NONE) {
         return;
     }
     if (action == REMOVE) {

diff --git a/src/hooks/dhcp/flex_option/flex_option.h b/src/hooks/dhcp/flex_option/flex_option.h
index de35cbbe56639742b49ac94e55f000d8bcee90fb..f17361afb6b33a488f593de67dc2de97ae867e6b 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.h
+++ b/src/hooks/dhcp/flex_option/flex_option.h
@@ -43,13 +43,11 @@ public:
     ///  - add (if not already existing)
     ///  - supersede (as add but also when already existing)
     ///  - remove
-    ///  - sub-options
     enum Action {
         NONE,
         ADD,
         SUPERSEDE,
-        REMOVE,
-        SUB_OPTIONS
+        REMOVE
     };
 
     /// @brief Base option configuration.
@@ -266,9 +264,9 @@ public:
         return (option_config_map_);
     }
 
-    /// @brief Get the sub-option config map of map.
+    /// @brief Get the sub-option config map of maps.
     ///
-    /// @return The sub-option config map of map.
+    /// @return The sub-option config map of maps.
     const SubOptionConfigMapMap& getSubOptionConfigMap() const {
         return (sub_option_config_map_);
     }
@@ -377,9 +375,6 @@ public:
                     }
                     logAction(REMOVE, opt_cfg->getCode(), "");
                     break;
-                case SUB_OPTIONS:
-                    // Done in another loop.
-                    break;
                 }
             }
         }
@@ -412,7 +407,6 @@ public:
                 uint32_t vendor_id = sub_cfg->getVendorId();
                 switch (sub_cfg->getAction()) {
                 case NONE:
-                case SUB_OPTIONS:
                     break;
                 case ADD:
                     // If no container and no magic add return
@@ -605,6 +599,13 @@ protected:
         return (option_config_map_);
     }
 
+    /// @brief Get a mutable reference to the sub-option config map of maps.
+    ///
+    /// @return The sub-option config map of maps.
+    SubOptionConfigMapMap& getMutableSubOptionConfigMap() {
+        return (sub_option_config_map_);
+    }
+
 private:
     /// @brief Option parameters.
     static const data::SimpleKeywords OPTION_PARAMETERS;
@@ -615,7 +616,7 @@ private:
     /// @brief The option config map (code and pointer to option config).
     OptionConfigMap option_config_map_;
 
-    /// @brief The sub-option config map of map.
+    /// @brief The sub-option config map of maps.
     SubOptionConfigMapMap sub_option_config_map_;
 
     /// @brief Parse an option config.

diff --git a/src/hooks/dhcp/flex_option/tests/sub_option_unittests.cc b/src/hooks/dhcp/flex_option/tests/sub_option_unittests.cc
index 82f761313c281f0da41b41ed7b0e293a86dc9707..b5d13c32605e00278a4d4cac0e65d23b8d2b4ab4 100644 (file)
--- a/src/hooks/dhcp/flex_option/tests/sub_option_unittests.cc
+++ b/src/hooks/dhcp/flex_option/tests/sub_option_unittests.cc
@@ -91,7 +91,25 @@ TEST_F(FlexSubOptionTest, subOptionConfigNotMap) {
     EXPECT_EQ("sub-option element is not a map", impl_->getErrMsg());
 }
 
-// multiple.
+// Verify that multiple actions are not accepted.
+TEST_F(FlexSubOptionTest, configMultipleAction) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(DHO_HOST_NAME);
+    option->set("code", code);
+
+    // add and sub-options.
+    ElementPtr add = Element::create(string("'abc'"));
+    option->set("add", add);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    ostringstream errmsg;
+    errmsg << "'sub-options' and 'add' are incompatible in the same entry";
+    EXPECT_EQ(errmsg.str(), impl_->getErrMsg());
+}
 
 // Verify that an unknown option keyword is rejected.
 TEST_F(FlexSubOptionTest, subOptionConfigUnknown) {
@@ -111,4 +129,304 @@ TEST_F(FlexSubOptionTest, subOptionConfigUnknown) {
     EXPECT_EQ("unknown parameter 'delete'", impl_->getErrMsg());
 }
 
+// Verify that a sub-option configuration must have code or name.
+TEST_F(FlexSubOptionTest, subOptionConfigNoCodeName) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    ostringstream errmsg;
+    errmsg << "'code' or 'name' must be specified: " << sub_option->str();
+    EXPECT_EQ(errmsg.str(), impl_->getErrMsg());
+}
+
+// Verify that a sub-option configuration must retrive a space.
+TEST_F(FlexSubOptionTest, subOptionConfigNoSpace) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("container is not defined: can't get space", impl_->getErrMsg());
+}
+
+// Verify that the v4 sub-option code must be an integer in [0..255].
+TEST_F(FlexSubOptionTest, subOptionConfigBadCode4) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    code = Element::create(false);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("'code' must be an integer: false", impl_->getErrMsg());
+
+    code = Element::create(-1);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), OutOfRange);
+    EXPECT_EQ("invalid 'code' value -1 not in [0..255]", impl_->getErrMsg());
+
+    code = Element::create(256);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), OutOfRange);
+    EXPECT_EQ("invalid 'code' value 256 not in [0..255]", impl_->getErrMsg());
+
+    code = Element::create(1);
+    sub_option->set("code", code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_EQ("", impl_->getErrMsg());
+    EXPECT_TRUE(impl_->getErrMsg().empty());
+
+    code = Element::create(254);
+    sub_option->set("code", code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty());
+}
+
+// Verify that the v6 option code must be an integer in [0..65535].
+TEST_F(FlexSubOptionTest, subOptionConfigBadCode6) {
+    CfgMgr::instance().setFamily(AF_INET6);
+
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    code = Element::create(false);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("'code' must be an integer: false", impl_->getErrMsg());
+
+    code = Element::create(-1);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), OutOfRange);
+    EXPECT_EQ("invalid 'code' value -1 not in [0..65535]", impl_->getErrMsg());
+
+    code = Element::create(65536);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), OutOfRange);
+    EXPECT_EQ("invalid 'code' value 65536 not in [0..65535]", impl_->getErrMsg());
+
+    code = Element::create(1);
+    sub_option->set("code", code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty());
+
+    code = Element::create(65535);
+    sub_option->set("code", code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty());
+}
+
+// Verify that the space must be a string.
+TEST_F(FlexSubOptionTest, subOptionConfigBadSpace) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(true);
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("'space' must be a string: true", impl_->getErrMsg());
+}
+
+// Verify that the space must be valid.
+TEST_F(FlexSubOptionTest, subOptionConfigInvalidSpace) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("-bad-"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    sub_option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("'-bad-' is not a valid space name", impl_->getErrMsg());
+}
+
+// Verify that the name must be a string.
+TEST_F(FlexSubOptionTest, subOptionConfigBadName) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    ElementPtr name = Element::create(true);
+    sub_option->set("name", name);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("'name' must be a string: true", impl_->getErrMsg());
+}
+
+// Verify that the name must not be empty.
+TEST_F(FlexSubOptionTest, subOptionConfigEmptyName) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    ElementPtr name = Element::create(string());
+    sub_option->set("name", name);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("'name' must not be empty", impl_->getErrMsg());
+}
+
+// Verify that the name must be a known option.
+TEST_F(FlexSubOptionTest, subOptionConfigUnknownName) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    ElementPtr name = Element::create(string("foobar"));
+    sub_option->set("name", name);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("no known 'foobar' sub-option in 'my-space' space",
+              impl_->getErrMsg());
+}
+
+// Verify that the definition is not required when csv-format is not specified.
+TEST_F(FlexSubOptionTest, subOptionConfigUnknownCodeNoCSVFormat) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    ElementPtr sub_code = Element::create(222);
+    sub_option->set("code", sub_code);
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty());
+
+    auto map = impl_->getSubOptionConfigMap();
+    EXPECT_EQ(1, map.count(109));
+    auto smap = map[109];
+    EXPECT_EQ(1, smap.count(222));
+}
+
+// Verify that the definition is not required when csv-format is false.
+TEST_F(FlexSubOptionTest, subOptionConfigUnknownCodeDisableCSVFormat) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    ElementPtr sub_code = Element::create(222);
+    sub_option->set("code", sub_code);
+    // Disable csv-format.
+    sub_option->set("csv-format", Element::create(false));
+    EXPECT_NO_THROW(impl_->testConfigure(options));
+    EXPECT_TRUE(impl_->getErrMsg().empty());
+
+    auto map = impl_->getSubOptionConfigMap();
+    EXPECT_EQ(1, map.count(109));
+    auto smap = map[109];
+    EXPECT_EQ(1, smap.count(222));
+}
+
+// Verify that the code must be a known sub-option when csv-format is true.
+TEST_F(FlexSubOptionTest, subOptionConfigUnknownCodeEnableCSVFormat) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    ElementPtr sub_options = Element::createList();
+    option->set("sub-options", sub_options);
+    ElementPtr sub_option = Element::createMap();
+    sub_options->add(sub_option);
+    ElementPtr space = Element::create(string("my-space"));
+    sub_option->set("space", space);
+    ElementPtr add = Element::create(string("'ab'"));
+    sub_option->set("add", add);
+    ElementPtr sub_code = Element::create(222);
+    sub_option->set("code", sub_code);
+    // Enable csv-format.
+    sub_option->set("csv-format", Element::create(true));
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("no known sub-option with code '222' in 'my-space' space",
+              impl_->getErrMsg());
+}
+
 } // end of anonymous namespace

diff --git a/src/hooks/dhcp/flex_option/tests/test_flex_option.h b/src/hooks/dhcp/flex_option/tests/test_flex_option.h
index 7ad2fc822693583ee03664e9324746396c89e55e..df0a9bf40cfce94bb113a1b9a144a7897eb277eb 100644 (file)
--- a/src/hooks/dhcp/flex_option/tests/test_flex_option.h
+++ b/src/hooks/dhcp/flex_option/tests/test_flex_option.h
@@ -10,6 +10,7 @@
 #define TEST_FLEX_OPTION_H
 
 #include <flex_option.h>
+#include <dhcp/libdhcp++.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <gtest/gtest.h>
 
@@ -23,7 +24,10 @@ public:
     /// Export getMutableOptionConfigMap.
     using FlexOptionImpl::getMutableOptionConfigMap;
 
-    /// @brief Configure clone which records the error.
+    /// Export getMutableSubOptionConfigMap.
+    using FlexOptionImpl::getMutableSubOptionConfigMap;
+
+    /// @Brief Configure clone which records the error.
     ///
     /// @param options The element with option config list.
     void testConfigure(isc::data::ConstElementPtr options) {
@@ -62,6 +66,7 @@ public:
 
     /// @brief Destructor.
     virtual ~BaseFlexOptionTest() {
+        LibDHCP::clearRuntimeOptionDefs();
         isc::dhcp::CfgMgr::instance().setFamily(AF_INET);
         impl_.reset();
     }