Ignore SNMP errors from alternate MIB instances by adobloug · Pull Request #4102 · Uninett/nav

adobloug · 2026-06-29T12:22:34Z

Scope and purpose

When ipdevpoll polls a device using community- or context-indexed BRIDGE-MIB
instances, and the device answers a query for one of those alternate instances
with an SNMP error such as noAccess, the entire job aborts.

pynetsnmp surfaces noAccess as an SnmpError reading
Packet for <ip> has error: Permission denied. MultiMibMixIn.__timeout_handler
only ignored TimeoutError for non-base (alternate) instances, so the SnmpError
propagated and aborted the plugin/job. This was observed against Cisco FirePower
(FTD) appliances, which return noAccess for community-indexed BRIDGE-MIB
queries, aborting every topo and inventory job for those devices each poll
cycle.

This extends __timeout_handler to also trap SnmpError for non-base instances,
matching the documented intent that errors for anything but the primary instance
should not derail collection. Errors on the base instance still propagate.

How to observe

On an affected device (e.g. Cisco FirePower), a community-indexed query returns
noAccess:

snmpbulkget -v2c -c 'public@<instance>' <ip> 1.3.6.1.2.1.17.1.4.1.2
Error in packet.
Reason: noAccess

Before this change, ipdevpoll.log shows repeated Job 'topo'/'inventory' ... aborted: Job aborted due to plugin failure (cause=Packet for <ip> has error: Permission denied). After, those jobs complete and the per-instance error is
ignored.

This pull request

changes SNMP error handling for alternate MIB instances

Contributor Checklist

Note: this stops the job aborts, but affected FirePower devices still expose a
broken community-indexed BRIDGE-MIB, so NAV will not collect per-VLAN CAM data
from them. That is a device-side limitation, not addressed here.

MultiMibMixIn queries community- or context-indexed MIB instances using alternate agents. __timeout_handler only trapped TimeoutError for these non-base instances, so any other SNMP error derailed collection from all instances and aborted the whole job. Cisco FirePower appliances answer community-indexed BRIDGE-MIB queries with the SNMP error-status noAccess, which pynetsnmp surfaces as an SnmpError reading "Packet for <ip> has error: Permission denied". This aborted every topo and inventory job for the affected devices. Trap SnmpError alongside TimeoutError for non-base instances, matching the documented intent that errors for anything but the primary instance should not derail collection. Errors on the base instance still propagate. Closes Uninett#4101 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

lunkwill42

Thanks, this seems quite reasonable — the diagnosis is spot on and the fix is the right shape. I traced the pynetsnmp error path myself and can confirm that trapping the base SnmpError for alternate instances is correct: there's no narrower class to catch for noAccess (unfortunately). The base-instance guard is also race-free, since _multiquery yields each instance sequentially, so errors on the primary instance still propagate as before.

I've left a few inline notes — the only two I'd genuinely like addressed are the method name (now a bit of a misnomer) and, if it's not too much bother, a test that drives a failure through _multiquery so we cover the wiring and not just the handler in isolation. Everything else is minor and optional.

lunkwill42 · 2026-07-09T13:16:51Z

@@ -656,17 +657,20 @@ class constructor; result is the actual result
        return integrator(results)

    def __timeout_handler(self, failure, descr):


Now that this also swallows access errors and not just timeouts, the name __timeout_handler undersells what it does — and it's easy to confuse with the _snmp_timeout_handler closure earlier in this same module. Could you rename it to something like __ignore_alternate_instance_error / __alternate_instance_errback? There are only three references to update: the def, the .addErrback(...) call in _multiquery, and the test.

lunkwill42 · 2026-07-09T13:16:51Z

        if self.agent_proxy is not self._base_agent:
-            failure.trap(TimeoutError, defer.TimeoutError)
-            self._logger.debug("ignoring timeout from %r", descr)
+            failure.trap(TimeoutError, defer.TimeoutError, SnmpError)


Just recording the reasoning here, since a future reader might wonder if trapping the base SnmpError is too broad — it isn't. In pynetsnmp only noAccess, resourceUnavailable and authorizationError are raised as SnmpError (distinguished only by the message string), so there is no finer-grained class to catch; genErr/noSuchName don't raise at all, they come back as empty results. One thing to be aware of: Snmpv3Error subclasses SnmpError, so a v3 auth/context failure on an alternate instance will now be swallowed too. That's consistent with how timeouts on alternates are already tolerated and only affects non-base instances, so I'm happy with it — just flagging it as a conscious choice.

lunkwill42 · 2026-07-09T13:16:51Z

+
+    def _handle(self, mixin, failure, descr="vlan100"):
+        # __timeout_handler is name-mangled
+        return mixin._MultiMibMixIn__timeout_handler(failure, descr)


These tests call the name-mangled handler directly, which verifies its logic but not that it's actually wired up as the errback in _multiquery — if the .addErrback(self.__timeout_handler, descr) line were dropped, all three would still pass. Testing the handler in isolation is a fair tradeoff given the alternative is driving a full inlineCallbacks/reactor deferred, but it'd be nice to also have one test that pushes a failing deferred through _multiquery and asserts the job survives — that's the behaviour #4101 is really about. Not blocking.

lunkwill42 · 2026-07-09T13:16:51Z

+    def test_when_base_instance_returns_snmp_error_then_it_should_propagate(self):
+        mixin = self._make_mixin()
+        # agent_proxy is left as the base agent
+        failure = _make_failure(SnmpError("Packet for 10.0.0.1 has error: genErr"))


Minor: genErr doesn't actually surface as an SnmpError in pynetsnmp (it comes back as an empty result), so this sample message is slightly misleading for a future reader. The handler ignores the message text so the test is correct either way — maybe just use a message that reflects a real trigger (Permission denied / Authorization error) or something neutral.

lunkwill42 · 2026-07-09T13:16:51Z

 from nav.mibs.types import LogicalMibInstance


+def _make_failure(exception):


Optional nit: this wrapper doesn't buy much over calling Failure(...) directly — _make_failure(SnmpError(...)) is a touch longer than Failure(SnmpError(...)) and adds one indirection. I'd inline it and drop the helper. If you keep it, you should still adhere to the stepdown rule and place it somewhere after the higher-level code that utilizes it.

lunkwill42 · 2026-07-09T13:16:51Z

+class TestMultiMibMixInTimeoutHandler:
+    """Tests for MultiMibMixIn.__timeout_handler()"""
+
+    def _make_mixin(self):


House-style note: We try to follow the step-down rule in test modules too, i.e. the tests read top-to-bottom as the high-level story with helpers (_make_mixin, _handle) placed below the tests that use them rather than above.

Rename __timeout_handler to __ignore_alternate_instance_error, since it now swallows access errors too and was easy to confuse with the _snmp_timeout_handler closure elsewhere in the module. Record in the docstring why trapping the base SnmpError is correct and that Snmpv3Error is deliberately caught along with it. Add a test that pushes a failing deferred through _multiquery, covering the errback wiring rather than the handler in isolation. Inline the _make_failure helper, drop the misleading genErr sample message, and reorder helpers below the tests per the step-down rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

sonarqubecloud · 2026-07-09T13:54:36Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

adobloug · 2026-07-09T13:55:44Z

Thanks for the thorough review, @lunkwill42! Pushed 2dac7b9 addressing the feedback:

Method name — renamed __timeout_handler → __ignore_alternate_instance_error (def, the .addErrback(...) in _multiquery, and the test).
_multiquery wiring test — added TestMultiMibMixInMultiquery, which pushes a failing deferred (alternate instance raising SnmpError) through _multiquery and asserts the job survives and the base result is still returned. This would fail if the errback were dropped, unlike the isolated handler tests.
Recorded reasoning — expanded the handler docstring to note that pynetsnmp only raises noAccess/resourceUnavailable/authorizationError as the base SnmpError (so there's no narrower class to trap), and that Snmpv3Error is deliberately swallowed on alternates too.
Misleading genErr sample — swapped for Authorization error, a message that actually surfaces as SnmpError.
_make_failure helper — inlined to Failure(...) and removed.
Step-down rule — moved _make_mixin/_handle below the tests that use them.

While at it, renamed three pre-existing test_should_* methods to test_it_should_* so they satisfy the naming-convention check.

lunkwill42 assigned adobloug Jul 8, 2026

lunkwill42 reviewed Jul 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ignore SNMP errors from alternate MIB instances#4102

Ignore SNMP errors from alternate MIB instances#4102
adobloug wants to merge 2 commits into
Uninett:5.18.xfrom
adobloug:bugfix/4101-ipdevpoll-noaccess-abort

adobloug commented Jun 29, 2026

Uh oh!

lunkwill42 left a comment

Uh oh!

lunkwill42 Jul 9, 2026

Uh oh!

lunkwill42 Jul 9, 2026

Uh oh!

lunkwill42 Jul 9, 2026

Uh oh!

lunkwill42 Jul 9, 2026

Uh oh!

lunkwill42 Jul 9, 2026

Uh oh!

lunkwill42 Jul 9, 2026

Uh oh!

sonarqubecloud Bot commented Jul 9, 2026

Uh oh!

adobloug commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -656,17 +657,20 @@ class constructor; result is the actual result
		return integrator(results)

		def __timeout_handler(self, failure, descr):

		from nav.mibs.types import LogicalMibInstance


		def _make_failure(exception):

Uh oh!

Conversation

adobloug commented Jun 29, 2026

Scope and purpose

How to observe

This pull request

Contributor Checklist

Uh oh!

lunkwill42 left a comment

Choose a reason for hiding this comment

Uh oh!

lunkwill42 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

lunkwill42 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

lunkwill42 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

lunkwill42 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

lunkwill42 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

lunkwill42 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jul 9, 2026

Quality Gate passed

Uh oh!

adobloug commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants