mirror of
https://github.com/jokob-sk/NetAlertX.git
synced 2026-09-17 08:44:42 -04:00
main
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Mauricio Camayo
|
d6b4696ac9 |
fix: track per-source delta so one instance's reset can't mask the other's spike
Addresses CodeRabbit review on PR #1765 (pullrequestreview-5069337680). pihole_monitor.py: - last_raw is now tracked per source ({"primary": N, "secondary": M} per device) instead of one combined value. Combining raw totals across sources before diffing let a counter reset on one instance silently net out against real traffic on the other - e.g. primary +2000 (a real spike) and secondary resetting 1000->5 (-995) would combine into a raw delta of only 1005, hiding most of the primary's actual spike behind the secondary's unrelated restart. - New aggregate_source_deltas(): diffs each source independently via compute_delta(), then sums only the valid deltas. A source with no valid delta this run (bootstrapping or just reset) contributes nothing and doesn't block the others; each source keeps its own reference point going forward. - State loaded from before this change (last_raw as a plain number, not per-source) is now tolerated instead of crashing - treated as no prior reference point, so every source just bootstraps fresh on the next run. README.md: - Fixed a self-contradicting line: a less frequent schedule means larger per-run deltas, so PIHOLEMON_MIN_BLOCKED may need *raising*, not lowering as it previously said. - Corrected PIHOLEMON_HISTORY_DAYS guidance: it's a retention window, not a detection delay. A new device becomes evaluable on its 3rd successful run (1st anchors the counter, 2nd records the first delta, 3rd has a baseline to compare against), not after the full retention window. Tests: 54 (up from 48). New coverage: aggregate_source_deltas() unit tests including the exact dual-source reset-masking scenario, a main()-level integration test for the same, and a regression test for tolerating pre-per-source state. Both the reset-masking fix and the legacy-state guard verified via mutation testing (reverted each, confirmed the relevant tests fail, restored). 99% line+branch coverage maintained. |
||
|
Mauricio Camayo
|
e543f14d08 |
fix: address round 2 of jokob-sk's maintainer review
References PR #1765. Docs: - Added PIHOLEMON to docs/PLUGINS.md and a new "Approach 4" section in docs/PIHOLE_GUIDE.md, leading with anomaly detection (the actual differentiator vs PIHOLEAPI) and explaining when to pick each plugin. - README/PLUGINS.md/config.json's UI-facing description all reordered and shortened to lead with anomaly detection instead of device import, and to drop implementation detail that belongs in the README, not the Settings page. - Trimmed the "Why not extend PIHOLEAPI" README section per feedback - useful context for a maintainer, not for an end user configuring the plugin. config.json / pihole_monitor.py: - RUN defaults to "disabled", matching every other non-core plugin. - VERIFY_SSL split into PRIMARY_VERIFY_SSL / SECONDARY_VERIFY_SSL - each instance can be http/https independently. Settings reordered so each *_VERIFY_SSL sits right under its matching *_PASSWORD. - GRAPHQL_TOKEN removed; graphql_token now reads the core API_TOKEN setting instead of a plugin-specific duplicate. - GRAPHQL_URL replaced with a GET_OWNER boolean - the endpoint is now derived from this app's own GRAPHQL_PORT (single source of truth) instead of a URL the user had to keep in sync by hand. - HISTORY_LENGTH (run count) replaced with HISTORY_DAYS (a real time window): state now stores [timestamp, delta] samples and trim_history() drops anything older than the window, so the baseline means the same thing regardless of schedule - a faster schedule adds more data points instead of shrinking the window. - STATE_FILE moved from the log folder to dbFolderPath, so the rolling anomaly baseline survives NetAlertX upgrades instead of being wiped with the logs. - netalertx_device_owner() (1 GraphQL call per device) replaced by netalertx_device_owners() (1 call per run, batched) - avoids N blocking round-trips on a large network. - Fixed a zero-baseline bug: `bool(... and baseline and ...)` silently exempted a device with an all-zero blocked-query history (0.0 is falsy in Python) from ever being flagged, even on its first real spike. Now checks `baseline is not None`. - Fixed the placeholder-MAC filter: only excluded the literal "ip-::", not Pi-hole's general "ip-<address>" placeholder pattern. Caught downstream by is_mac() either way, but now the actual placeholder check does what it looks like it does. - Fixed a cumulative-counter bug: Pi-hole's /api/stats/top_clients returns a count that's cumulative since FTL last started, not a per-interval or daily-resetting one (confirmed against FTL's own source and long-standing user reports that it doesn't reset at midnight). Comparing that raw total directly against a rolling average made any device's ordinary growing traffic look like an escalating anomaly. compute_delta() now diffs each run's raw count against the previous run's (state gained a per-key last_raw reference point alongside the delta history) - None (not 0) on the first-ever run for a device or right after a counter reset, so those runs re-anchor the reference point instead of fabricating or swallowing a delta. - RUN_SCHD default changed from every 6 hours to every 5 minutes now that the baseline window is real days, not run count, so a frequent schedule only adds data points instead of narrowing the window; also matches the default most other device-scanner plugins use. - RUN_SCHD gained the same live cron-validity checkmark ARPSCAN and other scanner plugins use (a ✓/✗ icon next to the field, validated client-side against a regex) - reuses the existing generic validateRegex() widget, nothing plugin-specific to build. Tests: 48 tests (up from 37), 99% line+branch coverage. Every fix above verified via mutation testing (deliberately broken, confirmed the relevant test fails, then restored). |
||
|
Mauricio CamayoandClaude Sonnet 5
|
ed21c86214 |
test: assert exact history window instead of len() >= 1
CodeRabbit follow-up on PR #1765 (https://github.com/netalertx/NetAlertX/pull/1765#discussion_r3888374014): test_main_history_length_never_produces_empty_or_growing_unbounded only asserted len(history) >= 1, which a mis-clamped history_length (e.g. keeping 4 items instead of 1) would still pass unnoticed. Replaced with test_main_history_length_clamps_and_trims_exactly, seeding distinct ordered values and asserting the exact retained history against each PIHOLEMON_HISTORY_LENGTH boundary. Verified it actually catches a broken clamp: temporarily reverted the max(1, ...) fix in pihole_monitor.py, confirmed this test fails ([] == [40]) while the rest of the suite still passes, then restored the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHJAArRiet4GmXUsxnNLdW |
||
|
Mauricio CamayoandClaude Sonnet 5
|
f564448617 |
fix: address CodeRabbit review findings on pihole_monitor plugin
Addresses 5 of the 6 actionable comments from CodeRabbit's review of PR #1765 (netalertx/NetAlertX#1765), plus adds test coverage: - fetch_top_blocked_clients() returns None on failure instead of {}, so a failed request can no longer be mistaken for "genuinely zero blocked queries this run" and silently write a false 0 into a device's rolling history baseline. main() now tracks a stats_complete flag and skips anomaly evaluation + history persistence entirely for a run with incomplete blocked-query data. - fetch_top_blocked_clients() is now called with count=max_clients (the existing PIHOLEMON_API_MAXCLIENTS setting) instead of a hardcoded default of 50, so clients beyond the top 50 are no longer silently dropped from anomaly detection. - New build_ip_to_mac() derives the IP->MAC identity map from every gathered device entry instead of from merge_device_entries()'s by-MAC-deduplicated output, which only kept one IP per device and silently lost a multi-IP device's other IPs (misattributing their blocked-query traffic to a bare IP instead of the real MAC). - PIHOLEMON_HISTORY_LENGTH is clamped to at least 1, so a negative setting can no longer reach the history[-history_length:] slice with a nonsensical negative-of-negative length. - PIHOLEMON_VERIFY_SSL now defaults to true (was false, matching the official PIHOLEAPI plugin's convention). README documents the http:// vs https:// credentials trade-off explicitly rather than forcing https:// - most home Pi-hole setups, including the one this plugin targets, run over plain HTTP on a trusted LAN. - Added test/plugins/test_pihole_monitor.py (37 tests, 99% line and branch coverage of pihole_monitor.py per pytest-cov - only the `if __name__ == '__main__':` entry-point guard is unreached): auth and deauth success/failure paths, the None-sentinel-on-failure contract, fetch_devices()'s own failure path, build_ip_to_mac()'s multi-IP fix, gather_device_entries()'s skip branches and fake-MAC fallback, netalertx_device_owner()'s success/failure/no-URL paths, and main()-level coverage for source aggregation, the stats_complete gate, the history_length boundary clamp, the CONSIDER_ONLINE fallback, an unconfigured-sources run, and the offline-device / invalid-MAC / unknown-IP / owner-lookup branches together in one run. Not addressed: CodeRabbit's suggestion to hard-reject http:// URLs in auth(). Diverges deliberately - it would break the plugin's majority use case (Pi-hole admin API on a trusted home LAN without TLS), which this repo's own PIHOLEAPI plugin also targets over plain HTTP. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CHJAArRiet4GmXUsxnNLdW |