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

Fix new boefjes issue for scheduler #3297

Merged
merged 16 commits into from
Aug 6, 2024
Merged

Fix new boefjes issue for scheduler #3297

merged 16 commits into from
Aug 6, 2024

Conversation

jpbruinsslot
Copy link
Contributor

@jpbruinsslot jpbruinsslot commented Jul 24, 2024

Changes

Wrong cache gets flushed during flushing of another cache

While working on #2786 I encountered an issue with how newly enabled boefjes were being handled by the scheduler. By taking note of the logs it seemed that the scheduler identified all current enabled boefjes to be new and would check if for those boefjes ooi's were available to run on.

This was mainly because of the following line in the code:

self.organisations_new_boefjes_cache[org.id] = {}

It would clear the new_boefjes cache every time the plugin_cache was being flushed. These caches are unrelated and flushing one should not clear the other one.

However the plugin cache in main isn't used and it's cache is also not being flushed by either get_plugin_by_id_and_org_id() or get_plugins_by_org_id() methods in the katalogus service. So we don't encounter this issue on main.

Thread locking mechanism for new_boefjes cache added

I noticed the new_boefjes cache did not implement a locking mechanism. Since multiple threads are accessing and writing to the shared cache I've added a thread lock for this cache as well.

Implemented separate locks for individual katalogus caches

Instead of one thread lock for all the katalogus caches, I've implemented individual locks for the caches. This ensures that calls to one cache will not block calls from other threads to the other caches.

Added thread safety tests

To ensure everything would be working accordingly and safeguard any future regression this PR specifically adds thread safety tests.

General refactoring of variable names, and code clean-ups

The long variable cache names were unnecessary and did not help with making the variable names more descriptive. Additionally, this PR also added several closures to some of the methods that access the caches and need to lock the resource, this to reduce some code duplication and enhance readability.

Issue link

Closes #3287

QA notes

This is issue is not seen on main but I can show its workings on the flexible scheduling feature branch. The main focus for QA would be checking if enabling of plugins will continue working correctly. E.g. when enabling a boefje that would start and execute scan because ooi's are the correct scan level.


Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@jpbruinsslot jpbruinsslot added the mula Issues related to the scheduler label Jul 24, 2024
@jpbruinsslot jpbruinsslot self-assigned this Jul 24, 2024
@jpbruinsslot jpbruinsslot marked this pull request as ready for review July 25, 2024 15:30
@jpbruinsslot jpbruinsslot requested a review from a team as a code owner July 25, 2024 15:30
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Looks good! I just have a few remarks/ suggestions to improve it even more. The provided PR context is really good and helpful, very much appreciated it. Keep up the good work

mula/scheduler/connectors/services/katalogus.py Outdated Show resolved Hide resolved
mula/scheduler/connectors/services/katalogus.py Outdated Show resolved Hide resolved
mula/scheduler/connectors/services/katalogus.py Outdated Show resolved Hide resolved
mula/scheduler/connectors/services/katalogus.py Outdated Show resolved Hide resolved
mula/scheduler/connectors/services/katalogus.py Outdated Show resolved Hide resolved
@jpbruinsslot
Copy link
Contributor Author

Looks good! I just have a few remarks/ suggestions to improve it even more. The provided PR context is really good and helpful, very much appreciated it. Keep up the good work

Thanks! Made changes according your suggestions

ammar92
ammar92 previously approved these changes Aug 2, 2024
* main:
  fix: Button height (#3316)
  Add user id to OOI (#3305)
  Translations update from Hosted Weblate (#3179)
  Restructure scheduler storage module (#3294)
  Fix CSRF error in API with token auth (#3313)
@stephanie0x00
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

Seems to look good. Have enabled multiple boefjes, rescheduled some tasks (boefjes and normalizers). Findings are created and tasks complete.

What doesn't work:

Nothing identified.

Bug or feature?:

n/a

@underdarknl underdarknl merged commit 4576580 into main Aug 6, 2024
9 checks passed
@underdarknl underdarknl deleted the fix/mula/new_boefjes branch August 6, 2024 08:53
jpbruinsslot added a commit that referenced this pull request Aug 6, 2024
* main:
  Fix new boefjes issue for scheduler (#3297)
  Add audit logging to CRUD actions using Django signals (#3314)
  fix: Button height (#3316)
  Add user id to OOI (#3305)
  Translations update from Hosted Weblate (#3179)
  Restructure scheduler storage module (#3294)
  Fix CSRF error in API with token auth (#3313)
jpbruinsslot added a commit that referenced this pull request Aug 6, 2024
* feature/mula/refactor-queue:
  Fix new boefjes issue for scheduler (#3297)
  Add audit logging to CRUD actions using Django signals (#3314)
  fix: Button height (#3316)
  Add user id to OOI (#3305)
  Translations update from Hosted Weblate (#3179)
  Restructure scheduler storage module (#3294)
  Fix CSRF error in API with token auth (#3313)
dekkers pushed a commit that referenced this pull request Aug 7, 2024
Co-authored-by: Jan Klopper <[email protected]>
Co-authored-by: ammar92 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mula Issues related to the scheduler
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

New boefjes not working correctly
4 participants