From 287b6fd8bd92d7caefb3172f93c888efa0f15b76 Mon Sep 17 00:00:00 2001
From: Calle Wilund <calle@scylladb.com>
Date: Mon, 6 Nov 2017 09:53:05 +0000
Subject: [PATCH 1/4] config_file: Add optional "error_handler" to yaml parse
 functions

Allowing parse errors / unknown options to be ignored.
---
 utils/config_file.cc | 41 +++++++++++++++++++++++++++--------------
 utils/config_file.hh | 23 +++++++++++++++++++----
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/utils/config_file.cc b/utils/config_file.cc
index 855b1b2fa1..e61533f39d 100644
--- a/utils/config_file.cc
+++ b/utils/config_file.cc
@@ -241,13 +241,18 @@ utils::config_file::add_options(bpo::options_description_easy_init& init) {
     return init;
 }
 
-void utils::config_file::read_from_yaml(const sstring& yaml) {
-    read_from_yaml(yaml.c_str());
+void utils::config_file::read_from_yaml(const sstring& yaml, error_handler h) {
+    read_from_yaml(yaml.c_str(), std::move(h));
 }
 
-void utils::config_file::read_from_yaml(const char* yaml) {
+void utils::config_file::read_from_yaml(const char* yaml, error_handler h) {
     std::unordered_map<sstring, cfg_ref> values;
 
+    if (!h) {
+        h = [](auto & opt, auto & msg, auto) {
+            throw std::invalid_argument(msg + " : " + opt);
+        };
+    }
     /*
      * Note: this is not very "half-fault" tolerant. I.e. there could be
      * yaml syntax errors that origin handles and still sets the options
@@ -261,7 +266,8 @@ void utils::config_file::read_from_yaml(const char* yaml) {
 
         auto i = std::find_if(_cfgs.begin(), _cfgs.end(), [&label](const config_src& cfg) { return cfg.name() == label; });
         if (i == _cfgs.end()) {
-            throw std::invalid_argument("Unknown option " + label);
+            h(label, "Unknown option", stdx::nullopt);
+            continue;
         }
 
         config_src& cfg = *i;
@@ -272,7 +278,8 @@ void utils::config_file::read_from_yaml(const char* yaml) {
         }
         switch (cfg.status()) {
         case value_status::Invalid:
-            throw std::invalid_argument("Option " + label + "is not applicable");
+            h(label, "Option is not applicable", cfg.status());
+            continue;
         case value_status::Unused:
         default:
             break;
@@ -281,7 +288,13 @@ void utils::config_file::read_from_yaml(const char* yaml) {
             continue;
         }
         // Still, a syntax error is an error warning, not a fail
-        cfg.set_value(node.second);
+        try {
+            cfg.set_value(node.second);
+        } catch (std::exception& e) {
+            h(label, e.what(), cfg.status());
+        } catch (...) {
+            h(label, "Could not convert value", cfg.status());
+        }
     }
 }
 
@@ -305,19 +318,19 @@ utils::config_file::configs utils::config_file::unset_values() const {
     return res;
 }
 
-future<> utils::config_file::read_from_file(file f) {
-    return f.size().then([this, f](size_t s) {
-        return do_with(make_file_input_stream(f), [this, s](input_stream<char>& in) {
-            return in.read_exactly(s).then([this](temporary_buffer<char> buf) {
-               read_from_yaml(sstring(buf.begin(), buf.end()));
+future<> utils::config_file::read_from_file(file f, error_handler h) {
+    return f.size().then([this, f, h](size_t s) {
+        return do_with(make_file_input_stream(f), [this, s, h](input_stream<char>& in) {
+            return in.read_exactly(s).then([this, h](temporary_buffer<char> buf) {
+               read_from_yaml(sstring(buf.begin(), buf.end()), h);
             });
         });
     });
 }
 
-future<> utils::config_file::read_from_file(const sstring& filename) {
-    return open_file_dma(filename, open_flags::ro).then([this](file f) {
-       return read_from_file(std::move(f));
+future<> utils::config_file::read_from_file(const sstring& filename, error_handler h) {
+    return open_file_dma(filename, open_flags::ro).then([this, h](file f) {
+       return read_from_file(std::move(f), h);
     });
 }
 
diff --git a/utils/config_file.hh b/utils/config_file.hh
index ec4f82fc7e..15e7ce309a 100644
--- a/utils/config_file.hh
+++ b/utils/config_file.hh
@@ -143,10 +143,25 @@ public:
     boost::program_options::options_description_easy_init&
     add_options(boost::program_options::options_description_easy_init&);
 
-    void read_from_yaml(const sstring&);
-    void read_from_yaml(const char *);
-    future<> read_from_file(const sstring&);
-    future<> read_from_file(file);
+    /**
+     * Default behaviour for yaml parser is to throw on
+     * unknown stuff, invalid opts or conversion errors.
+     *
+     * Error handling function allows overriding this.
+     *
+     * error: <option name>, <message>, <optional value_status>
+     *
+     * The last arg, opt value_status will tell you the type of
+     * error occurred. If not set, the option found does not exist.
+     * If invalid, it is invalid. Otherwise, a parse error.
+     *
+     */
+    using error_handler = std::function<void(const sstring&, const sstring&, stdx::optional<value_status>)>;
+
+    void read_from_yaml(const sstring&, error_handler = {});
+    void read_from_yaml(const char *, error_handler = {});
+    future<> read_from_file(const sstring&, error_handler = {});
+    future<> read_from_file(file, error_handler = {});
 
     using configs = std::vector<cfg_ref>;
 

From f1dd698600e1528123194ce7fb4ab4566552cc63 Mon Sep 17 00:00:00 2001
From: Calle Wilund <calle@scylladb.com>
Date: Mon, 6 Nov 2017 09:54:09 +0000
Subject: [PATCH 2/4] main: Use error handler for config parse

Treat all errors as loggable errors/warnings. Preserving previous
behaviour.
---
 main.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/main.cc b/main.cc
index a3eb9fcf65..148584a814 100644
--- a/main.cc
+++ b/main.cc
@@ -101,7 +101,13 @@ read_config(bpo::variables_map& opts, db::config& cfg) {
         file = relative_conf_dir("scylla.yaml").string();
     }
     return check_direct_io_support(file).then([file, &cfg] {
-        return cfg.read_from_file(file);
+        return cfg.read_from_file(file, [](auto & opt, auto & msg, auto status) {
+            auto level = log_level::warn;
+            if (status.value_or(db::config::value_status::Invalid) != db::config::value_status::Invalid) {
+                level = log_level::error;
+            }
+            startlog.log(level, "{} : {}", msg, opt);
+        });
     }).handle_exception([file](auto ep) {
         startlog.error("Could not read configuration file {}: {}", file, ep);
         return make_exception_future<>(ep);

From 959d729428b74fd18f56d9407afc5824a63f8d23 Mon Sep 17 00:00:00 2001
From: Calle Wilund <calle@scylladb.com>
Date: Mon, 6 Nov 2017 09:54:46 +0000
Subject: [PATCH 3/4] config: Resurrect command line aliases that where lost

---
 db/config.cc | 11 +++++++++++
 db/config.hh |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/db/config.cc b/db/config.cc
index 912e2af2d4..01fdafe07e 100644
--- a/db/config.cc
+++ b/db/config.cc
@@ -112,6 +112,17 @@ void config_file::named_value<db::config::seed_provider_type,
 
 }
 
+boost::program_options::options_description_easy_init&
+db::config::add_options(boost::program_options::options_description_easy_init& init) {
+    config_file::add_options(init);
+
+    data_file_directories.add_command_line_option(init, "datadir", "alias for 'data-file-directories'");
+    rpc_port.add_command_line_option(init, "thrift-port", "alias for 'rpc-port'");
+    native_transport_port.add_command_line_option(init, "cql-port", "alias for 'native-transport-port'");
+
+    return init;
+}
+
 boost::filesystem::path db::config::get_conf_dir() {
     using namespace boost::filesystem;
 
diff --git a/db/config.hh b/db/config.hh
index 977876cc08..08371e68a3 100644
--- a/db/config.hh
+++ b/db/config.hh
@@ -726,6 +726,9 @@ public:
 
     seastar::logging_settings logging_settings(const boost::program_options::variables_map&) const;
 
+    boost::program_options::options_description_easy_init&
+    add_options(boost::program_options::options_description_easy_init&);
+
 private:
     template<typename T>
     struct log_legacy_value : public named_value<T, value_status::Used> {

From 21b2c2e3105671ee5739e5d0e277f5e14bdc42c3 Mon Sep 17 00:00:00 2001
From: Calle Wilund <calle@scylladb.com>
Date: Mon, 6 Nov 2017 09:56:27 +0000
Subject: [PATCH 4/4] config_test: Use error handler (ignore errors) + add
 error test

Fixes #2933

Uses handler on main test, ignoring the invalid option present.
Also adds test to verify error handling works as expected.
---
 tests/config_test.cc | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/tests/config_test.cc b/tests/config_test.cc
index 2935a10383..5d071587f8 100644
--- a/tests/config_test.cc
+++ b/tests/config_test.cc
@@ -826,7 +826,11 @@ inline std::basic_ostream<Args...> & operator<<(std::basic_ostream<Args...> & os
 SEASTAR_TEST_CASE(test_parse_yaml) {
     config cfg;
 
-    cfg.read_from_yaml(cassandra_conf);
+    cfg.read_from_yaml(cassandra_conf, [](auto& opt, auto& msg, auto status) {
+        if (status != config::value_status::Invalid) {
+            throw std::invalid_argument(msg + " : " + opt);
+        }
+    });
 
     BOOST_CHECK_EQUAL(cfg.cluster_name(), "Test Cluster");
     BOOST_CHECK_EQUAL(cfg.cluster_name.is_set(), true);
@@ -861,4 +865,34 @@ SEASTAR_TEST_CASE(test_parse_yaml) {
     return make_ready_future<>();
 }
 
+SEASTAR_TEST_CASE(test_parse_broken) {
+    config cfg;
 
+    bool ok = false;
+
+    // this should become an "unknown option" kind of error.
+    cfg.read_from_yaml(R"foo(bork_bnork:
+    apa
+    ko
+)foo", [&](auto& opt, auto& msg, auto status) {
+        if (!status) { // unknown
+            ok = true;
+        }
+    });
+
+    BOOST_REQUIRE(ok);
+
+    ok = false;
+
+    // this should be a value parsing error.
+    cfg.read_from_yaml(R"foo(commitlog_segment_size_in_mb: flaska
+)foo", [&](auto& opt, auto& msg, auto status) {
+        if (status && status != config::value_status::Invalid) { // option exists and is valid.
+            ok = true;
+        }
+    });
+
+    BOOST_REQUIRE(ok);
+
+    return make_ready_future<>();
+}