Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SNMPv3 support to nav.Snmp.pynetsnmp implementation #2703

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Nov 1, 2023

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

@lunkwill42 lunkwill42 self-assigned this Nov 1, 2023
@lunkwill42 lunkwill42 requested review from andreassolberg, hmpf and stveit and removed request for andreassolberg November 1, 2023 15:27
@lunkwill42
Copy link
Member Author

I'm kind of reticent to add all these arguments directly to the Snmp class __init__ method. Another approach might be to create an SNMPv3 configuration (data)class and add that as a single optional argument to Snmp.__init__ - but I can't decide. Thoughts, anyone?

@lunkwill42 lunkwill42 linked an issue Nov 1, 2023 that may be closed by this pull request
@lunkwill42 lunkwill42 force-pushed the feature/snmpv3-synchronous branch from 597f90c to deae6cc Compare November 1, 2023 15:34
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #2703 (2131cda) into snmpv3 (f936740) will increase coverage by 0.00%.
The diff coverage is 59.01%.

@@           Coverage Diff           @@
##           snmpv3    #2703   +/-   ##
=======================================
  Coverage   55.19%   55.19%           
=======================================
  Files         561      562    +1     
  Lines       40946    41004   +58     
=======================================
+ Hits        22601    22634   +33     
- Misses      18345    18370   +25     
Files Coverage Δ
python/nav/Snmp/defines.py 100.00% <100.00%> (ø)
python/nav/Snmp/errors.py 100.00% <100.00%> (ø)
python/nav/Snmp/pynetsnmp.py 52.22% <43.18%> (-2.37%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

github-actions bot commented Nov 1, 2023

Test results

     12 files       12 suites   13m 19s ⏱️
3 217 tests 3 217 ✔️ 0 💤 0
9 126 runs  9 126 ✔️ 0 💤 0

Results for commit 2131cda.

♻️ This comment has been updated with latest results.

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.
@lunkwill42 lunkwill42 force-pushed the feature/snmpv3-synchronous branch from deae6cc to 03eba24 Compare November 2, 2023 11:46
@stveit
Copy link
Contributor

stveit commented Nov 2, 2023

I agree that a dataclass should be made for the parameter inputs

Comment on lines +91 to +104
self,
host: str,
community: str = "public",
version: Union[str, int] = "1",
port: Union[str, int] = 161,
retries: int = 3,
timeout: int = 1,
# SNMPv3-only options
sec_level: Optional[SecurityLevel] = None,
auth_protocol: Optional[AuthenticationProtocol] = None,
sec_name: Optional[str] = None,
auth_password: Optional[str] = None,
priv_protocol: Optional[PrivacyProtocol] = None,
priv_password: Optional[str] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean about the ugly __init__.

Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything else missing than tests?

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 silly situation.
Copy link

sonarcloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@lunkwill42
Copy link
Member Author

Anything else missing than tests?

Yes, I just pushed a small update for a silly bug.

Tests would be nice, but I have to think a bit about how do to them - the coverage on this file is a measly 54% as of now, so I might want to do them in a separate PR...

And a good question is whether we should act on @stveit's suggestion...

@lunkwill42 lunkwill42 merged commit 2131cda into Uninett:snmpv3 Nov 2, 2023
10 checks passed
@lunkwill42
Copy link
Member Author

Ah well, dammnit. Pushing to the wrong branch screwed up this PR, marking it as merged. That was not the intent - now I can't reopen it. I'll have to make a new one.

lunkwill42 added a commit that referenced this pull request Nov 13, 2023
 Add SNMPv3 support to nav.Snmp.pynetsnmp implementation #2703
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SNMPv3 session support to the synchronous libraries
3 participants