feat(dependencies): further adopt dependencies API to ksymbols and probes #3967

AlonZivony · 2024-04-09T11:01:28Z

1. Explain what the PR does

This PR expands and uses the dependencies API introduced in #3931 for other events dependencies handling.
It adds the probes dependencies to the dependencies manager, and use it to manage probes cancelling.
This PR also add the dependencies watchers for adding and removing the probes, and use the dependencies to cancel events with missing ksymbols.

Fix #3933

2. Explain how to test it

3. Other comments

AlonZivony · 2024-04-09T11:03:57Z

This PR was extracted from #3965 and will serve it for its logic

AlonZivony · 2024-04-09T11:23:47Z

This however won't solve the issue that we need to update the events_map in the eBPF so events removed won't be there anymore. @geyslan is this something you can do? I am a bit lost with how to do it with the new policies versioning mechanism.

geyslan · 2024-04-09T12:37:57Z

This however won't solve the issue that we need to update the events_map in the eBPF so events removed won't be there anymore. @geyslan is this something you can do? I am a bit lost with how to do it with the new policies versioning mechanism.

For sure, could you put TODO comments?

NDStrahilevitz

I want to go over the logic again later this week.

pkg/ebpf/tracee.go

pkg/ebpf/c/tracee.bpf.c

tests/integration/dependencies_test.go

AlonZivony · 2024-05-15T08:23:31Z

tests/integration/event_filters_test.go

+			return false, nil
+		}
+
+		// second stage: validate events


I have added support for unexpected events only in this function (ExpectAtLeastOneForEach) as I only used this one.
If its seems good to you @NDStrahilevitz I can add it to all other functions as well.
I admit that I think that the current API created by @geyslan is a bit too complicated - it has too many functions so the differences between them is hard to understand. Anyways this PR is not about the testing infrastructure, so I only open it here for discussion.

I admit that I think that the current API created by @geyslan is a bit too complicated

I agree with you @AlonZivony. Integration API must be rethought.

AlonZivony · 2024-05-19T10:17:33Z

After a conversation with @yanivagman we arrived to the following problems that should be addressed with the watchers mechanism:

Add watchers order should be from dependencies to dependants, and removed watchers as well. The dependants cannot work without the dependencies, so there is no logic in having them without their dependencies.
All watchers should be called after all the tree management operations are finished.
As some watchers might call RemoveEvent, we should either call watchers by order of invocation, or cancel irrelevant watchers (for example, cancel add watchers on event that its remove was called already).

yanivagman

Nice work @AlonZivony
The automatic handling of event dependencies will make it easier to implement dynamic policy modification in the future. @geyslan FYI

pkg/events/dependencies/manager.go

yanivagman · 2024-05-19T09:18:42Z

pkg/events/dependencies/manager.go

+}
+
+// removeNode removes the node from the tree.
+func (m *Manager) removeHandle(handle *HandleNode) bool {


What's the meaning of returning bool here? If it represent an error, let's use error instead

It represents if the remove was successful in the sense of if the handle existed in the manager.
I can replace it with an error, and create an error for "value not found". But as currently the remove cannot fail, the bool value seems cleaner to me.

I think using error for non existing value is a a common practice

I can replace it with an error, and create an error for "value not found".

So the check against it should be if errors.Is(err, dependencies.ValueNotFoundError) {. It's a good practice if we'll have more error types, if not... the check against the bool is cleaner.

yanivagman · 2024-05-19T09:22:56Z

pkg/events/dependencies/probes.go

+}
+
+func (hn *HandleNode) addDependant(dependant events.ID) {
+	hn.dependants = append(hn.dependants, dependant)


shouldn't we check that dependant not already exists in the list?

yanivagman · 2024-05-19T09:26:37Z

pkg/ebpf/tracee.go

-			probesToEvents[probeDep] = append(probesToEvents[probeDep], id)
+	for _, probe := range depsNode.GetDependencies().GetProbes() {
+		err := t.probes.Attach(probe.GetHandle(), t.cgroups)
+		if err != nil {


nit - to avoid extra indentation, use

if err == nil { continue }

yanivagman · 2024-05-19T09:37:20Z

pkg/ebpf/tracee.go

+	detachUnusedProbes := func(probesToCheck []events.Probe) {
+		for _, probe := range probesToCheck {
+			_, ok := t.eventsDependencies.GetHandle(probe.GetHandle())
+			if !ok {


nit - to avoid extra indentation, use:

if ok { continue }

yanivagman · 2024-05-19T09:41:04Z

pkg/ebpf/tracee.go

+	if err != nil {
+		return errfmt.WrapError(err)
+	}


This is not required since we call it anyway below

pkg/ebpf/tracee.go

Track dependencies on probes. This makes it easier to understand when new probes are added or old ones removed.

Change the handling of the probes attach and detach to use the dependencies watchers. This allows to handle correctly failure of events and future events updates.

Create an end-to-end test for the dependencies failure mechanism. This should check that the new dependencies mechanism is integrating well with Tracee. This PR introduce test events for the first time.

Move useful helper functions for tests in the logger and policies area to the testutils package.

Use integration tests existing functions instead of self implemented.

Change the watchers to support any type of node to trigger watchers, according to required type. Also add the ability for add watcher to cancel the add of the node to get less buggy order of operations.

github-actions bot added area/ebpf area/testing area/events labels Apr 9, 2024

github-actions bot assigned AlonZivony Apr 9, 2024

AlonZivony force-pushed the feature/further-dependencies-adaptation branch from 34ba958 to 89b530b Compare April 9, 2024 12:32

AlonZivony force-pushed the feature/further-dependencies-adaptation branch 7 times, most recently from 5542f04 to 706ea06 Compare April 14, 2024 13:44

github-actions bot added the area/logging label Apr 14, 2024

AlonZivony force-pushed the feature/further-dependencies-adaptation branch 3 times, most recently from d44a759 to 578bbb6 Compare April 30, 2024 16:43

AlonZivony marked this pull request as ready for review May 1, 2024 08:38

AlonZivony requested a review from NDStrahilevitz May 1, 2024 08:40

NDStrahilevitz approved these changes May 5, 2024

View reviewed changes

yanivagman added the milestone/v0.22.0 label May 9, 2024

github-actions bot added the area/build label May 12, 2024

AlonZivony commented May 15, 2024

View reviewed changes

AlonZivony force-pushed the feature/further-dependencies-adaptation branch from c134e0b to eaf952a Compare May 15, 2024 08:37

github-actions bot removed the area/build label May 15, 2024

AlonZivony force-pushed the feature/further-dependencies-adaptation branch from eaf952a to 4d3a9bf Compare May 15, 2024 08:49

yanivagman reviewed May 19, 2024

View reviewed changes

AlonZivony added 9 commits May 19, 2024 18:59

feat(dependencies): add probes dependency node

3654527

Track dependencies on probes. This makes it easier to understand when new probes are added or old ones removed.

feat(ebpf): handle probes with watchers

ed2dd7c

Change the handling of the probes attach and detach to use the dependencies watchers. This allows to handle correctly failure of events and future events updates.

feat(ebpf): use watchers for ksymbols validation

6741ea3

feat(events): create e2e for dependencies failure

fe486e4

Create an end-to-end test for the dependencies failure mechanism. This should check that the new dependencies mechanism is integrating well with Tracee. This PR introduce test events for the first time.

chore(tests): export tests helper functions to testutils

bc81a8f

Move useful helper functions for tests in the logger and policies area to the testutils package.

chore(tests): use existing functions for dependencies tests

d29e028

Use integration tests existing functions instead of self implemented.

Fixes

f96c4ea

Add attached kprobes check to dependencies tests

910d4d1

Use generic watchers for all dependencies nodes.

0f62fe0

Change the watchers to support any type of node to trigger watchers, according to required type. Also add the ability for add watcher to cancel the add of the node to get less buggy order of operations.

AlonZivony force-pushed the feature/further-dependencies-adaptation branch from 4d3a9bf to 0f62fe0 Compare May 22, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dependencies): further adopt dependencies API to ksymbols and probes #3967

feat(dependencies): further adopt dependencies API to ksymbols and probes #3967

AlonZivony commented Apr 9, 2024 •

edited

AlonZivony commented Apr 9, 2024

AlonZivony commented Apr 9, 2024

geyslan commented Apr 9, 2024

NDStrahilevitz left a comment

AlonZivony May 15, 2024

geyslan May 22, 2024

AlonZivony commented May 19, 2024

yanivagman left a comment

yanivagman May 19, 2024

AlonZivony May 19, 2024

yanivagman May 20, 2024

geyslan May 22, 2024

yanivagman May 19, 2024

yanivagman May 19, 2024

yanivagman May 19, 2024

yanivagman May 19, 2024

feat(dependencies): further adopt dependencies API to ksymbols and probes #3967

Are you sure you want to change the base?

feat(dependencies): further adopt dependencies API to ksymbols and probes #3967

Conversation

AlonZivony commented Apr 9, 2024 • edited

1. Explain what the PR does

2. Explain how to test it

3. Other comments

AlonZivony commented Apr 9, 2024

AlonZivony commented Apr 9, 2024

geyslan commented Apr 9, 2024

NDStrahilevitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlonZivony commented May 19, 2024

yanivagman 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

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

AlonZivony commented Apr 9, 2024 •

edited