update: split falco_rules.yaml according to the rules maturity
#149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
poiana
added
kind/feature
leogr
force-pushed
the
update/rules-files-split
branch
5 times, most recently
from
525e6f5
to
b76175a
Compare
leogr
changed the title
falco_rules.yaml according to the rules matority
This stricter naming convention is required to avoid corner cases in CI and the distribution system while providing a simple guideline for contributors. Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
leogr
force-pushed
the
update/rules-files-split
branch
from
b76175a
to
92c00b7
Compare
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
leogr
force-pushed
the
update/rules-files-split
branch
from
4cdd557
to
b75d9ec
Compare
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
leogr
changed the title
falco_rules.yaml according to the rules matorityfalco_rules.yaml according to the rules maturity
incertum
force-pushed
the
update/rules-files-split
branch
from
fc737e2
to
dd17829
Compare
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
|
@leogr 🚀 did a first pass and verified we didn't loose any rule or don't have a duplicated rules. Will do a second pass review later. Agreed with naming convention, looks good! |
|
@leogr just reviewed again and LGTM! Seems we still need some more CI adjustments. I think this is a great idea for now to duplicate the macros and lists in the respective rules files to ensure that they are self contained. In the future we can think of better ways to reduce duplication. You also mentioned that the comments within the rules files are inconsistent -- I do agree, it would be a much appreciated follow up cleanup. In that line, perhaps should we have all macros first then all lists vs now some are at the top and some right before the rule uses the macro (mostly when its a very specific macro)? WDYT? Maybe even have the rules and macros listed in alphabetical order or by "topic"? |
re: CI adjustments
Before sharing my final thoughts on that, I want to play a bit with the current approach when multiple rules files are loaded simultaneously. I will let you know.
The issues I found with comments are due to practical reasons. In particular:
However, all of this is for another PR for sure. I'll open an issue to track it. |
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
…: copy) Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
…: rename back) Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
leogr
force-pushed
the
update/rules-files-split
branch
from
5f8862a
to
688ded6
Compare
Rules files suggestionsfalco_rules.yamlComparing Major changes:
Patch changes:
|
Ok, I did some experiments, and my conclusion is that we should keep duplicate entries for now. In this way, duplicated items are just silently overwritten. The only con is that the loading order affects the end results when the duplicate item is not identical (for example, if it has been modified in one file but not in the other). The alternative would be to use an idiomatic syntax to make one rules file depend on an item defined in another other rule files, for example: This ☝️ would force the user to load another rules file with the For the record, here's the list of dups: |
leogr
changed the title
falco_rules.yaml according to the rules maturityfalco_rules.yaml according to the rules maturity
|
/hold cancel |
|
Hey @falcosecurity/rules-maintainers This PR is ready. PTAL 🙏 Once merged, I will tag each ruleset with version |
|
cc @falcosecurity/falco-maintainers |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: incertum, leogr 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 |
Agreed, instead of yet another doc, could we check how much of the comments could simply be removed (for example the engine version mentions could be removed) vs what information could be added to the re the duplicate macros: Could we add a CI check ensuring macros and lists are the same in all files? And while we do that ensure upstream rules have no duplicative rules aka no overriding and also ensure macros and lists only appear once per rules file and as said ensure they match up across rules files? |
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
/kind cleanup
/kind design
/kind documentation
Any specific area of the project related to this PR?
/area rules
/area registry
/area build
/area documentation
Proposed rule maturity level
This PR does not propose new rules or change the maturity level of any existing rule.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
While splitting rules, I faced some issues that I needed to address in this PR