Refactor rules loading result #2098

mstemm · 2022-06-28T16:56:20Z

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:

When loading rules, return a structure with the load result and lists of errors/warnings + file locations.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update: when loading rules, return a structure with the load result and lists of errors/warnings + file locations.

jasondellaluce · 2022-06-29T09:52:54Z

/milestone 0.33.0

mstemm · 2022-06-29T22:00:54Z

One follow-on change I'd like to make is to extend the context struct to point inside a condition string/output string. I think that might require some libs changes such as annotating ASTs with yaml file positions. I see that the parser object has a position but I think for multi-line strings the yaml parsing changes the position. Let me know if I'm wrong on that and I can work on merging the positions.

mstemm · 2022-06-29T23:54:39Z

~~I cherry-picked the commit from #2096 and included it in this PR temporarily, just so I could get the build/test working. Once 2096 is merged, I'll rebase and drop the cherry-picked commit.~~

Edit: rebased and dropped now.

mstemm · 2022-06-30T15:41:39Z

Here are some example outputs that use the result structure and print the contents in various ways:

Validating some files, no errors:

$ ./userspace/falco/falco -V /etc/falco/falco_rules.yaml -V /etc/falco/falco_rules.local.yaml
Thu Jun 30 08:32:49 2022: Falco version 0.0.0
Thu Jun 30 08:32:49 2022: Falco initialized with configuration file /etc/falco/falco.yaml
Thu Jun 30 08:32:49 2022: Validating rules file(s):
Thu Jun 30 08:32:49 2022:    /etc/falco/falco_rules.yaml
Thu Jun 30 08:32:49 2022:    /etc/falco/falco_rules.local.yaml
/etc/falco/falco_rules.yaml: Ok
/etc/falco/falco_rules.local.yaml: Ok

Validating some files, some errors + warnings. This version of the output is terse, which matches old behavior:

$ ./userspace/falco/falco -V /etc/falco/falco_rules.yaml -V /etc/falco/falco_rules.local.yaml
Thu Jun 30 08:35:19 2022: Falco version 0.0.0
Thu Jun 30 08:35:19 2022: Falco initialized with configuration file /etc/falco/falco.yaml
Thu Jun 30 08:35:19 2022: Validating rules file(s):
Thu Jun 30 08:35:19 2022:    /etc/falco/falco_rules.yaml
Thu Jun 30 08:35:19 2022:    /etc/falco/falco_rules.local.yaml
Error: /etc/falco/falco_rules.yaml: Ok
/etc/falco/falco_rules.local.yaml: Invalid

If you add -v you get full details on the errors + warnings:

$ ./userspace/falco/falco -V /etc/falco/falco_rules.yaml -V /etc/falco/falco_rules.local.yaml -v
Thu Jun 30 08:38:48 2022: Falco version 0.0.0
Thu Jun 30 08:38:48 2022: Falco initialized with configuration file /etc/falco/falco.yaml
Thu Jun 30 08:38:48 2022: Validating rules file(s):
Thu Jun 30 08:38:48 2022:    /etc/falco/falco_rules.yaml
Thu Jun 30 08:38:48 2022:    /etc/falco/falco_rules.local.yaml
/etc/falco/falco_rules.local.yaml: Invalid
1 Errors:
In rule 'My Invalid Rule': (/etc/falco/falco_rules.local.yaml:37:2)
    exception 'invalid exception': (/etc/falco/falco_rules.local.yaml:43:6)
------
    - name: invalid exception
      ^
------
LOAD_ERR_VALIDATE (Error validating rule/macro/list/exception objects): 'not-a-field' is not a supported filter field

1 Warnings:
In rule 'My Warning Rule': (/etc/falco/falco_rules.local.yaml:31:2)
------
- rule: My Warning Rule
  ^
------
LOAD_NO_EVTTYPE (Condition has no event-type restriction): Rule matches too many evt.type values. This has a significant performance penalty.

Error: /etc/falco/falco_rules.yaml: Ok
/etc/falco/falco_rules.local.yaml: Invalid

And if you add -o json_output=True you get a json version of the errors + warnings instead. The result is printed to stdout, so if you redirect stderr you get only the validation result. This matches older falco behavior:

$ ./userspace/falco/falco -V /etc/falco/falco_rules.yaml -V /etc/falco/falco_rules.local.yaml -v -o json_output=True 2>/dev/null | jq "."
{
  "falco_load_results": [
    {
      "errors": [],
      "name": "/etc/falco/falco_rules.yaml",
      "successful": true,
      "warnings": []
    },
    {
      "errors": [
        {
          "code": "LOAD_ERR_VALIDATE",
          "codedesc": "Error validating rule/macro/list/exception objects",
          "context": {
            "locations": [
              {
                "item_name": "My Invalid Rule",
                "item_type": "rule",
                "position": {
                  "column": 2,
                  "filename": "/etc/falco/falco_rules.local.yaml",
                  "line": 37,
                  "offset": 1246
                }
              },
              {
                "item_name": "invalid exception",
                "item_type": "exception",
                "position": {
                  "column": 6,
                  "filename": "/etc/falco/falco_rules.local.yaml",
                  "line": 43,
                  "offset": 1374
                }
              }
            ],
            "snippet": "    - name: invalid exception\n      ^\n"
          },
          "message": "'not-a-field' is not a supported filter field"
        }
      ],
      "name": "/etc/falco/falco_rules.local.yaml",
      "successful": false,
      "warnings": [
        {
          "code": "LOAD_NO_EVTTYPE",
          "codedesc": "A rule condition matches too many evt.type values. This has a significant performance penalty. Make the condition more specific by adding an evt.type field or further restricting the number of evt.type values in the condition.",
          "context": {
            "locations": [
              {
                "item_name": "My Warning Rule",
                "item_type": "rule",
                "position": {
                  "column": 2,
                  "filename": "/etc/falco/falco_rules.local.yaml",
                  "line": 31,
                  "offset": 1139
                }
              }
            ],
            "snippet": "- rule: My Warning Rule\n  ^\n"
          },
          "message": "Rule matches too many evt.type values. This has a significant performance penalty."
        }
      ]
    }
  ]
}

mstemm · 2022-06-30T15:44:19Z

Loading rules is pretty much the same, although you only get a terse error message on error:

./userspace/falco/falco -r /etc/falco/falco_rules.yaml -r /etc/falco/falco_rules.local.yaml
Thu Jun 30 08:43:05 2022: Falco version 0.0.0
Thu Jun 30 08:43:05 2022: Falco initialized with configuration file /etc/falco/falco.yaml
Thu Jun 30 08:43:05 2022: Loading rules from file /etc/falco/falco_rules.yaml:
Thu Jun 30 08:43:05 2022: Loading rules from file /etc/falco/falco_rules.local.yaml:
Error: /etc/falco/falco_rules.local.yaml: Invalid
 1 errors: [LOAD_ERR_VALIDATE (Error validating rule/macro/list/exception objects)]
 1 warnings: [LOAD_NO_EVTTYPE (Condition has no event-type restriction)]

And human-readable details with -v:

./userspace/falco/falco -r /etc/falco/falco_rules.yaml -r /etc/falco/falco_rules.local.yaml -v
Thu Jun 30 08:43:38 2022: Falco version 0.0.0
Thu Jun 30 08:43:38 2022: Falco initialized with configuration file /etc/falco/falco.yaml
Thu Jun 30 08:43:38 2022: Loading rules from file /etc/falco/falco_rules.yaml:
Thu Jun 30 08:43:38 2022: Loading rules from file /etc/falco/falco_rules.local.yaml:
/etc/falco/falco_rules.local.yaml: Invalid
1 Errors:
In rule 'My Invalid Rule': (/etc/falco/falco_rules.local.yaml:37:2)
   exception 'invalid exception': (/etc/falco/falco_rules.local.yaml:43:6)
------
   - name: invalid exception
     ^
------
LOAD_ERR_VALIDATE (Error validating rule/macro/list/exception objects): 'not-a-field' is not a supported filter field

1 Warnings:
In rule 'My Warning Rule': (/etc/falco/falco_rules.local.yaml:31:2)
------
- rule: My Warning Rule
 ^
------
LOAD_NO_EVTTYPE (Condition has no event-type restriction): Rule matches too many evt.type values. This has a significant performance penalty.

Error: /etc/falco/falco_rules.local.yaml: Invalid
1 errors: [LOAD_ERR_VALIDATE (Error validating rule/macro/list/exception objects)]
1 warnings: [LOAD_NO_EVTTYPE (Condition has no event-type restriction)]

jasondellaluce

Thanks for this effort Mark! I tried my best to review this, left you some comments.

On top of that, I think we also need to change the documentation here:

falco/falco.yaml

Line 77 in 35db0b4

json_output: false

json_output now does more than just formatting rule outputs, so I think we should document it properly. Alternatively, we should further separate responsibilities and have a separate config field like json_load_result.

userspace/engine/falco_load_result.h

jasondellaluce · 2022-07-12T11:04:54Z

userspace/engine/falco_load_result.h

+#include <nlohmann/json.hpp>
+
+// Represents the result of loading a rules file.
+class falco_load_result {


I would suggest renaming this into load_result.

Sure, I think we should put it in a falco namespace though as load_result by itself is generic. That brings up the question of whether we should add a namespace to falco_engine as well? The engine doesn't have any namespace at the moment, which is why I didn't use one for falco_load_result.

I don't want to get carried away with changes, so how about this--in this PR I simply add a falco namespace around load_result, and once this is merged we add a top level namespace to falco_engine in a follow-on PR? Maybe with an additional rename of the falco_engine class, we can bikeshed about it in the follow-on PR.

userspace/engine/falco_load_result.h

userspace/engine/rule_loader.h

test/falco_tests.yaml

jasondellaluce · 2022-07-18T10:08:57Z

test/falco_tests.yaml

+        item_name: some macro
+        code: FE_LOAD_ERR_VALIDATE
+        message: "Undefined macro 'foo' used in filter."
+    validate_warnings:


This probably connects to my other comment: here the warning is not meaningful. We should have failed at the first error.

Although I didn't want to tackle it in this PR, which is already quite big, I was hoping that someday rules loading would not always fail at the first error. The exception-based method is a good way to stop loading and unwind to a top-level location, but it will only show the first problem, not all of the potential problems in a set of rules.

With this version, the result contains any warnings up until the first error, or all warnings if there are no errors.

I think having the validate_warnings/validate_errors properties as a list will support a future change to return multiple errors/warnings if we decide to change the implementation. We'd just have to change the expected values for any tests that have multiple errors, or additional warnings after any errors.

Does that sound ok?

I see your point. I agree that validate_warnings and validate_errors must be lists to support multiple warnings and errors in the future, however I think changing our "fail at first error" model might be too much for a single PR. I'd suggest keeping the failure condition as-is for now, and eventually change it in a separate PR in which we document the rationale. Note that this would probably introduce user-facing breaking changes, so we might be to be careful about it!

Yeah I agree on not changing the rules loading code, it will not strive to return all errors. But I think it's fine to keep the test code the same, right? That's only visible internally and end users won't see the test code.

test/falco_tests.yaml

jasondellaluce · 2022-07-18T10:15:20Z

test/falco_test.py

+                for vres in vobj["falco_load_results"]:
+                    for warning in vres["warnings"]:
+                        if warning["code"] == warnobj["code"]:
+                            if ("message" in warnobj and warning["message"] == warnobj["message"]) or ("message_contains" in warnobj and warnobj["message_contains"] in warning["message"]):


So basically message and message_contains seem to be mandatory, right? Should we make it optional? Should we document this somewhere?

message_contains is only used for the test engine_version_mismatch, where the error message will be dependent on the current embedded falco engine version e.g. "Rules require engine version 9999999, but engine version is 14". If it's a big deal I could switch everything to be a substring match, but I thought the distinction was useful.

I believe all of the other error messages are static--they used to contain "in rule xxx" snippets but all of that info is in the context instead. The engine version is a strange dynamic thing that doesn't really fit into a context.

Related to the changes in falcosecurity/falco#2098, update the docs for the json_output config option to note that it controls both the output format for falco alerts as well as the format of rules loading/validation results. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

poiana · 2022-08-02T15:51:32Z

LGTM label has been added.

Git tree hash: 6fa1cdef4ca1b62680725be188d3a1a553cf5ff3

leogr

Hey @mstemm

Thank for this PR. It's a significant improvement.

Overall, it looks good to me. However, I've got a concern.
I don't see any real value in having non-verbose messages. The old behavior when loading rules was always to print verbose messages. It was advantageous. Think users that just copy/paste their Pod logs in a GitHub issue. Forcing them to redeploy Falco with -v would waste time.

That being said, I approve this PR anyway because I think it's valuable. Yet, I'd invite all @falcosecurity/falco-maintainers to reconsider verbosity mode when loading/validating rules. I believe verbosity should be enabled by default in those cases, or the previous behavior should be kept.

/approve

poiana · 2022-08-04T12:48:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasondellaluce, leogr, 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~~ [jasondellaluce,leogr,mstemm]

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

mstemm · 2022-08-04T14:54:17Z

Hey @mstemm

Thank for this PR. It's a significant improvement.

Overall, it looks good to me. However, I've got a concern. I don't see any real value in having non-verbose messages. The old behavior when loading rules was always to print verbose messages. It was advantageous. Think users that just copy/paste their Pod logs in a GitHub issue. Forcing them to redeploy Falco with -v would waste time.

That being said, I approve this PR anyway because I think it's valuable. Yet, I'd invite all @falcosecurity/falco-maintainers to reconsider verbosity mode when loading/validating rules. I believe verbosity should be enabled by default in those cases, or the previous behavior should be kept.

/approve

You're right about verbose--I really thought that the old behavior was to only print full details on the error when verbose was true. But I double-checked and 0.32.1 does print details both for -r and -V. So that's a regression and I'll change that in a follow-on (small) PR.

leogr · 2022-08-04T16:14:47Z

You're right about verbose--I really thought that the old behavior was to only print full details on the error when verbose was true. But I double-checked and 0.32.1 does print details both for -r and -V. So that's a regression and I'll change that in a follow-on (small) PR.

Thank you, Mark! 🙏

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>

The latest released falco always prints full details on errors when used with -r (read rules)/-V (validate rules). However #2098 changed this to only print full details when verbose is true. Fix the regression by always printing full details regardless of verbose/-v. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

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>

The latest released falco always prints full details on errors when used with -r (read rules)/-V (validate rules). However #2098 changed this to only print full details when verbose is true. Fix the regression by always printing errors when loading rules. Warnings will be printed only with -v. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Related to the changes in falcosecurity/falco#2098, update the docs for the json_output config option to note that it controls both the output format for falco alerts as well as the format of rules loading/validation results. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

The latest released falco always prints full details on errors when used with -r (read rules)/-V (validate rules). However #2098 changed this to only print full details when verbose is true. Fix the regression by always printing errors when loading rules. Warnings will be printed only with -v. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

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>

Related to the changes in falcosecurity/falco#2098, update the docs for the json_output config option to note that it controls both the output format for falco alerts as well as the format of rules loading/validation results. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

poiana added release-note dco-signoff: no kind/feature area/engine area/tests labels Jun 28, 2022

mstemm added the do-not-merge/work-in-progress label Jun 28, 2022

poiana added the size/XXL label Jun 28, 2022

poiana requested review from jasondellaluce and leogr June 28, 2022 16:56

poiana added the approved label Jun 28, 2022

mstemm force-pushed the refactor-rules-loading-result branch 3 times, most recently from fb55b0f to 3ca9396 Compare June 29, 2022 00:19

poiana added this to the 0.33.0 milestone Jun 29, 2022

mstemm force-pushed the refactor-rules-loading-result branch from 3ca9396 to 80f96d9 Compare June 29, 2022 19:55

mstemm force-pushed the refactor-rules-loading-result branch 4 times, most recently from 2ddb743 to a0d4d11 Compare June 29, 2022 23:46

jasondellaluce reviewed Jul 18, 2022

View reviewed changes

mstemm force-pushed the refactor-rules-loading-result branch from a0d4d11 to 0f601de Compare July 25, 2022 17:12

poiana added dco-signoff: yes and removed dco-signoff: no labels Jul 25, 2022

mstemm mentioned this pull request Jul 25, 2022

Update docs for json_output option to reflect alerts+rules result falcosecurity/falco-website#657

Merged

mstemm removed the do-not-merge/work-in-progress label Aug 2, 2022

jasondellaluce mentioned this pull request Aug 3, 2022

improve falco files loading performance #2151

Merged

leogr approved these changes Aug 4, 2022

View reviewed changes

poiana assigned leogr Aug 4, 2022

poiana merged commit a37e225 into master Aug 4, 2022

poiana deleted the refactor-rules-loading-result branch August 4, 2022 12:49

mstemm mentioned this pull request Aug 4, 2022

Support condition parse errors in rule loading results #2155

Merged

mstemm mentioned this pull request Aug 4, 2022

fix: print full rule load errors/warnings without verbose/-v #2156

Merged

jasondellaluce mentioned this pull request Sep 12, 2022

fix(userspace/engine): avoid reading duplicate exception values #2200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor rules loading result #2098

Refactor rules loading result #2098

mstemm commented Jun 28, 2022 •

edited by leogr

Loading

jasondellaluce commented Jun 29, 2022

mstemm commented Jun 29, 2022

mstemm commented Jun 29, 2022 •

edited

Loading

mstemm commented Jun 30, 2022 •

edited

Loading

mstemm commented Jun 30, 2022 •

edited

Loading

jasondellaluce left a comment

jasondellaluce Jul 12, 2022

mstemm Jul 25, 2022 •

edited

Loading

jasondellaluce Jul 18, 2022

mstemm Jul 25, 2022

jasondellaluce Jul 28, 2022

mstemm Jul 28, 2022

jasondellaluce Jul 18, 2022

mstemm Jul 25, 2022

poiana commented Aug 2, 2022

leogr left a comment

poiana commented Aug 4, 2022

mstemm commented Aug 4, 2022

leogr commented Aug 4, 2022

Refactor rules loading result #2098

Refactor rules loading result #2098

Conversation

mstemm commented Jun 28, 2022 • edited by leogr Loading

jasondellaluce commented Jun 29, 2022

mstemm commented Jun 29, 2022

mstemm commented Jun 29, 2022 • edited Loading

mstemm commented Jun 30, 2022 • edited Loading

mstemm commented Jun 30, 2022 • edited Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

jasondellaluce Jul 12, 2022

Choose a reason for hiding this comment

mstemm Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

jasondellaluce Jul 18, 2022

Choose a reason for hiding this comment

mstemm Jul 25, 2022

Choose a reason for hiding this comment

jasondellaluce Jul 28, 2022

Choose a reason for hiding this comment

mstemm Jul 28, 2022

Choose a reason for hiding this comment

jasondellaluce Jul 18, 2022

Choose a reason for hiding this comment

mstemm Jul 25, 2022

Choose a reason for hiding this comment

poiana commented Aug 2, 2022

leogr left a comment

Choose a reason for hiding this comment

poiana commented Aug 4, 2022

mstemm commented Aug 4, 2022

leogr commented Aug 4, 2022

mstemm commented Jun 28, 2022 •

edited by leogr

Loading

mstemm commented Jun 29, 2022 •

edited

Loading

mstemm commented Jun 30, 2022 •

edited

Loading

mstemm commented Jun 30, 2022 •

edited

Loading

mstemm Jul 25, 2022 •

edited

Loading