Allow append of new exceptions to rules #1768

sai-arigeli · 2021-11-02T11:16:40Z

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?:

poiana · 2021-11-02T11:16:42Z

@sai-arigeli: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 · 2021-11-02T11:16:49Z

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

mstemm · 2021-11-03T16:23:35Z

userspace/engine/lua/rule_loader.lua

+			   exceptions = {}
+			end
+
+			if eitem['fields'] == nil then


You should be able to use the fields/comps variables set at the top of this block, right?

mstemm · 2021-11-03T16:27:25Z

userspace/engine/lua/rule_loader.lua

+
+		   	-- You can't append exception fields or comps to an existing rule exception
+		     	if fields ~= nil then
+			   return false, build_error_with_context(v['context'], "Can not append exception fields to existing rule, only values"), warnings


The indentation looks okay in my editor, but doesn't seem okay here. I wouldn't go crazy trying to fix it, but if you can align the indent with whatever is being used by the other parts of this file it would help with readability.

mstemm · 2021-11-03T16:31:45Z

userspace/engine/lua/rule_loader.lua

-			      for _, values in ipairs(eitem['values']) do
-				 reitem['values'][#reitem['values'] + 1] = values
+			-- Insert the complete exception object
+			exceptions[#exceptions+1] = eitem


Don't you need to add the exceptions back to the original rule?

The line 639 is supposed to add the entire exception to the original rule. I have tested this specific insertion to the original table on a lua playground. Will update it if necessary.

Oh right--tables are references, so the local exceptions variable still points to the same table which is a part of the rule. See my earlier comment about assigning the exceptions variable, though.

poiana · 2021-11-04T11:10:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sai-arigeli
To complete the pull request process, please ask for approval from mstemm after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

…into sa-falco-21nov1

mstemm

Just a couple of small feedback things, the core logic looks okay and correct, and the tests cover the new functionality.

mstemm · 2021-11-04T17:51:03Z

userspace/engine/lua/rule_loader.lua

-		     end
+		     if new_exception then
+			local exceptions = state.rules_by_name[v['rule']]['exceptions']
+			if exceptions == nil then


I don't think there's any need for this check given that the rule object always has exceptions--it's added on line 538. If the check on line 538 were not there, this would not actually work, as the exceptions variable would end up pointing to a local table and not the table that's a part of the rule object.

mstemm · 2021-11-04T17:51:44Z

userspace/engine/lua/rule_loader.lua

-			      for _, values in ipairs(eitem['values']) do
-				 reitem['values'][#reitem['values'] + 1] = values
+			-- Insert the complete exception object
+			exceptions[#exceptions+1] = eitem


Oh right--tables are references, so the local exceptions variable still points to the same table which is a part of the rule. See my earlier comment about assigning the exceptions variable, though.

mstemm · 2021-11-04T18:02:41Z

userspace/engine/lua/rule_loader.lua

+			end
+
+			if fields == nil then
+			   return false, build_error_with_context(v['context'], "Rule exception new item "..eitem['name']..": must have fields property with a list of fields"), warnings


Can you try to use the local variables defined on lines 595 instead of accessing eitem['name'], eitem['fields'], etc? The prior version wasn't entirely consistent but did try to use those local variables, to make the code a bit easier to read.

If you prefer accessing the name, fields, etc. via eitem['xxx'] instead, that's also ok, but let's be consistent and remove the definitions on line 595-597.

poiana · 2021-11-04T19:17:35Z

@sai-arigeli: Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

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.

…into sa-falco-21nov1 Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

poiana · 2021-11-09T20:15:42Z

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

3d365ce Merge branch 'sa-falco-21nov1' of /~https://github.com/sai-arigeli/falco into sa-falco-21nov1
c6d08b9 Merge branch 'falcosecurity:master' into sa-falco-21nov1
e38f432 Removing redundant local variables

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. I understand the commands that are listed here.

sai-arigeli · 2021-11-10T18:30:37Z

Closing this PR due to merge issues. All commits squashed and comments addressed on #1780. Please continue the review there.

poiana added dco-signoff: no do-not-merge/release-note-label-needed labels Nov 2, 2021

poiana requested review from Kaizhe and mstemm November 2, 2021 11:16

poiana added the size/L label Nov 2, 2021

sai-arigeli marked this pull request as draft November 2, 2021 11:17

poiana added the do-not-merge/work-in-progress label Nov 2, 2021

sai-arigeli changed the title ~~Allow append of new exceptions to rules~~ wip: Allow append of new exceptions to rules Nov 2, 2021

mstemm requested changes Nov 3, 2021

View reviewed changes

poiana assigned mstemm Nov 3, 2021

mstemm requested changes Nov 3, 2021

View reviewed changes

sai-arigeli changed the title ~~wip: Allow append of new exceptions to rules~~ Allow append of new exceptions to rules Nov 4, 2021

sai-arigeli marked this pull request as ready for review November 4, 2021 11:37

poiana removed the do-not-merge/work-in-progress label Nov 4, 2021

sai-arigeli added 3 commits November 4, 2021 10:58

Allow append of new exceptions to rules

451590d

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Update negative regression tests and fix bugs

1376aed

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Addressing comments and minor indent fixes 1

2d97d09

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

sai-arigeli force-pushed the sa-falco-21nov1 branch from 12e0e73 to 2d97d09 Compare November 4, 2021 18:09

poiana added dco-signoff: yes and removed dco-signoff: no labels Nov 4, 2021

sai-arigeli added 4 commits November 4, 2021 11:35

Allow append of new exceptions to rules

87806e0

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Update negative regression tests and fix bugs

de41573

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Addressing comments and minor indent fixes 1

dbd081b

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

Merge branch 'sa-falco-21nov1' of /~https://github.com/sai-arigeli/falco …

3d365ce

…into sa-falco-21nov1

mstemm requested changes Nov 4, 2021

View reviewed changes

Merge branch 'falcosecurity:master' into sa-falco-21nov1

c6d08b9

poiana added the dco-signoff: no label Nov 4, 2021

poiana added do-not-merge/contains-merge-commits and removed dco-signoff: yes labels Nov 4, 2021

sai-arigeli added 2 commits November 9, 2021 12:11

Removing redundant local variables

e38f432

Merge branch 'sa-falco-21nov1' of /~https://github.com/sai-arigeli/falco …

a7b494e

…into sa-falco-21nov1 Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

mstemm mentioned this pull request Nov 9, 2021

fix(userspace/engine): supporting enabled-only overwritten rules #1775

Merged

sai-arigeli closed this Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow append of new exceptions to rules #1768

Allow append of new exceptions to rules #1768

sai-arigeli commented Nov 2, 2021 •

edited

Loading

poiana commented Nov 2, 2021

poiana commented Nov 2, 2021

mstemm Nov 3, 2021

mstemm Nov 3, 2021

mstemm Nov 3, 2021

sai-arigeli Nov 3, 2021

mstemm Nov 4, 2021

poiana commented Nov 4, 2021

mstemm left a comment

mstemm Nov 4, 2021

mstemm Nov 4, 2021

mstemm Nov 4, 2021

poiana commented Nov 4, 2021

poiana commented Nov 9, 2021

sai-arigeli commented Nov 10, 2021

Allow append of new exceptions to rules #1768

Allow append of new exceptions to rules #1768

Conversation

sai-arigeli commented Nov 2, 2021 • edited Loading

poiana commented Nov 2, 2021

poiana commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiana commented Nov 4, 2021

mstemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiana commented Nov 4, 2021

poiana commented Nov 9, 2021

sai-arigeli commented Nov 10, 2021

sai-arigeli commented Nov 2, 2021 •

edited

Loading