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 ------------------------------------------------------------------ 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` 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) diff --git a/python/nav/ipdevpoll/plugins/arp.py b/python/nav/ipdevpoll/plugins/arp.py index 37baab8249..8665fb0da4 100644 --- a/python/nav/ipdevpoll/plugins/arp.py +++ b/python/nav/ipdevpoll/plugins/arp.py @@ -69,11 +69,10 @@ 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() + 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 @@ -99,6 +98,18 @@ 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() + + 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/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..8d4a0a8b8b 100644 --- a/tests/unittests/ipdevpoll/plugins_arp_test.py +++ b/tests/unittests/ipdevpoll/plugins_arp_test.py @@ -1,7 +1,32 @@ +from datetime import datetime +from unittest.mock import patch + +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 +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) @@ -11,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()