update(rule_loader): deprecate enabled usage

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

unit_tests/engine/test_rule_loader.cpp

@@ -43,6 +43,17 @@ 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())
@@ -53,6 +64,8 @@ class engine_loader_test : public ::testing::Test {
 		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;
@@ -62,6 +75,28 @@ class engine_loader_test : public ::testing::Test {
 		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;
@@ -377,3 +412,142 @@ TEST_F(engine_loader_test, rule_override_extra_field)
 	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);
 }
+
+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");
+}

userspace/engine/rule_loader_reader.cpp

@@ -693,6 +693,7 @@ static void read_item(
 			    !item["priority"].IsDefined())
 			{
 				decode_val(item, "enabled", v.enabled, ctx);
+				cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_ENABLED_MESSAGE, ctx);
 				collector.enable(cfg, v);
 			}
 			else

userspace/engine/rule_loader_reader.h

@@ -24,10 +24,13 @@ 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."
+#define WARNING_APPEND_MESSAGE "'append' key is deprecated. Add an 'append' entry (e.g. 'condition: append') under 'override' instead."
+
+// Warning message used when `enabled` is used without override.
+#define WARNING_ENABLED_MESSAGE "The standalone 'enabled' key usage is deprecated. The correct approach requires also a 'replace' entry under the 'override' key (i.e. 'enabled: replace')."
 
 namespace rule_loader
 {