update(rule_loader): deprecate the append flag in falco rules

unit_tests/engine/test_rule_loader.cpp

@@ -43,6 +43,60 @@ class engine_loader_test : public ::testing::Test {
 		return ret;
 	}
 
+	// This must be kept in line with the (private) falco_engine::s_default_ruleset
+	uint64_t num_rules_for_ruleset(std::string ruleset = "falco-default-ruleset")
+	{
+		return m_engine->num_rules_for_ruleset(ruleset);
+	}
+
+	bool has_warnings()
+	{
+		return m_load_result->has_warnings();
+	}
+
+	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"];
+			// Debug:
+			// printf("msg: %s\n", msg.c_str());
+			if(msg.find(warning_msg) != std::string::npos)
+			{
+				return true;
+			}
+		}
+
+		return false;
+	}
+
+	bool check_error_message(std::string error_msg)
+	{
+		// if the loading is successful there are no errors
+		if(m_load_result->successful())
+		{
+			return false;
+		}
+
+		for(auto &err : m_load_result_json["errors"])
+		{
+			std::string msg = err["message"];
+			// Debug:
+			// printf("msg: %s\n", msg.c_str());
+			if(msg.find(error_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 +188,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");
@@ -145,7 +202,6 @@ TEST_F(engine_loader_test, rule_override_append)
 	 	"legit rule description with append");
 }
 
-
 TEST_F(engine_loader_test, rule_append)
 {
     std::string rules_content = R"END(
@@ -163,12 +219,14 @@ 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)");
 }
 
-
 TEST_F(engine_loader_test, rule_override_replace)
 {
     std::string rules_content = R"END(
@@ -258,7 +316,7 @@ TEST_F(engine_loader_test, rule_incorrect_override_type)
 	std::string rule_name = "failing_rule";
 
 	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
-	ASSERT_EQ(m_load_result_json["errors"][0]["message"], "Key 'priority' cannot be appended to, use 'replace' instead");
+	ASSERT_TRUE(check_error_message("Key 'priority' cannot be appended to, use 'replace' instead"));
 	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["context"]["snippet"]).find("priority: append") != std::string::npos);
 }
 
@@ -283,7 +341,11 @@ TEST_F(engine_loader_test, rule_incorrect_append_override)
 	std::string rule_name = "failing_rule";
 
 	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
-	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("'override' and 'append: true' cannot be used together") != std::string::npos);
+
+	// We should have at least one warning because the 'append' flag is deprecated.
+	ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE));
+
+	ASSERT_TRUE(check_error_message(OVERRIDE_APPEND_ERROR_MESSAGE));
 }
 
 TEST_F(engine_loader_test, rule_override_without_rule)
@@ -300,7 +362,7 @@ TEST_F(engine_loader_test, rule_override_without_rule)
 	std::string rule_name = "failing_rule";
 
 	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
-	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("no rule by that name already exists") != std::string::npos);
+	ASSERT_TRUE(check_error_message("no rule by that name already exists"));
 }
 
 TEST_F(engine_loader_test, rule_override_without_field)
@@ -322,7 +384,7 @@ TEST_F(engine_loader_test, rule_override_without_field)
 	std::string rule_name = "failing_rule";
 
 	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
-	ASSERT_EQ(m_load_result_json["errors"][0]["message"], "An append override for 'condition' was specified but 'condition' is not defined");
+	ASSERT_TRUE(check_error_message("An append override for 'condition' was specified but 'condition' is not defined"));
 }
 
 TEST_F(engine_loader_test, rule_override_extra_field)
@@ -346,5 +408,198 @@ TEST_F(engine_loader_test, rule_override_extra_field)
 	std::string rule_name = "failing_rule";
 
 	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
-	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("Unexpected key 'priority'") != std::string::npos);
+	ASSERT_TRUE(check_error_message("Unexpected key 'priority'"));
+}
+
+TEST_F(engine_loader_test, missing_enabled_key_with_override)
+{
+    std::string rules_content = R"END(
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+- rule: test_rule
+  desc: missing enabled key
+  condition: and proc.name = cat
+  override:
+    desc: replace
+    condition: append
+    enabled: replace
+)END";
+
+	// In the rule override we miss `enabled: true`
+	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_TRUE(check_error_message("'enabled' was specified but 'enabled' is not defined"));
+}
+
+TEST_F(engine_loader_test, rule_override_with_enabled)
+{
+    std::string rules_content = R"END(
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+- rule: test_rule
+  desc: correct override
+  condition: and proc.name = cat
+  enabled: true
+  override:
+    desc: replace
+    condition: append
+    enabled: replace
+)END";
+
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_FALSE(has_warnings());
+	// The rule should be enabled at the end.
+	EXPECT_EQ(num_rules_for_ruleset(), 1);
+}
+
+TEST_F(engine_loader_test, rule_not_enabled)
+{
+    std::string rules_content = R"END(
+- rule: test_rule
+  desc: rule not enabled
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+)END";
+
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_FALSE(has_warnings());
+	EXPECT_EQ(num_rules_for_ruleset(), 0);
+}
+
+TEST_F(engine_loader_test, rule_enabled_warning)
+{
+    std::string rules_content = R"END(
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+- rule: test_rule
+  enabled: true
+)END";
+
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_TRUE(check_warning_message(WARNING_ENABLED_MESSAGE));
+	// The rule should be enabled at the end.
+	EXPECT_EQ(num_rules_for_ruleset(), 1);
+}
+
+// todo!: Probably we shouldn't allow this syntax
+TEST_F(engine_loader_test, rule_enabled_is_ignored_by_append)
+{
+    std::string rules_content = R"END(
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+- rule: test_rule
+  condition: and proc.name = cat
+  append: true
+  enabled: true
+)END";
+
+	// 'enabled' is ignored by the append, this syntax is not supported
+	// so the rule is not enabled.
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	EXPECT_EQ(num_rules_for_ruleset(), 0);
+}
+
+// todo!: Probably we shouldn't allow this syntax
+TEST_F(engine_loader_test, rewrite_rule)
+{
+    std::string rules_content = R"END(
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+- rule: test_rule
+  desc: redefined rule syntax
+  condition: proc.name = cat
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: WARNING
+  enabled: true
+)END";
+
+	// The above syntax is not supported, we cannot override the content
+	// of a rule in this way.
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	// In this case the rule is completely overridden but this syntax is not supported.
+	EXPECT_EQ(num_rules_for_ruleset(), 1);
+
+	std::string rule_name = "test_rule";
+	auto rule_description = m_engine->describe_rule(&rule_name, {});
+	ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get<std::string>(), "proc.name = cat");
+}
+
+TEST_F(engine_loader_test, required_engine_version_semver)
+{
+    std::string rules_content = R"END(
+- required_engine_version: 0.26.0
+
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+)END";
+
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_FALSE(has_warnings());
+}
+
+TEST_F(engine_loader_test, required_engine_version_not_semver)
+{
+    std::string rules_content = R"END(
+- required_engine_version: 26
+
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+)END";
+
+	ASSERT_TRUE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_TRUE(check_warning_message(WARNING_ENGINE_VERSION_NOT_SEMVER));
+}
+
+TEST_F(engine_loader_test, required_engine_version_invalid)
+{
+    std::string rules_content = R"END(
+- required_engine_version: seven
+
+- rule: test_rule
+  desc: test rule description
+  condition: evt.type = close
+  output: user=%user.name command=%proc.cmdline file=%fd.name
+  priority: INFO
+  enabled: false
+
+)END";
+
+	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
+	ASSERT_TRUE(check_error_message("Unable to parse engine version"));
 }