From c8ce78315dbc0ad5ab3b2ad625e7d037ca843c30 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Tue, 15 Nov 2022 13:11:11 -0400 Subject: [PATCH 1/3] Fix missing option names At some point between 0.9.9 and 0.9.10 we removed the printing of option names when a value doesn't have a default, but this means the config is littered with things like: # This option sets the greater foo value. with no actual option name printed out when there is no default. This fixes it by always printing the option name in such a case, just with an empty value, e.g.: # This option sets the greater foo value. #big-foo= --- llarp/config/definition.cpp | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/llarp/config/definition.cpp b/llarp/config/definition.cpp index aa199ebea..76177603c 100644 --- a/llarp/config/definition.cpp +++ b/llarp/config/definition.cpp @@ -152,10 +152,8 @@ namespace llarp const std::string& section, std::vector comments) { auto& sectionComments = m_sectionComments[section]; - for (size_t i = 0; i < comments.size(); ++i) - { - sectionComments.emplace_back(std::move(comments[i])); - } + for (auto& c : comments) + sectionComments.emplace_back(std::move(c)); } void @@ -198,13 +196,22 @@ namespace llarp if (useValues and def->getNumberFound() > 0) { for (const auto& val : def->valuesAsString()) - fmt::format_to(sect_append, "\n{}={}\n", name, val); + fmt::format_to(sect_append, "\n{}={}", name, val); + *sect_append = '\n'; } - else if (not(def->hidden and not has_comment)) + else if (not def->hidden) { - for (const auto& val : def->defaultValuesAsString()) - fmt::format_to(sect_append, "\n{}{}={}\n", def->required ? "" : "#", name, val); + if (auto defaults = def->defaultValuesAsString(); not defaults.empty()) + for (const auto& val : defaults) + fmt::format_to(sect_append, "\n{}{}={}", def->required ? "" : "#", name, val); + else + // We have no defaults so we append it as "#opt-name=" so that we show the option name, + // and make it simple to uncomment and edit to the desired value. + fmt::format_to(sect_append, "\n#{}=", name); + *sect_append = '\n'; } + else if (has_comment) + *sect_append = '\n'; }); if (sect_str.empty()) From 68bb74a95dc2d060b4a56b605b1161ff468aef50 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Tue, 15 Nov 2022 13:14:15 -0400 Subject: [PATCH 2/3] Make [lokid]:rpc setting required in SN mode When running as a service node we can't do anything without a lokid rpc URL, and we don't necessarily have a good default for it. This makes it required so that we fail with an appropriate error message (rather than connect timeouts) if it is not specified. --- llarp/config/config.cpp | 1 + test/config/test_llarp_config_values.cpp | 49 +++++++++++------------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/llarp/config/config.cpp b/llarp/config/config.cpp index aa02da5c2..67d92fa2f 100644 --- a/llarp/config/config.cpp +++ b/llarp/config/config.cpp @@ -1179,6 +1179,7 @@ namespace llarp "lokid", "rpc", RelayOnly, + Required, Comment{ "oxenmq control address for for communicating with oxend. Depends on oxend's", "lmq-local-control configuration option. By default this value should be", diff --git a/test/config/test_llarp_config_values.cpp b/test/config/test_llarp_config_values.cpp index 8276cc59c..f0e9ebe17 100644 --- a/test/config/test_llarp_config_values.cpp +++ b/test/config/test_llarp_config_values.cpp @@ -75,6 +75,8 @@ run_config_test(mocks::Network env, std::string_view ini_str) throw std::runtime_error{"non zero return"}; } +const std::string ini_minimal = "[lokid]\nrpc=ipc://dummy\n"; + TEST_CASE("service node bind section on valid network", "[config]") { std::unordered_multimap env{ @@ -92,15 +94,14 @@ TEST_CASE("service node bind section on valid network", "[config]") REQUIRE(not mock_net.IsBogon(*maybe_addr)); } - SECTION("empty config") + SECTION("minimal config") { - std::string_view ini_str = ""; - REQUIRE_NOTHROW(run_config_test(env, ini_str)); + REQUIRE_NOTHROW(run_config_test(env, ini_minimal)); } SECTION("explicit bind via ifname") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] mock0=443 )"; @@ -108,7 +109,7 @@ mock0=443 } SECTION("explicit bind via ip address") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] inbound=1.1.1.1:443 )"; @@ -116,7 +117,7 @@ inbound=1.1.1.1:443 } SECTION("explicit bind via ip address with old syntax") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] 1.1.1.1=443 )"; @@ -125,7 +126,7 @@ inbound=1.1.1.1:443 } SECTION("ip spoof fails") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=8.8.8.8 public-port=443 @@ -136,7 +137,7 @@ inbound=1.1.1.1:443 } SECTION("explicit bind via ifname but fails from non existing ifname") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] ligma0=443 )"; @@ -145,7 +146,7 @@ ligma0=443 SECTION("explicit bind via ifname but fails from using loopback") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] lo=443 )"; @@ -154,7 +155,7 @@ lo=443 SECTION("explicit bind via explicit loopback") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] inbound=127.0.0.1:443 )"; @@ -162,7 +163,7 @@ inbound=127.0.0.1:443 } SECTION("public ip provided but no bind section") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -171,7 +172,7 @@ public-port=443 } SECTION("public ip provided with ip in bind section") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -196,7 +197,7 @@ TEST_CASE("service node bind section on nat network", "[config]") SECTION("public ip provided via inbound directive") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -209,7 +210,7 @@ inbound=10.1.1.1:443 SECTION("public ip provided with bind via ifname") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -222,7 +223,7 @@ mock0=443 SECTION("public ip provided bind via wildcard ip") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -232,7 +233,6 @@ inbound=0.0.0.0:443 )"; REQUIRE_THROWS(run_config_test(env, ini_str)); } - } TEST_CASE("service node bind section with multiple public ip", "[config]") @@ -242,14 +242,9 @@ TEST_CASE("service node bind section with multiple public ip", "[config]") {"mock0", llarp::IPRange::FromIPv4(2, 1, 1, 1, 32)}, {"lo", llarp::IPRange::FromIPv4(127, 0, 0, 1, 8)}, }; - SECTION("empty config") - { - std::string_view ini_str = ""; - REQUIRE_NOTHROW(run_config_test(env, ini_str)); - } SECTION("with old style wildcard for inbound and no public ip, fails") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] 0.0.0.0=443 )"; @@ -257,7 +252,7 @@ TEST_CASE("service node bind section with multiple public ip", "[config]") } SECTION("with old style wildcard for outbound") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] *=1443 )"; @@ -265,7 +260,7 @@ TEST_CASE("service node bind section with multiple public ip", "[config]") } SECTION("with wildcard via inbound directive no public ip given, fails") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] inbound=0.0.0.0:443 )"; @@ -274,7 +269,7 @@ inbound=0.0.0.0:443 } SECTION("with wildcard via inbound directive primary public ip given") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -286,7 +281,7 @@ inbound=0.0.0.0:443 } SECTION("with wildcard via inbound directive secondary public ip given") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=2.1.1.1 public-port=443 @@ -298,7 +293,7 @@ inbound=0.0.0.0:443 } SECTION("with bind via interface name") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] mock0=443 )"; From f9db657f64c069f54397f52951e0ddf7ececcb67 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Tue, 15 Nov 2022 13:15:54 -0400 Subject: [PATCH 3/3] Make Default&Required or Required&Hidden compilation failures Default & Required makes no sense: if we have a default it makes no sense to make it required. The previous behaviour when this was specified was to force an (uncommented) value in the config with the value, but this was only used in the test suite. Required & Hidden makes no sense either: if it's required to be specified we definitely don't want to hide it from the generated config file. These are now compile-time failures. --- llarp/config/definition.hpp | 10 +++ test/config/test_llarp_config_definition.cpp | 11 ++-- test/config/test_llarp_config_output.cpp | 68 +++++++++++--------- 3 files changed, 53 insertions(+), 36 deletions(-) diff --git a/llarp/config/definition.hpp b/llarp/config/definition.hpp index 8f9225c28..6cacd3180 100644 --- a/llarp/config/definition.hpp +++ b/llarp/config/definition.hpp @@ -202,6 +202,16 @@ namespace llarp OptionDefinition(std::string section_, std::string name_, Options&&... opts) : OptionDefinitionBase(section_, name_, opts...) { + constexpr bool has_default = + ((config::is_default_array || config::is_default) || ...); + constexpr bool has_required = + (std::is_same_v, config::Required_t> || ...); + constexpr bool has_hidden = + (std::is_same_v, config::Hidden_t> || ...); + static_assert( + not(has_default and has_required), "Default{...} and Required are mutually exclusive"); + static_assert(not(has_hidden and has_required), "Hidden and Required are mutually exclusive"); + (extractDefault(std::forward(opts)), ...); (extractAcceptor(std::forward(opts)), ...); (extractComments(std::forward(opts)), ...); diff --git a/test/config/test_llarp_config_definition.cpp b/test/config/test_llarp_config_definition.cpp index e3edec29c..80005d58c 100644 --- a/test/config/test_llarp_config_definition.cpp +++ b/test/config/test_llarp_config_definition.cpp @@ -104,7 +104,7 @@ TEST_CASE("OptionDefinition acceptor throws test", "[config]") TEST_CASE("OptionDefinition tryAccept missing option test", "[config]") { int unset = -1; - llarp::OptionDefinition def("foo", "bar", Required, Default{1}, [&](int arg) { + llarp::OptionDefinition def("foo", "bar", Required, [&](int arg) { (void)arg; unset = 0; // should never be called }); @@ -170,7 +170,6 @@ TEST_CASE("ConfigDefinition required test", "[config]") config.defineOption(std::make_unique>( "router", "threads", - Default{1}, Required)); CHECK_THROWS(config.validateRequiredFields()); @@ -186,13 +185,13 @@ TEST_CASE("ConfigDefinition section test", "[config]") config.defineOption(std::make_unique>( "foo", "bar", - Required, - Default{1})); + Required + )); config.defineOption(std::make_unique>( "goo", "bar", - Required, - Default{1})); + Required + )); CHECK_THROWS(config.validateRequiredFields()); diff --git a/test/config/test_llarp_config_output.cpp b/test/config/test_llarp_config_output.cpp index 13eb64c72..5ba47bf21 100644 --- a/test/config/test_llarp_config_output.cpp +++ b/test/config/test_llarp_config_output.cpp @@ -8,37 +8,37 @@ TEST_CASE("ConfigDefinition simple generate test", "[config]") { llarp::ConfigDefinition config{true}; - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Required); config.defineOption("foo", "baz", Default{2}); - config.defineOption(std::make_unique>( - "foo", "quux", Required, Default{"hello"})); + config.defineOption( + std::make_unique>("foo", "quux", Default{"hello"})); - config.defineOption("argle", "bar", RelayOnly, Required, Default{3}); + config.defineOption("argle", "bar", RelayOnly, Required); config.defineOption("argle", "baz", Default{4}); - config.defineOption("argle", "quux", Required, Default{"the quick brown fox"}); + config.defineOption("argle", "quux", Default{"the quick brown fox"}); - config.defineOption("not", "for-routers", ClientOnly, Required, Default{1}); + config.defineOption("not", "for-routers", ClientOnly, Default{1}); std::string output = config.generateINIConfig(); CHECK(output == R"raw([foo] -bar=1 +#bar= #baz=2 -quux=hello +#quux=hello [argle] -bar=3 +#bar= #baz=4 -quux=the quick brown fox +#quux=the quick brown fox )raw"); } @@ -46,16 +46,24 @@ TEST_CASE("ConfigDefinition useValue test", "[config]") { llarp::ConfigDefinition config{true}; - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); - constexpr auto expected = "[foo]\n\n\nbar=1\n"; + constexpr auto expected = R"raw([foo] + + +#bar=1 +)raw"; CHECK(config.generateINIConfig(false) == expected); CHECK(config.generateINIConfig(true) == expected); config.addConfigValue("foo", "bar", "2"); - constexpr auto expectedWhenValueProvided = "[foo]\n\n\nbar=2\n"; + constexpr auto expectedWhenValueProvided = R"raw([foo] + + +bar=2 +)raw"; CHECK(config.generateINIConfig(false) == expected); CHECK(config.generateINIConfig(true) == expectedWhenValueProvided); @@ -67,8 +75,7 @@ TEST_CASE("ConfigDefinition section comments test") config.addSectionComments("foo", {"test comment"}); config.addSectionComments("foo", {"test comment 2"}); - config.defineOption(std::make_unique>( - "foo", "bar", Required, Default{1})); + config.defineOption(std::make_unique>("foo", "bar", Default{1})); std::string output = config.generateINIConfig(); @@ -77,7 +84,7 @@ TEST_CASE("ConfigDefinition section comments test") # test comment 2 -bar=1 +#bar=1 )raw"); } @@ -87,18 +94,21 @@ TEST_CASE("ConfigDefinition option comments test") config.addOptionComments("foo", "bar", {"test comment 1"}); config.addOptionComments("foo", "bar", {"test comment 2"}); - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); - config.defineOption("foo", "far", Default{"abc"}, + config.defineOption( + "foo", + "far", + Default{"abc"}, Comment{ - "Fill in the missing values:", - "___defg", + "Fill in the missing values:", + "___defg", }); config.defineOption("client", "omg", ClientOnly, Default{1}, Comment{"hi"}); config.defineOption("relay", "ftw", RelayOnly, Default{1}, Comment{"bye"}); - // has comment, so still gets shown. + // has comment, but is hidden: we show only the comment but not the value/default. config.defineOption("foo", "old-bar", Hidden, Default{456}); config.addOptionComments("foo", "old-bar", {"old bar option"}); @@ -112,14 +122,13 @@ TEST_CASE("ConfigDefinition option comments test") # test comment 1 # test comment 2 -bar=1 +#bar=1 # Fill in the missing values: # ___defg #far=abc # old bar option -#old-bar=456 [relay] @@ -139,7 +148,7 @@ TEST_CASE("ConfigDefinition empty comments test") config.addOptionComments("foo", "bar", {"option comment"}); config.addOptionComments("foo", "bar", {""}); - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); std::string output = config.generateINIConfig(); @@ -150,7 +159,7 @@ TEST_CASE("ConfigDefinition empty comments test") # option comment # -bar=1 +#bar=1 )raw"); } @@ -161,10 +170,10 @@ TEST_CASE("ConfigDefinition multi option comments") config.addSectionComments("foo", {"foo section comment"}); config.addOptionComments("foo", "bar", {"foo bar option comment"}); - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); config.addOptionComments("foo", "baz", {"foo baz option comment"}); - config.defineOption("foo", "baz", Required, Default{1}); + config.defineOption("foo", "baz", Default{1}); std::string output = config.generateINIConfig(); @@ -173,10 +182,9 @@ TEST_CASE("ConfigDefinition multi option comments") # foo bar option comment -bar=1 +#bar=1 # foo baz option comment -baz=1 +#baz=1 )raw"); } -