[#1330] Revamped interface config parser

src/bin/dhcp4/json_config_parser.cc

@@ -416,13 +416,7 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set,
         ConstElementPtr ifaces_config = mutable_cfg->get("interfaces-config");
         if (ifaces_config) {
             parameter_name = "interfaces-config";
-            ElementPtr mutable_cfg =
-                boost::const_pointer_cast<Element>(ifaces_config);
-            if (check_only) {
-                // No re-detection in check only mode
-                mutable_cfg->set("re-detect", Element::create(false));
-            }
-            IfacesConfigParser parser(AF_INET);
+            IfacesConfigParser parser(AF_INET, check_only);
             CfgIfacePtr cfg_iface = srv_cfg->getCfgIface();
             parser.parse(cfg_iface, ifaces_config);
         }

src/bin/dhcp4/tests/classify_unittest.cc

-// Copyright (C) 2016-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016-2020 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
@@ -313,7 +313,6 @@ public:
     ClassifyTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
     }
 
     /// @brief Destructor.

src/bin/dhcp4/tests/decline_unittest.cc

-// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2020 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
@@ -184,7 +184,6 @@ public:
     DeclineTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
     }
 
     /// @brief Interface Manager's fake configuration control.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -2252,8 +2252,6 @@ TEST_F(Dhcpv4SrvTest, sanityCheck) {
 // Checks if received relay agent info option is echoed back to the client
 TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
-
     NakedDhcpv4Srv srv(0);
 
     // Use of the captured DHCPDISCOVER packet requires that
@@ -2299,8 +2297,6 @@ TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho) {
 // to the client
 TEST_F(Dhcpv4SrvTest, badRelayAgentInfoEcho) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
-
     NakedDhcpv4Srv srv(0);
 
     // Use of the captured DHCPDISCOVER packet requires that
@@ -2345,8 +2341,6 @@ TEST_F(Dhcpv4SrvTest, badRelayAgentInfoEcho) {
 // Checks if client port can be overridden in packets being sent.
 TEST_F(Dhcpv4SrvTest, portsClientPort) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
-
     NakedDhcpv4Srv srv(0);
 
     // By default te client port is supposed to be zero.
@@ -2388,7 +2382,6 @@ TEST_F(Dhcpv4SrvTest, portsClientPort) {
 // Checks if server port can be overridden in packets being sent.
 TEST_F(Dhcpv4SrvTest, portsServerPort) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
 
     // Do not use DHCP4_SERVER_PORT here as 0 means don't open sockets.
     NakedDhcpv4Srv srv(0);
@@ -3145,7 +3138,7 @@ TEST_F(Dhcpv4SrvTest, clientClassify) {
         "],"
         "\"valid-lifetime\": 4000 }";
 
-    ASSERT_NO_THROW(configure(config));
+    ASSERT_NO_THROW(configure(config, true, false));
 
     // Create a simple packet that we'll use for classification
     Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
@@ -3468,7 +3461,6 @@ TEST_F(Dhcpv4SrvTest, privateOption) {
 // Checks effect of persistency (aka always-true) flag on the PRL
 TEST_F(Dhcpv4SrvTest, prlPersistency) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
 
     ASSERT_NO_THROW(configure(CONFIGS[2]));
 
@@ -3547,7 +3539,7 @@ TEST_F(Dhcpv4SrvTest, relayOverride) {
         "\"valid-lifetime\": 4000 }";
 
     // Use this config to set up the server
-    ASSERT_NO_THROW(configure(config));
+    ASSERT_NO_THROW(configure(config, true, false));
 
     // Let's get the subnet configuration objects
     const Subnet4Collection* subnets =
@@ -3634,7 +3626,7 @@ TEST_F(Dhcpv4SrvTest, relayOverrideAndClientClass) {
         "\"valid-lifetime\": 4000 }";
 
     // Use this config to set up the server
-    ASSERT_NO_THROW(configure(config));
+    ASSERT_NO_THROW(configure(config, true, false));
 
     const Subnet4Collection* subnets =
         CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll();
@@ -3695,7 +3687,7 @@ TEST_F(Dhcpv4SrvTest, relayLinkSelect) {
         "\"valid-lifetime\": 4000 }";
 
     // Use this config to set up the server
-    ASSERT_NO_THROW(configure(config));
+    ASSERT_NO_THROW(configure(config, true, false));
 
     // Let's get the subnet configuration objects
     const Subnet4Collection* subnets =
@@ -3814,7 +3806,7 @@ TEST_F(Dhcpv4SrvTest, subnetSelect) {
         "\"valid-lifetime\": 4000 }";
 
     // Use this config to set up the server
-    ASSERT_NO_THROW(configure(config));
+    ASSERT_NO_THROW(configure(config, true, false));
 
     // Let's get the subnet configuration objects
     const Subnet4Collection* subnets =
@@ -4099,7 +4091,6 @@ TEST_F(Dhcpv4SrvTest, statisticsUnknownRcvd) {
 // in incoming client message.
 TEST_F(Dhcpv4SrvTest, emptyClientId) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
     Dhcp4Client client;
 
     EXPECT_NO_THROW(configure(CONFIGS[0], *client.getServer()));
@@ -4123,7 +4114,6 @@ TEST_F(Dhcpv4SrvTest, emptyClientId) {
 // in incoming client message.
 TEST_F(Dhcpv4SrvTest, tooLongClientId) {
     IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
     Dhcp4Client client;
 
     EXPECT_NO_THROW(configure(CONFIGS[0], *client.getServer()));

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -609,13 +609,17 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
 }
 
 void
-Dhcpv4SrvTest::configure(const std::string& config, const bool commit) {
-    configure(config, srv_, commit);
+Dhcpv4SrvTest::configure(const std::string& config,
+                         const bool commit,
+                         const bool open_sockets) {
+    configure(config, srv_, commit, open_sockets);
 }
 
 void
-Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv,
-                         const bool commit) {
+Dhcpv4SrvTest::configure(const std::string& config,
+                         NakedDhcpv4Srv& srv,
+                         const bool commit,
+                         const bool open_sockets) {
     ConstElementPtr json;
     try {
         json = parseJSON(config);
@@ -648,7 +652,12 @@ Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv,
     if (commit) {
         CfgMgr::instance().commit();
     }
+
+    // Opening sockets.
+    if (open_sockets) {
+        IfaceMgr::instance().openSockets4();
     }
+}
 
 std::pair<int, std::string>
 Dhcpv4SrvTest::configureWithStatus(const std::string& config, NakedDhcpv4Srv& srv,
@@ -752,6 +761,6 @@ Dhcpv4SrvTest::pretendReceivingPkt(NakedDhcpv4Srv& srv, const std::string& confi
     EXPECT_EQ(1, tested_stat->getInteger().first);
 }
 
-}; // end of isc::dhcp::test namespace
-}; // end of isc::dhcp namespace
-}; // end of isc namespace
+} // end of isc::dhcp::test namespace
+} // end of isc::dhcp namespace
+} // end of isc namespace

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -440,7 +440,11 @@ public:
     /// @param config String holding server configuration in JSON format.
     /// @param commit A boolean flag indicating if the new configuration
     /// should be committed (if true), or not (if false).
-    void configure(const std::string& config, const bool commit = true);
+    /// @param open_sockets  A boolean flag indicating if sockets should
+    /// be opened (if true), or not (if false).
+    void configure(const std::string& config,
+                   const bool commit = true,
+                   const bool open_sockets = true);
 
     /// @brief Configure specified DHCP server using JSON string.
     ///
@@ -448,8 +452,12 @@ public:
     /// @param srv Instance of the server to be configured.
     /// @param commit A boolean flag indicating if the new configuration
     /// should be committed (if true), or not (if false).
-    void configure(const std::string& config, NakedDhcpv4Srv& srv,
-                   const bool commit = true);
+    /// @param open_sockets  A boolean flag indicating if sockets should
+    /// be opened (if true), or not (if false).
+    void configure(const std::string& config,
+                   NakedDhcpv4Srv& srv,
+                   const bool commit = true,
+                   const bool open_sockets = true);
 
     /// @brief Configure specified DHCP server using JSON string.
     ///

src/bin/dhcp4/tests/dora_unittest.cc

@@ -581,8 +581,6 @@ public:
     DORATest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
-
         // Let's wipe all existing statistics.
         isc::stats::StatsMgr::instance().removeAll();
     }

src/bin/dhcp4/tests/host_options_unittest.cc

-// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016-2020 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
@@ -243,7 +243,6 @@ public:
     HostOptionsTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
     }
 
     /// @brief Verifies that host specific options override subnet specific

src/bin/dhcp4/tests/host_unittest.cc

@@ -318,8 +318,6 @@ public:
     HostTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
-
         // Let's wipe all existing statistics.
         isc::stats::StatsMgr::instance().removeAll();
     }

src/bin/dhcp4/tests/inform_unittest.cc

@@ -135,8 +135,6 @@ public:
     InformTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
-
         // Let's wipe all existing statistics.
         isc::stats::StatsMgr::instance().removeAll();
     }

src/bin/dhcp4/tests/out_of_range_unittest.cc

-// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2020 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
@@ -200,7 +200,6 @@ public:
         : Dhcpv4SrvTest(),
           d2_mgr_(CfgMgr::instance().getD2ClientMgr()),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
     }
 
     /// @brief Destructor.

src/bin/dhcp4/tests/release_unittest.cc

-// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2020 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
@@ -71,7 +71,6 @@ public:
     ReleaseTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
     }
 
     /// @brief Performs 4-way exchange to obtain new lease.

src/bin/dhcp4/tests/shared_network_unittest.cc

@@ -1025,7 +1025,6 @@ public:
     Dhcpv4SharedNetworkTest()
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
         StatsMgr::instance().removeAll();
     }
 

src/bin/dhcp6/json_config_parser.cc

@@ -540,13 +540,7 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set,
         ConstElementPtr ifaces_config = mutable_cfg->get("interfaces-config");
         if (ifaces_config) {
             parameter_name = "interfaces-config";
-            ElementPtr mutable_cfg =
-                boost::const_pointer_cast<Element>(ifaces_config);
-            if (check_only) {
-                // No re-detection in check only mode
-                mutable_cfg->set("re-detect", Element::create(false));
-            }
-            IfacesConfigParser parser(AF_INET6);
+            IfacesConfigParser parser(AF_INET6, check_only);
             CfgIfacePtr cfg_iface = srv_config->getCfgIface();
             parser.parse(cfg_iface, ifaces_config);
         }

src/lib/dhcpsrv/parsers/ifaces_config_parser.cc

-// Copyright (C) 2015-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2020 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
@@ -33,21 +33,23 @@ IfacesConfigParser::parseInterfacesList(const CfgIfacePtr& cfg_iface,
     }
 }
 
-IfacesConfigParser::IfacesConfigParser(const uint16_t protocol)
-    : protocol_(protocol) {
+IfacesConfigParser::IfacesConfigParser(const uint16_t protocol, bool test_mode)
+    : protocol_(protocol), test_mode_(test_mode) {
 }
 
 void
 IfacesConfigParser::parse(const CfgIfacePtr& cfg,
                           const isc::data::ConstElementPtr& ifaces_config) {
 
+    // Close sockets if not in test mode.
+    if (!test_mode_) {
+        IfaceMgr::instance().closeSockets();
+    }
+
     // Check for re-detect before calling parseInterfacesList()
     bool re_detect = getBoolean(ifaces_config, "re-detect");
     cfg->setReDetect(re_detect);
-    if (re_detect) {
-        // Interface clear will drop opened socket information
-        // so close them if the caller did not.
-        IfaceMgr::instance().closeSockets();
+    if (re_detect && !test_mode_) {
         IfaceMgr::instance().clearIfaces();
         IfaceMgr::instance().detectIfaces();
     }

src/lib/dhcpsrv/parsers/ifaces_config_parser.h

-// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2020 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
@@ -29,7 +29,8 @@ public:
     /// @brief Constructor
     ///
     /// @param protocol AF_INET for DHCPv4 and AF_INET6 for DHCPv6.
-    explicit IfacesConfigParser(const uint16_t protocol);
+    /// @param test_mode True if in test mode, False if not.
+    IfacesConfigParser(const uint16_t protocol, bool test_mode);
 
     /// @brief Parses content of the "interfaces-config".
     ///
@@ -55,6 +56,9 @@ private:
 
     /// @brief AF_INET for DHCPv4 and AF_INET6 for DHCPv6.
     int protocol_;
+
+    /// @brief Test mode.
+    bool test_mode_;
 };
 
 }

src/lib/dhcpsrv/tests/ifaces_config_parser_unittest.cc

-// Copyright (C) 2015,2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2020 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
@@ -59,7 +59,7 @@ TEST_F(IfacesConfigParserTest, interfaces) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser parser(AF_INET);
+    IfacesConfigParser parser(AF_INET, false);
     CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
     ASSERT_TRUE(cfg_iface);
     ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
@@ -98,6 +98,36 @@ TEST_F(IfacesConfigParserTest, interfaces) {
     EXPECT_TRUE(test_config.socketOpen("eth1", AF_INET));
 }
 
+// This test checks that the parser does not re-detect interfaces in test mode.
+TEST_F(IfacesConfigParserTest, testMode) {
+    // Creates fake interfaces with fake addresses.
+    IfaceMgrTestConfig test_config(true);
+
+    // Configuration with wildcard..
+    std::string config =
+         "{ \"interfaces\": [ \"*\" ], \"re-detect\": true }";
+
+    ElementPtr config_element = Element::fromJSON(config);
+
+    // Parse the configuration in test mode.
+    IfacesConfigParser parser(AF_INET, true);
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    ASSERT_TRUE(cfg_iface);
+    ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
+
+    // Verify we still have the eth1961 interface.
+    EXPECT_TRUE(IfaceMgr::instance().getIface("eth1961"));
+
+    // Reparse in not test mode.
+    IfacesConfigParser parser2(AF_INET, false);
+    CfgMgr::instance().clear();
+    cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    ASSERT_TRUE(cfg_iface);
+    ASSERT_NO_THROW(parser2.parse(cfg_iface, config_element));
+
+    // The eth1961 interface no longer exists.
+    EXPECT_FALSE(IfaceMgr::instance().getIface("eth1961"));
+}
 
 // This test checks that the parsed structure can be converted back to Element
 // tree.
@@ -116,7 +146,7 @@ TEST_F(IfacesConfigParserTest, toElement) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser parser(AF_INET);
+    IfacesConfigParser parser(AF_INET, false);
     CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
     ASSERT_TRUE(cfg_iface);
     ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
@@ -141,7 +171,7 @@ TEST_F(IfacesConfigParserTest, socketTypeRaw) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser parser(AF_INET);
+    IfacesConfigParser parser(AF_INET, false);
     CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
     ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
 
@@ -168,7 +198,7 @@ TEST_F(IfacesConfigParserTest, socketTypeDatagram) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser parser(AF_INET);
+    IfacesConfigParser parser(AF_INET, false);
     CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
     ASSERT_TRUE(cfg_iface);
     ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
@@ -188,7 +218,7 @@ TEST_F(IfacesConfigParserTest, socketTypeDatagram) {
 // Test that the configuration rejects the invalid socket type.
 TEST_F(IfacesConfigParserTest, socketTypeInvalid) {
     // For DHCPv4 we only accept the raw socket or datagram socket.
-    IfacesConfigParser parser4(AF_INET);
+    IfacesConfigParser parser4(AF_INET, false);
     CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
     std::string config = "{ \"interfaces\": [ ],"
         "\"dhcp-socket-type\": \"default\","
@@ -197,7 +227,7 @@ TEST_F(IfacesConfigParserTest, socketTypeInvalid) {
     ASSERT_THROW(parser4.parse(cfg_iface, config_element), DhcpConfigError);
 
     // For DHCPv6 we don't accept any socket type.
-    IfacesConfigParser parser6(AF_INET6);
+    IfacesConfigParser parser6(AF_INET6, false);
     config = "{ \"interfaces\": [ ],"
         " \"dhcp-socket-type\": \"udp\","
         " \"re-detect\": false }";
@@ -208,10 +238,10 @@ TEST_F(IfacesConfigParserTest, socketTypeInvalid) {
 // Tests that outbound-interface is parsed properly.
 TEST_F(IfacesConfigParserTest, outboundInterface) {
     // For DHCPv4 we accept 'use-routing' or 'same-as-inbound'.
-    IfacesConfigParser parser4(AF_INET);
+    IfacesConfigParser parser4(AF_INET, false);
 
     // For DHCPv6 we don't accept this at all.
-    IfacesConfigParser parser6(AF_INET6);
+    IfacesConfigParser parser6(AF_INET6, false);
 
     CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();