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

new(outputs): expose rule tags and event source in gRPC and json outputs #1714

Merged
merged 5 commits into from
Sep 21, 2021
Merged

new(outputs): expose rule tags and event source in gRPC and json outputs #1714

merged 5 commits into from
Sep 21, 2021

Conversation

jasondellaluce
Copy link
Contributor

@jasondellaluce jasondellaluce commented Aug 26, 2021 •
edited
Loading

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:

  1. Adds rule tags as a new field to the gRPC outputs service.
  2. Adds rule tags and event source fields to the json outputs.
  3. Updates the test suite to check that rule tags and event source are correctly retrievable through the gRPC api and through json outputs.

Which issue(s) this PR fixes:

Fixes #1647, #1713

Does this PR introduce a user-facing change?:

new(outputs): expose rule tags and event source in gRPC and json outputs

@poiana poiana requested review from leodido and mstemm August 26, 2021 12:35
@poiana
Copy link
Contributor

poiana commented Aug 26, 2021

Welcome @jasondellaluce! It looks like this is your first PR to falcosecurity/falco 🎉

@leogr
Copy link
Member

leogr commented Aug 26, 2021

/milestone 0.30.0

@poiana poiana added this to the 0.30.0 milestone Aug 26, 2021
@poiana poiana added size/L and removed size/M labels Aug 31, 2021
@jasondellaluce jasondellaluce changed the title new(grpc): add rule tags to outputs service new(outputs): expose rule tags and event source in gRPC and json outputs Aug 31, 2021
leogr
leogr reviewed Sep 15, 2021
View reviewed changes
Copy link
Member

@leogr leogr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just found a spacing issue (see the comment below), otherwise LGTM!
Btw, great improvements, thank you! 👍

Note for reviewers: since the Lua code has been changed, to avoid a headache, make sure your local Falco's built is pointing to the correct Lua files 😺

userspace/engine/falco_engine.cpp Outdated Show resolved Hide resolved
@leogr
Copy link
Member

leogr commented Sep 15, 2021

cc @Issif

leogr
leogr approved these changes Sep 16, 2021
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.

/approve

@poiana poiana added the lgtm label Sep 16, 2021
@poiana
Copy link
Contributor

poiana commented Sep 16, 2021

LGTM label has been added.

Git tree hash: e81a8c7a4fb3ef9c28bf009a85e0f09c7f34c363

@poiana
Copy link
Contributor

poiana commented Sep 16, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 7c98d00 into falcosecurity:master Sep 21, 2021
@jasondellaluce jasondellaluce deleted the grpc-outputs-tags branch September 21, 2021 17:00
@leodido
Copy link
Member

leodido commented Sep 22, 2021 •
edited
Loading

Late to the party...

Have you folks considered making the output of the tags configurable?

The reason they were not there was also that, as far as I remember, the tags are not a mandatory part (while output is) of the Falco rule definitions.

Furthermore, a test covering such a scenario (no tags defined in the rule) would have been helpful to understand what's the resulting json in that case (empty array? other? valid json?).

leodido
leodido reviewed Sep 22, 2021
View reviewed changes
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Great job Jason!

Left two comments

userspace/falco/outputs.proto Show resolved Hide resolved
userspace/falco/outputs_grpc.cpp
@@ -64,5 +64,9 @@ void falco::outputs::output_grpc::output(const message *msg)
auto host = grpc_res.mutable_hostname();
*host = m_hostname;

// tags
auto tags = grpc_res.mutable_tags();
*tags = {msg->tags.begin(), msg->tags.end()};
Copy link
Member

Choose a reason for hiding this comment

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

Let's check what we end up with when a Falco rule does not have tags defined.

Do we obtain ...,"tags":[],... or not? I'd prefer a clean JSON, but that's my personal preference only :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't have any preferences. Anyway, I agree with @leodido that a test covering such a scenario (especially for the JSON output format) would be really helpful to clarify the expected result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review! I opened a PR to address these concerns, so we can eventually move this conversation there: #1733

@Issif
Copy link
Member

Issif commented Sep 22, 2021 •
edited
Loading

I'm really happy of this PR, when Falco 0.30.0 is out, I'll update the format of json in Falcosidekick either 👍 .

This was referenced Sep 23, 2021
Merged
Closed
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogr leogr leogr approved these changes

@leodido leodido leodido left review comments

@mstemm mstemm mstemm approved these changes

Assignees

@leogr leogr

@mstemm mstemm

Labels
approved area/engine dco-signoff: yes kind/feature lgtm release-note size/L
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.

Expose event source and rule tag(s) in json output
6 participants
@jasondellaluce @poiana @leogr @leodido @Issif @mstemm