From 4497de20d3f703601951677273b01ac7939d7b4d Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 12 Dec 2024 10:59:42 +0000 Subject: [PATCH 1/5] Update prefix cache in paloaltoarp also This makes the paloaltoarp plugin less dependent on having its base class (`Arp`) run first to update the prefix cache. --- python/nav/ipdevpoll/plugins/arp.py | 14 +++++++---- python/nav/ipdevpoll/plugins/paloaltoarp.py | 1 + tests/unittests/ipdevpoll/plugins_arp_test.py | 24 +++++++++++++++++++ 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/python/nav/ipdevpoll/plugins/arp.py b/python/nav/ipdevpoll/plugins/arp.py index 37baab8249..b018814dd5 100644 --- a/python/nav/ipdevpoll/plugins/arp.py +++ b/python/nav/ipdevpoll/plugins/arp.py @@ -69,11 +69,7 @@ def can_handle(cls, netbox): @defer.inlineCallbacks def handle(self): - # Start by checking the prefix cache - prefix_cache_age = datetime.now() - self.prefix_cache_update_time - if prefix_cache_age > self.prefix_cache_max_age: - yield self._update_prefix_cache() - + yield self._check_and_update_prefix_cache() self._logger.debug("Collecting IP/MAC mappings") # Fetch standard MIBs @@ -99,6 +95,14 @@ def handle(self): yield self._process_data(mappings) + @classmethod + @defer.inlineCallbacks + def _check_and_update_prefix_cache(cls): + """Updates the prefix cache if deemed necessary""" + prefix_cache_age = datetime.now() - cls.prefix_cache_update_time + if prefix_cache_age > cls.prefix_cache_max_age: + yield cls._update_prefix_cache() + @defer.inlineCallbacks def _get_ip_mib(self): if not self.is_arista(): diff --git a/python/nav/ipdevpoll/plugins/paloaltoarp.py b/python/nav/ipdevpoll/plugins/paloaltoarp.py index 304d2060a7..4723f399c7 100644 --- a/python/nav/ipdevpoll/plugins/paloaltoarp.py +++ b/python/nav/ipdevpoll/plugins/paloaltoarp.py @@ -49,6 +49,7 @@ def can_handle(cls, netbox): @defer.inlineCallbacks def handle(self): """Handle plugin business, return a deferred.""" + self._check_and_update_prefix_cache() self._logger.debug("Collecting IP/MAC mappings for Paloalto device") configurations = yield self._get_paloalto_configurations(self.netbox) diff --git a/tests/unittests/ipdevpoll/plugins_arp_test.py b/tests/unittests/ipdevpoll/plugins_arp_test.py index e9c85b5683..ab62cf90d3 100644 --- a/tests/unittests/ipdevpoll/plugins_arp_test.py +++ b/tests/unittests/ipdevpoll/plugins_arp_test.py @@ -1,7 +1,31 @@ +from datetime import datetime +from unittest.mock import patch + +import pytest +import pytest_twisted + from nav.ipdevpoll.storage import ContainerRepository from nav.ipdevpoll.plugins.arp import ipv6_address_in_mappings, Arp +class TestCheckAndUpdatePrefixCache: + @pytest.mark.twisted + @pytest_twisted.inlineCallbacks + def test_when_cache_is_expired_it_should_call_update(self): + with patch.object(Arp, "_update_prefix_cache") as update: + Arp.prefix_cache_update_time = datetime.now() - Arp.prefix_cache_max_age * 2 + yield Arp._check_and_update_prefix_cache() + assert update.called + + @pytest.mark.twisted + @pytest_twisted.inlineCallbacks + def test_when_cache_is_not_expired_it_should_not_call_update(self): + with patch.object(Arp, "_update_prefix_cache") as update: + Arp.prefix_cache_update_time = datetime.now() + yield Arp._check_and_update_prefix_cache() + assert not update.called + + def test_none_in_mappings_should_not_raise(): mappings = [(None, None, None)] assert not ipv6_address_in_mappings(mappings) From b37d4a548e6d5d7f7c203af7de0b7d6aea945a4d Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 12 Dec 2024 11:00:00 +0000 Subject: [PATCH 2/5] Run the paloaltoarp plugin before the arp plugin --- python/nav/etc/ipdevpoll.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/etc/ipdevpoll.conf b/python/nav/etc/ipdevpoll.conf index 1b10c7eac3..00ee05753c 100644 --- a/python/nav/etc/ipdevpoll.conf +++ b/python/nav/etc/ipdevpoll.conf @@ -133,7 +133,7 @@ description: Checks for changes in the reverse DNS records of devices interval: 30m intensity: 0 plugins: - arp paloaltoarp + paloaltoarp arp description: The ip2mac job logs IP to MAC address mappings from routers and firewalls (i.e. from IPv4 ARP and IPv6 Neighbor caches) From 71b7814babafacb720658566e09aba397ab97d6b Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 12 Dec 2024 11:53:52 +0000 Subject: [PATCH 3/5] Skip ARP collection if ARP already collected 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. --- python/nav/ipdevpoll/plugins/arp.py | 7 +++++++ tests/unittests/ipdevpoll/plugins_arp_test.py | 13 +++++++++++++ 2 files changed, 20 insertions(+) diff --git a/python/nav/ipdevpoll/plugins/arp.py b/python/nav/ipdevpoll/plugins/arp.py index b018814dd5..8665fb0da4 100644 --- a/python/nav/ipdevpoll/plugins/arp.py +++ b/python/nav/ipdevpoll/plugins/arp.py @@ -70,6 +70,9 @@ def can_handle(cls, netbox): @defer.inlineCallbacks def handle(self): yield self._check_and_update_prefix_cache() + if self._is_arp_already_collected(): + self._logger.debug("ARP records already collected for this device") + return self._logger.debug("Collecting IP/MAC mappings") # Fetch standard MIBs @@ -103,6 +106,10 @@ def _check_and_update_prefix_cache(cls): if prefix_cache_age > cls.prefix_cache_max_age: yield cls._update_prefix_cache() + def _is_arp_already_collected(self): + """Returns True if ARP entries have already been collected in this run""" + return shadows.Arp in self.containers and bool(self.containers[shadows.Arp]) + @defer.inlineCallbacks def _get_ip_mib(self): if not self.is_arista(): diff --git a/tests/unittests/ipdevpoll/plugins_arp_test.py b/tests/unittests/ipdevpoll/plugins_arp_test.py index ab62cf90d3..8d4a0a8b8b 100644 --- a/tests/unittests/ipdevpoll/plugins_arp_test.py +++ b/tests/unittests/ipdevpoll/plugins_arp_test.py @@ -4,6 +4,7 @@ import pytest import pytest_twisted +from nav.ipdevpoll import shadows from nav.ipdevpoll.storage import ContainerRepository from nav.ipdevpoll.plugins.arp import ipv6_address_in_mappings, Arp @@ -35,3 +36,15 @@ def test_make_new_mappings_should_not_raise_on_empty_ip(): a = Arp(None, None, ContainerRepository()) mappings = [(None, '00:0b:ad:c0:ff:ee')] a._make_new_mappings(mappings) + + +def test_when_arp_records_exist_is_arp_already_collected_should_return_true(): + containers = ContainerRepository() + containers.factory(('192.168.0.1', '00:co:ff:ee:ba:be'), shadows.Arp) + plugin = Arp(None, None, containers) + assert plugin._is_arp_already_collected() + + +def test_when_arp_records_do_not_exist_is_arp_already_collected_should_return_false(): + plugin = Arp(None, None, ContainerRepository()) + assert not plugin._is_arp_already_collected() From 1a2b5831e7fdf58c1a899b7049aaaec49da28c9a Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 12 Dec 2024 12:02:43 +0000 Subject: [PATCH 4/5] Add news fragment. --- changelog.d/3252.fixed.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog.d/3252.fixed.md diff --git a/changelog.d/3252.fixed.md b/changelog.d/3252.fixed.md new file mode 100644 index 0000000000..34ef19318b --- /dev/null +++ b/changelog.d/3252.fixed.md @@ -0,0 +1,2 @@ +Stop SNMP-based `arp` plugin from stepping on the `paloaltoarp` plugins toes. +Note that this **requires** updating the plugin order in the `ip2mac` job in `ipdevpoll.conf` From f238f970b385c76a47ddab240ba40d2e8836580d Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 12 Dec 2024 12:11:02 +0000 Subject: [PATCH 5/5] Add release note about plugin order change 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. --- NOTES.rst | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/NOTES.rst b/NOTES.rst index ac45f53fa9..0afb4eedf1 100644 --- a/NOTES.rst +++ b/NOTES.rst @@ -20,9 +20,6 @@ to be able to upgrade to Python 3.11: * :mod:`django-crispy-forms` * :mod:`crispy-forms-foundation` - -NAV 5.12 -======== Deprecation warnings -------------------- .. warning:: The ``[paloaltoarp]`` section of :file:`ipdevpoll.conf`, used for @@ -33,6 +30,20 @@ Deprecation warnings analogous to configuration of SNMP-based fetching. :ref:`See below for more details<5.12-new-http-rest-api-management-profile-type>`. +Change ``ip2mac`` plugin order in :file:`ipdevpoll.conf` +-------------------------------------------------------- + +The Palo Alto ARP plugin in ipdevpoll had a problem which caused the ARP +records it collected from Palo Palo firewalls to be unduly closed by the +regular SNMP-based ARP plugin. This release of NAV fixes this by making the +SNMP-based ARP plugin a "fallback" mechanism that doesn't touch ARP collection +if another plugin has already collected ARP data. + +In order for this fix to work, **you must change the order of the plugins** in the +``[job_ip2mac]`` section of your :file:`ipdevpoll.conf` file, to ensure that +the ``paloaltoarp`` plugin is listed *before* the ``arp`` plugin. + + .. _5.12-new-http-rest-api-management-profile-type: New way to configure fetching of Palo Alto firewall ARP cache data ------------------------------------------------------------------