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

Allow append of new exceptions to rules #1780

Merged
merged 1 commit into from
Nov 18, 2021
Merged

Allow append of new exceptions to rules #1780

merged 1 commit into from
Nov 18, 2021

Conversation

sai-arigeli
Copy link
Contributor

@sai-arigeli sai-arigeli commented Nov 10, 2021 •
edited by leogr
Loading

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

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

/area build

/area engine

/area rules

/area tests

/area proposals

What this PR does / why we need it:
PR introduces appending of new fully valid exception objects to existing rules.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update: allow append of new exceptions to rules

@poiana poiana requested review from leodido and mstemm November 10, 2021 18:19
@poiana poiana added the size/L label Nov 10, 2021
@sai-arigeli sai-arigeli mentioned this pull request Nov 10, 2021
Closed
leodido
leodido reviewed Nov 10, 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.

Before reviewing... Why the commit comes from a GitHub user that sign-offs commit with @mstemm email?

@leodido
Copy link
Member

leodido commented Nov 10, 2021

I refer to this:

image

@leodido
Copy link
Member

leodido commented Nov 10, 2021

The DCO (implemented via the sign-off) requires the contributor's email (not another one) AFAIK

@sai-arigeli
Copy link
Contributor Author

The DCO (implemented via the sign-off) requires the contributor's email (not another one) AFAIK

@leodido I had wrongly infered that sign-off needs to be one of the owner. I corrected the commit message.

mstemm
mstemm requested changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

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

Just the one error return that should also have context and warnings.

userspace/engine/lua/rule_loader.lua Outdated Show resolved Hide resolved
@leodido
Copy link
Member

leodido commented Nov 11, 2021

@sai-arigeli I was not understanding because looking at your fork/other PRs via the GitHub events I saw that you were signing-off correctly there (sysdig or gmail email)...

image

@poiana poiana added the lgtm label Nov 12, 2021
@leogr
Copy link
Member

leogr commented Nov 12, 2021

Closing and reopening to trigger the CI
/close

@poiana
Copy link
Contributor

poiana commented Nov 12, 2021

@leogr: Closed this PR.

In response to this:

Closing and reopening to trigger the CI
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@poiana poiana closed this Nov 12, 2021
@leogr
Copy link
Member

leogr commented Nov 12, 2021

/reopen

@poiana
Copy link
Contributor

poiana commented Nov 12, 2021

@leogr: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@poiana poiana reopened this Nov 12, 2021
@leogr
Copy link
Member

leogr commented Nov 12, 2021

/milestone 0.31.0

@poiana poiana added this to the 0.31.0 milestone Nov 12, 2021
mstemm
mstemm requested changes Nov 12, 2021
View reviewed changes
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

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

I thought of another change--since this changes the behavior of how falco reads rules files, you should bump the falco engine version here: /~https://github.com/falcosecurity/falco/blob/master/userspace/engine/falco_engine_version.h#L19. Can you increment it to 11?

@poiana poiana removed the lgtm label Nov 12, 2021
@sai-arigeli
Copy link
Contributor Author

I thought of another change--since this changes the behavior of how falco reads rules files, you should bump the falco engine version here: /~https://github.com/falcosecurity/falco/blob/master/userspace/engine/falco_engine_version.h#L19. Can you increment it to 11?

@mstemm Fixed this!

@leogr
Copy link
Member

leogr commented Nov 15, 2021

I thought of another change--since this changes the behavior of how falco reads rules files, you should bump the falco engine version here: /~https://github.com/falcosecurity/falco/blob/master/userspace/engine/falco_engine_version.h#L19. Can you increment it to 11?

@mstemm Fixed this!

Hey @sai-arigeli

as per our policy, merge commits are not allowed. Could you use git rebase to fix this issue? Thanks in advance

@poiana poiana added size/L and removed size/XXL labels Nov 16, 2021
@sai-arigeli
Allow append of new exceptions to rules
34cebff 
Signed-off-by: Sai Arigeli <saiharisharigeli@gmail.com>

Return warnings after validation of rule exceptions

Signed-off-by: Sai Arigeli <saiharisharigeli@gmail.com>

Update FALCO_ENGINE_VERSION

Signed-off-by: Sai Arigeli <saiharisharigeli@gmail.com>
@sai-arigeli
Copy link
Contributor Author

@mstemm @leogr Addressed the change requests.

@poiana poiana added the lgtm label Nov 17, 2021
@poiana
Copy link
Contributor

poiana commented Nov 17, 2021

LGTM label has been added.

Git tree hash: 492d3631bcbc013216e768f3f89f738cff4d3e1e

leogr
leogr approved these changes Nov 18, 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.

LGTM
/approve

@poiana
Copy link
Contributor

poiana commented Nov 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leogr, sai-arigeli

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 23706da into falcosecurity:master Nov 18, 2021
@sai-arigeli sai-arigeli deleted the sa-falco-21nov10 branch November 18, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leodido leodido leodido left review comments

@mstemm mstemm mstemm approved these changes

@leogr leogr leogr approved these changes

Assignees

@leogr leogr

@mstemm mstemm

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

Successfully merging this pull request may close these issues.
5 participants
@sai-arigeli @leodido @leogr @poiana @mstemm