Address feedback from PR

- Instead of having a possibly null string pointer as the argument to
   enable_* and process_event, have wrapper versions that assume a
   default falco ruleset. The default ruleset name is a static member of
   the falco_engine class, and the default ruleset id is created/found
   in the constructor.
 - This makes the whole mechanism simple enough that it doesn't require
   seprarate testing, so remove the capability within falco to read a
   ruleset from the environment and remove automated tests that specify
   a ruleset.
 - Make pattern/tags/ruleset arguments to enable_* functions const.

(I'll squash this down before I commit)

test/falco_test.py

@@ -23,8 +23,6 @@ class FalcoTest(Test):
         self.should_detect = self.params.get('detect', '*', default=False)
         self.trace_file = self.params.get('trace_file', '*')
 
-        self.env = {}
-
         if not os.path.isabs(self.trace_file):
             self.trace_file = os.path.join(self.basedir, self.trace_file)
 
@@ -125,10 +123,6 @@ class FalcoTest(Test):
         if self.run_tags == '':
             self.run_tags=[]
 
-        self.ruleset = self.params.get('ruleset', '*', default='')
-        if self.ruleset != '':
-            self.env["FALCO_RULESET"] = self.ruleset
-
     def check_rules_warnings(self, res):
 
         found_warning = sets.Set()
@@ -250,7 +244,7 @@ class FalcoTest(Test):
         for tag in self.run_tags:
             cmd += ' -t {}'.format(tag)
 
-        self.falco_proc = process.SubProcess(cmd, env=self.env)
+        self.falco_proc = process.SubProcess(cmd)
 
         res = self.falco_proc.run(timeout=180, sig=9)
 

test/falco_tests.yaml.in

@@ -483,75 +483,3 @@ trace_files: !mux
       - open_11: 1
       - open_12: 0
       - open_13: 0
-
-  # Trying one of the combinations with a ruleset specified. Should get same output and no errors.
-  run_tags_c_ruleset:
-    detect: True
-    detect_level: WARNING
-    rules_file:
-      - rules/tagged_rules.yaml
-    trace_file: trace_files/open-multiple-files.scap
-    run_tags: [c]
-    ruleset: my-ruleset
-    detect_counts:
-      - open_1: 0
-      - open_2: 0
-      - open_3: 1
-      - open_4: 0
-      - open_5: 1
-      - open_6: 1
-      - open_7: 1
-      - open_8: 0
-      - open_9: 1
-      - open_10: 1
-      - open_11: 0
-      - open_12: 0
-      - open_13: 0
-
-  # Disabling one rule with a ruleset specified. Should get that one rule missing and no errors.
-  disabled_with_ruleset:
-    detect: True
-    detect_level: WARNING
-    rules_file:
-      - rules/tagged_rules.yaml
-    trace_file: trace_files/open-multiple-files.scap
-    disabled_rules:
-      - "open_4"
-    ruleset: my-ruleset
-    detect_counts:
-      - open_1: 1
-      - open_2: 1
-      - open_3: 1
-      - open_4: 0
-      - open_5: 1
-      - open_6: 1
-      - open_7: 1
-      - open_8: 1
-      - open_9: 1
-      - open_10: 1
-      - open_11: 1
-      - open_12: 1
-      - open_13: 1
-
-  # Enabling all rules with a ruleset specified. Should get all rules matching.
-  default_with_ruleset:
-    detect: True
-    detect_level: WARNING
-    rules_file:
-      - rules/tagged_rules.yaml
-    trace_file: trace_files/open-multiple-files.scap
-    ruleset: my-ruleset
-    detect_counts:
-      - open_1: 1
-      - open_2: 1
-      - open_3: 1
-      - open_4: 1
-      - open_5: 1
-      - open_6: 1
-      - open_7: 1
-      - open_8: 1
-      - open_9: 1
-      - open_10: 1
-      - open_11: 1
-      - open_12: 1
-      - open_13: 1

userspace/engine/falco_engine.cpp

@@ -56,6 +56,8 @@ falco_engine::falco_engine(bool seed_rng)
 	{
 		srandom((unsigned) getpid());
 	}
+
+	m_default_ruleset_id = find_ruleset_id(m_default_ruleset);
 }
 
 falco_engine::~falco_engine()
@@ -108,38 +110,39 @@ void falco_engine::load_rules_file(const string &rules_filename, bool verbose, b
 	load_rules(rules_content, verbose, all_events);
 }
 
-void falco_engine::enable_rule(string &pattern, bool enabled, string *ruleset)
+void falco_engine::enable_rule(const string &pattern, bool enabled, const string &ruleset)
 {
-	uint16_t ruleset_id = 0;
+	uint16_t ruleset_id = find_ruleset_id(ruleset);
-
-	if(ruleset)
-	{
-		ruleset_id = find_ruleset_id(*ruleset);
-	}
 
 	m_evttype_filter->enable(pattern, enabled, ruleset_id);
 }
 
-void falco_engine::enable_rule_by_tag(set<string> &tags, bool enabled, string *ruleset)
+void falco_engine::enable_rule(const string &pattern, bool enabled)
 {
-	uint16_t ruleset_id = 0;
+	enable_rule(pattern, enabled, m_default_ruleset);
+}
 
-	if(ruleset)
+void falco_engine::enable_rule_by_tag(const set<string> &tags, bool enabled, const string &ruleset)
-	{
+{
-		ruleset_id = find_ruleset_id(*ruleset);
+	uint16_t ruleset_id = find_ruleset_id(ruleset);
-	}
 
 	m_evttype_filter->enable_tags(tags, enabled, ruleset_id);
 }
 
-uint16_t falco_engine::find_ruleset_id(std::string &ruleset)
+void falco_engine::enable_rule_by_tag(const set<string> &tags, bool enabled)
 {
-	auto it = m_known_rulesets.find(ruleset);
+	enable_rule_by_tag(tags, enabled, m_default_ruleset);
+}
 
-	if(it == m_known_rulesets.end())
+uint16_t falco_engine::find_ruleset_id(const std::string &ruleset)
+{
+	auto it = m_known_rulesets.lower_bound(ruleset);
+
+	if(it == m_known_rulesets.end() ||
+	   it->first != ruleset)
 	{
-		m_known_rulesets[ruleset] = ++m_next_ruleset_id;
+		it = m_known_rulesets.emplace_hint(it,
-		it = m_known_rulesets.find(ruleset);
+						   std::make_pair(ruleset, m_next_ruleset_id++));
 	}
 
 	return it->second;
@@ -187,6 +190,11 @@ unique_ptr<falco_engine::rule_result> falco_engine::process_event(sinsp_evt *ev,
 	return res;
 }
 
+unique_ptr<falco_engine::rule_result> falco_engine::process_event(sinsp_evt *ev)
+{
+	return process_event(ev, m_default_ruleset_id);
+}
+
 void falco_engine::describe_rule(string *rule)
 {
 	return m_rules->describe_rule(rule);

userspace/engine/falco_engine.h

@@ -49,17 +49,23 @@ public:
 
 	//
 	// Enable/Disable any rules matching the provided pattern
-	// (regex). If ruleset is non-NULL, enable/disable these
+	// (regex). When provided, enable/disable these rules in the
-	// rules in the context of the provided ruleset. The ruleset
+	// context of the provided ruleset. The ruleset (id) can later
-	// can later be passed as an argument to process_event(). This
+	// be passed as an argument to process_event(). This allows
-	// allows for different sets of rules being active at once.
+	// for different sets of rules being active at once.
 	//
-	void enable_rule(std::string &pattern, bool enabled, std::string *ruleset = NULL);
+	void enable_rule(const std::string &pattern, bool enabled, const std::string &ruleset);
+
+	// Wrapper that assumes the default ruleset
+	void enable_rule(const std::string &pattern, bool enabled);
 
 	//
 	// Enable/Disable any rules with any of the provided tags (set, exact matches only)
 	//
-	void enable_rule_by_tag(std::set<std::string> &tags, bool enabled, std::string *ruleset = NULL);
+	void enable_rule_by_tag(const std::set<std::string> &tags, bool enabled, const std::string &ruleset);
+
+	// Wrapper that assumes the default ruleset
+	void enable_rule_by_tag(const std::set<std::string> &tags, bool enabled);
 
 	struct rule_result {
 		sinsp_evt *evt;
@@ -74,20 +80,25 @@ public:
 	// to enable_rule/enable_rule_by_tag(), you should look up the
 	// ruleset id and pass it to process_event().
 	//
-	uint16_t find_ruleset_id(std::string &ruleset);
+	uint16_t find_ruleset_id(const std::string &ruleset);
 
 	//
 	// Given an event, check it against the set of rules in the
 	// engine and if a matching rule is found, return details on
 	// the rule that matched. If no rule matched, returns NULL.
 	//
-	// If ruleset is non-NULL, use the enabled/disabled status
+	// When ruleset_id is provided, use the enabled/disabled status
 	// associated with the provided ruleset. This is only useful
 	// when you have previously called enable_rule/enable_rule_by_tag
-	// with a non-NULL ruleset.
+	// with a ruleset string.
 	//
 	// the returned rule_result is allocated and must be delete()d.
-	std::unique_ptr<rule_result> process_event(sinsp_evt *ev, uint16_t ruleset_id = 0);
+	std::unique_ptr<rule_result> process_event(sinsp_evt *ev, uint16_t ruleset_id);
+
+	//
+	// Wrapper assuming the default ruleset
+	//
+	std::unique_ptr<rule_result> process_event(sinsp_evt *ev);
 
 	//
 	// Print details on the given rule. If rule is NULL, print
@@ -172,6 +183,8 @@ private:
 	double m_sampling_multiplier;
 
 	std::string m_lua_main_filename = "rule_loader.lua";
+	std::string m_default_ruleset = "falco-default-ruleset";
+	uint32_t m_default_ruleset_id;
 
 	std::string m_extra;
 	bool m_replace_container_info;

userspace/falco/falco.cpp

@@ -133,8 +133,7 @@ std::list<string> cmdline_options;
 uint64_t do_inspect(falco_engine *engine,
 		    falco_outputs *outputs,
 		    sinsp* inspector,
-		    string &stats_filename,
+		    string &stats_filename)
-		    uint16_t ruleset_id)
 {
 	uint64_t num_evts = 0;
 	int32_t res;
@@ -194,7 +193,7 @@ uint64_t do_inspect(falco_engine *engine,
 		// engine, which will match the event against the set
 		// of rules. If a match is found, pass the event to
 		// the outputs.
-		unique_ptr<falco_engine::rule_result> res = engine->process_event(ev, ruleset_id);
+		unique_ptr<falco_engine::rule_result> res = engine->process_event(ev);
 		if(res)
 		{
 			outputs->handle_event(res->evt, res->rule, res->priority, res->format);
@@ -373,18 +372,6 @@ int falco_init(int argc, char **argv)
 		engine = new falco_engine();
 		engine->set_inspector(inspector);
 		engine->set_extra(output_format, replace_container_info);
-		string *ruleset = NULL;
-		string ruleset_env;
-		uint16_t ruleset_id = 0;
-
-		// The ruleset feature is really falco
-		// engine-specific, so we don't advertise it. But it
-		// is possible to specify an alternate ruleset via the environment.
-		if (getenv("FALCO_RULESET") != NULL)
-		{
-			ruleset_env = getenv("FALCO_RULESET");
-			ruleset = &ruleset_env;
-		}
 
 		outputs = new falco_outputs();
 		outputs->set_inspector(inspector);
@@ -454,16 +441,10 @@ int falco_init(int argc, char **argv)
 			throw std::invalid_argument("You can not specify both disabled (-D/-T) and enabled (-t) rules");
 		}
 
-		// If a ruleset was provided, we must first explicitly enable all rules.
-		if(ruleset)
-		{
-			engine->enable_rule(all_rules, true, ruleset);
-		}
-
 		for (auto pattern : disabled_rule_patterns)
 		{
 			falco_logger::log(LOG_INFO, "Disabling rules matching pattern: " + pattern + "\n");
-			engine->enable_rule(pattern, false, ruleset);
+			engine->enable_rule(pattern, false);
 		}
 
 		if(disabled_rule_tags.size() > 0)
@@ -472,7 +453,7 @@ int falco_init(int argc, char **argv)
 			{
 				falco_logger::log(LOG_INFO, "Disabling rules with tag: " + tag + "\n");
 			}
-			engine->enable_rule_by_tag(disabled_rule_tags, false, ruleset);
+			engine->enable_rule_by_tag(disabled_rule_tags, false);
 		}
 
 		if(enabled_rule_tags.size() > 0)
@@ -480,12 +461,12 @@ int falco_init(int argc, char **argv)
 
 			// Since we only want to enable specific
 			// rules, first disable all rules.
-			engine->enable_rule(all_rules, false, ruleset);
+			engine->enable_rule(all_rules, false);
 			for(auto tag : enabled_rule_tags)
 			{
 				falco_logger::log(LOG_INFO, "Enabling rules with tag: " + tag + "\n");
 			}
-			engine->enable_rule_by_tag(enabled_rule_tags, true, ruleset);
+			engine->enable_rule_by_tag(enabled_rule_tags, true);
 		}
 
 		outputs->init(config.m_json_output, config.m_notifications_rate, config.m_notifications_max_burst);
@@ -668,15 +649,10 @@ int falco_init(int argc, char **argv)
 		delete mesos_api;
 		mesos_api = 0;
 
-		if(ruleset)
-		{
-			ruleset_id = engine->find_ruleset_id(*ruleset);
-		}
 		num_evts = do_inspect(engine,
 				      outputs,
 				      inspector,
-				      stats_filename,
+				      stats_filename);
-				      ruleset_id);
 
 		duration = ((double)clock()) / CLOCKS_PER_SEC - duration;