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.

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

refactor(userspace/engine): split rule loader classes for a more testable design #2206

Merged
merged 12 commits into from
Sep 27, 2022

Conversation

jasondellaluce
Copy link
Contributor

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

This is a no-op change. The goal here is to further split the rule loader classes of the Falco rule engine to have a more testable design, thus allowing writing in-depth unit tests in future PRs.

The current class design is as follows:

  • rule_loader: defines a class that collects definitions (lists, macros, rules, plugin requirements, etc...) read from a YAML, and then uses those definitions to compile a ruleset. Inside the rule loader name path, many structs are defined (e.g. rule_info, macro_info, etc...)
  • rule_reader: defines a class that reads a YAML and uses rule_loader to collect the definitions read

The proposed changes are as follows:

  • rule_loader: this is now a simple namespace in which all base types and structs related to rule loading are defined (e.g. rule_info, macro_info, etc...)
  • rule_loader::collector: a class used to collect definitions (lists, macros, rules, plugin requirements, etc...)
  • rule_loader::reader: a stateless class that reads a YAML and uses rule_loader::collector to collect the definitions read
  • rule_loader::compiler: a stateless class that uses the definitions stored in a rule_loader::collector to compile a ruleset

With the new design, every class is a standalone interface where dependency injection is applicable, each with a specific responsibility. Not only this makes it easier to change/extend the rule loading logic in the future, but also allows adding in-depth unit tests for each of these components.

Which issue(s) this PR fixes:

Special notes for your reviewer:

  • The only code changes introduced are the removal of some using namespace xxx to reduce ambiguity
  • Git history has been preserved, all the code is the same as it was previously in rule_loader.cpp and rule_reader.cpp, just splitted in more files
  • Rule loading is implicitly tested already by some of our python-based regression tests, but that's very coarse grain and not exhaustive. I'm looking forward to porting those split those tests to more fine grain ones in each of these components, and add new ones as well.

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@poiana
Copy link
Contributor

poiana commented Sep 27, 2022

LGTM label has been added.

Git tree hash: 24b9b868b563bc26d068af3e78e3b14b644460ef

@leogr
Copy link
Member

leogr commented Sep 27, 2022

/milestone 0.33.0

@poiana poiana added this to the 0.33.0 milestone Sep 27, 2022
Copy link
Contributor

@FedeDP FedeDP 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
Copy link
Contributor

poiana commented Sep 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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 8aea093 into master Sep 27, 2022
@poiana poiana deleted the wip/improve-rule-loader-classes branch September 27, 2022 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants