[#3694] addressed review comments

Razvan Becheriu · Razvan Becheriu · commit de694a024f89 · 2025-02-07T14:11:10.000+02:00
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,9 @@
+2324.	[func]		razvan
+	It is not necessary to restart the server to apply changes in the
+	TLS configuration. Running the "config-reload" command is
+	sufficient.
+	(Gitlab #3694)
+
 Kea 2.7.6 (development) released on January 29, 2025
 
 2323.	[func]*		fdupont
diff --git a/src/bin/agent/ca_messages.cc b/src/bin/agent/ca_messages.cc
@@ -17,6 +17,7 @@ extern const isc::log::MessageID CTRL_AGENT_CONFIG_SYNTAX_WARNING = "CTRL_AGENT_
 extern const isc::log::MessageID CTRL_AGENT_FAILED = "CTRL_AGENT_FAILED";
 extern const isc::log::MessageID CTRL_AGENT_HTTPS_SERVICE_REUSED = "CTRL_AGENT_HTTPS_SERVICE_REUSED";
 extern const isc::log::MessageID CTRL_AGENT_HTTPS_SERVICE_STARTED = "CTRL_AGENT_HTTPS_SERVICE_STARTED";
+extern const isc::log::MessageID CTRL_AGENT_HTTPS_SERVICE_UPDATED = "CTRL_AGENT_HTTPS_SERVICE_UPDATED";
 extern const isc::log::MessageID CTRL_AGENT_HTTP_SERVICE_REUSED = "CTRL_AGENT_HTTP_SERVICE_REUSED";
 extern const isc::log::MessageID CTRL_AGENT_HTTP_SERVICE_STARTED = "CTRL_AGENT_HTTP_SERVICE_STARTED";
 extern const isc::log::MessageID CTRL_AGENT_RUN_EXIT = "CTRL_AGENT_RUN_EXIT";
@@ -38,6 +39,7 @@ const char* values[] = {
     "CTRL_AGENT_FAILED", "application experienced a fatal error: %1",
     "CTRL_AGENT_HTTPS_SERVICE_REUSED", "reused HTTPS service bound to address %1:%2",
     "CTRL_AGENT_HTTPS_SERVICE_STARTED", "HTTPS service bound to address %1:%2",
+    "CTRL_AGENT_HTTPS_SERVICE_UPDATED", "reused HTTPS service bound to address %1:%2 and updated TLS settings",
     "CTRL_AGENT_HTTP_SERVICE_REUSED", "reused HTTP service bound to address %1:%2",
     "CTRL_AGENT_HTTP_SERVICE_STARTED", "HTTP service bound to address %1:%2",
     "CTRL_AGENT_RUN_EXIT", "application is exiting the event loop",
diff --git a/src/bin/agent/ca_messages.h b/src/bin/agent/ca_messages.h
@@ -18,6 +18,7 @@ extern const isc::log::MessageID CTRL_AGENT_CONFIG_SYNTAX_WARNING;
 extern const isc::log::MessageID CTRL_AGENT_FAILED;
 extern const isc::log::MessageID CTRL_AGENT_HTTPS_SERVICE_REUSED;
 extern const isc::log::MessageID CTRL_AGENT_HTTPS_SERVICE_STARTED;
+extern const isc::log::MessageID CTRL_AGENT_HTTPS_SERVICE_UPDATED;
 extern const isc::log::MessageID CTRL_AGENT_HTTP_SERVICE_REUSED;
 extern const isc::log::MessageID CTRL_AGENT_HTTP_SERVICE_STARTED;
 extern const isc::log::MessageID CTRL_AGENT_RUN_EXIT;
diff --git a/src/bin/agent/ca_messages.mes b/src/bin/agent/ca_messages.mes
@@ -53,6 +53,11 @@ This informational message indicates that the server has started HTTPS service
 on the specified address and port. All control commands should be sent to this
 address and port over a TLS channel.
 
+% CTRL_AGENT_HTTPS_SERVICE_UPDATED reused HTTPS service bound to address %1:%2 and updated TLS settings
+This informational message indicates that the server has reused existing
+HTTPS service on the specified address and port. Note that any change in
+the TLS setup has been applied.
+
 % CTRL_AGENT_HTTP_SERVICE_REUSED reused HTTP service bound to address %1:%2
 This informational message indicates that the server has reused existing
 HTTP service on the specified address and port.
diff --git a/src/bin/agent/ca_process.cc b/src/bin/agent/ca_process.cc
@@ -145,7 +145,7 @@ CtrlAgentProcess::configure(isc::data::ConstElementPtr config_set,
                 if (listener->getTlsContext()) {
                     if (ctx->getTrustAnchor().empty()) {
                         // Can not switch from HTTPS to HTTP
-                        LOG_INFO(agent_logger, CTRL_AGENT_HTTPS_SERVICE_REUSED)
+                        LOG_ERROR(agent_logger, CTRL_AGENT_HTTPS_SERVICE_REUSED)
                             .arg(server_address.toText())
                             .arg(server_port);
                     } else {
@@ -161,10 +161,13 @@ CtrlAgentProcess::configure(isc::data::ConstElementPtr config_set,
                         it->second->config_->setAuthConfig(ctx->getAuthConfig());
                         it->second->config_->setHttpHeaders(ctx->getHttpHeaders());
                         getIOService()->post([listener, tls_context]() { listener->setTlsContext(tls_context); });
+                        LOG_INFO(agent_logger, CTRL_AGENT_HTTPS_SERVICE_UPDATED)
+                            .arg(server_address.toText())
+                            .arg(server_port);
                     }
                 } else if (!ctx->getTrustAnchor().empty()) {
                     // Can not switch from HTTP to HTTPS
-                    LOG_INFO(agent_logger, CTRL_AGENT_HTTP_SERVICE_REUSED)
+                    LOG_ERROR(agent_logger, CTRL_AGENT_HTTP_SERVICE_REUSED)
                         .arg(server_address.toText())
                         .arg(server_port);
                 }
@@ -270,7 +273,7 @@ CtrlAgentProcess::closeCommandSockets() {
     // We have stopped listeners but there may be some pending handlers
     // related to these listeners. Need to invoke these handlers.
     try {
-        getIOService()->pollOne();
+        getIOService()->poll();
     } catch (...) {
     }
 }
diff --git a/src/lib/config/config_messages.cc b/src/lib/config/config_messages.cc
@@ -35,11 +35,10 @@ extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLEAR_ERROR = "COMMAND_WAT
 extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLOSE_ERROR = "COMMAND_WATCH_SOCKET_CLOSE_ERROR";
 extern const isc::log::MessageID COMMAND_WATCH_SOCKET_MARK_READY_ERROR = "COMMAND_WATCH_SOCKET_MARK_READY_ERROR";
 extern const isc::log::MessageID HTTP_COMMAND_MGR_HTTPS_SERVICE_REUSED = "HTTP_COMMAND_MGR_HTTPS_SERVICE_REUSED";
+extern const isc::log::MessageID HTTP_COMMAND_MGR_HTTPS_SERVICE_UPDATED = "HTTP_COMMAND_MGR_HTTPS_SERVICE_UPDATED";
 extern const isc::log::MessageID HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED = "HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED";
 extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STARTED = "HTTP_COMMAND_MGR_SERVICE_STARTED";
 extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STOPPING = "HTTP_COMMAND_MGR_SERVICE_STOPPING";
-extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STOPPING_ALL = "HTTP_COMMAND_MGR_SERVICE_STOPPING_ALL";
-extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STOPPING_NO_DATA = "HTTP_COMMAND_MGR_SERVICE_STOPPING_NO_DATA";
 
 } // namespace config
 } // namespace isc
@@ -75,11 +74,10 @@ const char* values[] = {
     "COMMAND_WATCH_SOCKET_CLOSE_ERROR", "watch socket failed to close: %1",
     "COMMAND_WATCH_SOCKET_MARK_READY_ERROR", "watch socket failed to mark ready: %1",
     "HTTP_COMMAND_MGR_HTTPS_SERVICE_REUSED", "reused HTTPS service bound to address %1:%2",
+    "HTTP_COMMAND_MGR_HTTPS_SERVICE_UPDATED", "reused HTTPS service bound to address %1:%2 and updated TLS settings",
     "HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED", "reused HTTP service bound to address %1:%2",
     "HTTP_COMMAND_MGR_SERVICE_STARTED", "started %1 service bound to address %2 port %3",
     "HTTP_COMMAND_MGR_SERVICE_STOPPING", "Server is stopping %1 service %2",
-    "HTTP_COMMAND_MGR_SERVICE_STOPPING_ALL", "stopping %1 service %2",
-    "HTTP_COMMAND_MGR_SERVICE_STOPPING_NO_DATA", "Server is stopping all services including %1 service %2",
     NULL
 };
 
diff --git a/src/lib/config/config_messages.h b/src/lib/config/config_messages.h
@@ -36,11 +36,10 @@ extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLEAR_ERROR;
 extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLOSE_ERROR;
 extern const isc::log::MessageID COMMAND_WATCH_SOCKET_MARK_READY_ERROR;
 extern const isc::log::MessageID HTTP_COMMAND_MGR_HTTPS_SERVICE_REUSED;
+extern const isc::log::MessageID HTTP_COMMAND_MGR_HTTPS_SERVICE_UPDATED;
 extern const isc::log::MessageID HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED;
 extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STARTED;
 extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STOPPING;
-extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STOPPING_ALL;
-extern const isc::log::MessageID HTTP_COMMAND_MGR_SERVICE_STOPPING_NO_DATA;
 
 } // namespace config
 } // namespace isc
diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes
@@ -158,6 +158,11 @@ This informational message indicates that the server has reused existing
 HTTPS service on the specified address and port. Note that any change in
 the TLS setup was ignored.
 
+% HTTP_COMMAND_MGR_HTTPS_SERVICE_UPDATED reused HTTPS service bound to address %1:%2 and updated TLS settings
+This informational message indicates that the server has reused existing
+HTTPS service on the specified address and port. Note that any change in
+the TLS setup has been applied.
+
 % HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED reused HTTP service bound to address %1:%2
 This informational message indicates that the server has reused existing
 HTTP service on the specified address and port.
@@ -170,12 +175,3 @@ control commands.
 % HTTP_COMMAND_MGR_SERVICE_STOPPING Server is stopping %1 service %2
 This informational message indicates that the server has stopped
 HTTP/HTTPS service. When known the address and port are displayed.
-
-% HTTP_COMMAND_MGR_SERVICE_STOPPING_ALL stopping %1 service %2
-This informational message indicates that the server has stopped
-HTTP/HTTPS service. When known the address and port are displayed.
-
-% HTTP_COMMAND_MGR_SERVICE_STOPPING_NO_DATA Server is stopping all services including %1 service %2
-This informational message indicates that the server is stopping all
-HTTP/HTTPS services. When known the address and port are displayed for
-each service.
diff --git a/src/lib/config/http_command_mgr.cc b/src/lib/config/http_command_mgr.cc
@@ -58,7 +58,9 @@ class HttpCommandMgrImpl {
     void closeCommandSocket(HttpSocketInfoPtr info, bool remove);
 
     /// @brief Close control socket.
-    void closeCommandSockets();
+    ///
+    /// @param remove When true remove the listeners immediately.
+    void closeCommandSockets(bool remove = true);
 
     /// @brief Returns a const pointer to the HTTP listener.
     ///
@@ -133,7 +135,7 @@ HttpCommandMgrImpl::openCommandSocket(const isc::data::ConstElementPtr config) {
             if (listener->getTlsContext()) {
                 if (cmd_config->getTrustAnchor().empty()) {
                     // Can not switch from HTTPS to HTTP
-                    LOG_INFO(command_logger, HTTP_COMMAND_MGR_HTTPS_SERVICE_REUSED)
+                    LOG_ERROR(command_logger, HTTP_COMMAND_MGR_HTTPS_SERVICE_REUSED)
                         .arg(server_address.toText())
                         .arg(server_port);
                 } else {
@@ -151,10 +153,13 @@ HttpCommandMgrImpl::openCommandSocket(const isc::data::ConstElementPtr config) {
                     it->second->config_->setHttpHeaders(cmd_config->getHttpHeaders());
                     it->second->config_->setEmulateAgentResponse(cmd_config->getEmulateAgentResponse());
                     io_service_->post([listener, tls_context]() { listener->setTlsContext(tls_context); });
+                    LOG_INFO(command_logger, HTTP_COMMAND_MGR_HTTPS_SERVICE_UPDATED)
+                        .arg(server_address.toText())
+                        .arg(server_port);
                 }
             } else if (!cmd_config->getTrustAnchor().empty()) {
                 // Can not switch from HTTP to HTTPS
-                LOG_INFO(command_logger, HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED)
+                LOG_ERROR(command_logger, HTTP_COMMAND_MGR_HTTP_SERVICE_REUSED)
                     .arg(server_address.toText())
                     .arg(server_port);
             }
@@ -233,50 +238,22 @@ HttpCommandMgrImpl::closeCommandSocket(HttpSocketInfoPtr info, bool remove) {
                 sockets_.erase(it);
             }
         }
-    } else {
-        for (auto const& data : sockets_) {
-            ostringstream ep;
-            use_https = !data.second->config_->getCertFile().empty();
-            ep << "bound to address " << data.second->config_->getSocketAddress()
-               << " port " << data.second->config_->getSocketPort();
-
-            LOG_INFO(command_logger, HTTP_COMMAND_MGR_SERVICE_STOPPING_NO_DATA)
-                .arg(use_https ? "HTTPS" : "HTTP")
-                .arg(ep.str());
-            data.second->listener_->stop();
+        // We have stopped listeners but there may be some pending handlers
+        // related to these listeners. Need to invoke these handlers.
+        try {
+            io_service_->pollOne();
+        } catch (...) {
         }
-        if (remove) {
-            sockets_.clear();
-        }
-    }
-    // We have stopped listeners but there may be some pending handlers
-    // related to these listeners. Need to invoke these handlers.
-    try {
-        io_service_->pollOne();
-    } catch (...) {
+    } else {
+        closeCommandSockets(remove);
     }
 }
 
 void
-HttpCommandMgrImpl::closeCommandSockets() {
-    bool use_https = false;
-    for (auto const& data : sockets_) {
-        ostringstream ep;
-        use_https = !data.second->config_->getCertFile().empty();
-        ep << "bound to address " << data.second->config_->getSocketAddress()
-           << " port " << data.second->config_->getSocketPort();
-
-        LOG_INFO(command_logger, HTTP_COMMAND_MGR_SERVICE_STOPPING_ALL)
-            .arg(use_https ? "HTTPS" : "HTTP")
-            .arg(ep.str());
-        data.second->listener_->stop();
-    }
-    sockets_.clear();
-    // We have stopped listeners but there may be some pending handlers
-    // related to these listeners. Need to invoke these handlers.
-    try {
-        io_service_->pollOne();
-    } catch (...) {
+HttpCommandMgrImpl::closeCommandSockets(bool remove) {
+    auto copy = sockets_;
+    for (auto const& data : copy) {
+        closeCommandSocket(data.second, remove);
     }
 }
 
diff --git a/src/lib/config/http_command_mgr.h b/src/lib/config/http_command_mgr.h
@@ -64,11 +64,17 @@ class HttpCommandMgr : public boost::noncopyable {
     /// Creates http/https listener, or reuses the existing one reapplying
     /// changes.
     ///
+    /// @note This function in used internally by @ref openCommandSockets and it
+    /// should not be used directly, except for unittests.
+    ///
     /// @param config Configuration information for the http control socket.
     void openCommandSocket(const isc::data::ConstElementPtr config);
 
     /// @brief Close http control socket.
     ///
+    /// @note This function in used internally by @ref closeCommandSockets and it
+    /// should not be used directly, except for unittests.
+    ///
     /// @param info Configuration information for the http control socket.
     /// @param remove When true remove the listeners immediately.
     void closeCommandSocket(HttpSocketInfoPtr info = HttpSocketInfoPtr(), bool remove = true);
diff --git a/src/lib/config/unix_command_mgr.cc b/src/lib/config/unix_command_mgr.cc
@@ -523,7 +523,9 @@ class UnixCommandMgrImpl {
     void closeCommandSocket(UnixSocketInfoPtr info);
 
     /// @brief Shuts down any open unix control sockets
-    void closeCommandSockets();
+    ///
+    /// @param remove When true remove the listeners immediately.
+    void closeCommandSockets(bool remove = true);
 
     /// @brief Asynchronously accepts next connection.
     ///
@@ -679,61 +681,24 @@ UnixCommandMgrImpl::closeCommandSocket(UnixSocketInfoPtr info) {
         if (it != sockets_.end()) {
             sockets_.erase(it);
         }
-    } else {
-        for (auto const& data : sockets_) {
-            if (data.second->acceptor_ && data.second->acceptor_->isOpen()) {
-                if (use_external_) {
-                    IfaceMgr::instance().deleteExternalSocket(data.second->acceptor_->getNative());
-                }
-                data.second->acceptor_->close();
-                static_cast<void>(::remove(data.second->config_->getSocketName().c_str()));
-                static_cast<void>(::remove(data.second->config_->getLockName().c_str()));
-            }
-
-            // Stop all connections which can be closed. The only connection that won't
-            // be closed is the one over which we have received a request to reconfigure
-            // the server. This connection will be held until the UnixCommandMgr
-            // responds to such request.
-            connection_pool_.stopAll();
-            if (data.second->lock_fd_ != -1) {
-                close(data.second->lock_fd_);
-                data.second->lock_fd_ = -1;
-            }
+        try {
+            io_service_->pollOne();
+        } catch (...) {
         }
-    }
-    try {
-        io_service_->pollOne();
-    } catch (...) {
+    } else {
+        closeCommandSockets(false);
     }
 }
 
 void
-UnixCommandMgrImpl::closeCommandSockets() {
-    for (auto const& data : sockets_) {
-        if (data.second->acceptor_ && data.second->acceptor_->isOpen()) {
-            if (use_external_) {
-                IfaceMgr::instance().deleteExternalSocket(data.second->acceptor_->getNative());
-            }
-            data.second->acceptor_->close();
-            static_cast<void>(::remove(data.second->config_->getSocketName().c_str()));
-            static_cast<void>(::remove(data.second->config_->getLockName().c_str()));
-        }
-
-        // Stop all connections which can be closed. The only connection that won't
-        // be closed is the one over which we have received a request to reconfigure
-        // the server. This connection will be held until the UnixCommandMgr
-        // responds to such request.
-        connection_pool_.stopAll();
-        if (data.second->lock_fd_ != -1) {
-            close(data.second->lock_fd_);
-            data.second->lock_fd_ = -1;
-        }
+UnixCommandMgrImpl::closeCommandSockets(bool remove) {
+    auto copy = sockets_;
+    for (auto const& data : copy) {
+        closeCommandSocket(data.second);
     }
-    try {
-        io_service_->pollOne();
-    } catch (...) {
+    if (remove) {
+        sockets_.clear();
     }
-    sockets_.clear();
 }
 
 void
diff --git a/src/lib/config/unix_command_mgr.h b/src/lib/config/unix_command_mgr.h
@@ -73,6 +73,9 @@ class UnixCommandMgr : public boost::noncopyable {
     ///
     /// Creates acceptor, or reuses the existing one.
     ///
+    /// @note This function in used internally by @ref openCommandSockets and it
+    /// should not be used directly, except for unittests.
+    ///
     /// @throw BadSocketInfo When socket configuration is invalid.
     /// @throw SocketError When socket operation fails.
     ///
@@ -81,6 +84,9 @@ class UnixCommandMgr : public boost::noncopyable {
 
     /// @brief Shuts down any open unix control sockets.
     ///
+    /// @note This function in used internally by @ref closeCommandSockets and it
+    /// should not be used directly, except for unittests.
+    ///
     /// @param config Configuration information for the unix control socket.
     void closeCommandSocket(UnixSocketInfoPtr info = UnixSocketInfoPtr());
 
