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

Events flags embedding #4191

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Jul 16, 2024

1. Explain what the PR does

4dfd7da chore: inject deps manager into policy manager
9aff9d2 chore(policy): introduce ManagerConfig
443869e fix: validateKallsymsDependencies() nil deref

4dfd7da chore: inject deps manager into policy manager

It makes the access to event flags be done through the policy manager,
which is responsible for setting up the dependencies manager.

This also:

- Turn the event flags access thread-safe.
- Renames manager and config types.
- Fix t.eventSignatures[eventId] logic, now accessible via policy
  manager IsRequiredBySignature(), since the former was only flagging
  the event as signature, but not as required by one.

2. Explain how to test it

3. Other comments

This is a enabler for #3987.

@geyslan
Copy link
Member Author

geyslan commented Jul 17, 2024

It's passing all the tests. Gonna polish it and leave it for review asap.

@geyslan geyslan marked this pull request as ready for review July 18, 2024 11:57
@geyslan geyslan mentioned this pull request Jul 18, 2024
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.

Looks like a very good refactor required for future changes!

_, hasSignature := t.eventSignatures[eventId]
isSignature := t.policyManager.IsSignature(eventId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this change is correct.
We want to check if the event have signatures that depend on it (hasSignature) and not if the event itself is a signature (isSignature)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I've changed the nomenclature but need to double check the logic. Soon I'll be returning to this. Tks.

Copy link
Member Author

@geyslan geyslan Sep 5, 2024

Choose a reason for hiding this comment

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

Well, you are right and I detected the main issue. As I only moved logic into the new place, I noticed that it didn't save the events in the map as required by sigs, it just set if it's a signature. So, at that stage it didn't work as a shortcut.

My last change includes:

  Fix t.eventSignatures[eventId] logic, now accessible via policy
  manager IsRequiredBySignature(), since the former was only flagging
  the event as signature, but not as required by one.

Copy link
Member Author

Choose a reason for hiding this comment

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

E2E 1783: ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

@geyslan, I tested it and compared it with the main branch (which, as you mentioned, has a bug).

It makes the access to event flags be done through the policy manager,
which is responsible for setting up the dependencies manager.

This also:

- Turn the event flags access thread-safe.
- Renames manager and config types.
- Fix t.eventSignatures[eventId] logic, now accessible via policy
  manager IsRequiredBySignature(), since the former was only flagging
  the event as signature, but not as required by one.
Copy link
Contributor

@rscampos rscampos left a comment

Choose a reason for hiding this comment

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

LGTM

@geyslan geyslan merged commit d5b1952 into aquasecurity:main Sep 17, 2024
30 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.

3 participants