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

Improve argument parsing of the mqtt loader #850

Merged
merged 11 commits into from
Sep 23, 2024
Merged

Conversation

twiggler
Copy link
Contributor

@twiggler twiggler commented Sep 16, 2024

This PR improved upon the argument parsing of the mqtt loader.
More specifically,

  1. It checks for invalid values. For example, check that the specified number of peers is strictly positive
  2. It adds more helpful error messages.

Additionally,

  1. It deletes the legacy targetd loader (Targetd: Improve argument parser MQTT -loader and remove old Targetd-loader #849 was about two things, now not entirely sure how they are related).
  2. Fixes a typing issue.
  3. Removes the hostname prefix of CALLID messages
  4. Simplify mocking of MQTTClient

Closes #849

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 51.61290% with 15 lines in your changes missing coverage. Please review.

Project coverage is 76.19%. Comparing base (765c668) to head (dc1ed23).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
dissect/target/loaders/mqtt.py 51.61% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #850      +/-   ##
==========================================
+ Coverage   75.89%   76.19%   +0.29%     
==========================================
  Files         312      310       -2     
  Lines       26842    26674     -168     
==========================================
- Hits        20372    20323      -49     
+ Misses       6470     6351     -119     
Flag Coverage Δ
unittests 76.19% <51.61%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@twiggler twiggler marked this pull request as ready for review September 18, 2024 07:45
@twiggler twiggler requested a review from pyrco September 18, 2024 07:54
tests/loaders/test_mqtt.py Outdated Show resolved Hide resolved
@twiggler twiggler force-pushed the 849-mqtt-argument-parsing branch 2 times, most recently from 0c079ca to b6b55a0 Compare September 18, 2024 14:05
Copy link
Contributor

@pyrco pyrco left a comment

Choose a reason for hiding this comment

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

Probably better to remove the host/ip checks. I don't think it is worth the aded complexity as the broker connection could fail due to a number of reasons anyway.

dissect/target/loaders/mqtt.py Outdated Show resolved Hide resolved
tests/loaders/test_mqtt.py Outdated Show resolved Hide resolved
The hostname is already included in the payload
File could be deleted between check and use
This way we do not have scope imports from loaders.mqtt to the test body.
The benefits do not outweigh the  cost in complexity
Only a single test is using the mock and it is a single line to set up.
Note that using the context manager is only required when you want the mock teared down during the test.
@twiggler twiggler merged commit 45f7e76 into main Sep 23, 2024
17 of 18 checks passed
@twiggler twiggler deleted the 849-mqtt-argument-parsing branch September 23, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Targetd: Improve argument parser MQTT -loader and remove old Targetd-loader
2 participants