update(rule_loader): deprecate append key and add a warning

Signed-off-by: Andrea Terzolo <[email protected]>

unit_tests/engine/test_rule_loader.cpp

@@ -43,6 +43,25 @@ class engine_loader_test : public ::testing::Test {
 		return ret;
 	}
 
+	bool check_warning_message(std::string warning_msg)
+	{
+		if(!m_load_result->has_warnings())
+		{
+			return false;
+		}
+
+		for(auto &warn : m_load_result_json["warnings"])
+		{
+			std::string msg = warn["message"];
+			if(msg.find(warning_msg) != std::string::npos)
+			{
+				return true;
+			}
+		}
+
+		return false;
+	}
+
 	std::string m_sample_ruleset;
 	std::string m_sample_source;
 	sinsp_filter_check_list m_filterlist;
@@ -134,6 +153,9 @@ TEST_F(engine_loader_test, rule_override_append)
 	std::string rule_name = "legit_rule";
 	ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string;
 
+	// Here we don't use the deprecated `append` flag, so we don't expect the warning.
+	ASSERT_FALSE(check_warning_message(WARNING_APPEND_MESSAGE));
+
 	auto rule_description = m_engine->describe_rule(&rule_name, {});
 	ASSERT_EQ(rule_description["rules"][0]["info"]["condition"].template get<std::string>(),
 	 	"evt.type=open and proc.name = cat");
@@ -163,6 +185,9 @@ TEST_F(engine_loader_test, rule_append)
 	std::string rule_name = "legit_rule";
 	ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string;
 
+	// We should have at least one warning because the 'append' flag is deprecated.
+	ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE));
+
 	auto rule_description = m_engine->describe_rule(&rule_name, {});
 	ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get<std::string>(),
 	 	"(evt.type = open and proc.name = cat)");
@@ -283,6 +308,10 @@ TEST_F(engine_loader_test, rule_incorrect_append_override)
 	std::string rule_name = "failing_rule";
 
 	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
+
+	// We should have at least one warning because the 'append' flag is deprecated.
+	ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE));
+
 	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find(OVERRIDE_APPEND_ERROR_MESSAGE) != std::string::npos);
 }
 

userspace/engine/falco_load_result.cpp

@@ -66,7 +66,8 @@ static const std::string warning_codes[] = {
 	"LOAD_UNKNOWN_FILTER",
 	"LOAD_UNUSED_MACRO",
 	"LOAD_UNUSED_LIST",
-	"LOAD_UNKNOWN_ITEM"
+	"LOAD_UNKNOWN_ITEM",
+	"LOAD_DEPRECATED_ITEM"
 };
 
 const std::string& falco::load_result::warning_code_str(warning_code wc)
@@ -81,7 +82,8 @@ static const std::string warning_strings[] = {
 	"Unknown field or event-type in condition or output",
 	"Unused macro",
 	"Unused list",
-	"Unknown rules file item"
+	"Unknown rules file item",
+	"Used deprecated item"
 };
 
 const std::string& falco::load_result::warning_str(warning_code wc)
@@ -96,7 +98,8 @@ static const std::string warning_descs[] = {
 	"A rule condition or output refers to a field or evt.type that does not exist. This is normally an error, but if a rule has a skip-if-unknown-filter property, the error is downgraded to a warning.",
 	"A macro is defined in the rules content but is not used by any other macro or rule.",
 	"A list is defined in the rules content but is not used by any other list, macro, or rule.",
-	"An unknown top-level object is in the rules content. It will be ignored."
+	"An unknown top-level object is in the rules content. It will be ignored.",
+	"A deprecated item is employed by lists, macros, or rules."
 };
 
 const std::string& falco::load_result::warning_desc(warning_code wc)

userspace/engine/falco_load_result.h

@@ -54,7 +54,8 @@ class load_result {
 		LOAD_UNKNOWN_FILTER,
 		LOAD_UNUSED_MACRO,
 		LOAD_UNUSED_LIST,
-		LOAD_UNKNOWN_ITEM
+		LOAD_UNKNOWN_ITEM,
+		LOAD_DEPRECATED_ITEM
 	};
 
 	virtual ~load_result() = default;

userspace/engine/rule_loader_reader.cpp

@@ -434,27 +434,28 @@ static void read_item(
 		rule_loader::context ctx(item, rule_loader::context::LIST, name, parent);
 		rule_loader::list_info v(ctx);
 
-		bool append = false;
+		bool has_append_flag = false;
 		decode_val(item, "list", v.name, ctx);
 		decode_items(item, v.items, ctx);
 
-		decode_optional_val(item, "append", append, ctx);
-
+		decode_optional_val(item, "append", has_append_flag, ctx);
+		if(has_append_flag)
+		{
+			cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx);
+		}
+
 		std::set<std::string> override_append, override_replace;
 		std::set<std::string> overridable {"items"};
 		decode_overrides(item, overridable, overridable, override_append, override_replace, ctx);
 		bool has_overrides = !override_append.empty() || !override_replace.empty();
 
-		if(append == true && has_overrides)
-		{
-			THROW(true, OVERRIDE_APPEND_ERROR_MESSAGE, ctx);
-		}
+		THROW(has_append_flag && has_overrides, OVERRIDE_APPEND_ERROR_MESSAGE, ctx);
 
 		// Since a list only has items, if we have chosen to append them we can append the entire object
 		// otherwise we just want to redefine the list.
-		append |= override_append.find("items") != override_append.end();
+		has_append_flag |= override_append.find("items") != override_append.end();
 
-		if(append)
+		if(has_append_flag)
 		{
 			collector.append(cfg, v);
 		}
@@ -474,29 +475,30 @@ static void read_item(
 		rule_loader::macro_info v(ctx);
 		v.name = name;
 
-		bool append = false;
+		bool has_append_flag = false;
 		decode_val(item, "condition", v.cond, ctx);
 
 		// Now set the proper context for the condition now that we know it exists
 		v.cond_ctx = rule_loader::context(item["condition"], rule_loader::context::MACRO_CONDITION, "", ctx);
 
-		decode_optional_val(item, "append", append, ctx);
+		decode_optional_val(item, "append", has_append_flag, ctx);
+		if(has_append_flag)
+		{
+			cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx);
+		}
 
 		std::set<std::string> override_append, override_replace;
 		std::set<std::string> overridable {"condition"};
 		decode_overrides(item, overridable, overridable, override_append, override_replace, ctx);
 		bool has_overrides = !override_append.empty() || !override_replace.empty();
 
-		if(append == true && has_overrides)
-		{
-			THROW(true, OVERRIDE_APPEND_ERROR_MESSAGE, ctx);
-		}
+		THROW((has_append_flag && has_overrides), OVERRIDE_APPEND_ERROR_MESSAGE, ctx);
 
 		// Since a macro only has a condition, if we have chosen to append to it we can append the entire object
 		// otherwise we just want to redefine the macro.
-		append |= override_append.find("condition") != override_append.end();
+		has_append_flag |= override_append.find("condition") != override_append.end();
 
-		if(append)
+		if(has_append_flag)
 		{
 			collector.append(cfg, v);
 		}
@@ -517,6 +519,10 @@ static void read_item(
 
 		bool has_append_flag = false;
 		decode_optional_val(item, "append", has_append_flag, ctx);
+		if(has_append_flag)
+		{
+			cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx);
+		}
 
 		std::set<std::string> override_append, override_replace;
 		std::set<std::string> overridable_append {"condition", "output", "desc", "tags", "exceptions"};

userspace/engine/rule_loader_reader.h

@@ -24,7 +24,10 @@ limitations under the License.
 #include "falco_engine_version.h"
 
 // Error message used when both 'override' and 'append' are specified.
-#define OVERRIDE_APPEND_ERROR_MESSAGE "Keys 'override' and 'append: true' cannot be used together. Add an append entry (e.g. 'condition: append') under override instead."
+#define OVERRIDE_APPEND_ERROR_MESSAGE "Keys 'override' and 'append: true' cannot be used together. Add an append entry (e.g. 'condition: append') under 'override' instead."
+
+// Warning message used when `append` is used.
+#define WARNING_APPEND_MESSAGE "'append' key is deprecated. Add an append entry (e.g. 'condition: append') under 'override' instead."
 
 namespace rule_loader
 {