Rules result add compile condition context #2216

mstemm · 2022-09-20T16:22:53Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area rules

/area tests

/area proposals

/area CI

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(engine): Also include exact locations for rule condition compile errors (missing macros, etc).

mstemm · 2022-09-22T16:18:35Z

I'm going to wait for the changes in #2206 as that allows referring to things like rule_load_exception independently of the rule loader. Given that filter_rulesets often compile an ast to a filter, we really should document what exceptions filter rulesets can throw so falco knows which ones to catch and convert to rule loading errors.

leogr · 2022-09-27T08:43:15Z

/milestone 0.34.0

mstemm · 2022-10-10T23:56:59Z

This is ready for review now.

userspace/engine/filter_macro_resolver.cpp

tests/engine/test_filter_macro_resolver.cpp

jasondellaluce · 2022-11-02T17:42:13Z

userspace/engine/filter_macro_resolver.h

@@ -82,8 +87,9 @@ class filter_macro_resolver
 		struct visitor : public libsinsp::filter::ast::expr_visitor
 		{
 			std::unique_ptr<libsinsp::filter::ast::expr> m_node_substitute;
-			std::unordered_set<std::string>* m_unknown_macros;
-			std::unordered_set<std::string>* m_resolved_macros;
+			macro_info_map* m_unknown_macros;


See above, these can become references by having an explicit visitor(...) constructor. I think this would be safer and clearer now that we also deal with maps (it would have been a good improvement anyway).

yep, changed them to references.

jasondellaluce · 2022-11-02T18:06:14Z

userspace/engine/rule_loader_compiler.cpp

 		try {
-			source->ruleset->add(*out.at(rule_id), ast);
+			shared_ptr<gen_event_filter> filter(compiler.compile());


Of this, I don't like the fact that we bundle a sinsp_filter_compiler inside the rule_loader::compiler class. In the future, we potentially may want to have multiple implementation of filters, and in general I don't think this should be part of this class' responsibilities.

Given this, I propose two options. I'm leaning towards option 1 because it makes these changes less impactful and better modularizes each class' responsibilities.

Option 1
The only reason why we are passing a gen_event_filter, and compiling the filter here instead then in filter_ruleset, is to have access to the compiler's filter position in case of error. As such, we could simply create a new exception type (e.g. falco_rule_compile_exception) child of falco_exception. This new exception type would carry the AST position information in case of error. Then, make filter_ruleset::add throw both falco_rule_compile_exception and falco_exception by documentation, and catch those two errors here.

So here we would do a catch chain such as: if we have a falco_rule_compile_exception, then throw an error with a filter-condition-specific context with a specific position, otherwise if we have a generic falco_exception just throw an error with r.cond_ctx.

In this way, we would have all the information we need for information reporting here were we need it, by also making all these changes less impactful and by not changing the filter_ruleset interface (if not for the "throws" guarantees). Also, the rule_loader::compile class will not have more responsibilities than we should (e.g. compiling a filter), and we have a clear separation between the rule condition AST and its black-box compiled filter (of which each filter_ruleset implementation is solely responsible).

Option 2
Let's proceed with these changes, but in future PRs (reminder for ourselves):

Make gen_event_filter become a real interface, so that there can actually be multiple implementations of it

Add an intermediary class between rule_loader::compiler and filter_ruleset that takes care of 1) compiling the filter and report condition errors, 2) add it to the ruleset and to the state's rule list, and 3) enable/disable it on the ruleset. All these are becoming too many responsibilities for rule_loader::compiler, which is only supposed to resolve all the engine definitions in a list of rules.

I think of each step of rule loading, compilation, and filter management being separate responsibilities, so I'm not sure moving the compilation into the filter_ruleset is a good idea either. To me the filter_ruleset is just about managing collections of filters, where it can optimize/prefilter/etc based on stuff like event type, as the current one does.

I guess I would think of compilation as something that could be plugged into the rule loader and accessed via event source, as we currently do with filter_factory/formatter_factory. In a fully aligned design, the engine would just be configured with compilers, and a compiler could in turn be configured with fields, so a filter factory would be responsible for compilation too.

So I guess I'd lean towards option 2 with some tbd on the actual interfaces.

jasondellaluce · 2022-11-02T18:07:49Z

Sorry for being late, this was pushed over a lot due to the Falco release and KubeCon's attendance from most maintainers. Left you my comments.

Now that ASTs have parse positions and the compiler will return the position of the last error, use that in falco rules to return errors within condition strings instead of reporting the position as the beginning of the condition. This led to a change in the filter_ruleset interface--now, an ast is compiled to a filter before being passed to the filter_ruleset object. That avoids polluting the interface with a lot of details about rule_loader contexts, errors, etc. The ast is still provided in case the filter_ruleset wants to do indexing/analysis of the filter. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Now that ASTs contain parse positions, use them when reporting errors about unknown macros. When doing the first pass to find all macro references, save macros as a map<macro name,parse position> instead of a set<macro name>. While making that change, change the visitor struct to use references instead of pointers. In the second pass, when reporting any unresolved macro references, also report the parse position. The unit tests also check that the positions of macros are properly returned in the resolved/unresolved maps. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

jasondellaluce

LGTM, let's go with this for now, we may find a more elegant solution for filter compilation in the future.

jasondellaluce · 2022-12-01T10:45:22Z

tests/engine/test_filter_macro_resolver.cpp

@@ -20,9 +20,27 @@ limitations under the License.
 using namespace std;
 using namespace libsinsp::filter::ast;

+static pos_info create_pos(uint32_t idx, uint32_t line, uint32_t col)


This is good, but let's backport it to libs as a constructor/equality operator next!

poiana · 2022-12-01T10:55:39Z

LGTM label has been added.

Git tree hash: 20fefb817c596bd1ad353f0dba4e33802942bff2

FedeDP

/approve

poiana · 2022-12-01T16:03:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, jasondellaluce, mstemm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,jasondellaluce,mstemm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added release-note kind/feature dco-signoff: yes area/engine approved labels Sep 20, 2022

poiana requested review from FedeDP and Kaizhe September 20, 2022 16:23

poiana added the size/L label Sep 20, 2022

mstemm force-pushed the rules-result-add-compile-condition-context branch from 6b52071 to 0ce6779 Compare September 21, 2022 15:52

mstemm added the do-not-merge/hold label Sep 22, 2022

poiana added this to the 0.34.0 milestone Sep 27, 2022

mstemm force-pushed the rules-result-add-compile-condition-context branch 2 times, most recently from b5ac7da to f307364 Compare September 29, 2022 20:37

mstemm removed the do-not-merge/hold label Oct 10, 2022

jasondellaluce reviewed Nov 2, 2022

View reviewed changes

mstemm force-pushed the rules-result-add-compile-condition-context branch from f307364 to 0d15c9b Compare November 10, 2022 20:39

mstemm force-pushed the rules-result-add-compile-condition-context branch from 0d15c9b to 5ec7953 Compare November 10, 2022 22:02

jasondellaluce approved these changes Dec 1, 2022

View reviewed changes

poiana assigned jasondellaluce Dec 1, 2022

poiana added the lgtm label Dec 1, 2022

FedeDP approved these changes Dec 1, 2022

View reviewed changes

poiana assigned FedeDP Dec 1, 2022

poiana merged commit 910b8ff into master Dec 1, 2022

poiana deleted the rules-result-add-compile-condition-context branch December 1, 2022 16:03

jasondellaluce mentioned this pull request Dec 2, 2022

update(userspace/libsinsp/filter): add constructor and operators to pos_info falcosecurity/libs#764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rules result add compile condition context #2216

Rules result add compile condition context #2216

mstemm commented Sep 20, 2022 •

edited by LucaGuerra

Loading

mstemm commented Sep 22, 2022

leogr commented Sep 27, 2022

mstemm commented Oct 10, 2022

jasondellaluce Nov 2, 2022

mstemm Nov 10, 2022

jasondellaluce Nov 2, 2022

mstemm Nov 10, 2022

jasondellaluce commented Nov 2, 2022

jasondellaluce left a comment

jasondellaluce Dec 1, 2022

poiana commented Dec 1, 2022

FedeDP left a comment

poiana commented Dec 1, 2022

Rules result add compile condition context #2216

Rules result add compile condition context #2216

Conversation

mstemm commented Sep 20, 2022 • edited by LucaGuerra Loading

mstemm commented Sep 22, 2022

leogr commented Sep 27, 2022

mstemm commented Oct 10, 2022

jasondellaluce Nov 2, 2022

Choose a reason for hiding this comment

mstemm Nov 10, 2022

Choose a reason for hiding this comment

jasondellaluce Nov 2, 2022

Choose a reason for hiding this comment

mstemm Nov 10, 2022

Choose a reason for hiding this comment

jasondellaluce commented Nov 2, 2022

jasondellaluce left a comment

Choose a reason for hiding this comment

jasondellaluce Dec 1, 2022

Choose a reason for hiding this comment

poiana commented Dec 1, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Dec 1, 2022

mstemm commented Sep 20, 2022 •

edited by LucaGuerra

Loading