Skip to content
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: allow falco to match multiple rules on same event #2705

Merged
merged 9 commits into from
Aug 9, 2023
Merged

new: allow falco to match multiple rules on same event #2705

merged 9 commits into from
Aug 9, 2023

Conversation

loresuso
Copy link
Member

@loresuso loresuso commented Jul 28, 2023 •
edited
Loading

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

Any specific area of the project related to this PR?

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

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:
This PR adds a configuration key to Falco to let it match multiple rules on the same event, avoiding a well-known shadowing problem between rules. To not introduce breaking changes, the old behavior is kept and can still be used (expressed trough config). To implement this, Falco will simply continue trying to evaluate the condition of rules in the bucket of same evt.type rules, without stopping at the first match. Thinking about it, this should not cause any performance degradation, since the rules in the bucket are always evaluated in their entirety whenever there's no matching rule, and the cardinality of events not matching rules compared to the ones that do is orders of magnitude higher (meaning that evaluating all conditions of rules in the same bucket is something that we do all the time, so we can afford to do it even when a rule in a bucket is matching).

Putting this on wip while figuring out how to introduce some tests, but open to discuss!

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: allow falco to match multiple rules on same event

@poiana poiana requested review from FedeDP and Kaizhe July 28, 2023 13:41
@poiana poiana added the size/L label Jul 28, 2023
loresuso
loresuso commented Jul 28, 2023
View reviewed changes
falco.yaml Outdated Show resolved Hide resolved
@loresuso loresuso force-pushed the update/match-multiple-rules branch 5 times, most recently from 2d359a6 to 49f93cc Compare July 28, 2023 14:19
jasondellaluce
jasondellaluce reviewed Jul 28, 2023
View reviewed changes
userspace/engine/falco_engine.cpp Outdated Show resolved Hide resolved
userspace/engine/falco_engine.cpp Outdated Show resolved Hide resolved
@loresuso loresuso force-pushed the update/match-multiple-rules branch from 49f93cc to 6376ab3 Compare July 28, 2023 16:08
@loresuso loresuso mentioned this pull request Aug 2, 2023
Merged
@loresuso loresuso force-pushed the update/match-multiple-rules branch 2 times, most recently from 80e59c7 to dc5ebd6 Compare August 2, 2023 13:27
@loresuso loresuso changed the title wip: new: allow falco to match multiple rules on same event new: allow falco to match multiple rules on same event Aug 2, 2023
@github-actions
Copy link

github-actions bot commented Aug 8, 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

@loresuso loresuso force-pushed the update/match-multiple-rules branch from adc39db to 681e1ad Compare August 8, 2023 11:18
@jasondellaluce
Copy link
Contributor

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.

This is right, we're effectively adding a new feature to the engine and this should have a version bump.

@loresuso loresuso force-pushed the update/match-multiple-rules branch from 0c87d19 to 4185f8a Compare August 8, 2023 13:44
loresuso added 4 commits August 8, 2023 13:52
@loresuso
perf: avoid stack allocation and make use of switch to select behavio…
6c9f76b 
…r on rule matching strategy

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
@loresuso
update(falco.yaml): update rule_matching configuration setting descri…
9fe5755 
…ption

Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
@loresuso
update(userspace/engine): bump engine version to 21
de615bc 
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
@loresuso
update: bump falcosecurity/testing
85395d1 
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
@loresuso loresuso force-pushed the update/match-multiple-rules branch from 4185f8a to 85395d1 Compare August 8, 2023 13:54
@loresuso
update(userspace/engine): bump engine version to 22
8e55f10 
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
jasondellaluce
jasondellaluce previously approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@jasondellaluce jasondellaluce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Engine version has been bumped.

/unhold

@poiana poiana added the lgtm label Aug 8, 2023
@poiana
Copy link
Contributor

poiana commented Aug 8, 2023

LGTM label has been added.

Git tree hash: de83051a33205a8c91078ea85f7e0c4bd1f25771

@poiana poiana added the approved label Aug 8, 2023
@jasondellaluce
Copy link
Contributor

/milestone 0.36.0

@poiana poiana added this to the 0.36.0 milestone Aug 8, 2023
Andreagit97
Andreagit97 reviewed Aug 8, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want some unit tests on it or do we think e2e tests are enough @jasondellaluce?

userspace/engine/evttype_index_ruleset.cpp Outdated Show resolved Hide resolved
userspace/engine/falco_common.cpp Show resolved Hide resolved
userspace/engine/falco_engine.cpp Show resolved Hide resolved
userspace/engine/falco_engine.h Show resolved Hide resolved
@loresuso @Andreagit97
update: directly return match_found variable
1a8d7d8 
Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Lorenzo Susini <49318629+loresuso@users.noreply.github.com>
@poiana poiana removed the lgtm label Aug 8, 2023
@poiana poiana requested a review from jasondellaluce August 8, 2023 15:32
Andreagit97
Andreagit97 approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana poiana added the lgtm label Aug 9, 2023
@poiana
Copy link
Contributor

poiana commented Aug 9, 2023

LGTM label has been added.

Git tree hash: 7d862e87c59a59521718fef65e6fb05277ccdd90

@poiana
Copy link
Contributor

poiana commented Aug 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, jasondellaluce, loresuso

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 [Andreagit97,jasondellaluce]

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

@poiana poiana merged commit 6e50d2a into falcosecurity:master Aug 9, 2023
This was referenced Aug 9, 2023
Closed
Closed
@RomanenkoDenys RomanenkoDenys mentioned this pull request Sep 6, 2024
Merged
4 tasks
@deckhouse-BOaTswain deckhouse-BOaTswain mentioned this pull request Sep 24, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@jasondellaluce jasondellaluce jasondellaluce approved these changes

@FedeDP FedeDP Awaiting requested review from FedeDP

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@jasondellaluce jasondellaluce

@Andreagit97 Andreagit97

Labels
approved dco-signoff: yes kind/feature lgtm release-note size/L
Projects
Falco Roadmap
Archived in project
Milestone
0.36.0
Development

Successfully merging this pull request may close these issues.
4 participants
@loresuso @jasondellaluce @poiana @Andreagit97