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

fix(userspace): cleanup output of ruleset validation result #2248

Merged
merged 3 commits into from
Oct 12, 2022
Merged

fix(userspace): cleanup output of ruleset validation result #2248

merged 3 commits into from
Oct 12, 2022

Conversation

jasondellaluce
Copy link
Contributor

What type of PR is this?

/kind bug

/kind cleanup

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

This fixes two issues related to ruleset validation and error reporting:

  1. Currently, when validating and loading rulesets we exit at the first error encountered. However, we didn't skip the "list/macro is unused" checks, which ended up flooding the loading result when encountering an error due to most/all macros and list being obviously unused. Plus, this was wrong logically because we can potentially encounter errors even before reaching the ruleset compilation step.
  2. When printing validation result with json output enabled, we didn't fill-up the human-readable summary string. When encountering an error and making Falco terminate, this ended up in printing an empty string as a termination error.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(userspace): cleanup output of ruleset validation result

@poiana poiana requested review from FedeDP and Kaizhe October 12, 2022 09:59
@jasondellaluce
Copy link
Contributor Author

/milestone 0.33.0

@poiana poiana added this to the 0.33.0 milestone Oct 12, 2022
@poiana poiana added the size/M label Oct 12, 2022
@jasondellaluce jasondellaluce mentioned this pull request Oct 12, 2022
Closed
96 tasks
FedeDP
FedeDP previously approved these changes Oct 12, 2022
View reviewed changes
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 poiana added the lgtm label Oct 12, 2022
@poiana
Copy link
Contributor

poiana commented Oct 12, 2022

LGTM label has been added.

Git tree hash: 437ade52d70b6e002f99e3582ff5e5f75a118167

Andreagit97
Andreagit97 previously approved these changes Oct 12, 2022
View reviewed changes
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

@jasondellaluce jasondellaluce dismissed stale reviews from Andreagit97 and FedeDP via 955f109 October 12, 2022 11:11
@poiana poiana removed the lgtm label Oct 12, 2022
@poiana poiana requested review from Andreagit97 and FedeDP October 12, 2022 11:11
FedeDP
FedeDP previously approved these changes Oct 12, 2022
View reviewed changes
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 poiana added the lgtm label Oct 12, 2022
@poiana
Copy link
Contributor

poiana commented Oct 12, 2022

LGTM label has been added.

Git tree hash: dc7d9ebfe27eaaf07f0d9a9357ef215a448ea775

@jasondellaluce
fix(userspace/engine): avoid macro/list used checks if we encounter a…
2ec8b43 
…n error

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@jasondellaluce
fix(userspace/falco): make sure validation summary is populated even …
34596df 
…when json output is requested

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@jasondellaluce
fix(test): fix regresstion test
1b3ab5b 
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@poiana poiana removed the lgtm label Oct 12, 2022
@poiana poiana requested a review from FedeDP October 12, 2022 11:18
Andreagit97
Andreagit97 approved these changes Oct 12, 2022
View reviewed changes
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

@poiana poiana added the lgtm label Oct 12, 2022
@poiana
Copy link
Contributor

poiana commented Oct 12, 2022

LGTM label has been added.

Git tree hash: c02e2a934ba6d0f18a8287c4a04a1adc2d55b757

FedeDP
FedeDP approved these changes Oct 12, 2022
View reviewed changes
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 Oct 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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,jasondellaluce]

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 7d28637 into master Oct 12, 2022
@poiana poiana deleted the fix/rules-validation branch October 12, 2022 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@FedeDP FedeDP FedeDP approved these changes

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@FedeDP FedeDP

@Andreagit97 Andreagit97

Labels
approved area/engine dco-signoff: yes kind/bug kind/cleanup lgtm release-note size/M
Projects
None yet
Milestone
0.33.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jasondellaluce @poiana @FedeDP @Andreagit97