[#401,!254] kea-dhcp4 now merges CB options into current config

Basic merging works, need to add validation against definitions.

src/bin/dhcp4/tests/config_backend_unittest.cc
    TEST_F(Dhcp4CBTest, mergeOptions) - enabled and updated

src/lib/dhcpsrv/cfg_option.*
    CfgOption::merge(CfgOption& other) - new method, conformant
    to others, that merges/updates a config option from another

src/lib/dhcpsrv/srv_config.cc
    SrvConfig::merge4(SrvConfig& other) - modified to
    merge configured options

src/lib/dhcpsrv/tests/cfg_option_unittest.cc
    TEST_F(CfgOptionTest, merge) - new test

src/bin/dhcp4/tests/config_backend_unittest.cc

@@ -335,15 +335,19 @@ TEST_F(Dhcp4CBTest, mergeOptionDefs) {
 
 // This test verifies that externally configured options
 // merged correctly into staging configuration.
-// @todo enable test when SrvConfig can merge options.
-TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) {
+TEST_F(Dhcp4CBTest, mergeOptions) {
     string base_config =
         "{ \n"
-        "    \"option-data\": [ {"
-        "        \"name\": \"dhcp-message\","
-        "        \"data\": \"0A0B0C0D\","
-        "        \"csv-format\": false"
-        "     } ],"
+        "    \"option-data\": [ { \n"
+        "        \"name\": \"dhcp-message\", \n"
+        "        \"data\": \"0A0B0C0D\", \n"
+        "        \"csv-format\": false \n"
+        "     },{ \n" 
+        "        \"name\": \"host-name\", \n"
+        "        \"data\": \"old.example.com\", \n"
+        "        \"csv-format\": true \n"
+        "     } \n"
+        "    ], \n"
         "    \"config-control\": { \n"
         "       \"config-databases\": [ { \n"
         "               \"type\": \"memfile\", \n"
@@ -358,19 +362,28 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) {
 
     extractConfig(base_config);
 
-    // Create option two and add it to first backend.
-    OptionDescriptorPtr opt_two(new OptionDescriptor(
-        createOption<OptionString>(Option::V4, DHO_BOOT_FILE_NAME,
-                                   true, false, "my-boot-file")));
-    opt_two->space_name_ = DHCP4_OPTION_SPACE;
-    db1_->createUpdateOption4(ServerSelector::ALL(), opt_two);
+    OptionDescriptorPtr opt;
 
-    // Create option three and add it to second backend.
-    OptionDescriptorPtr opt_three(new OptionDescriptor(
-        createOption<OptionString>(Option::V4, DHO_BOOT_FILE_NAME,
-                                   true, false, "your-boot-file")));
-    opt_three->space_name_ = DHCP4_OPTION_SPACE;
-    db2_->createUpdateOption4(ServerSelector::ALL(), opt_three);
+    // Add host-name to the first backend.
+    opt.reset(new OptionDescriptor(
+              createOption<OptionString>(Option::V4, DHO_HOST_NAME,
+                                         true, false, "new.example.com")));
+    opt->space_name_ = DHCP4_OPTION_SPACE;
+    db1_->createUpdateOption4(ServerSelector::ALL(), opt);
+
+    // Add boot-file-name to the first backend.
+    opt.reset(new OptionDescriptor(
+              createOption<OptionString>(Option::V4, DHO_BOOT_FILE_NAME,
+                                         true, false, "my-boot-file")));
+    opt->space_name_ = DHCP4_OPTION_SPACE;
+    db1_->createUpdateOption4(ServerSelector::ALL(), opt);
+
+    // Add boot-file-name to the second backend.
+    opt.reset(new OptionDescriptor(
+              createOption<OptionString>(Option::V4, DHO_BOOT_FILE_NAME,
+                                         true, false, "your-boot-file")));
+    opt->space_name_ = DHCP4_OPTION_SPACE;
+    db2_->createUpdateOption4(ServerSelector::ALL(), opt);
 
     // Should parse and merge without error.
     ASSERT_NO_FATAL_FAILURE(configure(base_config, CONTROL_RESULT_SUCCESS, ""));
@@ -381,13 +394,21 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) {
     // Option definition from JSON should be there.
     CfgOptionPtr options = staging_cfg->getCfgOption();
 
+    // dhcp-message should come from the original config.
     OptionDescriptor found_opt = options->get("dhcp4", DHO_DHCP_MESSAGE);
     ASSERT_TRUE(found_opt.option_);
     EXPECT_EQ("0x0A0B0C0D", found_opt.option_->toHexString());
 
+    // host-name should come from the first back end, 
+    // (overwriting the original).
+    found_opt = options->get("dhcp4", DHO_HOST_NAME);
+    ASSERT_TRUE(found_opt.option_);
+    EXPECT_EQ("new.example.com", found_opt.option_->toString());
+
+    // booth-file-name should come from the first back end.
     found_opt = options->get("dhcp4", DHO_BOOT_FILE_NAME);
     ASSERT_TRUE(found_opt.option_);
-    EXPECT_EQ("my-boot-file", found_opt.formatted_value_);
+    EXPECT_EQ("my-boot-file", found_opt.option_->toString());
 }
 
 // This test verifies that externally configured shared-networks are

src/lib/dhcpsrv/cfg_option.cc

@@ -81,6 +81,18 @@ CfgOption::getVendorIdsSpaceNames() const {
     return (names);
 }
 
+void
+CfgOption::merge(CfgOption& other) {
+    // First we merge our options into other.
+    // This adds my opitions that are not
+    // in other, to other (i.e we skip over
+    // duplicates). 
+    mergeTo(other);
+
+    // Next we copy "other" on top of ourself.
+    other.copyTo(*this);
+}
+
 void
 CfgOption::mergeTo(CfgOption& other) const {
     // Merge non-vendor options.

src/lib/dhcpsrv/cfg_option.h

@@ -332,6 +332,22 @@ public:
     /// @throw isc::BadValue if the option space is invalid.
     void add(const OptionDescriptor& desc, const std::string& option_space);
 
+    /// @brief Merges another option configuration into this one.
+    ///
+    /// This method calls @c mergeTo() to add this configuration's
+    /// options into @c other (skipping any duplicates). It then calls
+    /// @c copyTo() to overwrite this configurations' options with
+    /// the merged set in @c other. 
+    ///
+    /// @warning The merge operation will affect the @c other configuration.
+    /// Therefore, the caller must not rely on the data held in the @c other
+    /// object after the call to @c merge. Also, the data held in @c other must
+    /// not be modified after the call to @c merge because it may affect the
+    /// merged configuration.
+    ///
+    /// @param option configurations to merge with.
+    void merge(CfgOption& other);
+
     /// @brief Merges this configuration to another configuration.
     ///
     /// This method iterates over the configuration items held in this
@@ -339,8 +355,6 @@ public:
     /// as a parameter. If an item exists in the destination it is not
     /// copied.
     ///
-    /// @note: this method is not longer used so should become private.
-    ///
     /// @param [out] other Configuration object to merge to.
     void mergeTo(CfgOption& other) const;
 

src/lib/dhcpsrv/srv_config.cc

@@ -177,23 +177,26 @@ SrvConfig::merge(ConfigBase& other) {
 
 void
 SrvConfig::merge4(SrvConfig& other) {
-    /// We merge objects in order of dependency (real or theoretical).
+    // We merge objects in order of dependency (real or theoretical).
 
-    /// Merge globals.
+    // Merge globals.
     mergeGlobals4(other);
 
-    /// Merge option defs
+    // Merge option defs
     cfg_option_def_->merge((*other.getCfgOptionDef()));
 
-    /// @todo merge options
+    // Merge options
+    // @todo should we sanity check and make sure
+    // that there are option defs for merged options?
+    cfg_option_->merge((*other.getCfgOption()));
 
     // Merge shared networks.
     cfg_shared_networks4_->merge(*(other.getCfgSharedNetworks4()));
 
-    /// Merge subnets.
+    // Merge subnets.
     cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4()));
 
-    /// @todo merge other parts of the configuration here.
+    // @todo merge other parts of the configuration here.
 }
 
 void

src/lib/dhcpsrv/tests/cfg_option_unittest.cc

@@ -213,8 +213,8 @@ TEST_F(CfgOptionTest, add) {
     EXPECT_TRUE(options->empty());
 }
 
-// This test verifies that two option configurations can be merged.
-TEST_F(CfgOptionTest, merge) {
+// This test verifies that one configuration can be merged into another.
+TEST_F(CfgOptionTest, mergeTo) {
     CfgOption cfg_src;
     CfgOption cfg_dst;
 
@@ -327,6 +327,87 @@ TEST_F(CfgOptionTest, copy) {
     EXPECT_EQ(10, container->size());
 }
 
+// This test verifies option definitions from one configuration
+// can be used to update definitions in another configuration.
+// In other words, definitions from an "other" configuration
+// can be merged into an existing configuration, with any
+// duplicates in other overwriting those in the existing
+// configuration.  
+TEST_F(CfgOptionTest, merge) {
+    CfgOption this_cfg;
+    CfgOption other_cfg;
+
+    // Create collection of options in option space dhcp6, with option codes
+    // from the range of 100 to 109 and holding one byte of data equal to 0xFF.
+    for (uint16_t code = 100; code < 110; ++code) {
+        OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0xFF)));
+        ASSERT_NO_THROW(this_cfg.add(option, false, DHCP6_OPTION_SPACE));
+    }
+
+    // Create collection of options in vendor space 123, with option codes
+    // from the range of 100 to 109 and holding one byte of data equal to 0xFF.
+    for (uint16_t code = 100; code < 110; code += 2) {
+        OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0xFF)));
+        ASSERT_NO_THROW(this_cfg.add(option, false, "vendor-123"));
+    }
+
+    // Create destination configuration (configuration that we merge the
+    // other configuration to).
+
+    // Create collection of options having even option codes in the range of
+    // 100 to 108.
+    for (uint16_t code = 100; code < 110; code += 2) {
+        OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0x01)));
+        ASSERT_NO_THROW(other_cfg.add(option, false, DHCP6_OPTION_SPACE));
+    }
+
+    // Create collection of options having odd option codes in the range of
+    // 101 to 109.
+    for (uint16_t code = 101; code < 110; code += 2) {
+        OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0x01)));
+        ASSERT_NO_THROW(other_cfg.add(option, false, "vendor-123"));
+    }
+
+    // Merge source configuration to the destination configuration. The options
+    // in the destination should be preserved. The options from the source
+    // configuration should be added.
+    ASSERT_NO_THROW(this_cfg.merge(other_cfg));
+
+
+    // Validate the options in the dhcp6 option space in the destination.
+    for (uint16_t code = 100; code < 110; ++code) {
+        OptionDescriptor desc = this_cfg.get(DHCP6_OPTION_SPACE, code);
+        ASSERT_TRUE(desc.option_);
+        ASSERT_EQ(1, desc.option_->getData().size());
+        // The options with even option codes should hold one byte of data
+        // equal to 0x1. These are the ones that we have initially added to
+        // the destination configuration. The other options should hold the
+        // values of 0xFF which indicates that they have been merged from the
+        // source configuration.
+        if ((code % 2) == 0) {
+            EXPECT_EQ(0x01, desc.option_->getData()[0]);
+        } else {
+            EXPECT_EQ(0xFF, desc.option_->getData()[0]);
+        }
+    }
+
+    // Validate the options in the vendor space.
+    for (uint16_t code = 100; code < 110; ++code) {
+        OptionDescriptor desc = this_cfg.get(123, code);
+        ASSERT_TRUE(desc.option_);
+        ASSERT_EQ(1, desc.option_->getData().size());
+        // This time, the options with even option codes should hold a byte
+        // of data equal to 0xFF. The other options should hold the byte of
+        // data equal to 0x01.
+        if ((code % 2) == 0) {
+            EXPECT_EQ(0xFF, desc.option_->getData()[0]);
+        } else {
+            EXPECT_EQ(0x01, desc.option_->getData()[0]);
+        }
+    }
+}
+
+
 // This test verifies that encapsulated options are added as sub-options
 // to the top level options on request.
 TEST_F(CfgOptionTest, encapsulate) {