New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

new(engine): add selective rule overrides #2981

Merged

poiana merged 7 commits into falcosecurity:master from LucaGuerra:new/rule-override

Dec 22, 2023

Contributor

LucaGuerra commented Dec 19, 2023 •

edited

Loading

What type of PR is this?

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

/kind feature

Any specific area of the project related to this PR?

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

/area engine

/area tests

What this PR does / why we need it:

This is a rather straightforward implementation of #1340 (comment) . See the comment for examples.

It is now possible to use the override key in rules, lists and macros. Some fields can be overridden: for lists and macros there is only one field that we care about, while for rules, the following fields have been selected:

Available for append: {"condition", "output", "desc", "tags", "exceptions"}
Available for replace: {"condition", "output", "desc", "priority", "tags", "exceptions", "enabled", "warn_evttypes", "skip-if-unknown-filter"}

As per the discussion linked above, it is an error to specify both append: true and any override. Given the syntax it is not possible to attempt to both replace and append the same field in the same entry.

Also, if you specify the override key you must specify all the fields that you wish to override and only those fields.

Which issue(s) this PR fixes:

Fixes #1340

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(engine): add selective overrides for Falco rules

Also, this needs to be documented ( cc @mikegcoleman )

LucaGuerra force-pushed the new/rule-override branch from 9510569 to 19debcc Compare

December 19, 2023 18:38

poiana added release-note do-not-merge/work-in-progress kind/feature size/XXL dco-signoff: yes area/engine area/tests labels

poiana requested review from Andreagit97 and Kaizhe

December 19, 2023 18:38

poiana added the approved label

github-actions bot commented Dec 19, 2023

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

poiana added the do-not-merge/hold label

Contributor Author

LucaGuerra commented Dec 19, 2023

/milestone 0.37.0

poiana added this to the 0.37.0 milestone

Contributor

incertum commented Dec 20, 2023

Thank you @LucaGuerra could you let me know when it would be ready to test drive it?

LucaGuerra force-pushed the new/rule-override branch from 19debcc to 997e5bd Compare

December 20, 2023 08:47

LucaGuerra commented

View reviewed changes

userspace/engine/rule_loader_collector.cpp Outdated

               {
               	auto prev = m_list_infos.at(info.name);
               	THROW(!prev,
-              	       "List has 'append' key but no list by that name already exists",
+              	       "List has 'append' key or an append override but no list by that name already exists",

Contributor Author

LucaGuerra Dec 20, 2023

Note for the reviewer: this string change is making our testing pipelines fail. No big deal, once we pick the right string I can update the pipelines.

Contributor Author

LucaGuerra Dec 22, 2023

I have updated the code to revert the error messages, I really want to be sure that our tests are happy before merging. Messages can be updated in a subsequent PR coordinated with testing.

LucaGuerra commented

View reviewed changes

userspace/engine/rule_loader_collector.cpp Show resolved Hide resolved

LucaGuerra commented

View reviewed changes

userspace/engine/rule_loader.h Show resolved Hide resolved

LucaGuerra force-pushed the new/rule-override branch 2 times, most recently from dcb1205 to 57a5ce5 Compare

December 20, 2023 11:34

LucaGuerra marked this pull request as ready for review

December 20, 2023 11:34

poiana requested a review from FedeDP

December 20, 2023 11:34

LucaGuerra changed the title ~~wip: new(engine): add selective overrides~~ new(engine): add selective overrides

poiana removed the do-not-merge/work-in-progress label

Contributor Author

LucaGuerra commented Dec 20, 2023

@incertum if you are able to test this it would be appreciated :)

leogr changed the title ~~new(engine): add selective overrides~~ new(engine): add selective rule overrides

incertum reviewed

View reviewed changes

Contributor

incertum left a comment

First pass review focused on the tests. Could we add some more? See comment.

Code implementation LGTM and @jasondellaluce already approved the approach as well 😄.

unit_tests/engine/test_rule_loader.cpp

+              #include "rule_loader_reader.h"
+              #include "rule_loader_compiler.h"
+              class engine_loader_test : public ::testing::Test {

Contributor

incertum Dec 21, 2023

Very neat setup @LucaGuerra much more elegant ❤️ than what I had tried in the past 🤦‍♀️

Contributor Author

LucaGuerra Dec 22, 2023

I noticed we actually do something similar in at least three different locations in the test suite (including this one). This is out of scope for this PR but we need to consolidate them and have one fixture to allow loading engine + rulesets.

Contributor

incertum Dec 22, 2023

Big +1, it would be super useful to do this cleanup!

unit_tests/engine/test_rule_loader.cpp Show resolved Hide resolved


          new(engine): add selective overrides

8f68987

Signed-off-by: Luca Guerra <luca@guerra.sh>

LucaGuerra added 3 commits

December 22, 2023 16:17


          update(engine): clarify override error messages

2b32858

Signed-off-by: Luca Guerra <luca@guerra.sh>


          chore(engine): bump engine version

81e9568

Signed-off-by: Luca Guerra <luca@guerra.sh>


          update(engine): throw an error if an unexpected top level key is foun…

5162bc0

…d in an override

Signed-off-by: Luca Guerra <luca@guerra.sh>

LucaGuerra force-pushed the new/rule-override branch from 57a5ce5 to aafc711 Compare

December 22, 2023 16:17

Contributor

incertum commented Dec 22, 2023

Also, for more complex cases (e.g. enabling/disabling with tags) I think they may be out of scope for this PR and if you can think about unit or integration tests for those cases it would be great to add them.

Agreed, just wanted to mention it once again 🙃

incertum reviewed

View reviewed changes

unit_tests/engine/test_rule_loader.cpp Outdated Show resolved Hide resolved

incertum reviewed

View reviewed changes

unit_tests/engine/test_rule_loader.cpp

+              	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
+              	// std::cout << m_load_result_json.dump(4) << std::endl;
+              	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("no rule by that name already exists") != std::string::npos);

Contributor

incertum Dec 22, 2023

[nit] you are overriding a rule, but no rule by that name already exists or similar

Contributor Author

LucaGuerra Dec 22, 2023

This is an "error message contains" not equals :)

Contributor

incertum Dec 22, 2023

:)

incertum reviewed

View reviewed changes

unit_tests/engine/test_rule_loader.cpp

+              	std::string rule_name = "failing_rule";
+              	ASSERT_FALSE(load_rules(rules_content, "rules.yaml"));
+              	ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("Unexpected key 'priority'") != std::string::npos);

Contributor

incertum Dec 22, 2023

[nit] same can we make the error message more expressive?

LucaGuerra added 3 commits

December 22, 2023 17:16


          update(engine): temporary replace for error messages

471870e

Signed-off-by: Luca Guerra <luca@guerra.sh>


          new(tests): add error testing for rule overrides

37bf76b

Signed-off-by: Luca Guerra <luca@guerra.sh>


          fix(engine): clarify error message for invalid append

ca6d383

Signed-off-by: Luca Guerra <luca@guerra.sh>

LucaGuerra force-pushed the new/rule-override branch from aafc711 to ca6d383 Compare

December 22, 2023 17:16

incertum approved these changes

View reviewed changes

Contributor

incertum left a comment

/approve

Wonderful work @LucaGuerra, thank you!

poiana assigned incertum

poiana added the lgtm label

Contributor

poiana commented Dec 22, 2023

LGTM label has been added.

Git tree hash: 417d7652d4a972fbad0cae0b2c25f0aab668b43b

jasondellaluce approved these changes

View reviewed changes

Contributor

jasondellaluce left a comment

/approve

/hold

Super job Luca! I'm approving and holding just so that you can decide when you feel confident in merging this, but it looks great to me.

poiana assigned jasondellaluce

Contributor

poiana commented Dec 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum, jasondellaluce, LucaGuerra

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~~ [LucaGuerra,incertum,jasondellaluce]

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

Contributor

incertum commented Dec 22, 2023

/unhold

Had spoken with Luca on slack. We are good to merge it.

poiana removed the do-not-merge/hold label

poiana merged commit 728c8d7 into falcosecurity:master

29 of 30 checks passed

LucaGuerra deleted the new/rule-override branch

December 22, 2023 23:24

Contributor

incertum commented Dec 22, 2023

The following 2 issues were duplicates of the primary issue #1340:

Link them for tracking.

mikegcoleman mentioned this pull request

Need documentation for Selective Override (Falco PR# 2981) falcosecurity/falco-website#1226

Closed

mikegcoleman commented Dec 24, 2023

Added a docs issue to track the documentation need

falcosecurity/falco-website#1226

Andreagit97 mentioned this pull request

[TRACKING] Breaking changes in Falco 0.37.0 #2763

Closed

6 tasks

pmusa commented Feb 1, 2024

I think this part also needs to be updated:
https://falco.org/docs/rules/controlling-rules/#via-custom-rule-definition

leogr mentioned this pull request

falcoSchema.json should be generated by Falco itself #3149

Closed

leogr mentioned this pull request

Access to more variables in output_fields #2127

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasondellaluce jasondellaluce approved these changes

incertum incertum approved these changes

Andreagit97 Awaiting requested review from Andreagit97

Kaizhe Awaiting requested review from Kaizhe

FedeDP Awaiting requested review from FedeDP

Labels

approved area/engine area/tests dco-signoff: yes kind/feature lgtm release-note size/XXL