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

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

Merged
merged 4 commits into from
Jun 10, 2024
Merged

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

merged 4 commits into from
Jun 10, 2024

Conversation

AlonZivony
Copy link
Contributor

@AlonZivony AlonZivony commented Apr 9, 2024 •
edited
Loading

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
Copy link
Contributor Author

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

@AlonZivony
Copy link
Contributor Author

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.

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

geyslan commented Apr 9, 2024

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?

@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch 7 times, most recently from 5542f04 to 706ea06 Compare April 14, 2024 13:44
@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch 3 times, most recently from d44a759 to 578bbb6 Compare April 30, 2024 16:43
@AlonZivony AlonZivony marked this pull request as ready for review May 1, 2024 08:38
@AlonZivony AlonZivony requested a review from NDStrahilevitz May 1, 2024 08:40
NDStrahilevitz
NDStrahilevitz approved these changes May 5, 2024
View reviewed changes
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

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

pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
tests/integration/dependencies_test.go Outdated Show resolved Hide resolved
tests/integration/dependencies_test.go Outdated Show resolved Hide resolved
tests/integration/dependencies_test.go Outdated Show resolved Hide resolved
AlonZivony
AlonZivony commented May 15, 2024
View reviewed changes
tests/integration/event_filters_test.go
return false, nil
}

// second stage: validate events
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 AlonZivony force-pushed the feature/further-dependencies-adaptation branch from c134e0b to eaf952a Compare May 15, 2024 08:37
@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch from eaf952a to 4d3a9bf Compare May 15, 2024 08:49
@AlonZivony
Copy link
Contributor Author

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

  1. 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.
  2. All watchers should be called after all the tree management operations are finished.
  3. 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
yanivagman reviewed May 19, 2024
View reviewed changes
Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/events/dependencies/probes.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch from 4d3a9bf to 0f62fe0 Compare May 22, 2024 14:50
@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch 4 times, most recently from 846ccee to e582e84 Compare June 3, 2024 08:28
@AlonZivony AlonZivony requested a review from yanivagman June 3, 2024 12:27
yanivagman
yanivagman approved these changes Jun 9, 2024
View reviewed changes
Copy link
Collaborator

@yanivagman yanivagman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mostly nit comments,
great work!

  1. A rebase is required
  2. Please fix race condition as discussed offline
  3. Consider changing the order of remove watchers

pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/events/dependencies/actions.go Outdated Show resolved Hide resolved
pkg/events/dependencies/actions.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch from a33dd49 to b016ebd Compare June 10, 2024 11:58
@AlonZivony
feat(dependencies): add probes nodes and make more generic
273ac35 
Add the option to track probes dependencies as node in the manager.
With this addition, the API was changed to support many node types.
The watchers interface was made more generic, and Actions were introduced.
The only supported Action currently available is node addition cancellation.
@AlonZivony
feat(trace): validate ksymbols using a dependencies manager watcher
8886c2e 
Introduced a watcher to the dependencies manager that tracks event
additions and validates that the event's ksymbols are available.
If the required symbols are missing, the watcher will cancel the
event via the manager.

Updated the current validation process to be per-event to align
with the watcher's API.
@AlonZivony
feat(trace): attach and detach probes using the dependencies manager
4f48f4b 
Introduced watchers to the dependencies manager that track probe
additions and removals, and attach or detach them accordingly.
If a probe attach fails, the watcher will cancel the operation.
@AlonZivony
test(dependencies): add e2e integration test for the dependencies man…
5f1f181 
…ager

Add e2e tests for the use of the dependencies manager in Tracee.
The test verifies that the ksymbols validation and attachments and detachments work well after the change.
@AlonZivony AlonZivony force-pushed the feature/further-dependencies-adaptation branch from b016ebd to 5f1f181 Compare June 10, 2024 14:13
@yanivagman yanivagman merged commit 6df67bc into aquasecurity:main Jun 10, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geyslan geyslan geyslan left review comments

@NDStrahilevitz NDStrahilevitz NDStrahilevitz approved these changes

@yanivagman yanivagman yanivagman approved these changes

Assignees

@AlonZivony AlonZivony

Labels
area/ebpf area/events area/logging area/testing milestone/v0.22.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cancelled events probes are not cleaned properly
4 participants
@AlonZivony @geyslan @yanivagman @NDStrahilevitz