Fix unintended breakdown of MIB object sub-indexes into symbolic representations #389
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports
|
Test results 3 files 3 suites 1m 29s ⏱️ Results for commit 83d6c4e. ♻️ This comment has been updated with latest results. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #389 +/- ##
=======================================
Coverage 98.66% 98.66%
=======================================
Files 77 77
Lines 9711 9715 +4
=======================================
+ Hits 9581 9585 +4
Misses 130 130 ☔ View full report in Codecov by Sentry. |
The OID class is designed to represent a tuple of ints. It was never designed to contain non-int elements (although it can translate non-int values to int elements when instances are constructed). The Zino code has bugs that actually would feed the OID class with non-int elements, and because the OID class accepted it, strange workarounds have been written by those who did not understand that this was not the intended behavior. This change enforces all elements to be int on instantiation - presumably at the cost of instantiation becoming slower.
An `Identifier` was intended to represent an object's MIB name, symbolic object name and numeric OID index. However, due to a misunderstanding of how PySNMP works, this was not always the case: `getMibSymbol` will actually break down the remaining OID index to one or more symbolic indices according to the MIB specification, so the third element of the `getMibSymbol()` call may actually be a tuple of various objects that represents individual indices. While this could be helpful in the codebase, it was not the intention of `_convert_varbind` and `Identifier`. To keep Zino's interface as intended here, this change tries to unroll all the symbolic indices back to a single OID object - likely with some performance penalty. We might want to look into changing the codebase to work with symbolic indexes at a later time.
bgpstatemonitortask.py contained some strange workarounds for behaviors that were actually bugs in the `_convert_varbind` implementation. These bugs have been fixed, causing the workarounds to fail: This fixes the failures by removing the workarounds.
lunkwill42
force-pushed
the
bugfix/symbolic-oid-interpretation-issues
branch
from
14ad020 to
29b2441
Compare
src/zino/snmp.py
Outdated
| def _index_to_tuple(index: ObjectIdentity) -> Tuple[int, ...]: | ||
| """Unrolls a PySNMP index object to a tuple of integers""" | ||
| try: | ||
| return tuple(index) | ||
| except TypeError: | ||
| try: | ||
| return index.asTuple() | ||
| except AttributeError: | ||
| try: | ||
| return index.getOid() | ||
| except AttributeError: | ||
| return (int(index),) | ||
|
|
||
|
|
There was a problem hiding this comment.
Upon testing the new SNMP framework I've found that this still isn't up to snuff, as some bits keep getting cut off. Notably, an object like .1.3.6.1.4.1.9.9.187.1.2.5.1.4.1.4.10.0.0.1 would normally be broken down to CISCO-BGP4-MIB::cbgpPeer2AdminStatus.ipv4."10.0.0.1", but what we want from our function is something more like CISCO-BGP4-MIB::cbgpPeer2AdminStatus.1.4.10.0.0.1. I.e. the index returned should be 1.4.10.0.0.1 (1 is the type of address represented, 4 is the number of subsequent octets that represent the address) - instead this only return 4.10.0.0.1. Only the bgpstatemonitortask cares right now, but this PR makes it expect the latter.
There was a problem hiding this comment.
I've pushed a new commit where the strategy becomes more or less the same as in the netsnmp-cffi replacement branch: Reconstruct a prefix from the mib and object name, and then strip that from the incoming OID argument to get the row_index suffix value.
The table index for an address like `10.0.0.42` will be `.1.4.10.0.0.42`, where `1 is the address type and `4` is the address length - while the remainder is the address itself. We don't really need the `1` and the `4` to detect the address type, as `ip_address` will parse things correctly if given just a series of bytes. It turns out that the index unrolling first implemented in `_convert_varbind()` will cause some elements of the `row_index` OID to be left out, causing mayhem for client code that relies on these values.
|
Scope and purpose
This fixes a problem discovered in the Zino SNMP abstraction layer during the construction of an alternative SNMP abstraction layer: The
_convert_varbind()function was intended to break down an object identity into a symbolic name and a numeric index (as represented by anIdentifierobject).However, because of a misunderstanding of how PySNMP works, and a slight bug in the implementation of the Zino
OIDclass, it turns out that the sub-indexes are also converted to symbolic representations if the MIB objects are defined that way - and the OID class appears to have allowed these non-integer elements to be represented within it.This was not understood as unintended behavior when the BGP state monitor task was authored, causing workarounds for the strange behavior to be written in that plugin, rather than the bug to be reported :-)
It may actually be in our interest to have some MIB-assisted symbolic breakdown of object identities, but the PySNMP objects should not leak through our abstraction layer (which is slated to be replaced), so this is something we might want to look into at a later stage.
This pull request
OIDclass.Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Zino can be found in the
README.