Merge pull request #3254 from Uninett/bugfix/paloalto-arp-priority

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

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
 ------------------------------------------------------------------

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`

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)

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():

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)

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()