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

new(userspace/falco): added an option to listen to changes on the config file and rules files, and trigger a Falco reload #1991

Merged
merged 5 commits into from
May 12, 2022

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented May 6, 2022

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

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

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

/area build

/area engine

/area rules

/area tests

/area proposals

What this PR does / why we need it:

This PR adds a new config option watch_config_files, to allow it to trigger a Falco restart whenever a change is detected in the rules files or conf file.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: added new `watch_config_files` config option, to trigger a Falco restart whenever a change is detected in the rules or config files

…fig file and rules files, and trigger a Falco reload.

Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
@poiana poiana added size/M and removed size/L labels May 6, 2022
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.

Just left a comment, otherwise SGTM!

❤️

userspace/falco/app_cmdline_options.cpp Outdated Show resolved Hide resolved
FedeDP added 2 commits May 6, 2022 11:20
…ch rules folders, when specified.

This means that when starting Falco passing to it a folder for its rules, it will properly manage
changes to any file inside the folders, plus any created/deleted file inside it.

Unified list of rules parsing, instead of having it done twice inside cmdline_options and configuration.
Instead, it is done only once, inside load_rules_files.

Signed-off-by: Federico Di Pierro <[email protected]>
@poiana poiana added size/L and removed size/M labels May 6, 2022
@@ -86,12 +86,17 @@ application::run_result application::load_rules_files()
}

falco_logger::log(LOG_DEBUG, "Configured rules filenames:\n");
for (auto filename : m_state->config->m_rules_filenames)
for (const auto& path : m_state->config->m_rules_filenames)
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 different from the old behavior because now we are printing exact same options as configured by user, instead of already expanded ones (in case of folders).
I don't know whether this is fine or not; i can easily update it to support real loaded rules files.

Copy link
Contributor Author

@FedeDP FedeDP May 6, 2022

Choose a reason for hiding this comment

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

I don't think this is wrong, i mean, in log debug, we also print the "loading rules from file X" line, below while loading rules.
Therefore i thought it would be good to have a point where we print the actually passed-by-user rules paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree Fede, I think this works better

@FedeDP FedeDP mentioned this pull request May 9, 2022
53 tasks
jasondellaluce
jasondellaluce previously approved these changes May 11, 2022
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

I love this! I also tested locally and it worked like a charm.

@poiana
Copy link
Contributor

poiana commented May 11, 2022

LGTM label has been added.

Git tree hash: 946e340c327e150806dc37638d1bfdbeab849b25

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.

Possibly found a typo (see my suggestion), otherwise LGTM

userspace/falco/app_cmdline_options.h Outdated Show resolved Hide resolved
Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Leonardo Grasso <[email protected]>
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

@poiana poiana added the lgtm label May 12, 2022
@poiana
Copy link
Contributor

poiana commented May 12, 2022

LGTM label has been added.

Git tree hash: 4ed35d3cc5c531ea84226e9d918a2b0287c306a9

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.

🥳

/approve

@poiana
Copy link
Contributor

poiana commented May 12, 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 [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 acbbcf7 into falcosecurity:master May 12, 2022
@leogr
Copy link
Member

leogr commented May 12, 2022

/milestone 0.32.0

@poiana poiana added this to the 0.32.0 milestone May 12, 2022
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