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

update(outputs): add configuration option for tags in json outputs #1733

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

jasondellaluce
Copy link
Contributor

@jasondellaluce jasondellaluce commented Sep 23, 2021

What type of PR is this?

/kind cleanup

/kind feature

Any specific area of the project related to this PR?

/area engine

/area tests

What this PR does / why we need it:
Since the recent merge of #1714, tags and source fields are now part of both the grpc and json outputs. However, the semantic of the json output is not clear in some corner cases, such as when no tags are defined in a rule. Also, users can't configure whether to include tags in the json outputs or not, which instead is possible for the output field. Moreover, no test cases are yet present for asserting the expected outcome of these corner cases. These issues were reported in #1714 (comment) by @leodido (thank you!!)

As such, this PR contributes with the following:

  • Adding the json_include_tags_property configuration option, so that users can decide whether to include tags in the json outputs or not. By default this is set to true to not introduce breaking changes for clients that currently expect the field to be present.
  • Clarifying the expected outcome in the corner cases arising when a rule has no defined tags.
  • Adding test cases asserting the expected outcomes of the corner cases, and updating the test suite to support the new configuration option.

Additional Context:
The table below describes the expected outcomes in the json outputs by using the new configuration option.

json_include_tags_property = true json_include_tags_property = false
Rule with no tags (1) json has a field tags=[] (2) json has no tags field
Rule with tags (3) json has a field tags=[...] (4) json has no tags field

These cases are covered by the following tests:

  • json_output_empty_tags_property: Case (1)
  • json_output_no_tags_property: Cases (2) and (4)
  • stdout_output_json_strict: Case (3)

Does this PR introduce a user-facing change?:

update(outputs): add configuration option for tags in json outputs

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@leogr
Copy link
Member

leogr commented Sep 24, 2021

/milestone 0.30.0
/cc @leodido
/cc @Issif

@poiana poiana added this to the 0.30.0 milestone Sep 24, 2021
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.
Great improvement 👍

Comment on lines +1114 to +1132
json_output_no_tags_property:
json_output: True
json_include_tags_property: False
detect: True
detect_level: WARNING
rules_file:
- rules/rule_append.yaml
trace_file: trace_files/cat_write.scap
stdout_contains: "^(?!.*\"tags\":[ ]*\\[.*\\],.*)"

json_output_empty_tags_property:
json_output: True
detect: True
detect_level: WARNING
rules_file:
- rules/rule_append.yaml
trace_file: trace_files/cat_write.scap
stdout_contains: "^(.*\"tags\":[ ]*\\[\\],.*)"

Copy link
Member

Choose a reason for hiding this comment

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

👍

@poiana
Copy link

poiana commented Sep 28, 2021

LGTM label has been added.

Git tree hash: 42163e871e0526a78dcab56258b75f3182c27bbf

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.

Thank you Jason for implementing my feedback. 🤗

/LGTM

@poiana
Copy link

poiana commented Sep 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasondellaluce, leodido

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 0eb170c into falcosecurity:master Sep 28, 2021
@jasondellaluce jasondellaluce deleted the json-output-fixes branch September 28, 2021 13:52
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.

None yet

4 participants