Add initial SNMPv3 support to ipdevpoll #2743
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lunkwill42
commented
Nov 16, 2023
python/nav/ipdevpoll/snmp/common.py
Outdated
|
|
||
| # SNMPv3 only | ||
| sec_level: Optional[ | ||
| Union[Literal["noAuthNoPriv"], Literal["authNoPriv"], Literal["authPriv"]] |
There was a problem hiding this comment.
Actually, the tests are failing on Python 3.7 because I introduced the use of Literal here.
We don't actually use the type annotations for anything automated (like actual type checking), and we're not quite ready to drop Python 3.7 support yet, so I'm thinking maybe just replace this with str. What do you think, @hmpf ?
There was a problem hiding this comment.
Or maybe better yet: Switch to using the existing Enums in nav.Snmp,.defines
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2743 +/- ##
==========================================
+ Coverage 55.46% 55.57% +0.10%
==========================================
Files 567 567
Lines 41172 41244 +72
==========================================
+ Hits 22837 22922 +85
+ Misses 18335 18322 -13 ☔ View full report in Codecov by Sentry. |
Dataclasses weren't a part of Python when this was originally written, but as we need SNMPParameters to grow, it makes sense to refactor this now.
A better design might subclass this into v1, v2c and v3 versions, but inheritance would not be straightforward: community is part of v1 and v2c, bulk parameters are valid for v2c and v3, and auth and privacy parameters are only valid for v3. This bunches everything together, but does not perform any error detection or corrections. Incoming profiles are assumed to be correct and verified as they are inserted into NAV.
PyCharm wasn't able to properly deduce the return value of Netbox.get_preferred_management_profile, so this adds a return value annotation.
This extends SNMPParameters to include a factory class method which does most of the work of `snmp_parameter_factory`, but which can also fetch SNMP management profile information on a box-by-box basis.
This new method converts an SNMPParameter instance to a dict that can be used as the kwargs of an AgentProxy init method.
Netbox shadow objects used to use the pseudo attributes `read_only` and `snmp_version` to store the SNMP v1/v2c read-only community and SNMP version. On the Netbox Django model, these are computed properties, based on the Netbox' preferred SNMP management profile, and have been deprecated for a while. Since ipdevpoll now needs to access more complicated SNMP management configuration for a Netbox, this changes the Netbox shadow model to store an SNMPParameters object directly, which will be populated from the Netbox' preferred SNMP profile. Usages of the removed attributes have been updated. Usages mostly concern themselves with verifying that a device actually has an SNMP profile at all. However, the ipdevpoll dataloader used the value of the pseudo attributes to detect whether a Netbox' management credentials had changed and needed to be reloaded from the database, and this patch does not fully replace that. This more or less fixes Uninett#2522
Insert a `v` to make output look more like one would usually refer to the different SNMP versions.
Ensures the snmp_parameters attribute is None if a Netbox doesn't actually have an SNMP profile.
This is the final step to make everything work.
Version 0.1.10 contains bugfixes that are crucial for SNMPv3 support. This also removes the upper bounds of the version number, as we control the pynetsnmp-2 dependency anyway.
`Literal` and `dict` cannot be used as type annotations under Python 3.7. This changes SNMPParameters to use and enforce existing Enums for several attributes, obviating the need for using Literal annotations, as well as using `Dict` over `dict`
An SNMPv3 conversation may start with a discovery phase, to learn the remote SNMP entity's authoritative EngineID. This initial pair of "discovery" and "report" packages are handled implicitly by NET-SNMP. However, this can result in a timeout error from the async call to get a value, rather than a timeout error that we generate "ourselves" when we have waited too long. This type of error is raised by pynetsnmp as "yet another timeout exception class", rather than one of the existing timeout exceptions. This adds `SnmpTimeoutError` to the list of handled timeout exceptions where timeouts are handled explicitly, while in the mibretriever retrieval methods it is translated to a regular TimeoutError exception.
lunkwill42
force-pushed
the
feature/snmpv3-ipdevpoll
branch
from
867321a
to
03cdad2
Compare
The ipdevpoll-internal Netbox data structure needs to be udpated when the netbox' preferred SNMP profile has changed in the database, to ensure we can still communicate with the netbox. This is simply done by comparing the new computed `snmp_parameters` attribute. But, we must also remember to actually store the new value in the Netbox.copy() method.
|
Kudos, SonarCloud Quality Gate passed!
|
hmpf
reviewed
Nov 20, 2023
hmpf
reviewed
Nov 20, 2023
hmpf
approved these changes
Nov 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds SNMPv3 support to ipdevpoll and the asynchronous SNMP libraries of NAV.
Summary:
Individual SNMP attributes were still stored directly on Netbox objects ("shadow" objects, not Django models). The pre-existing
SNMPParametersnamedtuple was changed into a dataclass with all necessary parameters for establishing SNMP v1/v2c/v3 communication, and eachNetboxobject now has ansnmp_parametersattribute attached to it (unless it has no SNMP profile, that is).SNMPv3-related bugs were found and fixed in the
pynetsnmp-2library during the development of this PR. This PR updates NAV's required version of this library as a result.Additionally, SNMPv3 communication may lead to another type of timeout error being raised in unexpected places, so the
SnmpTimeoutErrorfrom pynetsnmp needed to be handled in various places. Details are in the commit logs.Closes #2736
Fixes #2522 as part of reworking how ipdevpoll accesses SNMP credentials.