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 an SNMPv3 management profile type #2699

Merged

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Nov 1, 2023

While there is already a profile type for SNMP with a v1/v2c protocol version choice, none of the other attributes of this profile are relevant for SNMPv3, and it hardly serves us to complicate these profiles with reams of attributes that don't belong in v1/v2c.

This therefore adds a new profile type for SNMPv3, with the corresponding definition in a SeedDB management profile form.

It also adds the option to add generic warning messages to protocol forms as a whole, so that the SNMPv3 protocol form can be marked as experimental/not-yet-fully-implemented-in-the-backend

Closes #2693

SNMPv3 attributes are very distinct from either SNMPv1 or SNMPv2, so i
deserves a new type.
My browser tries to input my NAV username and password in these
unrelated profile fields.  This is not some kind of login form, so
this suggests to the browser to eschew autocompletion of usernames
and passwords.
The two password are only required dependent upon which security level
has been chosen for this profile.
This is used to signal to end users that while SNMPv3 profiles are
available, the functionality in NAV is not yet complete and cannot be
expected to work.

This notabene can be removed from `SNMPV3Form` once SNMPv3
implementation is considered complete.
@lunkwill42
Copy link
Member Author

Screenshot from SeedDB:

Screenshot 2023-11-01 at 14-16-08 Edit management profile SNMPv3 experiment

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #2699 (bca69ae) into master (e879e67) will increase coverage by 0.05%.
Report is 9 commits behind head on master.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master    #2699      +/-   ##
==========================================
+ Coverage   55.19%   55.24%   +0.05%     
==========================================
  Files         561      561              
  Lines       40917    40947      +30     
==========================================
+ Hits        22584    22622      +38     
+ Misses      18333    18325       -8     
Files Coverage Δ
...on/nav/web/seeddb/page/management_profile/forms.py 92.63% <100.00%> (+17.63%) ⬆️
python/nav/models/manage.py 72.10% <66.66%> (-0.09%) ⬇️

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link

github-actions bot commented Nov 1, 2023

Test results

     12 files       12 suites   14m 5s ⏱️
3 225 tests 3 225 ✔️ 0 💤 0
9 150 runs  9 150 ✔️ 0 💤 0

Results for commit bca69ae.

♻️ This comment has been updated with latest results.

These need to reflect the existence of the new SNMPv3 profile type.
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.

Looking good. I'm assuming what's missing is tests?

@lunkwill42
Copy link
Member Author

Looking good. I'm assuming what's missing is tests?

Correct! Still missing being able to run tests directly from my editor when working with NAV. Also, the coverage data generated inside the docker container will have different paths to reference the covered files, which aren't recognized by my IDE, so I cannot even view proper coverage data without pushing a PR to get some CodeCov feedback :P

As with SNMP v1/v2 profiles, it is good to know whether a profile
represents write access to a device or not, something PortAdmin or
Arnold will need to know in order to select the correct profile if there
are several.
@lunkwill42 lunkwill42 marked this pull request as ready for review November 9, 2023 10:07
@lunkwill42 lunkwill42 requested a review from hmpf November 9, 2023 10:07
Netbox.get_preferred_snmp_management_profile() also needs to be able
to look at SNMPv3 profiles and return those if they fit - as they
would take priority before SNMPv2 profiles, if there are any.
Copy link

sonarcloud bot commented Nov 9, 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
0.0% 0.0% Duplication

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.

Looks good, is it feature complete now? See also inline comment.

python/nav/models/manage.py Show resolved Hide resolved
@lunkwill42
Copy link
Member Author

Looks good, is it feature complete now? See also inline comment.

AFAIK, it is. But profiles are flexible, so we can always revisit it if we find we need more options later.

@hmpf hmpf self-requested a review November 10, 2023 10:04
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.

Looks fine to me but maybe @johannaengland will spot something. Though, the real challenge will be the actual implementation!

@lunkwill42 lunkwill42 merged commit 4d9de66 into Uninett:master Nov 13, 2023
13 checks passed
@lunkwill42 lunkwill42 deleted the feature/snmpv3-management-profile branch November 13, 2023 10:26
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.

Implement an SNMPv3 management profile type
3 participants