Add SNMPv3 support to nav.Snmp.pynetsnmp implementation #2703 #2710
Conversation
Codecov Report
@@ Coverage Diff @@
## snmpv3 #2710 +/- ##
==========================================
- Coverage 55.24% 55.19% -0.05%
==========================================
Files 561 562 +1
Lines 40947 41004 +57
==========================================
+ Hits 22622 22634 +12
- Misses 18325 18370 +45
... and 3 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
| self.handle = _MySnmpSession(self._build_cmdline()) | ||
| self.handle.open() | ||
|
|
||
| def _verify_snmpv3_params(self): |
There was a problem hiding this comment.
As @stveit suggested in the automerged one, a dataclass for the SNMPv3 config sounds better and better. Then this method could be on that class.
The instance would be stored as a whole in the Snmp init, which would make for more verbose code but probably better since theere is only one Snmp-class.
There was a problem hiding this comment.
I think this is a good idea(tm). But there is now a bunch of PRs based on this that are waiting to be merged, and all of them need to change as a result of this.
May I suggest doing a refactor PR to fix all the affected bits once they've all been merged? I can add it as a task to #1177 .
| params.extend(["-l", self.sec_level.value, "-u", self.sec_name]) | ||
| if self.auth_protocol: | ||
| params.extend(["-a", self.auth_protocol.value]) | ||
| if self.auth_password: | ||
| params.extend(["-A", self.auth_password]) | ||
| if self.priv_protocol: | ||
| params.extend(["-x", self.priv_protocol.value]) | ||
| if self.priv_password: | ||
| params.extend(["-X", self.priv_password]) |
There was a problem hiding this comment.
If the snmpv3-conf was stored in a dataclass, this could be a method on that class.
| params.extend( | ||
| ["-r", str(self.retries), "-t", str(self.timeout), f"{host}:{self.port}"] |
There was a problem hiding this comment.
Any specific reason these need to be at the end? I'd be tempted to merge this line with line 179.
There was a problem hiding this comment.
The -r and -t arguments probably don't. But the agent spec is usually placed last, due to what the documentation says, e.g. snmpcmd(1):
snmpcmd [OPTIONS] AGENT [PARAMETERS]
or snmpwalk(1):
snmpwalk [APPLICATION OPTIONS] [COMMON OPTIONS] AGENT [OID]
(and of course, 20 years of of conventional usage of these command line programs).
The version of the commands on my current computer seem to accept the arguments in arbitrary order, however, so I can't be sure whether this was always true, or whether it was introduced at some specific version.
There was a problem hiding this comment.
You could add a comment "# stays here: do not meddle with 20 years of ingrained habits" or something :)
Just a slight cleanup before starting on SNMPv3 support
These could be re-used elsewhere, like in the `SNMPv3Form`.
This adds the optional SNMPv3 parameters to `nav.Snmp.pynetsnmp.Snmp`, along with parameter verification based on the selected SNMP version.
This reworks the SNMP command line builder to include SNMPv3 parameters as needed.
Snmp.__init__() can now raise exceptions before `self.handle` is ever created, thereby causing errors to be logged from Snmp.__del_ everytime the garbage collector deletes such defunct object. This protects against that slight problem.
lunkwill42
force-pushed
the
feature/snmpv3-synchronous
branch
from
a613588
to
c3dd1a8
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
This adds the necessary arguments, validation and a reworked command line builder for SNMPv3 communication to the synchronous SNMP implementation in
nav.Snmp.pynetsnmp.Snmp.Closes #2700
This replaces #2703, which got "merged" due to an error, and cannot be reopened.