falco icon indicating copy to clipboard operation
falco copied to clipboard

Support condition parse errors in rule loading results

Open mstemm opened this issue 2 years ago • 3 comments

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 [email protected]

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

mstemm avatar Aug 04 '22 19:08 mstemm

/milestone 0.33.0

jasondellaluce avatar Aug 08 '22 08:08 jasondellaluce

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.

mstemm avatar Aug 08 '22 22:08 mstemm

LGTM label has been added.

Git tree hash: b4248c68a1461f976364e8d94ada00b5b45e1fd2

poiana avatar Sep 07 '22 08:09 poiana

[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 avatar Sep 07 '22 08:09 poiana