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

Avoid exception on adding ip device with wrong ip/hostname #2713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johannaengland
Copy link
Contributor

Closes #2696.

Doesn't really show a nice error message, only catches the exception.
If I should show a clearer error message I would need some pointers on how to do that, since I don't see a way to send the error message up

Copy link

sonarcloud bot commented Nov 3, 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

github-actions bot commented Nov 3, 2023

Test results

     12 files       12 suites   12m 45s ⏱️
3 221 tests 3 221 ✔️ 0 💤 0
9 138 runs  9 138 ✔️ 0 💤 0

Results for commit 17ece24.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

I think this solves the problem slightly at the wrong level. This will also conflict heavily with the ongoing changes in #2711 (which has shown me that the connectivity tests on the backend are named and structured in stupid ways).

Anyway, I have a working JS suggestion for checking the backend response and distinguishing between internal server errors and bad requests - and I think a request to check the connectivity of an invalid IP address should return a 400 Bad Request.

However, the functionality currently supports both IP addresses and host names as input. If you enter a hostname that resolves to multiple IP addresses in the netbox form, the JS UI will force you to select one of the IP addresses before allowing a connectivity test. If it only resolves to a single address, the hostname is used as input, and that hostname is fed directly to the underlying SNMP library, resulting in a sort of nondescript exception being raised.

A more proper solution should probably figure out the actual IP address at the Django view level, and return a 400 Bad Request with a JSON error payload if the IP addr is invalid. Actual unhandled errors from the SNMP layer should still cause a 500 internal error, but the front end should treat 500 and 400 differently.

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 55.19%. Comparing base (ccb83d1) to head (17ece24).
Report is 553 commits behind head on master.

Files Patch % Lines
python/nav/Snmp/pynetsnmp.py 50.00% 2 Missing ⚠️
python/nav/web/seeddb/page/netbox/edit.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2713      +/-   ##
==========================================
- Coverage   55.19%   55.19%   -0.01%     
==========================================
  Files         561      561              
  Lines       40917    40920       +3     
==========================================
+ Hits        22584    22585       +1     
- Misses      18333    18335       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Improperly caught exception/error handling when checking device connectivity from SeedDB
2 participants