[3292] Fixed configuration of options holding boolean values in DHCPv4.

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -137,19 +137,19 @@ public:
             params["name"] = param_value;
             params["space"] = "dhcp4";
             params["code"] = "56";
-            params["data"] = "AB CDEF0105";
+            params["data"] = "ABCDEF0105";
             params["csv-format"] = "False";
         } else if (parameter == "space") {
             params["name"] = "dhcp-message";
             params["space"] = param_value;
             params["code"] = "56";
-            params["data"] = "AB CDEF0105";
+            params["data"] = "ABCDEF0105";
             params["csv-format"] = "False";
         } else if (parameter == "code") {
             params["name"] = "dhcp-message";
             params["space"] = "dhcp4";
             params["code"] = param_value;
-            params["data"] = "AB CDEF0105";
+            params["data"] = "ABCDEF0105";
             params["csv-format"] = "False";
         } else if (parameter == "data") {
             params["name"] = "dhcp-message";
@@ -161,7 +161,7 @@ public:
             params["name"] = "dhcp-message";
             params["space"] = "dhcp4";
             params["code"] = "56";
-            params["data"] = "AB CDEF0105";
+            params["data"] = "ABCDEF0105";
             params["csv-format"] = param_value;
         }
         return (createConfigWithOption(params));
@@ -212,6 +212,47 @@ public:
         return (stream.str());
     }
 
+    Subnet::OptionDescriptor
+    getOptionFromSubnet(const IOAddress& subnet_address,
+                        const uint16_t option_code,
+                        const uint16_t expected_options_count = 1) {
+        Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(subnet_address);
+        if (!subnet) {
+            /// @todo replace toText() with the use of operator <<.
+            ADD_FAILURE() << "A subnet for the specified address "
+                          << subnet_address.toText()
+                          << "does not exist in Config Manager";
+        }
+        Subnet::OptionContainerPtr options =
+            subnet->getOptionDescriptors("dhcp4");
+        if (expected_options_count != options->size()) {
+            ADD_FAILURE() << "The number of options in the subnet '"
+                          << subnet_address.toText() << "' is different "
+                " than expected number of options '"
+                          << expected_options_count << "'";
+        }
+
+        // Get the search index. Index #1 is to search using option code.
+        const Subnet::OptionContainerTypeIndex& idx = options->get<1>();
+
+        // Get the options for specified index. Expecting one option to be
+        // returned but in theory we may have multiple options with the same
+        // code so we get the range.
+        std::pair<Subnet::OptionContainerTypeIndex::const_iterator,
+                  Subnet::OptionContainerTypeIndex::const_iterator> range =
+            idx.equal_range(option_code);
+        if (std::distance(range.first, range.second) > 1) {
+            ADD_FAILURE() << "There is more than one option having the"
+                " option code '" << option_code << "' in a subnet '"
+                          << subnet_address.toText() << "'. Expected "
+                " at most one option";
+        } else if (std::distance(range.first, range.second) == 0) {
+            return (Subnet::OptionDescriptor(OptionPtr(), false));
+        }
+
+        return (*range.first);
+    }
+
     /// @brief Test invalid option parameter value.
     ///
     /// This test function constructs the simple configuration
@@ -233,6 +274,24 @@ public:
         ASSERT_EQ(1, rcode_);
     }
 
+    /// @brief Test invalid option paramater value.
+    ///
+    /// This test function constructs the simple configuration
+    /// string and injects invalid option configuration into it.
+    /// It expects that parser will fail with provided option code.
+    ///
+    /// @param params Map of parameters defining an option.
+    void
+    testInvalidOptionParam(const std::map<std::string, std::string>& params) {
+        ConstElementPtr x;
+        std::string config = createConfigWithOption(params);
+        ElementPtr json = Element::fromJSON(config);
+        EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
+        ASSERT_TRUE(x);
+        comment_ = parseAnswer(rcode_, x);
+        ASSERT_EQ(1, rcode_);
+    }
+
     /// @brief Test option against given code and data.
     ///
     /// @param option_desc option descriptor that carries the option to
@@ -274,6 +333,39 @@ public:
                             expected_data_len));
     }
 
+    /// @brief Test option configuration.
+    ///
+    /// This function creates a configuration for a specified option using
+    /// a map of parameters specified as the argument. The map holds
+    /// name/value pairs which identifies option's configuration parameters:
+    /// - name
+    /// - space
+    /// - code
+    /// - data
+    /// - csv-format.
+    /// This function applies a new server configuration and checks that the
+    /// option being configured is inserted into CfgMgr. The raw contents of
+    /// this option are compared with the binary data specified as expected
+    /// data passed to this function.
+    ///
+    /// @param params Map of parameters defining an option.
+    /// @param option_code Option code.
+    /// @param expected_data Array containing binary data expected to be stored
+    /// in the configured option.
+    /// @param expected_data_len Length of the array holding reference data.
+    void testConfiguration(const std::map<std::string, std::string>& params,
+                           const uint16_t option_code,
+                           const uint8_t* expected_data,
+                           const size_t expected_data_len) {
+        std::string config = createConfigWithOption(params);
+        ASSERT_TRUE(executeConfiguration(config, "parse option configuration"));
+        // The subnet should now hold one option with the code 19.
+        Subnet::OptionDescriptor desc =
+            getOptionFromSubnet(IOAddress("192.0.2.24"), 19);
+        ASSERT_TRUE(desc.option);
+        testOption(desc, option_code, expected_data, expected_data_len);
+    }
+
     /// @brief Parse and Execute configuration
     ///
     /// Parses a configuration and executes a configuration of the server.
@@ -1394,7 +1486,7 @@ TEST_F(Dhcp4ParserTest, optionDataDefaults) {
         "    \"name\": \"dhcp-message\","
         "    \"space\": \"dhcp4\","
         "    \"code\": 56,"
-        "    \"data\": \"AB CDEF0105\","
+        "    \"data\": \"ABCDEF0105\","
         "    \"csv-format\": False"
         " },"
         " {"
@@ -1467,7 +1559,7 @@ TEST_F(Dhcp4ParserTest, optionDataTwoSpaces) {
         "    \"name\": \"dhcp-message\","
         "    \"space\": \"dhcp4\","
         "    \"code\": 56,"
-        "    \"data\": \"AB CDEF0105\","
+        "    \"data\": \"ABCDEF0105\","
         "    \"csv-format\": False"
         " },"
         " {"
@@ -1644,7 +1736,6 @@ TEST_F(Dhcp4ParserTest, optionDataEncapsulate) {
         " } ]"
         "}";
 
-
     json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
@@ -1700,7 +1791,7 @@ TEST_F(Dhcp4ParserTest, optionDataInSingleSubnet) {
         "          \"name\": \"dhcp-message\","
         "          \"space\": \"dhcp4\","
         "          \"code\": 56,"
-        "          \"data\": \"AB CDEF0105\","
+        "          \"data\": \"ABCDEF0105\","
         "          \"csv-format\": False"
         "        },"
         "        {"
@@ -1751,6 +1842,91 @@ TEST_F(Dhcp4ParserTest, optionDataInSingleSubnet) {
     testOption(*range.first, 23, foo2_expected, sizeof(foo2_expected));
 }
 
+// The goal of this test is to check that the option carrying a bolean
+// value can be configured using one of the values: "true", "false", "0"
+// or "1".
+TEST_F(Dhcp4ParserTest, optionDataBoolean) {
+    // Create configuration. Use standard option 19 (ip-forwarding).
+    std::map<std::string, std::string> params;
+    params["name"] = "ip-forwarding";
+    params["space"] = "dhcp4";
+    params["code"] = "19";
+    params["data"] = "true";
+    params["csv-format"] = "true";
+
+    std::string config = createConfigWithOption(params);
+    ASSERT_TRUE(executeConfiguration(config, "parse configuration with a"
+                                     " boolean value"));
+
+    // The subnet should now hold one option with the code 19.
+    Subnet::OptionDescriptor desc = getOptionFromSubnet(IOAddress("192.0.2.24"),
+                                                        19);
+    ASSERT_TRUE(desc.option);
+
+    // This option should be set to "true", represented as 0x1 in the option
+    // buffer.
+    uint8_t expected_option_data[] = {
+        0x1
+    };
+    testOption(desc, 19, expected_option_data, sizeof(expected_option_data));
+
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // Configure the option with the "1" value. This should have the same
+    // effect as if "true" was specified.
+    params["data"] = "1";
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // The value of "1" with a few leading zeros should work too.
+    params["data"] = "00001";
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // Configure the option with the "false" value.
+    params["data"] = "false";
+    // The option buffer should now hold the value of 0.
+    expected_option_data[0] = 0;
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // Specifying "0" should have the same effect as "false".
+    params["data"] = "0";
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // The same effect should be for multiple 0 chars.
+    params["data"] = "00000";
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // Bogus values should not be accepted.
+    params["data"] = "bugus";
+    testInvalidOptionParam(params);
+
+    params["data"] = "2";
+    testInvalidOptionParam(params);
+
+    // Now let's test that it is possible to use binary format.
+    params["data"] = "0";
+    params["csv-format"] = "false";
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // The binary 1 should work as well.
+    params["data"] = "1";
+    expected_option_data[0] = 1;
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+    // As well as an even number of digits.
+    params["data"] = "01";
+    testConfiguration(params, 19, expected_option_data,
+                      sizeof(expected_option_data));
+
+}
+
 // Goal of this test is to verify options configuration
 // for multiple subnets.
 TEST_F(Dhcp4ParserTest, optionDataInMultipleSubnets) {
@@ -1828,6 +2004,8 @@ TEST_F(Dhcp4ParserTest, optionDataInMultipleSubnets) {
     testOption(*range2.first, 23, foo2_expected, sizeof(foo2_expected));
 }
 
+
+
 // Verify that empty option name is rejected in the configuration.
 TEST_F(Dhcp4ParserTest, optionNameEmpty) {
     // Empty option names not allowed.
@@ -1878,14 +2056,6 @@ TEST_F(Dhcp4ParserTest, optionDataUnexpectedPrefix) {
     testInvalidOptionParam("0x0102", "data");
 }
 
-// Verify that option data consisting od an odd number of
-// hexadecimal digits is rejected in the configuration.
-TEST_F(Dhcp4ParserTest, optionDataOddLength) {
-    // Option code 0 is reserved and should not be accepted
-    // by configuration parser.
-    testInvalidOptionParam("123", "data");
-}
-
 // Verify that either lower or upper case characters are allowed
 // to specify the option data.
 TEST_F(Dhcp4ParserTest, optionDataLowerCase) {
@@ -2201,7 +2371,7 @@ TEST_F(Dhcp4ParserTest, vendorOptionsHex) {
         "    \"name\": \"option-one\","
         "    \"space\": \"vendor-4491\"," // VENDOR_ID_CABLE_LABS = 4491
         "    \"code\": 100," // just a random code
-        "    \"data\": \"AB CDEF0105\","
+        "    \"data\": \"ABCDEF0105\","
         "    \"csv-format\": False"
         " },"
         " {"
@@ -2333,7 +2503,7 @@ buildHooksLibrariesConfig(const std::vector<std::string>& libraries) {
         "    \"name\": \"dhcp-message\","
         "    \"space\": \"dhcp4\","
         "    \"code\": 56,"
-        "    \"data\": \"AB CDEF0105\","
+        "    \"data\": \"ABCDEF0105\","
         "    \"csv-format\": False"
         " },"
         " {"

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -464,7 +464,7 @@ OptionDataParser::createOption() {
     }
 
     // Get option data from the configuration database ('data' field).
-    const std::string option_data = string_values_->getParam("data");
+    std::string option_data = string_values_->getParam("data");
 
     // Transform string of hexadecimal digits into binary format.
     std::vector<uint8_t> binary;
@@ -480,6 +480,12 @@ OptionDataParser::createOption() {
         // Otherwise, the option data is specified as a string of
         // hexadecimal digits that we have to turn into binary format.
         try {
+            // The decodeHex function expects that the string contains an
+            // even number of digits. If we don't meet this requirement,
+            // we have to insert a leading 0.
+            if (!option_data.empty() && option_data.length() % 2) {
+                option_data = option_data.insert(0, "0");
+            }
             util::encode::decodeHex(option_data, binary);
         } catch (...) {
             isc_throw(DhcpConfigError, "option data is not a valid"