From a8345327d46502517df8a614a4325fe40353c161 Mon Sep 17 00:00:00 2001 From: Federico Di Pierro Date: Tue, 9 Apr 2024 12:28:13 +0200 Subject: [PATCH] chore(unit_tests,userspace/falco): throw an exception when included config file is not present. Signed-off-by: Federico Di Pierro --- falco.yaml | 5 +- unit_tests/falco/test_configuration.cpp | 62 +++++++------------ userspace/falco/app/actions/load_config.cpp | 8 +-- userspace/falco/configuration.cpp | 4 +- userspace/falco/configuration.h | 2 +- userspace/falco/yaml_helper.h | 67 +++++++-------------- 6 files changed, 49 insertions(+), 99 deletions(-) diff --git a/falco.yaml b/falco.yaml index 429ec71e..e71d35b8 100644 --- a/falco.yaml +++ b/falco.yaml @@ -125,11 +125,8 @@ # exactly in the order they are specified. # Therefore, loaded config files *can* override values from main config file. # Also, nested include is not allowed, ie: included config files won't be able to include other config files. -# When relative path is used, the config file is assumed to be in the same folder as the main config file. -# Otherwise, absolute path must be used. # -# Like for rules (below), specifying a folder will load all the configs files present in it -# in a lexicographical order. +# Like for 'rules_file', specifying a folder will load all the configs files present in it in a lexicographical order. configs_files: - /etc/falco/config.d diff --git a/unit_tests/falco/test_configuration.cpp b/unit_tests/falco/test_configuration.cpp index 2418b08c..89dd0a7c 100644 --- a/unit_tests/falco/test_configuration.cpp +++ b/unit_tests/falco/test_configuration.cpp @@ -38,7 +38,6 @@ static std::string sample_yaml = " - elem3\n"; static std::vector loaded_conf_files; -static std::vector loaded_conf_warnings; TEST(Configuration, configuration_exceptions) { @@ -139,7 +138,7 @@ TEST(Configuration, configuration_config_files_secondary_fail) outfile.close(); yaml_helper conf; - ASSERT_ANY_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_ANY_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); std::filesystem::remove("main.yaml"); std::filesystem::remove("conf_2.yaml"); @@ -186,11 +185,10 @@ TEST(Configuration, configuration_config_files_ok) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); // main + conf_2 + conf_3 ASSERT_EQ(loaded_conf_files.size(), 3); - ASSERT_EQ(loaded_conf_warnings.size(), 0); ASSERT_TRUE(conf.is_defined("foo")); ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); @@ -220,9 +218,8 @@ TEST(Configuration, configuration_config_files_ok) TEST(Configuration, configuration_config_files_relative_main) { /* - * Test that when main config file is in a different folder, - * other config files are searched relative to its folder, - * when they are specified as relative paths + * Test that relative path are treated as relative to cwd and not to main config folder, + * and that absolute includes are ok too. */ const auto temp_main = std::filesystem::temp_directory_path() / "main.yaml"; // So, conf_2 will be looked up in the same folder as main config file, @@ -259,13 +256,10 @@ TEST(Configuration, configuration_config_files_relative_main) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file(temp_main.string(), loaded_conf_files, loaded_conf_warnings)); + ASSERT_NO_THROW(conf.load_from_file(temp_main.string(), loaded_conf_files)); - // main + conf_3 - ASSERT_EQ(loaded_conf_files.size(), 2); - - // conf_2 gives warning - ASSERT_EQ(loaded_conf_warnings.size(), 1); + // main + conf_2 + conf_3 + ASSERT_EQ(loaded_conf_files.size(), 3); ASSERT_TRUE(conf.is_defined("foo")); ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); @@ -273,9 +267,11 @@ TEST(Configuration, configuration_config_files_relative_main) ASSERT_EQ(conf.get_scalar("base_value.id", 0), 1); ASSERT_TRUE(conf.is_defined("base_value.name")); ASSERT_EQ(conf.get_scalar("base_value.name", ""), "foo"); - ASSERT_FALSE(conf.is_defined("foo2")); // failed to include the config file since it is relative and is not under /tmp - ASSERT_FALSE(conf.is_defined("base_value_2")); // failed to include the config file since it is relative and is not under /tmp - ASSERT_TRUE(conf.is_defined("base_value_3.id")); // conf_3 was correctly included since it is absolute + ASSERT_TRUE(conf.is_defined("foo2")); + ASSERT_EQ(conf.get_scalar("foo2", ""), "bar2"); + ASSERT_TRUE(conf.is_defined("base_value_2")); + ASSERT_EQ(conf.get_scalar("base_value_2.id", 0), 2); + ASSERT_TRUE(conf.is_defined("base_value_3.id")); ASSERT_EQ(conf.get_scalar("base_value_3.id", 0), 3); std::filesystem::remove(temp_main.string()); @@ -315,11 +311,10 @@ TEST(Configuration, configuration_config_files_override) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); // main + conf_2 + conf_3 ASSERT_EQ(loaded_conf_files.size(), 3); - ASSERT_EQ(loaded_conf_warnings.size(), 0); ASSERT_TRUE(conf.is_defined("foo")); ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); @@ -339,7 +334,7 @@ TEST(Configuration, configuration_config_files_override) TEST(Configuration, configuration_config_files_unexistent) { - /* Test that including an unexistent file just skips it */ + /* Test that including an unexistent file throws an exception */ const std::string main_conf_yaml = yaml_helper::configs_key + ":\n" " - conf_5.yaml\n" @@ -352,18 +347,7 @@ TEST(Configuration, configuration_config_files_unexistent) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); - - // main - ASSERT_EQ(loaded_conf_files.size(), 1); - - // conf_5 unexistent - ASSERT_EQ(loaded_conf_warnings.size(), 1); - - ASSERT_TRUE(conf.is_defined("base_value.id")); - ASSERT_EQ(conf.get_scalar("base_value.id", 0), 1); - ASSERT_TRUE(conf.is_defined("base_value.name")); - ASSERT_EQ(conf.get_scalar("base_value.name", ""), "foo"); + ASSERT_ANY_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); std::filesystem::remove("main.yaml"); } @@ -391,11 +375,10 @@ TEST(Configuration, configuration_config_files_scalar_configs_files) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); // main + conf_2 ASSERT_EQ(loaded_conf_files.size(), 2); - ASSERT_EQ(loaded_conf_warnings.size(), 0); ASSERT_TRUE(conf.is_defined("foo")); ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); @@ -427,11 +410,10 @@ TEST(Configuration, configuration_config_files_empty_configs_files) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); // main ASSERT_EQ(loaded_conf_files.size(), 1); - ASSERT_EQ(loaded_conf_warnings.size(), 0); ASSERT_TRUE(conf.is_defined("foo")); ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); @@ -458,7 +440,7 @@ TEST(Configuration, configuration_config_files_self) outfile.close(); yaml_helper conf; - ASSERT_ANY_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_ANY_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); std::filesystem::remove("main.yaml"); } @@ -510,14 +492,12 @@ TEST(Configuration, configuration_config_files_directory) outfile.close(); yaml_helper conf; - ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files, loaded_conf_warnings)); + ASSERT_NO_THROW(conf.load_from_file("main.yaml", loaded_conf_files)); - // main + conf_2 + conf_3 + // main + conf_2 + conf_3. + // test/foo is not parsed. ASSERT_EQ(loaded_conf_files.size(), 3); - // Found a folder (test/foo) that will be skipped - ASSERT_EQ(loaded_conf_warnings.size(), 1); - ASSERT_TRUE(conf.is_defined("foo")); ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); ASSERT_TRUE(conf.is_defined("base_value.id")); diff --git a/userspace/falco/app/actions/load_config.cpp b/userspace/falco/app/actions/load_config.cpp index e8fa12c0..9f6efd05 100644 --- a/userspace/falco/app/actions/load_config.cpp +++ b/userspace/falco/app/actions/load_config.cpp @@ -32,12 +32,11 @@ falco::app::run_result falco::app::actions::load_config(const falco::app::state& // List of loaded conf files, ie: s.options.conf_filename // plus all the `configs_files` expanded list of configs. std::vector loaded_conf_files; - std::vector loaded_conf_warnings; try { if (!s.options.conf_filename.empty()) { - s.config->init(s.options.conf_filename, loaded_conf_files, loaded_conf_warnings, s.options.cmdline_config_options); + s.config->init(s.options.conf_filename, loaded_conf_files, s.options.cmdline_config_options); } else { @@ -66,11 +65,6 @@ falco::app::run_result falco::app::actions::load_config(const falco::app::state& { falco_logger::log(falco_logger::level::INFO, std::string(" ") + path + "\n"); } - - for (const auto &warn : loaded_conf_warnings) - { - falco_logger::log(falco_logger::level::WARNING, warn + "\n"); - } } s.config->m_buffered_outputs = !s.options.unbuffered_outputs; diff --git a/userspace/falco/configuration.cpp b/userspace/falco/configuration.cpp index 2a533751..61e632cc 100644 --- a/userspace/falco/configuration.cpp +++ b/userspace/falco/configuration.cpp @@ -91,11 +91,11 @@ void falco_configuration::init(const std::vector& cmdline_options) } void falco_configuration::init(const std::string& conf_filename, std::vector& loaded_conf_files, - std::vector& loaded_conf_warnings, const std::vector &cmdline_options) + const std::vector &cmdline_options) { try { - config.load_from_file(conf_filename, loaded_conf_files, loaded_conf_warnings); + config.load_from_file(conf_filename, loaded_conf_files); } catch(const std::exception& e) { diff --git a/userspace/falco/configuration.h b/userspace/falco/configuration.h index 97d5341b..2eaab095 100644 --- a/userspace/falco/configuration.h +++ b/userspace/falco/configuration.h @@ -86,7 +86,7 @@ public: falco_configuration(); virtual ~falco_configuration() = default; - void init(const std::string& conf_filename, std::vector& loaded_conf_files, std::vector& loaded_conf_warnings, const std::vector& cmdline_options); + void init(const std::string& conf_filename, std::vector& loaded_conf_files, const std::vector& cmdline_options); void init(const std::vector& cmdline_options); std::string dump(); diff --git a/userspace/falco/yaml_helper.h b/userspace/falco/yaml_helper.h index 1f245030..8836302a 100644 --- a/userspace/falco/yaml_helper.h +++ b/userspace/falco/yaml_helper.h @@ -92,70 +92,49 @@ public: /** * Load the YAML document from the given file path. */ - void load_from_file(const std::string& path, std::vector& loaded_config_files, std::vector& loaded_config_warnings) + void load_from_file(const std::string& path, std::vector& loaded_config_files) { loaded_config_files.clear(); - loaded_config_warnings.clear(); m_root = load_from_file_int(path, loaded_config_files); const auto ppath = std::filesystem::path(path); - const auto config_folder = ppath.parent_path(); // Parse files to be included std::vector include_files; get_sequence>(include_files, configs_key); for(const std::string& include_file : include_files) { - // If user specifies a relative include file, - // make it relative to main config file folder, - // instead of cwd. auto include_file_path = std::filesystem::path(include_file); - if (!include_file_path.is_absolute()) - { - include_file_path = config_folder / include_file; - } if (include_file_path == ppath) { throw std::runtime_error( "Config error: '" + configs_key + "' directive tried to recursively include main config file: " + path + "."); } - if (std::filesystem::exists(include_file_path)) + if (!std::filesystem::exists(include_file_path)) { - if (std::filesystem::is_regular_file(include_file_path)) - { - include_config_file(include_file_path.string(), loaded_config_files); - } - else if (std::filesystem::is_directory(include_file_path)) - { - std::vector v; - const auto it_options = std::filesystem::directory_options::follow_directory_symlink - | std::filesystem::directory_options::skip_permission_denied; - for (auto const& dir_entry : std::filesystem::directory_iterator(include_file_path, it_options)) - { - if (std::filesystem::is_regular_file(dir_entry.path())) - { - v.push_back(dir_entry.path().string()); - } - // We don't support nested directories - else - { - loaded_config_warnings.push_back("Included config file has wrong type: " + dir_entry.path().string()); - } - } - std::sort(v.begin(), v.end()); - for (const auto &f : v) - { - include_config_file(f, loaded_config_files); - } - } - else - { - loaded_config_warnings.push_back("Included config entry has wrong type: " + include_file_path.string()); - } + throw std::runtime_error("Included config entry not existent: " + include_file_path.string()); } - else + if (std::filesystem::is_regular_file(include_file_path)) { - loaded_config_warnings.push_back("Included config entry unexistent: " + include_file_path.string()); + include_config_file(include_file_path.string(), loaded_config_files); + } + else if (std::filesystem::is_directory(include_file_path)) + { + std::vector v; + const auto it_options = std::filesystem::directory_options::follow_directory_symlink + | std::filesystem::directory_options::skip_permission_denied; + for (auto const& dir_entry : std::filesystem::directory_iterator(include_file_path, it_options)) + { + if (std::filesystem::is_regular_file(dir_entry.path())) + { + v.push_back(dir_entry.path().string()); + } + } + std::sort(v.begin(), v.end()); + for (const auto &f : v) + { + include_config_file(f, loaded_config_files); + } } } }