-
Notifications
You must be signed in to change notification settings - Fork 902
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.
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
[closed in favor of a new follow up PR] cleanup/new/feat(load-rules): expand rules append support and feature partial overrides #2671
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: incertum 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:
Approvers can indicate their approval by writing |
/milestone 0.36.0 |
5cbff17
to
a54d7d7
Compare
@jasondellaluce and @leogr these features have been requested by 3+ adopters and I am also supportive especially now that we introduce the rules maturity framework and we will disable all non stable rules by default. Hence we urgently need easier customization options for folks who prefer not to create a new rules file. Less concerned about the actual code implementation than the capability, hence happy to make any changes. @jasondellaluce we could either also have you do the fixes for the tags + enable filters here or in a follow up, whichever you prefer. Lastly, would you have more test ideas? |
Hey @incertum I understand the importance of this but I'm not totally sure I understand what this feature does. Could you summarize it, please? Also, why do we need Finally, two tests are currently not passing, PTAL 🙏 |
…mode Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
* backward compatibility (maintaining same definition of final valid rule) * new strict unit tests * new INVALID priority category was needed Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
…d mode Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
…d mode without other keys Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
c72f6e7
to
94f62e5
Compare
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 |
Let's say
Adopter wants to change the priority and add an output field, but Falco currently does not allow it in Falco 0.35.1 :/ Instead adopter needs to copy and paste the entire rule in their custom rules file like this, defying the purposes of supporting
To make matters more confusing e.g.:
was possible but nothing else was. Adopters asked to support all possible scenarios of appending to and/or overriding a previous key when providing a new possibly incomplete rules definition while ensuring we still have one complete Falco rule at the end. I hope the
|
The report is available in the PR's workflow summary: https://github.com/falcosecurity/falco/actions/runs/5764238799?pr=2671. For testing it locally, you follow the instructions on https://github.com/falcosecurity/testing, and use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave my thoughts here.
From my understanding, this PR is bringing different kinds of contributions:
- Extenting support to
append
for rules only, fromcondition
andexceptions
to alsotags
andoutput
- Allow partial definitions of rules only, merging them with definitions from before
I'm onboard with point (1), it's a nice and long-requested feature. I would honestly also append to desc
at this point
Point (2) is instead confusing to me in this form, and also a breaking change. Consider this case
- rule: Rule 1
condition: evt.type=open
desc: Desc of Rule 1
output: Output of Rule 1
priority: WARNING
exceptions: [...]
- rule: Rule 1
condition: evt.type=openat
desc: Desc of Rule 1
output: Output of Rule 1
priority: CRITICAL
Up until now, this was an acceptable input, and Rule 1
would end up having no exceptions
, as being re-defined without them. This is not a case for just exceptions
, but for all the non-mandatory fields (e.g. tags
, warn_evttypes
, skip-if-unknown-filter
, exceptions
), which ended up having the default value for all those fields if re-defined without them being explicit. This is a change in behavior.
Also, merging while also append
-ing seems confusing to me. What gets appended and what doesn't? And what's the logic for that? If both are defined, who takes priority?
My proposal is instead to make merging explicit and mutually-explusive with appending. In this way, the current behavior will always be preserved, but you could have something like:
- rule: Rule 1
condition: evt.type=open
desc: Desc of Rule 1
output: Output of Rule 1
priority: WARNING
exceptions: [...]
- rule: Rule 1
condition: evt.type=openat
merge: true
- rule: Rule 1
priority: CRITICAL
merge: true
- rule: Rule 1
condition: and proc.name=cat
append: true
This way:
- All previous behavior will be left unchanged
append
will not work together withmerge
, and will only supportcondition
,exceptions
,output
, andtags
- When
enabled
is defined only, it will be interpreted as havingmerge: true
implicitly for backward-compatibility reasons (but it should raise a warning IMO) merge
will always require a previous definitions to be present, just likeappend
, and will not cause unpredictable behavior when changing loading order of rules files
This is much more clear to read, implement, understand for newcomers. The only downside that I see is that you'll need two distinct definitions for merging and appending if you need to do both, which I think is an acceptable tradeoff.
Let's gather more feedback from the adopters who need a very convenient solution @tspearconquest and @jonny-wg2? Thanks in advance! I do understand that the separation is cleaner in terms of software engineering, however while some improvement it may still not be the best UX. Meaning if (as you pointed out) I need to re-define one append field and one partial override field for one upstream rule I need to re-define it twice in my custom files. With the current changes I can do it in one swing. Re the exceptions case, hmmm upstream rules have no exceptions at the moment right? Why would someone write a custom rule with an exception / aka a non required field to then re-define it again. Is it an edge case we want to sign up to support in general? Could there be another way to handle the exception cases to support a one swing re-definition of previous rules?
In summary, if we could customize an upstream rule with just one re-definition, I think even I would consider giving it a try, else I likely stick with a custom approach of just re-writing the entire rules files. |
@jasondellaluce maybe add a third option |
|
After reading the conversation and finally understanding the situation, I tend to agree with @jasondellaluce . In particular, this feature must not introduce any backward incompatible change. To avoid disrupting our adopters, supporting the previous behavior is a must-have, IMO. (If in the future we want to review the whole syntax because we are not satisfied with it, it would ok but we will also have to think about proper versioning and a deprecation plan; but this should be a separate discussion). Recap
👍
👍
👍
👍 For example, assuming
However, if Other considerations -Do not forget the
For example, I think this is easy to read and understand:
Rather than this other:
Stating that I agree with introducing |
Looking at the example here: #2671 (review) - rule: Rule 1
condition: evt.type=open
desc: Desc of Rule 1
output: Output of Rule 1
priority: WARNING
exceptions: [...]
- rule: Rule 1
condition: evt.type=openat
merge: true
- rule: Rule 1
priority: CRITICAL
merge: true
- rule: Rule 1
condition: and proc.name=cat
append: true I've not fully understood this - rule: Rule 1
condition: evt.type=openat
merge: true # <--- What does this `merge` do on the rule? using this partial rule should I expect the original
BTW not sure why we use a boolean for Final question about always the same yaml snippet. I imagine that we could put together these 2 block in just one, right? 👇 - rule: Rule 1
condition: evt.type=openat
merge: true
- rule: Rule 1
priority: CRITICAL
merge: true - rule: Rule 1
condition: evt.type=openat
priority: CRITICAL
merge: true |
Yes. I'm totally open to better names, but you got the idea.
I like this too! However, we'd still need to support
Yes. |
Thank you, yes i would go with something like
Yeah maybe we could depracte the actual
ty! |
Top Level Requirement: Adopters can customize a rule by re-defining it just once, not twice. This would be best UX from my perspective (being an adopter myself), because it is a lot of overhead to maintain Falco rules. Can we overload After reading all new feedback, sharing my new favorite I would recommend. It may be the most powerful one covering all possible use cases in a non ambiguous way for once and it would only require a one time re-definition of a previously defined rule. It would also more naturally align with database concepts where you always define the A mix match example:
Alternatively instead of overloading
Other new suggestions to accomplish all goals with just one re-definition? |
@jasondellaluce @leogr @Andreagit97 I am out at a conference until end of week. |
What type of PR is this?
/kind cleanup
/kind feature
Any specific area of the project related to this PR?
/area engine
What this PR does / why we need it:
Expand rules append support and feature partial overrides / merges
Which issue(s) this PR fixes:
Consolidated issue #1340.
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: