[#400,!243] kea-dhcp4 now merges in option definitions

src/bin/dhcp4/tests/config_backend_unittest.cc
    TEST_F(Dhcp4CBTest, mergeGlobals) - enabled/revamped

src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc
    TEST(CfgOptionDefTest, merge) - new test

src/lib/dhcpsrv/cfg_option_def.*
    CfgOptionDef::merge(CfgOptionDef& other)  - new method
    which merges option definitions

Removed const from "other" parameter in in the following merge methods:

    CfgSharedNetworks4::merge(CfgSharedNetworks4& other)
    CfgSubnets4::merge(CfgSharedNetworks4Ptr networks, CfgSubnets4& other);
    SrvConfig::merge(ConfigBase& other)
    SrvConfig::merge4(SrvConfig& other)
    SrvConfig::mergeGlobals4(SrvConfig& other)
    ConfigBase::merge(ConfigBase& other)

src/bin/dhcp4/tests/config_backend_unittest.cc

@@ -259,16 +259,22 @@ TEST_F(Dhcp4CBTest, mergeGlobals) {
 
 // This test verifies that externally configured option definitions
 // merged correctly into staging configuration.
-// @todo enable test when SrvConfig can merge option definitions.
-TEST_F(Dhcp4CBTest, DISABLED_mergeOptionDefs) {
+TEST_F(Dhcp4CBTest, mergeOptionDefs) {
     string base_config =
         "{ \n"
-        "    \"option-def\": [ {"
-        "        \"name\": \"one\","
-        "        \"code\": 100,"
-        "        \"type\": \"ipv4-address\","
-        "        \"space\": \"isc\""
-        "     } ],"
+        "    \"option-def\": [ { \n"
+        "        \"name\": \"one\", \n"
+        "        \"code\": 1, \n"
+        "        \"type\": \"ipv4-address\", \n"
+        "        \"space\": \"isc\" \n"
+        "     }, \n"
+        "     { \n"
+        "        \"name\": \"two\", \n"
+        "        \"code\": 2, \n"
+        "        \"type\": \"string\", \n"
+        "        \"space\": \"isc\" \n"
+        "     } \n"
+        "   ], \n"
         "    \"config-control\": { \n"
         "       \"config-databases\": [ { \n"
         "               \"type\": \"memfile\", \n"
@@ -283,33 +289,47 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptionDefs) {
 
     extractConfig(base_config);
 
-    // Create option two and add it to first backend.
-    OptionDefinitionPtr def_two(new OptionDefinition("two", 234, "string"));
-    def_two->setOptionSpaceName("dhcp4");
-    db1_->createUpdateOptionDef4(ServerSelector::ALL(), def_two);
+    // Create option one replacement and add it to first backend.
+    OptionDefinitionPtr def;
+    def.reset(new OptionDefinition("one", 101, "uint16"));
+    def->setOptionSpaceName("isc");
+    db1_->createUpdateOptionDef4(ServerSelector::ALL(), def);
 
-    // Create option three and add it to second backend.
-    OptionDefinitionPtr def_three(new OptionDefinition("three", 235, "string"));
-    def_three->setOptionSpaceName("dhcp4");
-    db2_->createUpdateOptionDef4(ServerSelector::ALL(), def_two);
+    // Create option three and add it to first backend.
+    def.reset(new OptionDefinition("three", 3, "string"));
+    def->setOptionSpaceName("isc");
+    db1_->createUpdateOptionDef4(ServerSelector::ALL(), def);
+
+    // Create option four and add it to second backend.
+    def.reset(new OptionDefinition("four", 4, "string"));
+    def->setOptionSpaceName("isc");
+    db2_->createUpdateOptionDef4(ServerSelector::ALL(), def);
 
     // Should parse and merge without error.
     ASSERT_NO_FATAL_FAILURE(configure(base_config, CONTROL_RESULT_SUCCESS, ""));
 
     // Verify the composite staging is correct.
     SrvConfigPtr staging_cfg = CfgMgr::instance().getStagingCfg();
-
-    // Option definition from JSON should be there.
     ConstCfgOptionDefPtr option_defs = staging_cfg->getCfgOptionDef();
-    OptionDefinitionPtr found_def = option_defs->get("isc", 100);
-    ASSERT_TRUE(found_def);
 
-    // Option definition from db1 should be there.
-    found_def = option_defs->get("dhcp4", 234);
+    // Definition "one" from first backend should be there.
+    OptionDefinitionPtr found_def = option_defs->get("isc", "one");
     ASSERT_TRUE(found_def);
+    EXPECT_EQ(101, found_def->getCode());
+    EXPECT_EQ(OptionDataType::OPT_UINT16_TYPE, found_def->getType());
 
-    // Option definition from db2 should not be there.
-    found_def = option_defs->get("dhcp4", 235);
+    // Definition "two" from JSON config should be there.
+    found_def = option_defs->get("isc", "two");
+    ASSERT_TRUE(found_def);
+    EXPECT_EQ(2, found_def->getCode());
+
+    // Definition "three" from first backened should be there.
+    found_def = option_defs->get("isc", "three");
+    ASSERT_TRUE(found_def);
+    EXPECT_EQ(3, found_def->getCode());
+
+    // Definition "four" from first backened should not be there.
+    found_def = option_defs->get("isc", "four");
     ASSERT_FALSE(found_def);
 }
 

src/lib/dhcpsrv/cfg_option_def.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015,2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -206,5 +206,36 @@ CfgOptionDef::toElement() const {
     return (result);
 }
 
+void
+CfgOptionDef::merge(CfgOptionDef& other) {
+
+    if (other.getContainer().getOptionSpaceNames().empty()) {
+        // Nothing to merge, don't waste cycles.
+        return;
+    }
+
+
+    // Iterate over this config's definitions in each space.
+    // If either a definition's name or code already exist in
+    // that space in "other", skip it.  Otherwise, add it to "other".
+    auto spaces = option_definitions_.getOptionSpaceNames();
+    for (auto space = spaces.begin(); space != spaces.end(); ++space) {
+        OptionDefContainerPtr my_defs = getAll(*space);
+        for (auto my_def = my_defs->begin(); my_def != my_defs->end(); ++my_def) {
+            if ((other.get(*space, (*my_def)->getName())) ||
+                (other.get(*space, (*my_def)->getCode()))) {
+                // Already in "other" so skip it.
+                continue;
+            }
+
+            // Not in "other" so add it.
+            other.add(*my_def, *space);
+        }
+    }
+
+    // Replace the current definitions with the merged set.
+    other.copyTo(*this);
+}
+
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcpsrv/cfg_option_def.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015,2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -123,6 +123,27 @@ public:
     /// @return a pointer to unparsed configuration
     virtual isc::data::ElementPtr toElement() const;
 
+    /// @brief Merges specified option definitions configuration into this
+    /// configuration.
+    ///
+    /// This method merges the option defintiions from the @c other
+    /// configuration into this configuration.  The merged set of
+    /// definitions is created as follows:
+    ///
+    /// Iterator over the definitions in each name space in this configuration:
+    /// If either the definition's name or code are defined in @c other
+    /// then skip over the definition otherwise add it to @other.
+    ///
+    /// Replace this configuration's definitions with the defnitions
+    /// in @c other using @c copyTo().
+    ///
+    /// @warning The merge operation affects @c other.
+    /// 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.
+    void merge(CfgOptionDef& other);
+
 private:
 
     /// @brief A collection of option definitions.

src/lib/dhcpsrv/cfg_shared_networks.cc

@@ -20,7 +20,7 @@ CfgSharedNetworks4::hasNetworkWithServerId(const IOAddress& server_id) const {
 }
 
 void
-CfgSharedNetworks4::merge(const CfgSharedNetworks4& other) {
+CfgSharedNetworks4::merge(CfgSharedNetworks4& other) {
     auto& index = networks_.get<SharedNetworkNameIndexTag>();
 
     // Iterate over the subnets to be merged. They will replace the existing

src/lib/dhcpsrv/cfg_shared_networks.h

@@ -150,7 +150,7 @@ public:
     ///
     /// @param other the shared network configuration to be merged into this
     /// configuration.
-    void merge(const CfgSharedNetworks4& other);
+    void merge(CfgSharedNetworks4& other);
 };
 
 /// @brief Pointer to the configuration of IPv4 shared networks.

src/lib/dhcpsrv/cfg_subnets4.cc

@@ -57,7 +57,7 @@ CfgSubnets4::del(const ConstSubnet4Ptr& subnet) {
 
 void
 CfgSubnets4::merge(CfgSharedNetworks4Ptr networks,
-                   const CfgSubnets4& other) {
+                   CfgSubnets4& other) {
     auto& index = subnets_.get<SubnetSubnetIdIndexTag>();
 
     // Iterate over the subnets to be merged. They will replace the existing

src/lib/dhcpsrv/cfg_subnets4.h

@@ -85,7 +85,7 @@ public:
     /// to the same SrvConfig instance we are merging into.
     /// @param other the subnet configuration to be merged into this
     /// configuration.
-    void merge(CfgSharedNetworks4Ptr networks, const CfgSubnets4& other);
+    void merge(CfgSharedNetworks4Ptr networks, CfgSubnets4& other);
 
     /// @brief Returns pointer to the collection of all IPv4 subnets.
     ///

src/lib/dhcpsrv/srv_config.cc

@@ -159,10 +159,10 @@ SrvConfig::equals(const SrvConfig& other) const {
 }
 
 void
-SrvConfig::merge(const ConfigBase& other) {
+SrvConfig::merge(ConfigBase& other) {
     ConfigBase::merge(other);
     try {
-        const SrvConfig& other_srv_config = dynamic_cast<const SrvConfig&>(other);
+        SrvConfig& other_srv_config = dynamic_cast<SrvConfig&>(other);
         if (CfgMgr::instance().getFamily() == AF_INET) {
             merge4(other_srv_config);
         } else {
@@ -176,13 +176,14 @@ SrvConfig::merge(const ConfigBase& other) {
 }
 
 void
-SrvConfig::merge4(const SrvConfig& other) {
+SrvConfig::merge4(SrvConfig& other) {
     /// We merge objects in order of dependency (real or theoretical).
 
     /// Merge globals.
     mergeGlobals4(other);
 
-    /// @todo merge option defs
+    /// Merge option defs
+    cfg_option_def_->merge((*other.getCfgOptionDef()));
 
     /// @todo merge options
 
@@ -196,7 +197,7 @@ SrvConfig::merge4(const SrvConfig& other) {
 }
 
 void
-SrvConfig::mergeGlobals4(const SrvConfig& other) {
+SrvConfig::mergeGlobals4(SrvConfig& other) {
     // Iterate over the "other" globals, adding/overwriting them into
     // this config's list of globals.
     for (auto other_global : other.getConfiguredGlobals()->mapValue()) {

src/lib/dhcpsrv/srv_config.h

@@ -516,7 +516,7 @@ public:
     ///
     /// @param other An object holding the configuration to be merged
     /// into this configuration.
-    virtual void merge(const ConfigBase& other);
+    virtual void merge(ConfigBase& other);
 
     /// @brief Updates statistics.
     ///
@@ -662,7 +662,7 @@ private:
     ///
     /// @param other An object holding the configuration to be merged
     /// into this configuration.
-    void merge4(const SrvConfig& other);
+    void merge4(SrvConfig& other);
 
 
     /// @brief Merges the DHCPv4 globals specified in the given configuration
@@ -687,7 +687,7 @@ private:
     ///
     /// @param other An object holding the configuration to be merged
     /// into this configuration.
-    void mergeGlobals4(const SrvConfig& other);
+    void mergeGlobals4(SrvConfig& other);
 
     /// @brief Sequence number identifying the configuration.
     uint32_t sequence_;

src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -250,7 +250,7 @@ TEST(CfgOptionDefTest, unparse) {
     CfgOptionDef cfg;
 
     // Add some options.
-    cfg.add(OptionDefinitionPtr(new 
+    cfg.add(OptionDefinitionPtr(new
         OptionDefinition("option-foo", 5, "uint16")), "isc");
     cfg.add(OptionDefinitionPtr(new
         OptionDefinition("option-bar", 5, "uint16", true)), "dns");
@@ -262,7 +262,7 @@ TEST(CfgOptionDefTest, unparse) {
     rec->addRecordField("uint16");
     rec->addRecordField("uint16");
     cfg.add(rec, "dns");
-    
+
     // Unparse
     std::string expected = "[\n"
         "{\n"
@@ -303,4 +303,50 @@ TEST(CfgOptionDefTest, unparse) {
     isc::test::runToElementTest<CfgOptionDef>(expected, cfg);
 }
 
+// This test verifies that configured option definitions can be merged
+// correctly.
+TEST(CfgOptionDefTest, merge) {
+    CfgOptionDef to;         // Configuration we are merging to.
+
+    // Add some options to the "to" config.
+    to.add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint16"))), "isc");
+    to.add((OptionDefinitionPtr(new OptionDefinition("two", 2, "uint16"))), "isc");
+    to.add((OptionDefinitionPtr(new OptionDefinition("three", 3, "uint16"))), "fluff");
+    to.add((OptionDefinitionPtr(new OptionDefinition("four", 4, "uint16"))), "fluff");
+
+    // Clone the "to" config and use that for merging.
+    CfgOptionDef to_clone;
+    to.copyTo(to_clone);
+
+    // Ensure they are equal before we do anything.
+    ASSERT_TRUE(to.equals(to_clone));
+
+    // Merge from an empty config.
+    CfgOptionDef empty_from;
+    ASSERT_NO_THROW(to_clone.merge(empty_from));
+
+    // Should have the same content as before.
+    ASSERT_TRUE(to.equals(to_clone));
+
+    // Construct a non-empty "from" config.
+    // Options "two" and "three" will be updated definitions and "five" will be new.
+    CfgOptionDef from;
+    from.add((OptionDefinitionPtr(new OptionDefinition("two", 22, "uint16"))), "isc");
+    from.add((OptionDefinitionPtr(new OptionDefinition("three", 3, "string"))), "fluff");
+    from.add((OptionDefinitionPtr(new OptionDefinition("five", 5, "uint16"))), "fluff");
+
+    // Now let's clone "from" config and use that manually construct the expected config.
+    CfgOptionDef expected;
+    from.copyTo(expected);
+    expected.add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint16"))), "isc");
+    expected.add((OptionDefinitionPtr(new OptionDefinition("four", 4, "uint16"))), "fluff");
+
+    // Do the merge.
+    ASSERT_NO_THROW(to_clone.merge(from));
+
+    // Verify we have the expected content.
+    ASSERT_TRUE(expected.equals(to_clone));
+}
+
+
 }

src/lib/process/config_base.cc

@@ -83,7 +83,7 @@ ConfigBase::copy(ConfigBase& other) const {
 }
 
 void
-ConfigBase::merge(const ConfigBase& other) {
+ConfigBase::merge(ConfigBase& other) {
     // Merge logging info.
     if (!other.logging_info_.empty()) {
         logging_info_ = other.logging_info_;

src/lib/process/config_base.h

@@ -77,7 +77,7 @@ public:
     ///
     /// @param other the other configuration to be merged into this
     /// configuration.
-    virtual void merge(const ConfigBase& other);
+    virtual void merge(ConfigBase& other);
 
     /// @brief Converts to Element representation
     ///