[#4507] Improved fatal interface-add error

diff --git a/changelog_unreleased/4507-fix-service-sockets-require-all-true-behavior b/changelog_unreleased/4507-fix-service-sockets-require-all-true-behavior
new file mode 100644 (file)
index 0000000..147b2b6
--- /dev/null
+++ b/changelog_unreleased/4507-fix-service-sockets-require-all-true-behavior
@@ -0,0 +1,4 @@
+[func]         fdupont
+       Made reconfig commands which triggered a fatal error not to
+       return a success answer when the server is shutting down.
+       (Gitlab #4507)

diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
index 070e42b17d3be80711cf03f2967acb7d775aad23..1e67ef0e19334f29e737771be0f85330c61a6fa6 100644 (file)
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
@@ -93,7 +93,7 @@ void signalHandler(int signo) {
 namespace isc {
 namespace dhcp {
 
-ControlledDhcpv4Srv* ControlledDhcpv4Srv::server_ = NULL;
+ControlledDhcpv4Srv* ControlledDhcpv4Srv::server_ = 0;
 
 void
 ControlledDhcpv4Srv::init(const std::string& file_name) {
@@ -198,6 +198,11 @@ ControlledDhcpv4Srv::loadConfigFile(const std::string& file_name) {
     return (result);
 }
 
+bool
+ControlledDhcpv4Srv::getShutdown() const {
+    return (Dhcpv4Srv::shutdown_);
+}
+
 ConstElementPtr
 ControlledDhcpv4Srv::commandShutdownHandler(const string&, ConstElementPtr args) {
     if (!ControlledDhcpv4Srv::getInstance()) {
@@ -413,6 +418,12 @@ ControlledDhcpv4Srv::commandConfigSetHandler(const string&,
     // the logging first in case there's a configuration failure.
     int rcode = 0;
     isc::config::parseAnswer(rcode, result);
+    if (getShutdown() && (rcode == CONTROL_RESULT_SUCCESS)) {
+        // Do not return success when a fatal error was triggered.
+        rcode = CONTROL_RESULT_FATAL_ERROR;
+        message = "Reconfiguration triggered a fatal error: shutting down.";
+        result = isc::config::createAnswer(rcode, message);
+    }
     if (rcode == CONTROL_RESULT_SUCCESS) {
         CfgMgr::instance().getStagingCfg()->applyLoggingCfg();
 
@@ -808,6 +819,9 @@ ControlledDhcpv4Srv::commandInterfaceAddHandler(const std::string&,
 
     ostringstream msg;
     if (!error) {
+        if (getShutdown()) {
+            return (isc::config::createAnswer(CONTROL_RESULT_FATAL_ERROR, "Interface configuration uodate triggered a fatal error: shutting down."));
+        }
         return (isc::config::createAnswer(CONTROL_RESULT_SUCCESS, "Interface configuration successfully updated."));
     } else {
         msg << "Updating used interfaces failed: " << message;

diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h
index 5727090fbca20738d049e08ebf41c35ba724caa0..d0d6831900d55e0ed3512e98e123aaec4f77881f 100644 (file)
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.h
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h
@@ -73,6 +73,9 @@ public:
     /// @param exit_value integer value to the process should exit with.
     virtual void shutdownServer(int exit_value);
 
+    /// @brief Return the server shutdown flag value.
+    bool getShutdown() const;
+
     /// @brief Configuration processor
     ///
     /// This is a method for handling incoming configuration updates.

diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
index a72df2cdc80be2d2c511c664edf5b64fac15b65d..ef0ada5fec7641b0e37f8769d6493e30d1394a62 100644 (file)
--- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
@@ -2602,6 +2602,41 @@ TEST_F(CtrlChannelDhcpv4SrvTest, interfaceAdd) {
     EXPECT_EQ(response, expected);
 }
 
+// Tests if interface-add can trigger a fatal failure.
+TEST_F(CtrlChannelDhcpv4SrvTest, interfaceAddFatal) {
+    interfaces_ = "";
+    IfacePtr eth0 = IfaceMgrTestConfig::createIface("eth0", ETH0_INDEX,
+                                                    "11:22:33:44:55:66");
+    auto detectIfaces = [&](bool update_only) {
+        if (!update_only) {
+            // No IPv4 address: will trigger an error.
+            eth0->addAddress(IOAddress("fe80::3a60:77ff:fed5:cdef"));
+            eth0->addAddress(IOAddress("2001:db8:1::1"));
+            IfaceMgr::instance().addInterface(eth0);
+        }
+        return (false);
+    };
+    IfaceMgr::instance().setDetectCallback(detectIfaces);
+    IfaceMgr::instance().clearIfaces();
+    IfaceMgr::instance().closeSockets();
+    IfaceMgr::instance().detectIfaces();
+    createUnixChannelServer();
+    PktFilterPtr filter(new PktFilterTestStub());
+    IfaceMgr::instance().setPacketFilter(filter);
+    SKIP_IF(skipped_);
+    // Override the on fail callback to unconditionally make the error fatal.
+    CfgIface::open_sockets_failed_callback_ =
+        [](ReconnectCtlPtr) {
+            ControlledDhcpv4Srv::getInstance()->shutdownServer(EXIT_FAILURE);
+        };
+    std::string response;
+
+    std::string command = "{ \"command\": \"interface-add\", \"arguments\": { \"interfaces\": [ \"eth0\" ] } }";
+
+    sendUnixCommand(command, response);
+    EXPECT_EQ(response, "{ \"result\": 5, \"text\": \"Interface configuration uodate triggered a fatal error: shutting down.\" }");
+}
+
 // This test verifies that disable DHCP service command performs sanity check on
 // parameters.
 TEST_F(CtrlChannelDhcpv4SrvTest, dhcpDisableBadParam) {

diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
index ab38cf3d1d88b69e1d9e0de8892a17edecc64770..188b6050fdcb5dec27ca587cb226c56de0d7fe22 100644 (file)
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
@@ -96,7 +96,7 @@ void signalHandler(int signo) {
 namespace isc {
 namespace dhcp {
 
-ControlledDhcpv6Srv* ControlledDhcpv6Srv::server_ = NULL;
+ControlledDhcpv6Srv* ControlledDhcpv6Srv::server_ = 0;
 
 void
 ControlledDhcpv6Srv::init(const std::string& file_name) {
@@ -201,6 +201,11 @@ ControlledDhcpv6Srv::loadConfigFile(const std::string& file_name) {
     return (result);
 }
 
+bool
+ControlledDhcpv6Srv::getShutdown() const {
+    return (Dhcpv6Srv::shutdown_);
+}
+
 ConstElementPtr
 ControlledDhcpv6Srv::commandShutdownHandler(const string&, ConstElementPtr args) {
     if (!ControlledDhcpv6Srv::getInstance()) {
@@ -416,6 +421,12 @@ ControlledDhcpv6Srv::commandConfigSetHandler(const string&,
     // the logging first in case there's a configuration failure.
     int rcode = 0;
     isc::config::parseAnswer(rcode, result);
+    if (getShutdown() && (rcode == CONTROL_RESULT_SUCCESS)) {
+        // Do not return success when a fatal error was triggered.
+        rcode = CONTROL_RESULT_FATAL_ERROR;
+        message = "Reconfiguration triggered a fatal error: shutting down.";
+        result = isc::config::createAnswer(rcode, message);
+    }
     if (rcode == CONTROL_RESULT_SUCCESS) {
         CfgMgr::instance().getStagingCfg()->applyLoggingCfg();
 
@@ -811,6 +822,9 @@ ControlledDhcpv6Srv::commandInterfaceAddHandler(const std::string&,
 
     ostringstream msg;
     if (!error) {
+        if (getShutdown()) {
+            return (isc::config::createAnswer(CONTROL_RESULT_FATAL_ERROR, "Interface configuration uodate triggered a fatal error: shutting down."));
+        }
         return (isc::config::createAnswer(CONTROL_RESULT_SUCCESS, "Interface configuration successfully updated."));
     } else {
         msg << "Updating used interfaces failed: " << message;

diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h
index afad02b913113e6c4efa2544608c568a6ac78ebb..ae9751ee48c14f3b68052bd5932b1bc48c0a820d 100644 (file)
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.h
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h
@@ -73,6 +73,9 @@ public:
     /// @param exit_value integer value to the process should exit with.
     virtual void shutdownServer(int exit_value);
 
+    /// @brief Return the server shutdown flag value.
+    bool getShutdown() const;
+
     /// @brief Configuration processor
     ///
     /// This is a method for handling incoming configuration updates.

diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
index fa952c17ae6a4155129fa5159db24825a1400c41..1375beb54780a3055182ea8bfd1f561e90e1fe84 100644 (file)
--- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
@@ -2596,6 +2596,44 @@ TEST_F(CtrlChannelDhcpv6SrvTest, interfaceAdd) {
     EXPECT_EQ(response, expected);
 }
 
+// Tests if interface-add can trigger a fatal failure.
+TEST_F(CtrlChannelDhcpv6SrvTest, interfaceAddFatal) {
+    interfaces_ = "";
+    IfacePtr eth0 = IfaceMgrTestConfig::createIface("eth0", ETH0_INDEX,
+                                                    "11:22:33:44:55:66");
+    auto detectIfaces = [&](bool update_only) {
+        if (!update_only) {
+            eth0->addAddress(IOAddress("10.0.0.1"));
+            eth0->addAddress(IOAddress("fe80::3a60:77ff:fed5:cdef"));
+            eth0->addAddress(IOAddress("2001:db8:1::1"));
+            IfaceMgr::instance().addInterface(eth0);
+        } else {
+            // Trigger an error on interface-add.
+            eth0->flag_running_ = false;
+        }
+        return (false);
+    };
+    IfaceMgr::instance().setDetectCallback(detectIfaces);
+    IfaceMgr::instance().clearIfaces();
+    IfaceMgr::instance().closeSockets();
+    IfaceMgr::instance().detectIfaces();
+    createUnixChannelServer();
+    PktFilter6Ptr filter(new PktFilter6TestStub());
+    IfaceMgr::instance().setPacketFilter(filter);
+    SKIP_IF(skipped_);
+    // Override the on fail callback to unconditionally make the error fatal.
+    CfgIface::open_sockets_failed_callback_ =
+        [](ReconnectCtlPtr) {
+            ControlledDhcpv6Srv::getInstance()->shutdownServer(EXIT_FAILURE);
+        };
+    std::string response;
+
+    std::string command = "{ \"command\": \"interface-add\", \"arguments\": { \"interfaces\": [ \"eth0\" ] } }";
+
+    sendUnixCommand(command, response);
+    EXPECT_EQ(response, "{ \"result\": 5, \"text\": \"Interface configuration uodate triggered a fatal error: shutting down.\" }");
+}
+
 // This test verifies that disable DHCP service command performs sanity check on
 // parameters.
 TEST_F(CtrlChannelDhcpv6SrvTest, dhcpDisableBadParam) {