Merge pull request #1248 from notlesh/config-fixes-2020-04-29

Config fixes 2020 04 29

llarp/config/config.cpp

@@ -124,6 +124,18 @@ namespace llarp
 
     conf.defineOption<bool>(
         "router", "block-bogons", false, DefaultBlockBogons, AssignmentAcceptor(m_blockBogons));
+
+    conf.defineOption<std::string>(
+        "router", "contact-file", false, "", AssignmentAcceptor(m_routerContactFile));
+
+    conf.defineOption<std::string>(
+        "router", "encryption-privkey", false, "", AssignmentAcceptor(m_encryptionKeyFile));
+
+    conf.defineOption<std::string>(
+        "router", "ident-privkey", false, "", AssignmentAcceptor(m_identityKeyFile));
+
+    conf.defineOption<std::string>(
+        "router", "transport-privkey", false, "", AssignmentAcceptor(m_transportKeyFile));
   }
 
   void
@@ -165,16 +177,25 @@ namespace llarp
 
     // TODO: make sure this is documented
     // TODO: refactor to remove freehand options map
-    conf.defineOption<std::string>(
-        "network", "upstream-dns", false, true, "", [this](std::string arg) {
-          m_options.emplace("upstream-dns", std::move(arg));
-        });
+    conf.defineOption<std::string>("dns", "upstream", false, true, "", [this](std::string arg) {
+      m_options.emplace("upstream", std::move(arg));
+    });
+
+    // TODO: the m_options is fixed in another branch/PR, this will conflict when merged
+    //       you're welcome
 
     // TODO: make sure this is documented
-    conf.defineOption<std::string>(
-        "network", "local-dns", false, true, "", [this](std::string arg) {
-          m_options.emplace("local-dns", std::move(arg));
-        });
+    conf.defineOption<std::string>("dns", "local-dns", false, true, "", [this](std::string arg) {
+      m_options.emplace("local-dns", arg);
+      m_options.emplace("bind", arg);
+    });
+
+    // TODO: we'll only support "bind" going forward, for now make sure bind and local-dns are
+    //       equivalent
+    conf.defineOption<std::string>("dns", "bind", false, true, "", [this](std::string arg) {
+      m_options.emplace("local-dns", arg);
+      m_options.emplace("bind", arg);
+    });
   }
 
   LinksConfig::LinkInfo
@@ -186,6 +207,7 @@ namespace llarp
     //    this is somewhat of a backwards- and forwards-compatibility thing
 
     LinkInfo info;
+    info.port = 0;
     info.addressFamily = AF_INET;
     info.interface = str(name);
 
@@ -207,7 +229,7 @@ namespace llarp
   {
     (void)params;
 
-    constexpr auto DefaultOutboundLinkValue = "1090";
+    constexpr auto DefaultOutboundLinkValue = "0";
 
     conf.defineOption<std::string>(
         "bind", "*", false, false, DefaultOutboundLinkValue, [this](std::string arg) {
@@ -223,8 +245,6 @@ namespace llarp
       assert(name != "*");  // handled by defineOption("bind", "*", ...) above
 
       m_InboundLinks.emplace_back(std::move(info));
-
-      return true;
     });
   }
 
@@ -424,6 +444,7 @@ namespace llarp
 
       ConfigDefinition conf;
       initializeConfig(conf, params);
+      addBackwardsCompatibleConfigOptions(conf);
 
       ConfigParser parser;
       if (!parser.LoadFile(fname))
@@ -496,6 +517,34 @@ namespace llarp
     logging.defineConfigOptions(conf, params);
   }
 
+  void
+  Config::addBackwardsCompatibleConfigOptions(ConfigDefinition& conf)
+  {
+    auto addIgnoreOption = [&](const std::string& section, const std::string& name) {
+      conf.defineOption<std::string>(section, name, false, true, "", [=](std::string arg) {
+        (void)arg;
+        LogWarn("*** WARNING: The config option [", section, "]:", name, " is deprecated");
+      });
+    };
+
+    addIgnoreOption("system", "user");
+    addIgnoreOption("system", "group");
+    addIgnoreOption("system", "pidfile");
+
+    addIgnoreOption("api", "authkey");
+
+    addIgnoreOption("netdb", "dir");
+
+    // these weren't even ever used!
+    addIgnoreOption("router", "max-routers");
+    addIgnoreOption("router", "min-routers");
+
+    // TODO: this may have been a synonym for [router]worker-threads
+    addIgnoreOption("router", "threads");
+
+    addIgnoreOption("metrics", "json-metrics-path");
+  }
+
   void
   ensureConfig(
       const fs::path& defaultDataDir, const fs::path& confFile, bool overwrite, bool asRouter)

llarp/config/config.hpp

@@ -55,6 +55,11 @@ namespace llarp
 
     size_t m_JobQueueSize;
 
+    std::string m_routerContactFile;
+    std::string m_encryptionKeyFile;
+    std::string m_identityKeyFile;
+    std::string m_transportKeyFile;
+
     void
     defineConfigOptions(ConfigDefinition& conf, const ConfigGenParameters& params);
   };
@@ -185,6 +190,13 @@ namespace llarp
     void
     initializeConfig(ConfigDefinition& conf, const ConfigGenParameters& params);
 
+    /// Insert config entries for backwards-compatibility (e.g. so that the config system will
+    /// tolerate old values that are no longer accepted)
+    ///
+    /// @param conf is the config to modify
+    void
+    addBackwardsCompatibleConfigOptions(ConfigDefinition& conf);
+
     // Load a config from the given file
     bool
     Load(const char* fname, bool isRelay, fs::path defaultDataDir);

llarp/config/definition.cpp

@@ -16,6 +16,18 @@ namespace llarp
   {
   }
 
+  template <>
+  bool
+  OptionDefinition<bool>::fromString(const std::string& input)
+  {
+    if (input == "false" || input == "off" || input == "0" || input == "no")
+      return false;
+    else if (input == "true" || input == "on" || input == "1" || input == "yes")
+      return true;
+    else
+      throw std::invalid_argument(stringify(input, " is not a valid bool"));
+  }
+
   ConfigDefinition&
   ConfigDefinition::defineOption(OptionDefinition_ptr def)
   {
@@ -37,12 +49,17 @@ namespace llarp
   ConfigDefinition&
   ConfigDefinition::addConfigValue(string_view section, string_view name, string_view value)
   {
+    // see if we have an undeclared handler to fall back to in case section or section:name is
+    // absent
+    auto undItr = m_undeclaredHandlers.find(std::string(section));
+    bool haveUndeclaredHandler = (undItr != m_undeclaredHandlers.end());
+
+    // get section, falling back to undeclared handler if needed
     auto secItr = m_definitions.find(std::string(section));
     if (secItr == m_definitions.end())
     {
       // fallback to undeclared handler if available
-      auto undItr = m_undeclaredHandlers.find(std::string(section));
-      if (undItr == m_undeclaredHandlers.end())
+      if (not haveUndeclaredHandler)
         throw std::invalid_argument(stringify("no declared section [", section, "]"));
       else
       {
@@ -53,10 +70,20 @@ namespace llarp
     }
 
     // section was valid, get definition by name
+    // fall back to undeclared handler if needed
     auto& sectionDefinitions = secItr->second;
     auto defItr = sectionDefinitions.find(std::string(name));
     if (defItr == sectionDefinitions.end())
-      throw std::invalid_argument(stringify("no declared option [", section, "]:", name));
+    {
+      if (not haveUndeclaredHandler)
+        throw std::invalid_argument(stringify("no declared option [", section, "]:", name));
+      else
+      {
+        auto& handler = undItr->second;
+        handler(section, name, value);
+        return *this;
+      }
+    }
 
     OptionDefinition_ptr& definition = defItr->second;
     definition->parseValue(std::string(value));

llarp/config/definition.hpp

@@ -168,17 +168,19 @@ namespace llarp
             stringify("duplicate value for ", name, ", previous value: ", parsedValues[0]));
       }
 
+      parsedValues.emplace_back(fromString(input));
+    }
+
+    T
+    fromString(const std::string& input)
+    {
       std::istringstream iss(input);
       T t;
       iss >> t;
       if (iss.fail())
-      {
         throw std::invalid_argument(stringify(input, " is not a valid ", typeid(T).name()));
-      }
       else
-      {
-        parsedValues.emplace_back(std::move(t));
-      }
+        return t;
     }
 
     std::string
@@ -239,6 +241,12 @@ namespace llarp
     std::function<void(T)> acceptor;
   };
 
+  /// Specialization for bool types. We don't want to use stringstream parsing in this
+  /// case because we want to accept "truthy" and "falsy" string values (e.g. "off" == false)
+  template <>
+  bool
+  OptionDefinition<bool>::fromString(const std::string& input);
+
   using UndeclaredValueHandler =
       std::function<void(string_view section, string_view name, string_view value)>;
 

llarp/config/key_manager.cpp

@@ -33,11 +33,27 @@ namespace llarp
 
     fs::path root = config.router.m_dataDir;
 
-    // TODO: use fs::path, or at least support windows-style separators
-    m_rcPath = root / our_rc_filename;
-    m_idKeyPath = root / our_identity_filename;
-    m_encKeyPath = root / our_enc_key_filename;
-    m_transportKeyPath = root / our_transport_key_filename;
+    // utility function to assign a path, using the specified config parameter if present and
+    // falling back to root / defaultName if not
+    auto deriveFile = [&](const std::string& defaultName, const std::string& option) {
+      if (option.empty())
+      {
+        return root / defaultName;
+      }
+      else
+      {
+        fs::path file(option);
+        if (not file.is_absolute())
+          throw std::runtime_error(stringify("override for ", defaultName, " cannot be relative"));
+
+        return file;
+      }
+    };
+
+    m_rcPath = deriveFile(our_rc_filename, config.router.m_routerContactFile);
+    m_idKeyPath = deriveFile(our_identity_filename, config.router.m_identityKeyFile);
+    m_encKeyPath = deriveFile(our_enc_key_filename, config.router.m_encryptionKeyFile);
+    m_transportKeyPath = deriveFile(our_transport_key_filename, config.router.m_transportKeyFile);
 
     m_usingLokid = config.lokid.whitelistRouters;
     m_lokidRPCAddr = config.lokid.lokidRPCAddr;

llarp/router/router.cpp

@@ -469,7 +469,7 @@ namespace llarp
       {
         LogError("No bootstrap files specified in config file, and the default");
         LogError("bootstrap file ", defaultBootstrapFile, " does not exist.");
-        LogError("Please provide a bootstrap file (e.g. run 'lokinet-bootstrap'");
+        LogError("Please provide a bootstrap file (e.g. run 'lokinet-bootstrap)'");
         throw std::runtime_error("No bootstrap files available.");
       }
     }
@@ -1241,8 +1241,9 @@ namespace llarp
 
     for (const auto af : afs)
     {
-      if (!link->Configure(netloop(), "*", af, m_OutboundPort))
+      if (not link->Configure(netloop(), "*", af, m_OutboundPort))
         continue;
+
       _linkManager.AddLink(std::move(link), false);
       return true;
     }

test/config/test_llarp_config_definition.cpp

@@ -363,3 +363,71 @@ TEST_CASE("ConfigDefinition multiple values", "[config]")
   CHECK(values[1] == 2);
   CHECK(values[2] == 3);
 }
+
+TEST_CASE("ConfigDefinition [bind]iface regression", "[config regression]")
+{
+  llarp::ConfigDefinition config;
+
+  std::string val1;
+  std::string undeclaredName;
+  std::string undeclaredValue;
+
+  config.defineOption<std::string>(
+      "bind", "*", false, false, "1090", [&](std::string arg) { val1 = arg; });
+
+  config.addUndeclaredHandler("bind", [&](string_view, string_view name, string_view value) {
+    undeclaredName = std::string(name);
+    undeclaredValue = std::string(value);
+  });
+
+  config.addConfigValue("bind", "enp35s0", "1091");
+  CHECK_NOTHROW(config.acceptAllOptions());
+
+  CHECK(val1 == "1090");
+  CHECK(undeclaredName == "enp35s0");
+  CHECK(undeclaredValue == "1091");
+}
+
+TEST_CASE("ConfigDefinition truthy/falsy bool values", "[config]")
+{
+  // truthy values
+  for (auto val : {"true", "on", "yes", "1"})
+  {
+    llarp::OptionDefinition<bool> def("foo", "bar", false, false);
+
+    // defaults to false
+    auto maybe = def.getValue();
+    CHECK(maybe.has_value());
+    CHECK(maybe.value() == false);
+
+    // val should result in true
+    CHECK_NOTHROW(def.parseValue(val));
+    maybe = def.getValue();
+    CHECK(maybe.has_value());
+    CHECK(maybe.value() == true);
+  }
+
+  // falsy values
+  for (auto val : {"false", "off", "no", "0"})
+  {
+    llarp::OptionDefinition<bool> def("foo", "bar", false, true);
+
+    // defaults to true
+    auto maybe = def.getValue();
+    CHECK(maybe.has_value());
+    CHECK(maybe.value() == true);
+
+    // val should result in false
+    CHECK_NOTHROW(def.parseValue(val));
+    maybe = def.getValue();
+    CHECK(maybe.has_value());
+    CHECK(maybe.value() == false);
+  }
+
+  // illegal values
+  for (auto val : {"", " ", "TRUE", "argle", " false", "2"})
+  {
+    llarp::OptionDefinition<bool> def("foo", "bar", false, true);
+    CHECK_THROWS(def.parseValue(val));
+  }
+}