[#399,!215] kea-dhcp4 now merges in config backend shared netwokrs

src/lib/dhcpsrv/cfg_shared_networks.*
    CfgSharedNetworks4::merge() - new method to merge
    shared network configs

src/lib/dhcpsrv/cfg_subnets4.*
    CfgSubnets4::merge() - reworked to reflect new rules

src/lib/dhcpsrv/srv_config.cc
    SrvConfig::merge() - now merges shared networks

src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc
    checkMergedSubnet() - convenience function for
    verifying subnet content

    TEST(CfgSubnets4Test, mergeSubnets) - revamped to
    reflect new network assigment rules

src/bin/dhcp4/tests/config_backend_unittest.cc
    TEST_F(Dhcp4CBTest, mergeSharedNetworks)  - enabled test

src/bin/dhcp4/tests/config_backend_unittest.cc

@@ -391,7 +391,7 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) {
 // This test verifies that externally configured shared-networks are
 // merged correctly into staging configuration.
 // @todo enable test when SrvConfig can merge shared networks.
-TEST_F(Dhcp4CBTest, DISABLED_mergeSharedNetworks) {
+TEST_F(Dhcp4CBTest, mergeSharedNetworks) {
     string base_config =
         "{ \n"
         "    \"interfaces-config\": { \n"
@@ -442,7 +442,7 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeSharedNetworks) {
     ASSERT_TRUE(staged_network);
 
     // Subnet3, which is in db2 should not have been merged, since it is
-    // first found, first used?
+    // backend data is first found, first used.
     staged_network = networks->getByName("three");
     ASSERT_FALSE(staged_network);
 }

src/lib/dhcpsrv/cfg_shared_networks.cc

-// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2017-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
@@ -19,6 +19,51 @@ CfgSharedNetworks4::hasNetworkWithServerId(const IOAddress& server_id) const {
     return (network_it != index.cend());
 }
 
+void
+CfgSharedNetworks4::merge(const CfgSharedNetworks4& other) {
+    auto& index = networks_.get<SharedNetworkNameIndexTag>();
+
+    // Iterate over the subnets to be merged. They will replace the existing
+    // subnets with the same id. All new subnets will be inserted into the
+    // configuration into which we're merging.
+    auto other_networks = other.getAll();
+    for (auto other_network = other_networks->begin();
+         other_network != other_networks->end(); ++other_network) {
+
+        // In theory we should drop subnet assignments from "other". The
+        // idea being  those that come from the CB should not have subnets_
+        // populated.  We will quietly throw them away, just in case.
+        (*other_network)->delAll();
+
+        // Check if the other network exists in this config.
+        auto existing_network = index.find((*other_network)->getName());
+        if (existing_network != index.end()) {
+
+            // Somehow the same instance is in both, skip it.
+            if (*existing_network == *other_network) {
+                continue;
+            }
+
+            // Network exists, which means we're updating it.
+            // First we need to move its subnets to the new
+            // version of the network.
+            const Subnet4Collection* subnets = (*existing_network)->getAllSubnets();
+
+            Subnet4Collection copy_subnets(*subnets);
+            for (auto subnet = copy_subnets.cbegin(); subnet != copy_subnets.cend(); ++subnet) {
+                (*existing_network)->del((*subnet)->getID());
+                (*other_network)->add(*subnet);
+            }
+
+            // Now we discard the existing copy of the network.
+            index.erase(existing_network);
+        }
+
+        // Add the new/updated nework.
+        networks_.push_back(*other_network);
+    }
+
+}
 
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcpsrv/cfg_shared_networks.h

-// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2017-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
@@ -124,7 +124,32 @@ public:
     /// @return true if there is a network with a specified server identifier.
     bool hasNetworkWithServerId(const asiolink::IOAddress& server_id) const;
 
-
+    /// @brief Merges specified shared network configuration into this configuration.
+    ///
+    /// This method merges networks from the @c other configuration into this
+    /// configuration. The general rule is that existing networks are replaced
+    /// by the networks from @c other.
+    ///
+    /// For each network in @c other, do the following:
+    ///
+    /// - Any associated subnets are removed.  Shared networks retreived from
+    /// config backends, do not carry their associated subnets (if any) with them.
+    /// Subnet assignments are maintained by subnet merges.
+    /// - If a shared network of the same name,  already exists in this
+    /// configuration:
+    ///     - All of its associated subnets are movde to the "other" network
+    ///     - The existing network is removed from this configuration
+    /// - The "other" network is added
+    ///
+    /// @warning The merge operation may 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 other the shared network configuration to be merged into this
+    /// configuration.
+    void merge(const CfgSharedNetworks4& other);
 };
 
 /// @brief Pointer to the configuration of IPv4 shared networks.

src/lib/dhcpsrv/cfg_subnets4.cc

@@ -56,15 +56,15 @@ CfgSubnets4::del(const ConstSubnet4Ptr& subnet) {
 }
 
 void
-CfgSubnets4::merge(const CfgSubnets4& other) {
+CfgSubnets4::merge(CfgSharedNetworks4Ptr networks,
+                   const CfgSubnets4& other) {
     auto& index = subnets_.get<SubnetSubnetIdIndexTag>();
 
     // Iterate over the subnets to be merged. They will replace the existing
     // subnets with the same id. All new subnets will be inserted into the
     // configuration into which we're merging.
     auto other_subnets = other.getAll();
-    for (auto other_subnet = other_subnets->begin();
-         other_subnet != other_subnets->end();
+    for (auto other_subnet = other_subnets->begin(); other_subnet != other_subnets->end();
          ++other_subnet) {
 
         // Check if there is a subnet with the same ID.
@@ -72,55 +72,46 @@ CfgSubnets4::merge(const CfgSubnets4& other) {
         if (subnet_it != index.end()) {
 
             // Subnet found.
-            auto subnet = *subnet_it;
+            auto existing_subnet = *subnet_it;
 
-            // Continue if the merged and existing subnets are the same instance.
-            if (subnet == *other_subnet) {
+            // Continue if the existing subnet and other subnet
+            // are the same instance skip it.
+            if (existing_subnet == *other_subnet) {
                 continue;
             }
 
-            // If the merged subnet belongs to a shared network we need to
-            // discard this shared network so as it is merged into the existing
-            // shared network or left not unassigned if the existing subnet
-            // is unassigned.
-            SharedNetwork4Ptr other_network;
-            (*other_subnet)->getSharedNetwork(other_network);
-            if (other_network) {
-                other_network->del((*other_subnet)->getID());
-            }
-
-            // Check if the subnet belongs to a shared network.
+            // We're going to replace the existing subnet with the other
+            // version. If it belongs to a shared network, we need
+            // remove it from that network.
             SharedNetwork4Ptr network;
-            subnet->getSharedNetwork(network);
+            existing_subnet->getSharedNetwork(network);
             if (network) {
-                // The subnet belongs to a shared network. The shared network
-                // instance holds a pointer to the subnet so we need to remove
-                // the existing subnet from the shared network it belongs to.
-                network->del(subnet->getID());
-
-                // The new subnet instance must be added to the existing shared
-                // network.
-                network->add(*other_subnet);
+                network->del(existing_subnet->getID());
             }
 
-            // The existing subnet may now be removed.
+            // Now we remove the existing subnet.
             index.erase(subnet_it);
         }
-    }
 
-    // Make another pass over the merged subnets to add them. Any existing
-    // instances with the same IDs have been removed.
-    for (auto other_subnet = other_subnets->begin();
-         other_subnet != other_subnets->end();
-         ++other_subnet) {
+        // Add the "other" subnet to the our collection of subnets.
+        subnets_.push_back(*other_subnet);
 
-        // Continue if the merged and existing subnets are the same instance.
-        auto subnet_it = index.find((*other_subnet)->getID());
-        if ((subnet_it != index.end()) && ((*subnet_it) == (*other_subnet))) {
-            continue;
+        // If it belongs to a shared network, find the network and
+        // add the subnet to it
+        std::string network_name = (*other_subnet)->getSharedNetworkName();
+        if (!network_name.empty()) {
+            SharedNetwork4Ptr network = networks->getByName(network_name);
+            if (network) {
+                network->add(*other_subnet);
+            } else {
+                // This implies the shared-network collection we were given
+                // is out of sync with the subnets we were given.
+                isc_throw(InvalidOperation, "Cannot assign subnet ID of '"
+                          << (*other_subnet)->getID()
+                          << " to shared network: " << network_name
+                          << ", network does not exist");
+            }
         }
-
-        subnets_.push_back(*other_subnet);
     }
 }
 

src/lib/dhcpsrv/cfg_subnets4.h

@@ -10,6 +10,7 @@
 #include <asiolink/io_address.h>
 #include <cc/cfg_to_element.h>
 #include <dhcp/pkt4.h>
+#include <dhcpsrv/cfg_shared_networks.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/subnet_id.h>
 #include <dhcpsrv/subnet_selector.h>
@@ -55,30 +56,23 @@ public:
     /// this configuration the subnet from @c other configuration is inserted.
     ///
     /// The complexity of the merge process stems from the associations between
-    /// the subnets and shared networks. Although, the subnets in this
-    /// configuration are replaced by the subnets from @c other, the existing
-    /// shared networks should not be affected. The new subnets must be
-    /// inserted into the exsiting shared networks. The @c CfgSharedNetworks4
-    /// is responsible for merging the shared networks and this merge must
-    /// be triggered before the merge of the subnets. Therefore, this method
-    /// assumes that existing shared networks have been already merged.
-    ///
-    /// These are the rules concerning the shared network associations that
-    /// this method follows:
-    /// - If there is a subnet in this configuration and it is associated with
-    ///   a shared network, the shared network is preserved and the new subnet
-    ///   instance (replacing existing one) is associated with it. The old
-    ///   subnet instance is removed from the shared network.
-    /// - If there is a subnet in this configuration and it is not associated
-    ///   with any shared network, the new subnet instance replaces the existing
-    ///   subnet instance and its association with a shared network is discarded.
-    ///   As a result, the configuration will contain new subnet instance but
-    ///   not associated with any shared network.
-    /// - If there is no subnet with the given ID, the new subnet instance is
-    ///   inserted into the configuration and the association with a shared
-    ///   network (if present) will be preserved. As a result, the configuration
-    ///   will hold the instance of the new subnet with the shared network
-    ///   it originally belonged to.
+    /// the subnets and shared networks.  It is assumed that subnets in @c other
+    /// are the authority on their shared network assignments. It is also
+    /// assumed that @ networks is the list of shared networks that should be
+    /// used in making assignments.  The general concept is that the overarching
+    /// merge process will first merge shared networks and then pass that list of
+    /// networks into this method. Subnets from @c other are then merged into this
+    /// configuration as follows:
+    ///
+    /// For each subnet in @c other:
+    ///
+    /// - If a subnet of the same ID already exists in this configuration:
+    ///    -# If it belongs to a shared network, remove it from that network
+    ///    -# Remove the subnet from this configuration and discard it
+    ///
+    /// - Add the subnet from other to this configuration.
+    /// - If that subnet is associated to shared network, find that network
+    ///   in @ networks and add that subnet to it.
     ///
     /// @warning The merge operation affects the @c other configuration.
     /// Therefore, the caller must not rely on the data held in the @c other
@@ -86,9 +80,12 @@ public:
     /// not be modified after the call to @c merge because it may affect the
     /// merged configuration.
     ///
+    /// @param networks collection of shared networks that to which assignments
+    /// should be added.  In other words, the list of shared networks that belong
+    /// to the same SrvConfig instance we are merging into.
     /// @param other the subnet configuration to be merged into this
     /// configuration.
-    void merge(const CfgSubnets4& other);
+    void merge(CfgSharedNetworks4Ptr networks, const CfgSubnets4& other);
 
     /// @brief Returns pointer to the collection of all IPv4 subnets.
     ///

src/lib/dhcpsrv/shared_network.cc

@@ -254,18 +254,21 @@ SharedNetwork4::add(const Subnet4Ptr& subnet) {
     Impl::add(subnets_, subnet);
     // Associate the subnet with this network.
     setSharedNetwork(subnet);
+    subnet->setSharedNetworkName(name_);
 }
 
 void
 SharedNetwork4::del(const SubnetID& subnet_id) {
     Subnet4Ptr subnet = Impl::del<Subnet4Ptr>(subnets_, subnet_id);
     clearSharedNetwork(subnet);
+    subnet->setSharedNetworkName("");
 }
 
 void
 SharedNetwork4::delAll() {
     for (auto subnet = subnets_.cbegin(); subnet != subnets_.cend(); ++subnet) {
         clearSharedNetwork(*subnet);
+        (*subnet)->setSharedNetworkName("");
     }
     subnets_.clear();
 }

src/lib/dhcpsrv/srv_config.cc

@@ -39,7 +39,7 @@ SrvConfig::SrvConfig()
       decline_timer_(0), echo_v4_client_id_(true), dhcp4o6_port_(0),
       d2_client_config_(new D2ClientConfig()),
       configured_globals_(Element::createMap()),
-      cfg_consist_(new CfgConsistency()), 
+      cfg_consist_(new CfgConsistency()),
       server_tag_("") {
 }
 
@@ -162,10 +162,18 @@ SrvConfig::merge(const ConfigBase& other) {
     ConfigBase::merge(other);
 
     try {
-        /// @todo merge other parts of the configuration here.
-
         const SrvConfig& other_srv_config = dynamic_cast<const SrvConfig&>(other);
-        cfg_subnets4_->merge(*other_srv_config.getCfgSubnets4());
+
+        /// We merge objects in order of dependency (real or theoretical).
+        /// @todo merge globals
+        /// @todo merge option defs
+        /// @todo merge options
+
+        // Merge shared networks.
+        cfg_shared_networks4_->merge(*(other_srv_config.getCfgSharedNetworks4()));
+
+        /// Merge subnets.
+        cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other_srv_config.getCfgSubnets4()));
 
         /// @todo merge other parts of the configuration here.
 

src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc

@@ -17,6 +17,20 @@ using namespace asiolink;
 
 namespace {
 
+void checkMergedNetwork(const CfgSharedNetworks4& networks, const std::string& name,
+                       const Triplet<uint32_t>& exp_valid,
+                       const std::vector<SubnetID>& exp_subnets) {
+    auto network = networks.getByName(name);
+    ASSERT_TRUE(network) << "expected network: " << name << " not found";
+    ASSERT_EQ(exp_valid, network->getValid()) << " network valid lifetime wrong";
+    const Subnet4Collection* subnets = network->getAllSubnets();
+    ASSERT_EQ(exp_subnets.size(), subnets->size()) << " wrong number of subnets";
+    for (auto exp_id : exp_subnets) {
+        ASSERT_TRUE(network->getSubnet(exp_id))
+                    << " did not find expected subnet: " << exp_id;
+    }
+}
+
 // This test verifies that shared networks can be added to the configruation
 // and retrieved by name.
 TEST(CfgSharedNetworks4Test, getByName) {
@@ -158,4 +172,88 @@ TEST(CfgSharedNetworks4Test, unparse) {
     test::runToElementTest<CfgSharedNetworks4>(expected, cfg);
 }
 
+// This test verifies that shared-network configurations are properly merged.
+TEST(CfgSharedNetworks4Test, mergeNetworks) {
+    Subnet4Ptr subnet1(new Subnet4(IOAddress("192.0.1.0"),
+                                   26, 1, 2, 100, SubnetID(1)));
+    Subnet4Ptr subnet2(new Subnet4(IOAddress("192.0.2.0"),
+                                   26, 1, 2, 100, SubnetID(2)));
+    Subnet4Ptr subnet3(new Subnet4(IOAddress("192.0.3.0"),
+                                   26, 1, 2, 100, SubnetID(3)));
+    Subnet4Ptr subnet4(new Subnet4(IOAddress("192.0.4.0"),
+                                   26, 1, 2, 100, SubnetID(4)));
+
+    // Create network1 and add two subnets to it
+    SharedNetwork4Ptr network1(new SharedNetwork4("network1"));
+    network1->setValid(Triplet<uint32_t>(100));
+    ASSERT_NO_THROW(network1->add(subnet1));
+    ASSERT_NO_THROW(network1->add(subnet2));
+
+    // Create network2 with no subnets.
+    SharedNetwork4Ptr network2(new SharedNetwork4("network2"));
+    network2->setValid(Triplet<uint32_t>(200));
+
+    // Create network3 with one subnets.
+    SharedNetwork4Ptr network3(new SharedNetwork4("network3"));
+    network3->setValid(Triplet<uint32_t>(300));
+    ASSERT_NO_THROW(network3->add(subnet3));
+
+    // Create our "existing" configured networks.
+    // Add all three networks to the existing config.
+    CfgSharedNetworks4 cfg_to;
+    ASSERT_NO_THROW(cfg_to.add(network1));
+    ASSERT_NO_THROW(cfg_to.add(network2));
+    ASSERT_NO_THROW(cfg_to.add(network3));
+
+    // Merge in an "empty" config. Should have the original config, still intact.
+    CfgSharedNetworks4 cfg_from;
+    ASSERT_NO_THROW(cfg_to.merge(cfg_from));
+
+    ASSERT_EQ(3, cfg_to.getAll()->size());
+    ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network1", Triplet<uint32_t>(100),
+                                               std::vector<SubnetID>{SubnetID(1), SubnetID(2)}));
+
+    ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet<uint32_t>(200),
+                                               std::vector<SubnetID>()));
+
+    ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network3", Triplet<uint32_t>(300),
+                                               std::vector<SubnetID>{SubnetID(3)}));
+
+    // Create network1b, this is an "update" of network1
+    // We'll double the valid time and add subnet4 to it
+    SharedNetwork4Ptr network1b(new SharedNetwork4("network1"));
+    network1b->setValid(Triplet<uint32_t>(200));
+    ASSERT_NO_THROW(network1b->add(subnet4));
+
+    // Network2 we will not touch.
+
+    // Create network3b, this is an "update" of network3.
+    // We'll double it's valid time, but leave off the subnet.
+    SharedNetwork4Ptr network3b(new SharedNetwork4("network3"));
+    network3b->setValid(Triplet<uint32_t>(600));
+
+    // Create our "existing" configured networks.
+    ASSERT_NO_THROW(cfg_from.add(network1b));
+    ASSERT_NO_THROW(cfg_from.add(network3b));
+
+    ASSERT_NO_THROW(cfg_to.merge(cfg_from));
+
+    // Should still have 3 networks.
+
+    // Network1 should have doubled its valid life time but still only have
+    // the orignal two subnets.  Merge should discard assocations on CB
+    // subnets and preserve the associations from existing config.
+    ASSERT_EQ(3, cfg_to.getAll()->size());
+    ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network1", Triplet<uint32_t>(200),
+                                               std::vector<SubnetID>{SubnetID(1), SubnetID(2)}));
+
+    // No changes to network2.
+    ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet<uint32_t>(200),
+                                               std::vector<SubnetID>()));
+
+    // Network1 should have doubled its valid life time and still subnet3.
+    ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network3", Triplet<uint32_t>(600),
+                                               std::vector<SubnetID>{SubnetID(3)}));
+}
+
 } // end of anonymous namespace