Parcourir la source

fix: honour --config, and refuse to start when a named config is missing

The packaged systemd units have always started the services as

    /usr/bin/smartbotic-webserver --config /etc/smartbotic-automation/webserver.json

but neither service ever parsed a flag. Both read argv[1] as a path, so
config_path became the literal string "--config", no such file existed, and
the service logged

    Config file not found at --config, using defaults

and carried on. Every .deb install has therefore been running on built-in
defaults, ignoring its packaged /etc config entirely - including
database_address and database_project, the two settings that decide which
database and which multi-tenant namespace the service talks to. The failure was
invisible because the defaults happen to be reasonable, so a single-project
machine looks fine right up until it shares a daemon with another project.

Adds lib/common/config_arg.{hpp,cpp} - one parseConfigArg - accepting
"--config <path>", "--config=<path>", "-c <path>", "-c=<path>" and the
original bare positional argument, which stays supported. Unknown arguments are
ignored rather than rejected, so nothing a deployment already passes breaks.

It also reports whether the path was asked for or defaulted, and the services
now treat those differently. Falling back to defaults is fine when nobody named
a file. It is not fine when someone did: the settings they meant to apply would
be silently replaced, with a warning as the only trace. Both services now log an
error and exit 1 instead.

Verified all four calling forms against the built binaries: --config with a
missing file exits 1, --config with a real file loads it, --config=<path> exits
1 on a missing file, the bare positional form still loads, and no arguments
still falls back to ./config/<service>.json with a warning. Full node suite
43/43.

DEPLOYMENT NOTE: upgrading a .deb install makes these services read their
/etc config for the first time. A host that has been running on defaults will
pick up its real configured values on restart, which is the intent but is still
a behaviour change - check /etc/smartbotic-automation/*.json matches what that
host is actually expected to talk to before rolling this out.
fszontagh il y a 1 mois
Parent
commit
c7653217ea
5 fichiers modifiés avec 123 ajouts et 8 suppressions
  1. 1 0
      CMakeLists.txt
  2. 61 0
      lib/common/config_arg.cpp
  3. 42 0
      lib/common/config_arg.hpp
  4. 8 4
      src/runner/main.cpp
  5. 11 4
      src/webserver/main.cpp

+ 1 - 0
CMakeLists.txt

@@ -20,6 +20,7 @@ add_library(smartbotic_common STATIC
     lib/common/error.cpp
     lib/common/string_utils.cpp
     lib/common/config_defaults.cpp
+    lib/common/config_arg.cpp
 )
 target_include_directories(smartbotic_common PUBLIC
     ${CMAKE_CURRENT_SOURCE_DIR}/lib

+ 61 - 0
lib/common/config_arg.cpp

@@ -0,0 +1,61 @@
+#include "config_arg.hpp"
+
+namespace smartbotic::common {
+
+namespace {
+
+// Returns the value part of "--config=/etc/x.json" style arguments, or an empty
+// string when the argument does not carry one.
+std::string valueAfterEquals(const std::string& argument, const std::string& flag) {
+    const std::string prefix = flag + "=";
+    if (argument.rfind(prefix, 0) == 0) {
+        return argument.substr(prefix.size());
+    }
+    return {};
+}
+
+}  // namespace
+
+ConfigArg parseConfigArg(int argc, char* argv[], const std::filesystem::path& default_path) {
+    ConfigArg result;
+    result.path = default_path;
+    result.explicitly_given = false;
+
+    for (int i = 1; i < argc; ++i) {
+        const std::string argument = argv[i];
+
+        if (argument == "--config" || argument == "-c") {
+            // The value is the next argument. Without one this is a malformed
+            // command line, so leave the default in place rather than treating
+            // the flag itself as a path.
+            if (i + 1 < argc) {
+                result.path = argv[i + 1];
+                result.explicitly_given = true;
+                ++i;
+            }
+            continue;
+        }
+
+        const std::string equals_value =
+            !valueAfterEquals(argument, "--config").empty()
+                ? valueAfterEquals(argument, "--config")
+                : valueAfterEquals(argument, "-c");
+        if (!equals_value.empty()) {
+            result.path = equals_value;
+            result.explicitly_given = true;
+            continue;
+        }
+
+        // A bare first argument is the original calling convention and stays
+        // supported. Anything beginning with '-' is some other flag and is not
+        // a path.
+        if (i == 1 && !argument.empty() && argument[0] != '-') {
+            result.path = argument;
+            result.explicitly_given = true;
+        }
+    }
+
+    return result;
+}
+
+}  // namespace smartbotic::common

+ 42 - 0
lib/common/config_arg.hpp

@@ -0,0 +1,42 @@
+#pragma once
+
+#include <filesystem>
+#include <string>
+
+namespace smartbotic::common {
+
+struct ConfigArg {
+    std::filesystem::path path;
+
+    /**
+     * True when the caller named a config file on the command line, false when
+     * the built-in default path is being used.
+     *
+     * The distinction decides what a missing file means. Falling back to
+     * defaults is reasonable when nobody asked for a particular file; it is
+     * never reasonable when someone did, because the settings they meant to
+     * apply - database address, project namespace, ports - would be silently
+     * replaced by other values.
+     */
+    bool explicitly_given = false;
+};
+
+/**
+ * Read the config file location from a service's command line.
+ *
+ * Accepts, in order of preference:
+ *   --config <path>     --config=<path>
+ *   -c <path>           -c=<path>
+ *   <path>              (bare first argument, the original form)
+ *
+ * The flag forms exist because the packaged systemd units have always passed
+ * "--config /etc/.../webserver.json", while the services only ever read
+ * argv[1] as a path. That mismatch made every .deb install run on built-in
+ * defaults with nothing but a warning to say so.
+ *
+ * Unknown arguments are ignored rather than rejected, so this stays additive
+ * for anything else a deployment may already be passing.
+ */
+ConfigArg parseConfigArg(int argc, char* argv[], const std::filesystem::path& default_path);
+
+}  // namespace smartbotic::common

+ 8 - 4
src/runner/main.cpp

@@ -3,6 +3,7 @@
 #include "runner_service.hpp"
 #include "logging/logger.hpp"
 #include "config/config_loader.hpp"
+#include "common/config_arg.hpp"
 
 using namespace smartbotic;
 
@@ -23,15 +24,18 @@ int main(int argc, char* argv[]) {
     LOG_INFO("SmartBotic Runner starting...");
 
     // Load configuration
-    std::filesystem::path config_path = "./config/runner.json";
-    if (argc > 1) {
-        config_path = argv[1];
-    }
+    const auto config_arg = common::parseConfigArg(argc, argv, "./config/runner.json");
+    const std::filesystem::path config_path = config_arg.path;
 
     runner::RunnerServiceConfig config;
     if (std::filesystem::exists(config_path)) {
         config = runner::RunnerService::loadConfig(config_path);
         LOG_INFO("Loaded configuration from {}", config_path.string());
+    } else if (config_arg.explicitly_given) {
+        // See the webserver's main for why this is fatal rather than a warning.
+        LOG_ERROR("Config file not found at {}. It was given on the command line, "
+                  "so refusing to start on built-in defaults", config_path.string());
+        return 1;
     } else {
         LOG_WARN("Config file not found at {}, using defaults", config_path.string());
     }

+ 11 - 4
src/webserver/main.cpp

@@ -3,6 +3,7 @@
 #include "webserver_service.hpp"
 #include "logging/logger.hpp"
 #include "config/config_loader.hpp"
+#include "common/config_arg.hpp"
 
 using namespace smartbotic;
 
@@ -23,15 +24,21 @@ int main(int argc, char* argv[]) {
     LOG_INFO("SmartBotic WebServer starting...");
 
     // Load configuration
-    std::filesystem::path config_path = "./config/webserver.json";
-    if (argc > 1) {
-        config_path = argv[1];
-    }
+    const auto config_arg = common::parseConfigArg(argc, argv, "./config/webserver.json");
+    const std::filesystem::path config_path = config_arg.path;
 
     webserver::WebServerServiceConfig config;
     if (std::filesystem::exists(config_path)) {
         config = webserver::WebServerService::loadConfig(config_path);
         LOG_INFO("Loaded configuration from {}", config_path.string());
+    } else if (config_arg.explicitly_given) {
+        // Someone named this file, so starting on defaults would run the
+        // service against a different database, project namespace and port
+        // than they asked for - and the only sign would be a warning nobody
+        // reads. Refuse to start instead.
+        LOG_ERROR("Config file not found at {}. It was given on the command line, "
+                  "so refusing to start on built-in defaults", config_path.string());
+        return 1;
     } else {
         LOG_WARN("Config file not found at {}, using defaults", config_path.string());
     }