From 8112f6210b751f77a8bf6f0847042c855071560c Mon Sep 17 00:00:00 2001 From: Federico Di Pierro Date: Fri, 19 Jan 2024 09:23:18 +0100 Subject: [PATCH] chore(userspace,unit_tests): enable override of main config from secondary config files. Moreover, do not trigger an exception when an included file is not present; just print a warning. Finally, add more tests. Signed-off-by: Federico Di Pierro --- unit_tests/falco/test_configuration.cpp | 58 ++++++++++++++++++++++--- userspace/falco/yaml_helper.h | 49 ++++++++++----------- 2 files changed, 75 insertions(+), 32 deletions(-) diff --git a/unit_tests/falco/test_configuration.cpp b/unit_tests/falco/test_configuration.cpp index 13f80c43..f65c70aa 100644 --- a/unit_tests/falco/test_configuration.cpp +++ b/unit_tests/falco/test_configuration.cpp @@ -185,7 +185,38 @@ TEST(Configuration, configuration_include_files) ASSERT_TRUE(conf.is_defined("base_value_3.name")); ASSERT_EQ(conf.get_scalar("base_value_3.name", ""), "foo3"); - /* Test that included config files are not able to override configs from main file */ + /* + * 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 + */ + const auto temp_main = std::filesystem::temp_directory_path() / "main.yaml"; + const std::string main_conf_yaml_relative_absolute = + "includes:\n" + " - conf_2.yaml\n" + " - " + std::filesystem::current_path().string() + "/conf_3.yaml\n" + "foo: bar\n" + "base_value:\n" + " id: 1\n" + " name: foo\n"; + outfile.open(temp_main.string()); + outfile << main_conf_yaml_relative_absolute; + outfile.close(); + + ASSERT_NO_THROW(conf.load_from_file(temp_main.string())); + + ASSERT_TRUE(conf.is_defined("foo")); + ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); + 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_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_EQ(conf.get_scalar("base_value_3.id", 0), 3); + + /* Test that included config files are able to override configs from main file */ const std::string conf_yaml_3_override = "base_value:\n" " id: 3\n"; @@ -193,9 +224,19 @@ TEST(Configuration, configuration_include_files) outfile << conf_yaml_3_override; outfile.close(); - ASSERT_ANY_THROW(conf.load_from_file("main.yaml")); + ASSERT_NO_THROW(conf.load_from_file("main.yaml")); + ASSERT_TRUE(conf.is_defined("foo")); + ASSERT_EQ(conf.get_scalar("foo", ""), "bar"); + ASSERT_TRUE(conf.is_defined("base_value.id")); + ASSERT_EQ(conf.get_scalar("base_value.id", 0), 3); // overridden! + ASSERT_FALSE(conf.is_defined("base_value.name")); // no more present since entire `base_value` block was overridden + ASSERT_TRUE(conf.is_defined("foo2")); + ASSERT_EQ(conf.get_scalar("foo2", ""), "bar2"); + ASSERT_TRUE(conf.is_defined("base_value_2.id")); + ASSERT_EQ(conf.get_scalar("base_value_2.id", 0), 2); + ASSERT_FALSE(conf.is_defined("base_value_3.id")); // not defined - /* Test that including an unexistent file triggers an exception */ + /* Test that including an unexistent file just skips it */ const std::string main_conf_unexistent_yaml = "includes:\n" " - conf_5.yaml\n" @@ -207,9 +248,13 @@ TEST(Configuration, configuration_include_files) outfile << main_conf_unexistent_yaml; outfile.close(); - ASSERT_ANY_THROW(conf.load_from_file("main.yaml")); + ASSERT_NO_THROW(conf.load_from_file("main.yaml")); + 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"); - /* Test that a single file can be included as a scalar (thanks to get_sequence_from_node magic)*/ + /* Test that a single file can be included as a scalar (thanks to get_sequence_from_node magic) */ const std::string main_conf_yaml_no_list = "includes: conf_2.yaml\n" "foo: bar\n" @@ -232,12 +277,13 @@ TEST(Configuration, configuration_include_files) ASSERT_EQ(conf.get_scalar("foo2", ""), "bar2"); ASSERT_TRUE(conf.is_defined("base_value_2.id")); ASSERT_EQ(conf.get_scalar("base_value_2.id", 0), 2); - + // Cleanup everything std::filesystem::remove("main.yaml"); std::filesystem::remove("conf_2.yaml"); std::filesystem::remove("conf_3.yaml"); std::filesystem::remove("conf_4.yaml"); + std::filesystem::remove(temp_main.string()); } TEST(Configuration, configuration_environment_variables) diff --git a/userspace/falco/yaml_helper.h b/userspace/falco/yaml_helper.h index 9b0d9b99..a21a2462 100644 --- a/userspace/falco/yaml_helper.h +++ b/userspace/falco/yaml_helper.h @@ -95,7 +95,7 @@ public: m_root = load_from_file_int(path); const auto ppath = std::filesystem::path(path); - const auto config_folder = ppath.std::filesystem::path::parent_path(); + const auto config_folder = ppath.parent_path(); // Parse files to be included std::vector include_files; get_sequence>(include_files, "includes"); @@ -107,36 +107,33 @@ public: auto include_file_path = std::filesystem::path(include_file); if (!include_file_path.is_absolute()) { - include_file_path = config_folder; - include_file_path += include_file; + include_file_path = config_folder / include_file; } - auto loaded_nodes = load_from_file_int(include_file_path.string()); - for(auto n : loaded_nodes) + if (std::filesystem::exists(include_file_path) && std::filesystem::is_regular_file(include_file_path)) { - /* - * To avoid recursion hell, - * we don't support `includes` directives from included config files - * (that use load_from_file_int recursively). - */ - const auto &key = n.first.Scalar(); - if (key == "includes") + auto loaded_nodes = load_from_file_int(include_file_path.string()); + for(auto n : loaded_nodes) { - throw std::runtime_error("Config error: 'includes' directive in included config file " + include_file + "."); - } - - YAML::Node node; - get_node(node, key); - if (!node.IsDefined()) - { - // There was no such node in root config file; proceed. - node = n.second; - } - else - { - throw std::runtime_error("Config error: included config files cannot override root config nodes: " - + include_file + " tried to override '" + key + "'."); + /* + * To avoid recursion hell, + * we don't support `includes` directives from included config files + * (that use load_from_file_int recursively). + */ + const auto &key = n.first.Scalar(); + if (key == "includes") + { + throw std::runtime_error("Config error: 'includes' directive in included config file " + include_file + "."); + } + // We allow to override keys. + // We don't need to use `get_node()` here, + // since key is a top-level one. + m_root[key] = n.second; } } + else + { + falco_logger::log(falco_logger::level::WARNING, "Included config file unexistent or wrong type: " + include_file_path.string()); + } } }