From 9d71228e746bf55f7328cc639512112c9ec75698 Mon Sep 17 00:00:00 2001 From: Stephen Shelton Date: Fri, 13 Mar 2020 14:45:33 -0600 Subject: [PATCH] Replace config visit pattern with explicit lookups This is an initial pass at doing explicit value checks when handling config parsing, as opposed to using a visiting pattern. The latter made it difficult to check for conditions such as missing required values, multiple values, etc. It was also generally less readable (think declarative) which further made it difficult to get a grasp for what our actual configuration file requirements were. --- llarp/config/config.cpp | 328 ++++++++++++++---------- llarp/config/config.hpp | 56 ++-- llarp/config/ini.cpp | 48 +++- llarp/config/ini.hpp | 31 ++- llarp/service/config.cpp | 15 +- test/config/test_llarp_config_ini.cpp | 2 +- test/util/test_llarp_util_log_level.cpp | 33 --- 7 files changed, 305 insertions(+), 208 deletions(-) diff --git a/llarp/config/config.cpp b/llarp/config/config.cpp index ca9b9952f..af5c87965 100644 --- a/llarp/config/config.cpp +++ b/llarp/config/config.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -92,138 +93,171 @@ namespace llarp return {}; } - void - RouterConfig::fromSection(string_view key, string_view val) + bool + RouterConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "job-queue-size") - { - auto sval = svtoi(val); - if (sval >= 1024) - { - m_JobQueueSize = sval; - LogInfo("Set job queue size to ", m_JobQueueSize); - } - } - if (key == "default-protocol") + // [router]:job-queue-size + auto parsedValue = parser.getSingleSectionValue(values, "router", "job-queue-size", false); + if (not parsedValue.empty()) { - m_DefaultLinkProto = str(val); - LogInfo("overriding default link protocol to '", val, "'"); - } - if (key == "netid") - { - if (val.size() <= NetID::size()) - { - m_netId = str(val); - LogInfo("setting netid to '", val, "'"); - } + int val = svtoi(parsedValue); + if (val < 1024) + throw std::invalid_argument("invalid value for [router]:job-queue-size, must be 1024 or greater"); else - { - llarp::LogError("invalid netid '", val, "', is too long"); - } + m_JobQueueSize = val; } - if (key == "max-connections") + + // [router]:default-protocol + parsedValue = parser.getSingleSectionValue(values, "router", "default-protocol", false); + if (not parsedValue.empty()) + m_DefaultLinkProto = parsedValue; + + // [router]:netid + parsedValue = parser.getSingleSectionValue(values, "router", "netid", true); + assert(not parsedValue.empty()); // gauranteed by getSingleSectionValue() with required == true + if(parsedValue.size() > NetID::size()) + throw std::invalid_argument("value for [router]:netid is too long"); + m_netId = str(parsedValue); + + // [router]:max-connections + parsedValue = parser.getSingleSectionValue(values, "router", "max-connections", false); + if (not parsedValue.empty()) { - auto ival = svtoi(val); - if (ival > 0) - { - m_maxConnectedRouters = ival; - LogInfo("max connections set to ", m_maxConnectedRouters); - } + int val = svtoi(parsedValue); + if (val < 1) + throw std::invalid_argument("invalid value for [router]:max-connections"); + else + m_maxConnectedRouters = val; } - if (key == "min-connections") + + // [router]:min-connections + parsedValue = parser.getSingleSectionValue(values, "router", "min-connections", false); + if (not parsedValue.empty()) { - auto ival = svtoi(val); - if (ival > 0) - { - m_minConnectedRouters = ival; - LogInfo("min connections set to ", m_minConnectedRouters); - } + int val = svtoi(parsedValue); + if (val < 1) + throw std::invalid_argument("invalid value for [router]:min-connections"); + else + m_minConnectedRouters = val; } - if (key == "nickname") + + // additional check that min <= max + if (m_minConnectedRouters > m_maxConnectedRouters) + throw std::invalid_argument("[router]:min-connections must be less than [router]:max-connections"); + + // [router]:nickname + parsedValue = parser.getSingleSectionValue(values, "router", "nickname", false); + if (not parsedValue.empty()) { - m_nickname = str(val); - // set logger name here + m_nickname = str(parsedValue); + // TODO: side effect here, no side effects in config parsing!! LogContext::Instance().nodeName = nickname(); - LogInfo("nickname set"); - } - if (key == "encryption-privkey") - { - m_encryptionKeyfile = str(val); - LogDebug("encryption key set to ", m_encryptionKeyfile); - } - if (key == "contact-file") - { - m_ourRcFile = str(val); - LogDebug("rc file set to ", m_ourRcFile); - } - if (key == "transport-privkey") - { - m_transportKeyfile = str(val); - LogDebug("transport key set to ", m_transportKeyfile); } - if ((key == "identity-privkey" || key == "ident-privkey")) - { - m_identKeyfile = str(val); - LogDebug("identity key set to ", m_identKeyfile); - } - if (key == "public-address" || key == "public-ip") - { - llarp::LogInfo("public ip ", val, " size ", val.size()); - if (val.size() < 17) + + // [router]:encryption-privkey + parsedValue = parser.getSingleSectionValue(values, "router", "encryption-privkey", false); + if (not parsedValue.empty()) + m_encryptionKeyfile = str(parsedValue); + + // [router]:contact-file + parsedValue = parser.getSingleSectionValue(values, "router", "contact-file", false); + if (not parsedValue.empty()) + m_ourRcFile = str(parsedValue); + + // [router]:transport-privkey + parsedValue = parser.getSingleSectionValue(values, "router", "transport-privkey", false); + if (not parsedValue.empty()) + m_transportKeyfile = str(parsedValue); + + // [router]:identity-privkey OR + // [router]:ident-privkey + // apparently loki-launcher made its own config files at one point and typoed this, + // so we support both + parsedValue = parser.getSingleSectionValue(values, "router", "identity-privkey", false); + if (parsedValue.empty()) + parsedValue = parser.getSingleSectionValue(values, "router", "ident-privkey", false); + if (not parsedValue.empty()) + m_identKeyfile = str(parsedValue); + + // [router]:public-address OR + // [router]:public-ip + // apparently loki-launcher made its own config files at one point and typoed this, + // so we support both + parsedValue = parser.getSingleSectionValue(values, "router", "public-address", false); + if (parsedValue.empty()) + parsedValue = parser.getSingleSectionValue(values, "router", "public-ip", false); + if (not parsedValue.empty()) + { + llarp::LogInfo("public ip ", parsedValue, " size ", parsedValue.size()); + if(parsedValue.size() < 17) { // assume IPv4 - llarp::Addr a(val); + llarp::Addr a(parsedValue); llarp::LogInfo("setting public ipv4 ", a); m_addrInfo.ip = *a.addr6(); m_publicOverride = true; } } - if (key == "public-port") + + // [router]:public-port + parsedValue = parser.getSingleSectionValue(values, "router", "public-port", false); + if (not parsedValue.empty()) { - llarp::LogInfo("Setting public port ", val); - int p = svtoi(val); + llarp::LogInfo("Setting public port ", parsedValue); + int p = svtoi(parsedValue); // Not needed to flip upside-down - this is done in llarp::Addr(const // AddressInfo&) m_ip4addr.sin_port = p; m_addrInfo.port = p; m_publicOverride = true; } - if (key == "worker-threads" || key == "threads") + + // [router]:worker-threads OR + // [router]:threads + // apparently loki-launcher made its own config files at one point and typoed this, + // so we support both + parsedValue = parser.getSingleSectionValue(values, "router", "worker-threads", false); + if (parsedValue.empty()) + parsedValue = parser.getSingleSectionValue(values, "router", "threads", false); + if (not parsedValue.empty()) { - m_workerThreads = svtoi(val); - if (m_workerThreads <= 0) - { - LogWarn("worker threads invalid value: '", val, "' defaulting to 1"); - m_workerThreads = 1; - } + int val = svtoi(parsedValue); + if(val <= 0) + throw std::invalid_argument("invalid value for [router]:worker-threads"); else - { - LogDebug("set to use ", m_workerThreads, " worker threads"); - } + m_workerThreads = val; } - if (key == "net-threads") + + // [router]:public-port + parsedValue = parser.getSingleSectionValue(values, "router", "public-port", false); + if (not parsedValue.empty()) { - m_numNetThreads = svtoi(val); - if (m_numNetThreads <= 0) - { - LogWarn("net threads invalid value: '", val, "' defaulting to 1"); - m_numNetThreads = 1; - } + int val = svtoi(parsedValue); + if (val <= 0) + throw std::invalid_argument("invalid value for [router]:public-port"); else - { - LogDebug("set to use ", m_numNetThreads, " net threads"); - } + m_numNetThreads = val; } - if (key == "block-bogons") + + // [router]:block-bogons + parsedValue = parser.getSingleSectionValue(values, "router", "block-bogons", false); + if (not parsedValue.empty()) { - m_blockBogons = setOptBool(val); + auto val = setOptBool(parsedValue); + if (not val.has_value()) + throw std::invalid_argument("invalid value for [router]:block-bogons"); + else + m_blockBogons = val; } + + return true; } - void - NetworkConfig::fromSection(string_view key, string_view val) + bool + NetworkConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "profiling") + /* + if(key == "profiling") { m_enableProfiling = setOptBool(val); } @@ -240,21 +274,28 @@ namespace llarp { m_netConfig.emplace(str(key), str(val)); // str()'s here for gcc 5 compat } + */ + + return true; } - void - NetdbConfig::fromSection(string_view key, string_view val) + bool + NetdbConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "dir") + /* + if(key == "dir") { m_nodedbDir = str(val); } + */ + return true; } - void - DnsConfig::fromSection(string_view key, string_view val) + bool + DnsConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "upstream") + /* + if(key == "upstream") { llarp::LogInfo("add upstream resolver ", val); netConfig.emplace("upstream-dns", str(val)); // str() for gcc 5 compat @@ -264,11 +305,14 @@ namespace llarp llarp::LogInfo("set local dns to ", val); netConfig.emplace("local-dns", str(val)); // str() for gcc 5 compat } + */ + return true; } - void - LinksConfig::fromSection(string_view key, string_view val) + bool + LinksConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { + /* uint16_t proto = 0; std::unordered_set parsed_opts; @@ -317,33 +361,41 @@ namespace llarp // str() here for gcc 5 compat m_InboundLinks.emplace_back(str(key), AF_INET, proto, std::move(opts)); } + */ + return true; } - void - ConnectConfig::fromSection(string_view /*key*/, string_view val) + bool + ConnectConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - routers.emplace_back(val.begin(), val.end()); + // routers.emplace_back(val.begin(), val.end()); + return true; } - void - ServicesConfig::fromSection(string_view key, string_view val) + bool + ServicesConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - services.emplace_back(str(key), str(val)); // str()'s here for gcc 5 compat + // services.emplace_back(str(key), str(val)); // str()'s here for gcc 5 compat + return true; } - void - SystemConfig::fromSection(string_view key, string_view val) + bool + SystemConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "pidfile") + /* + if(key == "pidfile") { pidfile = str(val); } + */ + return true; } - void - ApiConfig::fromSection(string_view key, string_view val) + bool + ApiConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "enabled") + /* + if(key == "enabled") { m_enableRPCServer = IsTrueValue(val); } @@ -355,12 +407,15 @@ namespace llarp { // TODO: add pubkey to whitelist } + */ + return true; } - void - LokidConfig::fromSection(string_view key, string_view val) + bool + LokidConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "service-node-seed") + /* + if(key == "service-node-seed") { usingSNSeed = true; ident_keyfile = std::string{val}; @@ -381,21 +436,27 @@ namespace llarp { lokidRPCPassword = str(val); } + */ + return true; } - void - BootstrapConfig::fromSection(string_view key, string_view val) + bool + BootstrapConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "add-node") + /* + if(key == "add-node") { routers.emplace_back(val.begin(), val.end()); } + */ + return true; } - void - LoggingConfig::fromSection(string_view key, string_view val) + bool + LoggingConfig::parseSectionValues(const ConfigParser& parser, const SectionValues_t& values) { - if (key == "type" && val == "syslog") + /* + if(key == "type" && val == "syslog") { // TODO(despair): write event log syslog class #if defined(_WIN32) @@ -443,25 +504,26 @@ namespace llarp ::abort(); } } + */ + return true; } template < typename Section > Section find_section(const ConfigParser &parser, const std::string &name) { - Section ret; + Section section; - auto visitor = [&ret](const ConfigParser::Section_t& section) -> bool { - for (const auto& sec : section) - { - ret.fromSection(sec.first, sec.second); - } - return true; + auto visitor = [&](const ConfigParser::SectionValues_t& sectionValues) { + return section.parseSectionValues(parser, sectionValues); }; + // TODO: exceptions, please. fuck. + // parser.VisitSection just passes-through the return value of our + // lambda from above if(parser.VisitSection(name.c_str(), visitor)) { - return ret; + return section; } return {}; diff --git a/llarp/config/config.hpp b/llarp/config/config.hpp index a5d754350..6b106de15 100644 --- a/llarp/config/config.hpp +++ b/llarp/config/config.hpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -16,7 +17,8 @@ struct llarp_config; namespace llarp { - struct ConfigParser; + using SectionValues_t = llarp::ConfigParser::SectionValues_t; + using Config_impl_t = llarp::ConfigParser::Config_impl_t; inline const char* lokinetEnv(string_view suffix); @@ -88,8 +90,8 @@ namespace llarp nonstd::optional< bool > blockBogons() const { return fromEnv(m_blockBogons, "BLOCK_BOGONS"); } // clang-format on - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; class NetworkConfig @@ -111,8 +113,8 @@ namespace llarp const NetConfig& netConfig() const { return m_netConfig; } // clang-format on - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; class NetdbConfig @@ -125,16 +127,16 @@ namespace llarp std::string nodedbDir() const { return fromEnv(m_nodedbDir, "NODEDB_DIR"); } // clang-format on - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct DnsConfig { std::unordered_multimap netConfig; - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; class LinksConfig @@ -160,31 +162,31 @@ namespace llarp const Links& inboundLinks() const { return m_InboundLinks; } // clang-format on - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct ConnectConfig { std::vector routers; - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct ServicesConfig { - std::vector> services; - void - fromSection(string_view key, string_view val); + std::vector< std::pair< std::string, std::string > > services; + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct SystemConfig { std::string pidfile; - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; class ApiConfig @@ -199,8 +201,8 @@ namespace llarp std::string rpcBindAddr() const { return fromEnv(m_rpcBindAddr, "RPC_BIND_ADDR"); } // clang-format on - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct LokidConfig @@ -212,15 +214,15 @@ namespace llarp std::string lokidRPCUser; std::string lokidRPCPassword; - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct BootstrapConfig { - std::vector routers; - void - fromSection(string_view key, string_view val); + std::vector< std::string > routers; + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct LoggingConfig @@ -228,8 +230,8 @@ namespace llarp bool m_LogJSON = false; FILE* m_LogFile = stdout; - void - fromSection(string_view key, string_view val); + bool + parseSectionValues(const ConfigParser& parser, const SectionValues_t& values); }; struct Config diff --git a/llarp/config/ini.cpp b/llarp/config/ini.cpp index 0f6ef740e..1ce0a54be 100644 --- a/llarp/config/ini.cpp +++ b/llarp/config/ini.cpp @@ -1,11 +1,13 @@ #include #include +#include #include #include #include #include +#include namespace llarp { @@ -129,7 +131,7 @@ namespace llarp LogError(m_FileName, " invalid line (", lineno, "): '", line, "'"); return false; } - Section_t& sect = m_Config[str(sectName)]; + SectionValues_t& sect = m_Config[str(sectName)]; LogDebug(m_FileName, ": ", sectName, ".", k, "=", v); sect.emplace(str(k), str(v)); // str()'s here for gcc 5 compat } @@ -143,16 +145,56 @@ namespace llarp } void - ConfigParser::IterAll(std::function visit) + ConfigParser::IterAll( + std::function< void(string_view, const SectionValues_t&) > visit) { for (const auto& item : m_Config) visit(item.first, item.second); } + const std::string emptyStr = ""; + + const std::string& + ConfigParser::getSingleSectionValue( + const SectionValues_t& values, + const std::string& section, + const std::string& key, + bool required, + bool tolerateMultiples) const + { + // section is provided for clarity / sanity check + assert(m_Config.find(section) != m_Config.end()); + assert( & m_Config.find(section)->second == &values); + + const auto sectionItr = m_Config.find(section); + if (sectionItr == m_Config.end()) + throw std::invalid_argument(stringify("could not find section ", section)); + + const SectionValues_t& sectionValues = sectionItr->second; + + if (not tolerateMultiples and sectionValues.count(key) > 1) + throw std::invalid_argument(stringify("config [", section, "]:", key, " cannot have multiple values for ", key)); + + const auto itr = sectionValues.find(key); + if (itr == sectionValues.end()) { + if (required) + throw std::invalid_argument(stringify("config [", section, "]:", key, " is missing")); + else + return emptyStr; + } + + return itr->second; + } + bool ConfigParser::VisitSection( - const char* name, std::function visit) const + const char* name, + std::function< bool(const SectionValues_t& sect) > visit) const { + // m_Config is effectively: + // unordered_map< string, unordered_multimap< string, string >> + // in human terms: a map of of sections + // where a section is a multimap of k:v pairs auto itr = m_Config.find(name); if (itr == m_Config.end()) return false; diff --git a/llarp/config/ini.hpp b/llarp/config/ini.hpp index 0e611264d..d392df506 100644 --- a/llarp/config/ini.hpp +++ b/llarp/config/ini.hpp @@ -12,8 +12,8 @@ namespace llarp { struct ConfigParser { - using Section_t = std::unordered_multimap; - using Config_impl_t = std::unordered_map; + using SectionValues_t = std::unordered_multimap< std::string, std::string >; + using Config_impl_t = std::unordered_map< std::string, SectionValues_t >; /// clear parser void Clear(); @@ -32,12 +32,35 @@ namespace llarp /// iterate all sections and thier values void - IterAll(std::function visit); + IterAll(std::function< void(string_view, const SectionValues_t&) > visit); /// visit a section in config read only by name /// return false if no section or value propagated from visitor bool - VisitSection(const char* name, std::function visit) const; + VisitSection(const char* name, + std::function< bool(const SectionValues_t&) > visit) const; + + /// Obtain a section value for the given key, additionally imposing the + /// provided constraints. an `invalid_argument` will be thrown if the + /// constraints aren't met. + /// + /// The `section` parameter is redundant and added for readability, but a call to + /// m_Config[section] should result in the same object as `values`. + /// + /// @param values is the SectionValues map in which to search for values + /// @param section should correspond to INI section tag related to this config + /// @param key is the key to look up + /// @param bool constrains whether this key must exist + /// @param tolerateMultiples constrains whether multiples are allowed + /// @return the first matching entry if found or empty string if not found + /// @throws std::invalid_argument if constrants aren't met or if `section` is not found + const std::string& + getSingleSectionValue( + const SectionValues_t& values, + const std::string& section, + const std::string& key, + bool required, + bool tolerateMultiples = false) const; private: bool diff --git a/llarp/service/config.cpp b/llarp/service/config.cpp index 0bed10e3c..2c6750fbd 100644 --- a/llarp/service/config.cpp +++ b/llarp/service/config.cpp @@ -12,13 +12,14 @@ namespace llarp ConfigParser parser; if (!parser.LoadFile(fname)) return false; - parser.IterAll([&](string_view name, const ConfigParser::Section_t& section) { - Config::section_t values; - values.first.assign(name.begin(), name.end()); - for (const auto& item : section) - values.second.emplace_back(item.first, item.second); - services.emplace_back(values); - }); + parser.IterAll( + [&](string_view name, const ConfigParser::SectionValues_t& section) { + Config::section_t values; + values.first.assign(name.begin(), name.end()); + for(const auto& item : section) + values.second.emplace_back(item.first, item.second); + services.emplace_back(values); + }); return services.size() > 0; } diff --git a/test/config/test_llarp_config_ini.cpp b/test/config/test_llarp_config_ini.cpp index c78eee9fc..fdba409bc 100644 --- a/test/config/test_llarp_config_ini.cpp +++ b/test/config/test_llarp_config_ini.cpp @@ -20,7 +20,7 @@ TEST_F(TestINIParser, TestParseEmpty) TEST_F(TestINIParser, TestParseOneSection) { - llarp::ConfigParser::Section_t sect; + llarp::ConfigParser::SectionValues_t sect; // this is an anti pattern don't write this kind of code with configpaser auto assertVisit = [§](const auto& section) -> bool { sect = section; diff --git a/test/util/test_llarp_util_log_level.cpp b/test/util/test_llarp_util_log_level.cpp index 52cbdd05e..47332d6d4 100644 --- a/test/util/test_llarp_util_log_level.cpp +++ b/test/util/test_llarp_util_log_level.cpp @@ -63,36 +63,3 @@ TEST_F(LogLevelTest, TestLogLevelToString) EXPECT_EQ("???", LogLevelToString(llarp::eLogNone)); } -TEST_F(LogLevelTest, TestLoggingConfigSideEffects) -{ - // restore original runtime level when we're done - llarp::LogContext& logContext = llarp::LogContext::Instance(); - auto original = logContext.runtimeLevel; - - // LoggingConfig::fromSection updates the runtime level as it reads in conf - // values, so feed it values and ensure that the runtime level is updated - // appropriately - llarp::LoggingConfig config; - - config.fromSection("level", "Trace"); - EXPECT_EQ(llarp::eLogTrace, logContext.runtimeLevel); - - config.fromSection("level", "Debug"); - EXPECT_EQ(llarp::eLogDebug, logContext.runtimeLevel); - - config.fromSection("level", "Info"); - EXPECT_EQ(llarp::eLogInfo, logContext.runtimeLevel); - - config.fromSection("level", "Warn"); - EXPECT_EQ(llarp::eLogWarn, logContext.runtimeLevel); - - config.fromSection("level", "Error"); - EXPECT_EQ(llarp::eLogError, logContext.runtimeLevel); - - config.fromSection("level", "None"); - EXPECT_EQ(llarp::eLogNone, logContext.runtimeLevel); - - - SetLogLevel(original); -} -