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(falco): print all events with flags #2771

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

LucaGuerra
Copy link
Contributor

@LucaGuerra LucaGuerra commented Sep 5, 2023

What type of PR is this?

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

/kind feature
/area engine

What this PR does / why we need it:

This patch changes the way events are printed, especially in markdown format. See this page: https://falco.org/docs/reference/rules/supported-events/ . There are significant changes:

  • All events are displayed, not just syscalls
  • Flags are also displayed
  • Generic events are also included
  • The "default" column now reflects whether or not the -A flag is required

Updating the event list is just a matter of running ./userspace/falco/falco --list-syscall-events --markdown and saving the output in the supported-events.md in the website.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update!: the --list-syscall-events flag is now called --list-events and lists all events


// This is the result of running the following command:
// FALCO="falco -c ./falco.yaml"
// echo $($FALCO --version | grep 'Engine:' | awk '{print $2}') $(echo $($FALCO --version | grep 'Schema version:' | awk '{print $3}') $($FALCO --list --markdown | grep '^`' | sort) $($FALCO --list-syscall-events | sort) | sha256sum)
// It represents the fields supported by this version of Falco,
// the event types, and the underlying driverevent schema. It's used to
// detetect changes in engine version in our CI jobs.
#define FALCO_ENGINE_CHECKSUM "41b5dc700216b243d294b40c46264d4e89d0ee00098fdc1c21bb4b1e7639da06"
#define FALCO_ENGINE_CHECKSUM "98c6e665031b95c666a9ab02d5470e7008e8636bf02f4cc410912005b90dff5c"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for the reviewer: the list of events did not change, but the format did so we need to update the checksum. Updating the engine version should be OK but if I forgot something and it's not please let me know and I'll revert

Comment on lines +195 to +196
printf("\n\n## Plugin events\n\n");
print_events(events_bc.pluginevents, s.options.markdown);
Copy link
Member

Choose a reason for hiding this comment

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

Do we intend to print all events (regardless of the data source) here? If so, I would change the option name from --list-syscall-events to --list-events.

cc @falcosecurity/falco-maintainers

Copy link
Member

Choose a reason for hiding this comment

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

yes agree with leo i would change it to --list-events

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 noticed that this flag never printed syscall events, but always printed all events. Yes, I introduced it and decided the name 😅 . So I agree with you, let me change it to --list-events.

@leogr
Copy link
Member

leogr commented Sep 5, 2023

  • The "default" column now reflects whether or not the -A flag is required

Why do we need this? 🤔
It may be confusing for users.

@LucaGuerra
Copy link
Contributor Author

Why do we need this? 🤔
It may be confusing for users.

Even after our recent changes and optimizations, some events require the -A flag to be considered.

We need to convey this information to users somehow otherwise people would (rightly) expect all events to work in all cases! If you have ideas about how to make it clearer I'd be happy to hear it but that's the one idea I found right now.

Signed-off-by: Luca Guerra <luca@guerra.sh>
Signed-off-by: Luca Guerra <luca@guerra.sh>
Signed-off-by: Luca Guerra <luca@guerra.sh>
Copy link
Member

@Andreagit97 Andreagit97 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'm okay with keeping the -A distinction

@poiana
Copy link
Contributor

poiana commented Sep 7, 2023

LGTM label has been added.

Git tree hash: b9547690cc9b80af1e6cfe8037c4065c3535f65e

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 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, LucaGuerra

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 [Andreagit97,FedeDP,LucaGuerra]

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 a22dac6 into falcosecurity:master Sep 7, 2023
17 checks passed
@LucaGuerra LucaGuerra deleted the new/print-all-events branch September 7, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants