Support condition parse errors in rule loading results #2155

mstemm · 2022-08-04T19:38:10Z

In #2098, we reworked how rules loading errors/warnings were returned
to provide a richer set of information, including locations/context
for the errors/warnings.

That did not include locations within condition expressions,
though. When parsing a condition expression resulted in a
warning/error, the location simply pointed to the condition property
of the rule.

This commit improves this to handle parse errors:

When libsinsp::filter::parser::parse() throws an exception, use
get_pos() to get the position within the condition string.
Add a new context() constructor that takes a filter pos_info instead
of a YAML::Mark.

Doing this required some small changes to the context struct,
though. Previously, the name (e.g. yaml filename) was held separate
from the list of locations. However, condition strings don't directly
reflect any content in the yaml file. Yaml block scalars remove
whitespace/unwrap strings. Macro references/list references are
replaced with their contents.

To handle this, move the "name" (e.g. filename) from the context into
each location. This allows a chain of locations to start with file
positions but transition to offsets within a condition expression.

Also allow a context to contain an alternate content string which is
used to build the snippet. For contexts related to condition strings,
the content is the condition.

Finally, when printing snippets that contain very long lines (> a
static const 160 chars), instead of printing the entire line, print
the 160 chars surrounding the position.

Signed-off-by: Mark Stemm mark.stemm@gmail.com

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

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

What this PR does / why we need it:

Add better support for parse errors in rule conditions when displaying rule load errors.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

jasondellaluce

This is a very good improvement and LGTM for the most part! I left you few comments on the code, and I'd add:

Instead of having a static snippet_width, can't we make it a parameter of the snippet() method?
Seems like we use YAML::Mark just to access pos, line, and column. Can't we create a small struct for this, so that we don't depend on YAML::Mark and on other position-like structs like the one from libsinsp's parser. For sake of simplicity, even a type alias would work for now. This would work better for when a third case of position struct will come, and would also make it easier to write unit tests for rule_loader (still a TODO) without dealing with the YAML library.

jasondellaluce · 2022-08-05T09:13:37Z

userspace/engine/rule_loader.cpp

 {
+	location loc = {name, YAML::Mark(), "rules content", ""};


Out of curiosity, why we do this here?

Just so every context has some location, So for the top level context, it's a simple "somewhere in this file" location.

jasondellaluce · 2022-08-05T09:15:33Z

userspace/engine/rule_loader.cpp

+	std::replace(name.begin(), name.end(), '\n', ' ');
+	std::replace(name.begin(), name.end(), '\r', ' ');
+
+	std::string item_type = "condition expression";


We're starting to have many of these types. Can't we have an enum or something to centralize them?

Yes, I made an enum.

jasondellaluce · 2022-08-05T09:18:46Z

userspace/engine/rule_loader.cpp

+		libsinsp::filter::parser::pos_info pos;
+		p.get_pos(pos);


I think we have a 0-params overload that allows:

Suggested change

libsinsp::filter::parser::pos_info pos;

p.get_pos(pos);

auto pos = p.get_pos();

I created a standalone location struct and YAML::Marks and parser::pos_infos are converted now.

jasondellaluce · 2022-08-08T08:28:30Z

/milestone 0.33.0

mstemm · 2022-08-08T22:12:40Z

The test failure was not related to these changes, and was actually a design flaw in the earlier PR that wasn't detected until I changed how very long snippet lines were returned.

If reading rules files A and B, it's possible that reading file A has no warnings, but after reading file B there are warnings in file A, because B overrides macros/rules/lists from A. Two files that show this are https://github.com/falcosecurity/falco/blob/master/test/rules/single_rule.yaml and https://github.com/falcosecurity/falco/blob/master/test/rules/override_macro.yaml. override_macro overrides the macro is_cat, but doing that causes the list used in the original definition of is_cat to become unused.

I'm going to fix that in a separate PR. It's going to have a lot of the changes that are in this PR (moving filenames into the context positions, handling long snippet lines), but it won't have the specific changes for handling condition parse errors.

So putting this one on hold for now.

jasondellaluce

Changes LGTM! I have a minor suggestion but other than that I think this is ready for approval.

jasondellaluce · 2022-08-30T14:56:43Z

userspace/engine/rule_loader.cpp

-	auto it = rules_contents.find(name);
+	if(alt_content.empty() && it == rules_contents.end())
+	{
+		if(it == rules_contents.end())


The nested if statement seems useless here, am I wrong?

Yeah, this is redundant. I removed it.

In #2098 and #2158, we reworked how rules loading errors/warnings were returned to provide a richer set of information, including locations/context for the errors/warnings. That did *not* include locations within condition expressions, though. When parsing a condition expression resulted in a warning/error, the location simply pointed to the condition property of the rule. This commit improves this to handle parse errors: - When libsinsp::filter::parser::parse() throws an exception, use get_pos() to get the position within the condition string. - Add a new context() constructor that takes a filter pos_info instead of a YAML::Mark. Now that positions aren't always related to the location of yaml nodes, Make up a generic "position" struct for locations and convert YAML::Mark and parser positions to a position struct. Also allow a context to contain an alternate content string which is used to build the snippet. For contexts related to condition strings, the content is the condition. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Use an enum instead of a string for the item_type aka "parts of a rules file" field of contexts. The set of values is mostly defined by the contexts that were already created. There are a couple of forward-looking values for rule outputs/macro conditions/etc. that may be useful for later. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Update a few tests related to rules loading to use new names for items (e.g. "rules content" for top level errors instead of "file") Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

jasondellaluce

/approve

poiana · 2022-09-07T08:11:41Z

LGTM label has been added.

Git tree hash: b4248c68a1461f976364e8d94ada00b5b45e1fd2

FedeDP

/approve

poiana · 2022-09-07T08:13:04Z

[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 kind/feature area/engine dco-signoff: yes do-not-merge/release-note-label-needed labels Aug 4, 2022

poiana requested review from Kaizhe and mfdii August 4, 2022 19:38

poiana added approved size/L release-note-none and removed do-not-merge/release-note-label-needed labels Aug 4, 2022

mstemm force-pushed the rules-result-add-condition-context branch from 28a7cd7 to 9e69502 Compare August 4, 2022 19:39

jasondellaluce reviewed Aug 5, 2022

View reviewed changes

mstemm force-pushed the rules-result-add-condition-context branch from 9e69502 to 9fe7a3b Compare August 5, 2022 14:44

poiana added this to the 0.33.0 milestone Aug 8, 2022

mstemm added the do-not-merge/hold label Aug 8, 2022

mstemm force-pushed the rules-result-add-condition-context branch from acb4b0c to c6cdeb8 Compare August 22, 2022 21:42

poiana added size/XL and removed size/L labels Aug 22, 2022

mstemm force-pushed the rules-result-add-condition-context branch from 22b3269 to 0d42aeb Compare August 22, 2022 23:21

mstemm removed the do-not-merge/hold label Aug 22, 2022

mstemm force-pushed the rules-result-add-condition-context branch 4 times, most recently from 9ba9b62 to cf7415b Compare August 23, 2022 17:09

jasondellaluce reviewed Aug 30, 2022

View reviewed changes

mstemm force-pushed the rules-result-add-condition-context branch from cf7415b to 8616dc0 Compare September 6, 2022 18:50

mstemm force-pushed the rules-result-add-condition-context branch from 8616dc0 to 3f38a08 Compare September 6, 2022 18:55

mstemm added 2 commits September 6, 2022 15:48

Update tests for rules loading (item names)

598a4e9

Update a few tests related to rules loading to use new names for items (e.g. "rules content" for top level errors instead of "file") Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

mstemm force-pushed the rules-result-add-condition-context branch from 3f38a08 to 598a4e9 Compare September 6, 2022 22:48

jasondellaluce approved these changes Sep 7, 2022

View reviewed changes

poiana assigned jasondellaluce Sep 7, 2022

poiana added the lgtm label Sep 7, 2022

FedeDP approved these changes Sep 7, 2022

View reviewed changes

poiana assigned FedeDP Sep 7, 2022

poiana merged commit 103d7e0 into master Sep 7, 2022

poiana deleted the rules-result-add-condition-context branch September 7, 2022 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support condition parse errors in rule loading results #2155

Support condition parse errors in rule loading results #2155

mstemm commented Aug 4, 2022 •

edited

Loading

jasondellaluce left a comment

jasondellaluce Aug 5, 2022

mstemm Aug 22, 2022

jasondellaluce Aug 5, 2022

mstemm Aug 22, 2022

jasondellaluce Aug 5, 2022

mstemm Aug 22, 2022

jasondellaluce commented Aug 8, 2022

mstemm commented Aug 8, 2022

jasondellaluce left a comment

jasondellaluce Aug 30, 2022

mstemm Sep 6, 2022

jasondellaluce left a comment

poiana commented Sep 7, 2022

FedeDP left a comment

poiana commented Sep 7, 2022

	libsinsp::filter::parser::pos_info pos;
	p.get_pos(pos);
	auto pos = p.get_pos();

Support condition parse errors in rule loading results #2155

Support condition parse errors in rule loading results #2155

Conversation

mstemm commented Aug 4, 2022 • edited Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasondellaluce commented Aug 8, 2022

mstemm commented Aug 8, 2022

jasondellaluce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasondellaluce left a comment

Choose a reason for hiding this comment

poiana commented Sep 7, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Sep 7, 2022

mstemm commented Aug 4, 2022 •

edited

Loading