10000 chore: make dependencies manager a singleton by geyslan · Pull Request #4161 · aquasecurity/tracee · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

chore: make dependencies manager a singleton #4161

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
Merged
merged 2 commits into from
Jul 1, 2024
Merged

chore: make dependencies manager a singleton #4161

merged 2 commits into from
Jul 1, 2024

Conversation

geyslan
Copy link
Member
@geyslan geyslan commented Jun 28, 2024 •
edited
Loading

1. Explain what the PR does

83c39cf chore: mv dependencies tests to same pkg
5b6c9fc chore: make dependencies manager a singleton (temp)

5b6c9fc chore: make dependencies manager a singleton (temp)

This tidies up the dependencies manager by making it a singleton,
temporarily. It's a necessary step to decouple the manager from the
Tracee object without touching much code, allowing it to be accessed
from other parts of the codebase and be detached from other logic.

It introduces an also temporary ResetManagerFromTests() to reset the
manager state between tests.

When elements are decoupled and redesign, the manager will not be a
singleton anymore, but it will be accessed via dependency injection.

NOTE: It continues being not thread-safe. This will be addressed in a
future PR.

2. Explain how to test it

3. Other comments

@geyslan geyslan requested review from rscampos and yanivagman June 28, 2024 18:25
@geyslan geyslan self-assigned this Jun 28, 2024
@geyslan geyslan force-pushed the dep-manager-singleton branch from ff716b0 to 2ef9365 Compare June 28, 2024 19:05
@yanivagman
Copy link
Collaborator
yanivagman commented Jun 30, 2024 •
edited
Loading

Why do we want it to be a singleton?
In general, I'm against using singletons in tracee and prefer to define clear interfaces for each component and use dependency injection to provide references to the necessary components to each other. This maintains loose coupling and makes it easier to test and modify components in isolation.

@geyslan
Copy link
Member Author
geyslan commented Jun 30, 2024

DI is good as well but currently to decouple modules from Tracee we should inject those modules (changing function/method signatures) of all related to it. This demands a clear vision of the relation of the elements.

And it also only works if the injected is thread safe what dependencies manager isn't yet.

About the coupling of the singleton in tests... We can use methods to control its visibility only for the test build tag. That's a change that I was about to do to this PR, so the Reset function wouldn't be available to production, only in tests.

Anyhoo, if you do prefer I can step back, make dependencies manager thread-safe first and in futures PRs:

  • initialize it and inject it to the policy manager creation, removing it from the Tracee object.
  • decouple event states from Tracee making it private to the policy manager.

WDYT?

Last note: the main focus on making the transitional objects into singletons is to make the code logic easy in the tidying process, without touching much code. When the objects with relations are moved and tidied, the singleton pattern should be removed leaving the injection to do the job.

@geyslan
Copy link
Member Author
geyslan commented Jun 30, 2024

Correction:

  • initialize it and inject it to the policy manager creation, removing it from leaving it to the Tracee object as well.

As it would be thread safe, all that demands access to it should get a copy from Tracee's.

@yanivagman
Copy link
Collaborator

Last note: the main focus on making the transitional objects into singletons is to make the code logic easy in the tidying process, without touching much code. When the objects with relations are moved and tidied, the singleton pattern should be removed leaving the injection to do the job.

If this step is only transitional and the goal is to make the logic easy before removing the singleton, I'm ok with it.
I'll complete the review with that comment in mind.

yanivagman
yanivagman approved these changes Jul 1, 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.

LGTM, considering #4161 (comment)

@geyslan
chore: make dependencies manager a singleton (temp)
5b6c9fc 
This tidies up the dependencies manager by making it a singleton,
temporarily. It's a necessary step to decouple the manager from the
Tracee object without touching much code, allowing it to be accessed
from other parts of the codebase and be detached from other logic.

It introduces an also temporary ResetManagerFromTests() to reset the
manager state between tests.

When elements are decoupled and redesign, the manager will not be a
singleton anymore, but it will be accessed via dependency injection.

NOTE: It continues being not thread-safe. This will be addressed in a
future PR.
@geyslan geyslan force-pushed the dep-manager-singleton branch from 2ef9365 to 3cd64e9 Compare July 1, 2024 12:34
@geyslan geyslan force-pushed the dep-manager-singleton branch from 3cd64e9 to 83c39cf Compare July 1, 2024 12:54
@geyslan geyslan merged commit 2aa25b1 into aquasecurity:main Jul 1, 2024
32 checks passed
@geyslan geyslan deleted the dep-manager-singleton branch February 19, 2025 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanivagman yanivagman yanivagman approved these changes

@rscampos rscampos Awaiting requested review from rscampos

Assignees

@geyslan geyslan

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

Successfully merging this pull request may close these issues.
2 participants
@geyslan @yanivagman
0