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.

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

Dependencies tree manager #3931

Merged

Conversation

AlonZivony
Copy link
Collaborator

@AlonZivony AlonZivony commented Mar 26, 2024

1. Explain what the PR does

Create a manager object for the dependencies between event.
It helps to follow what events are really used, and will enable to effectively cancel and add events and probes.

This is a first step towards my current goal of having fallbacks for dependencies, to have a proper solution for the process_execute_failed event.
I will create another PR after this one that support the fallback of dependencies, and afterwards work with it on the event.

The work on this PR exposed multiple problems or I might even call them bugs in our current code:

  1. When an event fails, we just cancel all of its dependencies events even though they might be chosen or a dependency of another event. However, we don't cancel events that depend on it. This PR address this issue - Fix Events cancellation removes chosen dependencies events #3932
  2. We don't cancel events properly upon failure. We don't detach their probes nor remove them from the policies filter maps. this wasn't addressed in this PR, but this PR exposes an easy way to do these things - using the watcher functions we can make sure to update everything upon each change to the events and their decencies. This PR does not address this issue, only creates the framework to solve it in future PRs

2. Explain how to test it

3. Other comments

@AlonZivony AlonZivony force-pushed the feature/managing-dependencies branch from 5571903 to f9d83d8 Compare March 26, 2024 11:10
@AlonZivony AlonZivony changed the title Before using the dependencies tree Dependencies tree manager Mar 26, 2024
@AlonZivony AlonZivony force-pushed the feature/managing-dependencies branch 4 times, most recently from f41827f to e896171 Compare March 28, 2024 11:57
@AlonZivony
Copy link
Collaborator Author

The current objects are not thread safe.
Do you want me to fix them to be or add some documentation about it?
Currently it doesn't matter, but in the future it might do.

@AlonZivony AlonZivony force-pushed the feature/managing-dependencies branch 2 times, most recently from 7f3537d to 703e2b3 Compare March 28, 2024 17:04
@geyslan
Copy link
Member

geyslan commented Apr 1, 2024

@AlonZivony I'll be on this soon.

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

I reviewed the code and tests and didn't find any apparent flaws. I also ran the unit tests locally and checked the manager generated fields values with some prints. LGTM.

E2E 1433 is green. 👍🏼

Furthermore, put a few comments for discussion.

pkg/events/dependencies/event.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
pkg/events/dependencies/event.go Outdated Show resolved Hide resolved
pkg/events/dependencies/manager.go Outdated Show resolved Hide resolved
@yanivagman
Copy link
Collaborator

I'm going to review this soon. Please wait with merging it

Create a manager object for the dependencies between event.
It helps to follow what events are really used, and will enable to effectively cancel and add events and probes.
@AlonZivony AlonZivony force-pushed the feature/managing-dependencies branch from 703e2b3 to c87c2a9 Compare April 3, 2024 12:01
@yanivagman yanivagman merged commit d5899a0 into aquasecurity:main Apr 4, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events cancellation removes chosen dependencies events
3 participants