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

chore(userspace/falco): deprecate old 'rules_file' config key. #3162

Merged
merged 3 commits into from
Apr 16, 2024
Merged

chore(userspace/falco): deprecate old 'rules_file' config key. #3162

merged 3 commits into from
Apr 16, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Apr 10, 2024

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:

Deprecates old 'rules_file' config option in favor of new 'rules_files'.
Old config option is still kept for backward compatiblity (and will possibly go away with Falco 1.0.0).
So:

  • only rules_files present -> ok we use it
  • only rules_file present -> ok we use it but give a warning about deprecation
  • both keys used -> throw an exception

Example output:

  • only rules_file present:
sudo ./userspace/falco/falco -c ../../falco.yaml  -r ../rules/falco_rules.yaml 
Wed Apr 10 17:26:45 2024: Using deprecated config key 'rules_file'. Please use new 'rules_files' config key.
Wed Apr 10 17:26:45 2024: Falco version: 0.38.0-182+e840a4a (x86_64)
Wed Apr 10 17:26:45 2024: Falco initialized with configuration files:
Wed Apr 10 17:26:45 2024:    ../../falco.yaml
  • both keys present:
sudo ./userspace/falco/falco -c ../falco.yaml  -r ../rules/falco_rules.yaml -o configs_files=../../falco.yaml
Wed Apr 10 17:26:32 2024: Using deprecated config key 'rules_file'. Please use new 'rules_files' config key.
Error: Error reading config file (../falco.yaml): both 'rules_files' and 'rules_file' keys set

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

chore(userspace/falco): deprecated old 'rules_file' config key

@FedeDP
chore(userspace/falco): deprecate old 'rules_file' config key.
06fcc35 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@poiana poiana requested a review from jasondellaluce April 10, 2024 15:30
@poiana poiana requested a review from Kaizhe April 10, 2024 15:30
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 10, 2024

/hold for discussion

@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 10, 2024

/cc @leogr

@poiana poiana requested a review from leogr April 10, 2024 15:30
@poiana poiana added the size/M label Apr 10, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 10, 2024

/milestone 0.38.0

@poiana poiana added this to the 0.38.0 milestone Apr 10, 2024
FedeDP
FedeDP commented Apr 10, 2024
View reviewed changes
userspace/falco/configuration.cpp
@@ -167,6 +167,18 @@ void falco_configuration::merge_configs_files(const std::string& config_name, st
}
}

void falco_configuration::init_logger()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is somewhat a fix: initialize the logger before loading anything else from the config; this allows us to log at the correct level in case we need to.
NOTE: this wasn't a bug since no logging was present in the yaml loading phase in configuration.cpp; but now we have to log a warning when both new and old keys are defined.

All in all, logging is quite always the first thing that needs to be inited, thus the change.

FedeDP
FedeDP commented Apr 10, 2024
View reviewed changes
userspace/falco/configuration.cpp
load_engine_config(config_name);
m_log_level = config.get_scalar<std::string>("log_level", "info");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done twice :)

leogr
leogr reviewed Apr 11, 2024
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Overall SGTM,

just left a comment for the doc comment :D

falco.yaml Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
@FedeDP @leogr
chore(falco.yaml): small improvements.
27fe089 
Co-authored-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the chore/rules_files_conf_key branch from c5cabcd to 27fe089 Compare April 15, 2024 06:46
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 15, 2024

/cc @incertum

@poiana poiana requested a review from incertum April 15, 2024 15:24
incertum
incertum reviewed Apr 15, 2024
View reviewed changes
userspace/falco/configuration.cpp
}
if (num_rules_files_opts == 2)
{
throw std::logic_error("Error reading config file (" + config_name + "): both 'rules_files' and 'rules_file' keys set");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw std::logic_error("Error reading config file (" + config_name + "): both 'rules_files' and 'rules_file' keys set");
throw std::logic_error("Error reading config file (" + config_name + "): both 'rules_files' (plural form) and 'rules_file' (singular form) keys set. Please only use new 'rules_files' config key (plural form).");

@incertum
Copy link
Contributor

LGTM, just minor rewording suggestion.

@FedeDP @incertum
chore(userspace/falco): make it clear singular vs plural.
04f95fc 
Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the chore/rules_files_conf_key branch from 5b6882b to 04f95fc Compare April 16, 2024 06:40
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 16, 2024

Thanks Melissa, agreed with the clarification, committed :)

@poiana poiana added the lgtm label Apr 16, 2024
incertum
incertum approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@incertum incertum 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 Apr 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@FedeDP FedeDP mentioned this pull request Apr 16, 2024
Closed
11 tasks
@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 16, 2024

Going to unhold in a couple of minutes, waiting for master CI to end.

@FedeDP
Copy link
Contributor Author

FedeDP commented Apr 16, 2024

/unhold

@poiana poiana merged commit e21a3a5 into master Apr 16, 2024
33 checks passed
@poiana poiana deleted the chore/rules_files_conf_key branch April 16, 2024 14:39
@FedeDP FedeDP mentioned this pull request May 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incertum incertum incertum approved these changes

@leogr leogr leogr approved these changes

@jasondellaluce jasondellaluce Awaiting requested review from jasondellaluce

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@leogr leogr

@incertum incertum

Labels
approved area/engine dco-signoff: yes kind/cleanup lgtm release-note size/M
Projects
Falco Roadmap
Archived in project
Milestone
0.38.0
Development

Successfully merging this pull request may close these issues.
4 participants
@FedeDP @incertum @poiana @leogr