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

Abort SNMP-based ARP collection if ARP records seem to have been collected earlier in the same ipdevpoll job run #3254

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Dec 12, 2024

Fixes #3252 by the following strategy:

  1. Makes the SNMP arp plugin a sort of "fallback" plugin: If it detects that ARP mappings have already been added to the container registry, it skips all SNMP-based processing of ARP, to avoid stepping on some other plugin's toes.
  2. Changes the default run order of plugins in the ip2mac ipdevpoll job: Vendor specific plugins like paloaltoarp should run first, and if they they either declined to run, or didn't find anything, it cannot hurt that SNMP-based collection is run.

@lunkwill42 lunkwill42 self-assigned this Dec 12, 2024
Copy link

github-actions bot commented Dec 12, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 3 0 0.61s
✅ PYTHON ruff 3 0 0.01s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Dec 12, 2024

Test results

    9 files      9 suites   8m 15s ⏱️
2 162 tests 2 162 ✅ 0 💤 0 ❌
4 063 runs  4 063 ✅ 0 💤 0 ❌

Results for commit f238f97.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.58%. Comparing base (a1d8287) to head (f238f97).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
+ Coverage   60.54%   60.58%   +0.03%     
==========================================
  Files         606      606              
  Lines       43723    43733      +10     
  Branches       48       48              
==========================================
+ Hits        26474    26494      +20     
+ Misses      17237    17227      -10     
  Partials       12       12              

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

@lunkwill42 lunkwill42 marked this pull request as ready for review December 12, 2024 12:12
@lunkwill42 lunkwill42 requested review from a team and jorund1 December 12, 2024 12:22
This makes the paloaltoarp plugin less dependent on having its base
class (`Arp`) run first to update the prefix cache.
@lunkwill42 lunkwill42 force-pushed the bugfix/paloalto-arp-priority branch from b04a11e to 5ac2bef Compare December 12, 2024 13:05
@lunkwill42
Copy link
Member Author

Found a bug myself, in the way the extracted cache function was being called. Force-pushed changes since no-one had reviewed yet.

This ensures that the SNMP-based `arp` plugin exits early if it appears
that some other plugin has already collected Arp records before it.

This makes the `arp` plugin a sort of fallback in a scenario where
vendor specific ARP collection plugins run first, rather than having
multiple ARP plugin step on each others toes.
This also removes the premature 5.12 heading: The already existing
`Unreleased` section takes it place, and will be updated as standard
procedure on the next feature release.
@lunkwill42 lunkwill42 force-pushed the bugfix/paloalto-arp-priority branch from 5ac2bef to f238f97 Compare December 12, 2024 13:09
Copy link

sonarcloud bot commented Dec 12, 2024

Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

Couldn't spot anything besides a thought for future improvements.

NOTES.rst Show resolved Hide resolved
@lunkwill42 lunkwill42 merged commit ca1205e into master Dec 13, 2024
15 checks passed
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.

[BUG] ARP records from Palo Alto firewalls keep getting closed and re-opened
2 participants