Refactor policy

[yanivagman]

1. Explain what the PR does

commit 9e6ddcd
Author: Yaniv Agman [email protected]
Date: Thu Nov 28 17:32:23 2024 +0200

test(pkg/cmd/flags): add unit tests for policy parsing functions

Add unit tests for recently added policy parsing functions:
- parseScopeFilters
- createSinglePolicy
- parseEventFilters

commit ec0ec43
Author: Yaniv Agman [email protected]
Date: Thu Nov 28 14:16:57 2024 +0200

refactor: simplify CreatePolicies function structure

Split the large CreatePolicies function into smaller, focused functions
for better readability and maintainability. The main changes:

- Split policy creation logic into separate functions
- Simplified function signatures and parameter passing
- Made variable naming more consistent
- Improved error handling organization

No functional changes, only code reorganization.

commit d575f3d
Author: Yaniv Agman [email protected]
Date: Mon Nov 25 17:54:48 2024 +0200

refactor: align filters with policy rule structure

Restructure filters to match tracee's policy structure by moving event-specific
filters into policy.Rules. Main changes:

- Move DataFilter, ScopeFilter and RetFilter from top-level maps into
  policy.Rules[eventID]
- Remove filter.go and its flag as event filtering is now handled through
  the policy structure
- Adjust all filter operations to work with the new Rules-based structure

This change better reflects tracee's policy design where each event has
its own set of filters under a rule, preparing the stage for moving from
matched_policies to matched_rules resolution.

commit cc8947d
Author: Yaniv Agman [email protected]
Date: Thu Nov 28 11:48:17 2024 +0200

docs: split filter help into scope and events help

Split the filter help message into separate scope and events help messages
to match the CLI structure. Moved documentation from the deprecated filter
flag into their respective --scope and --events flag help messages.

2. Explain how to test it

3. Other comments

[geyslan]

@rscampos this conflicts with your effort.

[geyslan]

Could this logic set an internal flag to be checked via a Rules public method as a shortcut in the pipeline?

[yanivagman]

We already set ps.filterableInUserland a few lines below and use it in the pipeline by FilterableInUserland(), what else do you suggest?

[geyslan]

As you pinpointed we already have a short circuit:

tracee/pkg/ebpf/events_pipeline.go

Lines 307 to 311 in 8d22a07

	// Short circuit if there are no policies in userland that need filtering.
	if !t.policyManager.FilterableInUserland(bitmap) {
	event.MatchedPoliciesUser = bitmap // store untouched bitmap to be used in sink stage
	return bitmap
	}

But I think it would be faster if we only check a boolean (the hasUserlandFilters promoted as a Policies field). So instead of acquiring mutex and doing the bitwise, we would only be required to acquire and return true or false.

tracee/pkg/policy/policy_manager.go

Lines 565 to 570 in 8d22a07

	func (m *Manager) FilterableInUserland(bitmap uint64) bool {
	m.mu.RLock()
	defer m.mu.RUnlock()
	return (bitmap & m.ps.filterInUserland()) != 0
	}

Also, as PolicyManager has its own mutex, these atomic aren't necessary anymore (reminiscences of another era):

tracee/pkg/policy/policies.go

Lines 88 to 90 in 8d22a07

	func (ps *policies) filterInUserland() uint64 {
	return atomic.LoadUint64(&ps.filterableInUserland)
	}

[yanivagman]

But this hasUserlandFilters variable doesn't takes the UID and PID filters into account as done below, so what do you suggest exactly?
Anyway, I'm not saying we shouldn't do such a shortcut to avoid mutexes, just that I'm not sure it is related to this PR that handles policy structure change

[geyslan]

I'm aware that you're just following the current structure definition but, just as a reminder, we'll better encapsulate it converting all public objects to private and make them accessible only by specific methods as we already started in policy.Manager.

[yanivagman]

Yes, I agree we should eventually encapsulate these stuff and use a clear interface instead of accessing public fields. This is definitely something we will need to do

[geyslan]

I've skimmed it and put some thoughts. When the conflicts are resolved, gonna check it again.