-
Notifications
You must be signed in to change notification settings - Fork 58
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
Combined schedulers #3839
base: main
Are you sure you want to change the base?
Combined schedulers #3839
Conversation
57d040f
to
da3f337
Compare
* main: (64 commits) Bug fix: KAT-alogus parameter is now organization member instead of organization code (#3895) Remove sigrid workflows (#3920) Updated packages (#3898) Fix mula migrations Debian package (#3919) Adds loggers to report flow (#3872) Add additional check if task already run for report scheduler (#3900) Create separate finding for Microsoft RDP port (#3882) fix: 🐛 allow boefje completion with 404 (#3893) Feature/improve rename bulk modal (#3885) Update scheduler folder structure (#3883) Translations update from Hosted Weblate (#3870) Increase max number of PostgreSQL connections (#3889) Fix for task id as valid UUID (#3744) Add `auto_calculate_deadline` attribute to Scheduler (#3869) Ignore specific url parameters when following location headers (#3856) Let mailserver inherit l1 (#3704) Change plugins enabling in report flow to checkboxes (#3747) Fix rocky katalogus tests and delete unused fixtures (#3884) Enable/disable scheduled reports (#3871) optimize locking in katalogus.py, reuse available data (#3752) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Just a few remarks while skimming through it, not a detailed look yet
auto_calculate_deadline=False, | ||
) | ||
ID: str = "report" | ||
ITEM_TYPE: Any = models.ReportTask |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any
should be a type
?
create_schedule=True, | ||
auto_calculate_deadline=False, | ||
) | ||
ID: str = "report" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ID: str = "report" | |
ID: Literal["report"] = "report" |
] | ||
), | ||
) | ||
return not count > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return not count > 0 | |
return not count |
* main: (21 commits) Bump django from 5.0.9 to 5.0.10 in /rocky (#3940) Do not let enabling plugins affect the global plugin cache (#3944) Fix typing in more places and configure mypy to follow imports (#3932) Updates CWE archive to 4.16 (#3943) Report flaws (#3880) Translations update from Hosted Weblate (#3939) Fix report recipe API (#3942) Boefje runonce functionality in scheduler (#3906) fix: 🔨 do not store CDN findings (#3931) Dont check for Locations on local Ip's. (#3894) add unpkg.com to disallowed hostnames in CSP (#3927) Update website_discovery.py (#3921) Add export http boefje (#3901) Bump python-multipart from 0.0.9 to 0.0.18 in /bytes (#3925) Fix layout issues on scheduled reports page (#3930) Create scheduled report with zero objects selectable (#3907) Improve the KATalogus `/plugins` endpoint performance (#3892) Add bgp.jsonl and bgp-meta.json to .gitignore (#3928) Update pre-commit and all hooks (#3923) add support for detecting Lame dns delegations on ip ranges (#3899) ...
Current state (16/12) of this PR.
|
Warning
Ready for review, not for merging
Changes
app.py
: Removal of creation of organisational schedulers. We don't gather all available organisations from the katalogus and create individual boefje, report, and normalizer schedulers.app.py
: Removal of continuous checking of added or removed organisations. We removed themonitor_organisations
method that was running in a thread to check if we added or removed organisation and create schedulers for it.models/
:Task
andSchedule
get a specificorganisation
field. Since we don't differentiate tasks that are pushed on an organisation queue anymore, we need to be able to determine for what organisation an item on a queue is from.1schedulers/
: Since we've consolidated the organisational schedulers to 3 types of schedulers, and we assume that we get task creation from consolidated message queues, we need to update the schedulers to handle that.BoefjeScheduler
server/
queues/
andschedulers/
endpoint. The notion of a queue and scheduler are used interchangeably. Therefore it makes more sense at the moment to merge those endpoints to avoid confusion and simplify the code.Task
from a scheduler. Task runner will now be able to batch pop tasks from a scheduler using filters.Next steps and impact
/queues
to/schedulers/{id}/pop
, additionally it will return a paginated result instead of a singleTask
, this is because the pop endpoint now supports filtering with multiple tasks returns. Services that rely on the scheduler pop endpoint need to update their interfaces (Update services that rely on/pop
endpoint of scheduler #3961)/queues
toschedulers/{id}/push
, services that interface with the push endpoints (rocky) need to update their interfaces. (Update services that rely on/push
endpoint of scheduler #3962)organisation
fields are added toTask
,Schedule
, services using these models need to update their specifications. (Model definition updates: addition oforganisation
field toTask
andSchedule
models #3965)Issue link
#3838
QA notes
Please add some information for QA on how to test the newly created code.
Code Checklist
.env
changes files if required and changed the.env-dist
accordingly.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.
Footnotes
The attentive reader will know that the organisation information is present in the
data
specification in aTask
, and can be filtered on by using aFilterRequest
. However in practice the scheduler is mainly used in the context of multiple organisations and filtering on this id is deemed developer friendly when interfacing with the scheduler. Hence the choice explicitly defining theorganisation
field on a top level. ↩as mentioned in the code this iterating over all organisations in order to check if there are enabled plugins is very inefficient, please refer to issue Katalogus caching in the scheduler #3357 ↩