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): honor new plugins exposed suggested output formats #3388

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Oct 22, 2024

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

Since falcosecurity/libs#2116, the plugin API now exposes a way for extractor plugins to declare that certain fields should be used as output formats.
Honor the request, and add a suggested_formats config option to eventually disable the suggested formats.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(userspace,cmake): honor new plugins exposed suggested output formats

@poiana
Copy link
Contributor

poiana commented Oct 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 22, 2024

/milestone 0.40.0

@poiana poiana added this to the 0.40.0 milestone Oct 22, 2024
@poiana poiana added the size/M label Oct 22, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 22, 2024

/cc @jasondellaluce

@poiana poiana requested a review from jasondellaluce October 22, 2024 07:05
@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 22, 2024

CI is failing because there is some problem with the install-zig action :/

@FedeDP FedeDP force-pushed the new/suggested_formats branch from e65e004 to 073c74b Compare October 22, 2024 13:00
@FedeDP FedeDP changed the title new(userspace,cmake): honor new plugins exposed suggested output formats new(userspace): honor new plugins exposed suggested output formats Oct 22, 2024
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.

I'm all in for this. From a consistency standpoint, since we have the similar append_output in the config already, what do you think about being this part of it? I'm thinking something like:

append_output:
- match:
     source: syscall
   suggested_fields: true

This way we can also control it per-source. We can still enable this by default for all sources.

userspace/falco/app/actions/init_falco_engine.cpp Outdated Show resolved Hide resolved
@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 23, 2024

I'm thinking something like:

I really like the idea! Will implement it asap!

# This means that any extractor plugin that indicates some of its fields
# as suggested output formats, will see these fields in the output
# in the form "foo_bar=$foo.bar"
append_output:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, we enable suggested_output for all rules, tags and sources.

@FedeDP FedeDP force-pushed the new/suggested_formats branch from e8f8312 to 7af2445 Compare October 23, 2024 13:37
@@ -100,6 +100,7 @@ class falco_configuration {
std::set<std::string> m_tags;
std::string m_rule;
std::string m_format;
bool m_suggested_output = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When missing, default value is false.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason why it is enabled by default in the config but disabled if missing?

Not saying this is wrong, I'm just wondering why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because i want it to be effective only when explicitly enabled; i don't want it to have any effect if you specify eg:

append_output:
   - match:
       source: syscall
     extra_output: "on CPU %evt.cpu"

Ie: in this example, i only want on CPU %evt.cpu to be appended to output, not the plugin suggested fields, unless suggested_output: true is also set.

But i am also open for more opinions.

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 23, 2024

/cc @LucaGuerra

@poiana poiana requested a review from LucaGuerra October 23, 2024 13:38
@FedeDP FedeDP force-pushed the new/suggested_formats branch from 7af2445 to 71dda9f Compare November 5, 2024 10:12
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.

LGTM

@poiana poiana added the lgtm label Nov 5, 2024
@FedeDP FedeDP force-pushed the new/suggested_formats branch from 71dda9f to 4452195 Compare November 11, 2024 08:37
@leogr leogr requested a review from jasondellaluce November 16, 2024 05:04
@FedeDP FedeDP force-pushed the new/suggested_formats branch from 4452195 to 95cab34 Compare November 21, 2024 08:58
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 21, 2024

/cc @LucaGuerra that first worked on the new output configs.

@poiana poiana merged commit 35d8618 into master Dec 5, 2024
34 checks passed
@poiana poiana deleted the new/suggested_formats branch December 5, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants