Feature/efficient reporting #2516

Donnype · 2024-02-15T21:16:32Z

Changes

This PR adds several changes that should be backward compatible with current reporting, but opens up the possibility to make significant performance improvements:

The parent BaseReport class now has a collect_data method that takes in multiple oois. By default this generates the data through the already existing generate_data method. It returns the data grouped by the input_ooi for easy compatibility.
The views have been rewritten to use the collect_data method, moving iteration to the report classes in a backward compatible way.
Octopoes now has a /query-many endpoint that you also give a path query, but takes in several sources (references), and returns the same result as running that path on all sources separately.
Octopoes and the query module have been updated to be able to handle this kind of query to make sure XTDB does the heavy lifting by using or-clauses. Still for compatibility we had to connect this back to the source through some datalog magic.
The Mail report has been rewritten to override the collect_data method to make use of this feature. When the other reports do this as well, the amount of queries to octopoes will remain constant instead of scaling with the number of input objects. This is a TODO for a follow up PR that I will create, including benchmark results.

Note: the logic around the Query object in this endpoint gets a bit complicated at this point. We should reserve some time to refactor this at some point, but this would give quite some changes perhaps.

Issue link

Closes #2446 (partly, follow up PR incoming)

Demo

See tests.

Notes for QA

Follow the regular report generation flow, but now expect a performance boost generating (aggregate) reports.

Code Checklist

All the commits in this PR are properly PGP-signed and verified.
This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
I have written unit tests for the changes or fixes I made.
For any non-trivial functionality, I have added integration and/or end-to-end tests.
I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
I have made corresponding changes to the documentation, if necessary.
I have included comments in the code to elaborate on what is not self-evident from the code itself, including references to issues and discussions online, or implicit behavior of an interface.

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.

Signed-off-by: Donny Peeters <[email protected]>

Use new query functionality in API as well.

…t-coordination into feature/efficient-reporting

Signed-off-by: Donny Peeters <[email protected]>

…all-reports' into feature/efficient-reporting

…re/efficient-reporting

dekkers

In general I am very happy to see this and it looks good, I have just a few small remarks. I think it is a nice big step towards more efficient report generation.

octopoes/octopoes/api/router.py

rocky/reports/report_types/definitions.py

rocky/reports/report_types/ipv6_report/report.py

rocky/reports/report_types/definitions.py

octopoes/octopoes/connector/octopoes.py

octopoes/octopoes/xtdb/query.py

madelondohmen · 2024-02-22T07:44:27Z

In the meantime, some naming changes have been made to the Mail Report (#2513). I think your code will overwrite this, so it would be good to compare the two and make sure these changes won't be overwritten.

Donnype · 2024-02-22T14:18:19Z

In the meantime, some naming changes have been made to the Mail Report (#2513). I think your code will overwrite this, so it would be good to compare the two and make sure these changes won't be overwritten.

Yes I noticed those, should be fixed I think 👍

Signed-off-by: Donny Peeters <[email protected]>

ammar92

It looks good overal. I just have a few suggestions that might improve it even more

octopoes/tests/integration/test_xtdb_client.py

rocky/reports/report_types/aggregate_organisation_report/report.py

rocky/reports/report_types/definitions.py

rocky/reports/report_types/mail_report/report.py

rocky/reports/report_types/name_server_report/report.py

- Fix typing issue: add valid time string cast.

rocky/reports/report_types/mail_report/report.py

…s and change list comprehensions

…t-coordination into feature/efficient-reporting

Signed-off-by: Donny Peeters <[email protected]>

stephanie0x00 · 2024-02-29T09:40:31Z

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.

What works:

Generated an aggregate report successfully for multiple domains. Tasks are created and seem to complete as expected.

What doesn't work:

n/a

Bug or feature?:

n/a

* main: (85 commits) Fix wrong solving of merge conflict (#2585) Add metrics collection for scheduler using prometheus (#2468) Hotfix for where_in queries for abstract types (#2577) Update django (#2587) Fix octopoes typing (#2555) Create findings report (#2393) Raise exception if boefje input OOI has been deleted (#2573) Set a timeout on hanging test ssl container (#2560) Feature/efficient reporting (#2516) Updated findings database. Removed old findings, added Impact, Source… (#2569) add unit test for web report (#2528) Add pool size config and logs (#2541) Quick fix for PDF table overflow (#2562) Fix/2527 octopoes unicode (#2558) Add return typing to report test fixtures (#2557) Sort vulnerabilities in vulnerability report (#2378) Disable ruff split-on-trailing-comma and update ruff (#2544) Select all oois triggers toggle all (#2536) Remove unnecessary toplevel dependencies (#2554) Make valid time required parameter in the octopoes API (#2543) ...

Donnype added 8 commits February 14, 2024 13:53

Change report interface to support multiple oois being handled

0ee2a76

Signed-off-by: Donny Peeters <[email protected]>

Add a query-many endpoint and rewrite the mail report to use it.

54b0e43

Signed-off-by: Donny Peeters <[email protected]>

Style

97bb105

Signed-off-by: Donny Peeters <[email protected]>

Fixes for the benchmark suite

f7ef285

WIP: add where_in to query

32bac52

Signed-off-by: Donny Peeters <[email protected]>

Add where_in functionality to the XTDB queries

b9b6f62

Be able to match input sources back to their output.

e700349

Use new query functionality in API as well.

Fix benchmarking

f3be1fa

Donnype requested a review from a team as a code owner February 15, 2024 21:16

Donnype and others added 15 commits February 16, 2024 10:31

Merge branch 'feature/efficient-reporting' of github.com:minvws/nl-ka…

ecab36d

…t-coordination into feature/efficient-reporting

Merge branch 'main' into feature/efficient-reporting

66c8d31

Add comments to code

c9ca5ae

Signed-off-by: Donny Peeters <[email protected]>

Style

d9e65a8

Signed-off-by: Donny Peeters <[email protected]>

Fix unit test

012dccb

Signed-off-by: Donny Peeters <[email protected]>

Add collect_data method to web_system_report

c50dda3

Signed-off-by: Donny Peeters <[email protected]>

Add collect_data method to name_server_report

9117c8d

Signed-off-by: Donny Peeters <[email protected]>

Add collect_data method to ipv6 report

53bbbbc

Signed-off-by: Donny Peeters <[email protected]>

Style

748fcb7

Signed-off-by: Donny Peeters <[email protected]>

Merge branch 'main' into feature/efficient-reporting-for-all-reports

302123e

Merge remote-tracking branch 'origin/feature/efficient-reporting-for-…

124f314

…all-reports' into feature/efficient-reporting

Merge branch 'feature/efficient-reporting-for-all-reports' into featu…

44e7745

…re/efficient-reporting

Merge branch 'main' into feature/efficient-reporting

9f12f53

Style fix

8e7d92c

Fix typing and an import

9445e6f

dekkers reviewed Feb 21, 2024

View reviewed changes

PR feedback

d0f65f6

Signed-off-by: Donny Peeters <[email protected]>

Donnype requested a review from dekkers February 22, 2024 15:48

ammar92 reviewed Feb 23, 2024

View reviewed changes

PR comments

31dcd48

Donnype requested a review from ammar92 February 23, 2024 14:59

ammar92 previously approved these changes Feb 23, 2024

View reviewed changes

underdarknl added this to the OpenKAT v1.15 milestone Feb 27, 2024

Merge branch 'main' into feature/efficient-reporting

3799be7

ammar92 dismissed their stale review via 3799be7 February 27, 2024 09:45

ammar92 and others added 2 commits February 27, 2024 10:48

Update definitions.py

344d02a

- Revert any call since mypy cries in terror for some unknown reason.

fc60890

- Fix typing issue: add valid time string cast.

ammar92 reviewed Feb 27, 2024

View reviewed changes

rocky/reports/report_types/mail_report/report.py Outdated Show resolved Hide resolved

Donnype and others added 12 commits February 27, 2024 13:29

Fix unit test for ipv6 report

7bd0850

Fix integration tests

39da260

Style

acf00fe

Merge branch 'main' into feature/efficient-reporting

4e9a9f1

Add missing Error type while catching exceptions parsing query result…

60f8e41

…s and change list comprehensions

Merge branch 'feature/efficient-reporting' of github.com:minvws/nl-ka…

955592b

…t-coordination into feature/efficient-reporting

Fix findings endpoint

87d59eb

Remove old code and fix related bugs in the collect_data() method

d9f9c53

Fix bug ignoring web checks

07bc876

Fix: finding_types field default should be a dict, not a list

f56176a

Merge branch 'main' into feature/efficient-reporting

647501b

Fix merge conflict test issue

7d2a04d

Signed-off-by: Donny Peeters <[email protected]>

dekkers approved these changes Feb 29, 2024

View reviewed changes

dekkers merged commit aa231a3 into main Feb 29, 2024
26 checks passed

dekkers deleted the feature/efficient-reporting branch February 29, 2024 10:14

Donnype mentioned this pull request Feb 29, 2024

Update left over reports with the new query many endpoint #2578

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/efficient reporting #2516

Feature/efficient reporting #2516

Donnype commented Feb 15, 2024 •

edited

Loading

dekkers left a comment

madelondohmen commented Feb 22, 2024

Donnype commented Feb 22, 2024

ammar92 left a comment

stephanie0x00 commented Feb 29, 2024

Feature/efficient reporting #2516

Feature/efficient reporting #2516

Conversation

Donnype commented Feb 15, 2024 • edited Loading

Changes

Issue link

Demo

Notes for QA

Code Checklist

Communication

Checklist for code reviewers:

Checklist for QA:

dekkers left a comment

Choose a reason for hiding this comment

madelondohmen commented Feb 22, 2024

Donnype commented Feb 22, 2024

ammar92 left a comment

Choose a reason for hiding this comment

stephanie0x00 commented Feb 29, 2024

Checklist for QA:

What works:

What doesn't work:

Bug or feature?:

Donnype commented Feb 15, 2024 •

edited

Loading