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

update!(config): soft deprecation of drop stats counters in syscall_event_drops #3015

Merged
merged 3 commits into from
Jan 25, 2024
Merged

update!(config): soft deprecation of drop stats counters in syscall_event_drops #3015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d534324
update!(config): add deprecation notice for syscall_event_drops
incertum Jan 16, 2024
9223e1d
cleanup(configs): adjust old stats deprecation notice
incertum Jan 23, 2024
0330cee
update(config): soft deprecation of old stats
incertum Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions falco.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
# Falco logging / alerting / metrics related to software functioning (advanced)
# output_timeout
# syscall_event_timeouts
# syscall_event_drops
# syscall_event_drops [DEPRECATED] -> Use `metrics` instead, `syscall_event_drops` will be removed in Falco 0.38!
# metrics
# Falco performance tuning (advanced)
# syscall_buf_size_preset [DEPRECATED] -> Replaced by `engine.<driver>.buf_size_preset` starting Falco 0.38!
Expand Down Expand Up @@ -792,7 +792,7 @@ output_timeout: 2000
syscall_event_timeouts:
max_consecutives: 1000

# [Stable] `syscall_event_drops`
# [Stable] `syscall_event_drops` -> Use `metrics` instead, `syscall_event_drops` will be removed in Falco 0.38!
#
# Generates "Falco internal: syscall event drop" rule output when `priority=debug` at minimum
#
Expand Down
6 changes: 6 additions & 0 deletions userspace/falco/app/actions/load_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ static falco::app::run_result apply_deprecated_options(falco::app::state& s)
return run_result::fatal("You can not specify more than one of -e, -g (--gvisor-config), --modern-bpf, --nodriver, and the FALCO_BPF_PROBE env var");
}

if(s.config->m_min_priority == falco_common::PRIORITY_DEBUG)
Copy link
Member

Choose a reason for hiding this comment

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

Personally i had no time to check if we can really drop it, anyone willing to help @falcosecurity/falco-maintainers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leogr encouraged me to just get this PR going and then make the necessary decisions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it still is worth investigating a bit since it seems not really used. However, if we are in doubt, we can still postpone this deprecation to 0.38.0 (and remove it in 0.39.0). Let's see if we find any info.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I took some time to check it, I'm ok with deprecating this! If we agree I would love to do it in Falco 0.37.0 so that in the next dev cycle we can clean up the code!

If we are not sure, I propose to deprecate it now, and if we face some blockers when we try to remove it in the next dev-cycle we can expand the depreciation also for Falco 0.38.0, WDYT? btw I don't think this is the case, I don't see big blockers in removing it

{
falco_logger::log(falco_logger::level::WARNING,
"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! Use 'metrics' config instead. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! Use 'metrics' config instead. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");
"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! If you rely on this config use instead 'metrics.output_rule' to monitor the number of drops. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");

Copy link
Member

Choose a reason for hiding this comment

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

I would try to highlight that syscall_event_drops is enabled by default but it probably not many users rely on it, they probably don't even know that is config is doing something under the hood

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion! Added.

}

// Please note: is not possible to mix command line options and configs to obtain a configuration
// we need to use only one method. For example, is not possible to set the gvisor-config through
// the command line and the gvisor-root through the config file. For this reason, if we detect
Expand Down
Loading