feat: Add SNMP Monitor #4717

mattv8 · 2024-04-27T01:18:36Z

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Description (Updated 06 June 2024 to reflect new scope)

This PR introduces a new SNMP monitor type and extends the functionality of JSON query monitoring with additional comparison operators. The following changes have been made:

Uptime Kuma Core

Broke out the JSON query evaluation logic into a new utility function for better reusability and clarity, called evaluateJsonQuery(data: any, jsonPath: string, jsonPathOperator: string, expectedValue: any), which returns { status: boolean, response: any } where status is the UP/DOWN evaluation and response is the result of the JSON query evaluation.

New SNMP Monitor Type:

Added new fields for SNMP configuration, including community string, OID (Object Identifier), and SNMP version.
Updated the database schema to include SNMP-related columns.
Utilizes new evaluateJsonQuery() to determine monitor status based on SNMP values and configured conditions.
Utilizes net-snmp library for SNMP queries.

JSON Query Monitoring:

Utilizes new evaluateJsonQuery() to determine monitor status based on JSON query evaluation and configured conditions.
Extended the UI to allow users to select the desired comparison operator (==, !=, <, <=, >, >=).
Introduced a new property in the monitoring configuration to hold the comparison operator.
Ensured backward compatibility by defaulting to == if the operator is not specified in existing configurations.

Resolves #1675
Resolves #4715
Resolves #4614
Resolves #4617

Type of change

Please delete any options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).

net-snmp over snmp-native is: -more robust -more popular -better documented -supports v3

Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.

@CommanderStorm

Thanks for pointing it out @CommanderStorm!

DB must allow nulls otherwise this will break other monitors.

knex requires down function

Updates appropriate values async when editing the SNMP monitor

mattv8 · 2024-04-30T21:51:04Z

@CommanderStorm I'm ready for review. It is not passing ES Lint, although when I run npm run lint:prod --fix there are some changes to server/model/monitor.js that are unrelated to this PR. I'm unsure as how to proceed on the linting side of things.

Co-authored-by: Frank Elsinga <[email protected]>

https://github.com/louislam/uptime-kuma/pull/4717/files/b4bd003626986be1faab7142710f9443273d89fc#r1585590243

Co-Authored-By: Frank Elsinga <[email protected]>

chakflying · 2024-05-24T11:33:53Z

The idea behind my suggestion of using json-query for value comparisons, is that it can provide the maximum freedom for users to process and transform the input data before comparison.
If all we are using the library for is running a few preset queries, we are not providing that freedom to the user. Then we may as well have not used the library.
However, I also see that there is benefit in being able to set a custom condition/comparison operator other than ==, since it can simplify the expression in complicated cases. (Basically the idea in feat: extend monitor JSON data evaluation operators #4617)

My ideal implementation would be like this:

We reuse existing database columns json_path and expected_value, instead of creating new one snmpControlValue
We name the new column for the comparison operator json_path_operator instead of the current snmp_condition (as it was in feat: extend monitor JSON data evaluation operators #4617)
Optionally, we can default json_path to $.value for people who do not require any transformation.
When the monitor runs its check,
- we obtain the value from SNMP as it's currently done: const value = varbinds[0].value;
- we evaluate this value according to user's inputted json_path:
```
   let expression = jsonata(this.jsonPath);
   let result = await expression.evaluate(value);
```
- We compare result with expected_value using the user's chosen operator (==, >=, etc)
- This comparison produces a true/false result which will determine the monitor status

This implementation would be maximally compatible with the existing json-query monitor, and have the benefit that the user has much more freedom, e.g. can quickly check if the SNMP value is odd by doing $.value % 2 != 0 (I think).

I really appreciate the work that you have put into this feature, it looks great already. I don't want this to be like "great, but here's more work for you". It's difficult to reach an ideal solution when we all have different ideas of how it would look like. I'm also not in a good position to comment since I don't have an SNMP device to test. Feel free to discuss if needed.

chakflying · 2024-05-24T11:40:40Z

BTW, if we are reusing radiusPassword for the storing the community string, then I guess a new database column for that is also not needed anymore?

mattv8 · 2024-05-28T00:47:29Z

Hey @chakflying I appreciate your thoughts on all this! Currently on mobile and on vacation, so forgive this for being brief. I'll jump back into this in a week or so when I'm back in town and solid internet.

Yes, what you're describing I feel is absolutely workable. I think we need to walk the line between absolute freedom of an open text input for json-queries and the simplicity of the comparison drop down. Let me work through this and see what I can come up with!

-Reuse expected_value for snmpControlValue -Create jsonPathOperator for snmpCondition

Utilizes the JSON Query library to handle comparison logic.

mattv8 · 2024-06-05T22:13:43Z

I realize I've now pushed a lot of change all at once and @CommanderStorm might not like it (understandably 😉), but I've retooled the SNMP monitor to use json-query based logic, and I've also created a new utility function that includes conditionals, and re-worked the HTTP(s) - JSON Query monitor, effectively implementing the desired outcome of #4617. Lemme know what you two think once you've had the chance to take a look. I've tested SNMP with it, I'll create a server with a JSON response and test that as well.

@qu4cks4lb3r I'd be curious to see if this meets your expectations for the conditionals in the json-query monitor, and perhaps you could help test.

@chakflying

-This is less abstract -Maximum compatibility with @chakflying's existing json-query monitor code.

@chakflying

-Maximum compatibility with @chakflying's existing json-query monitor code.

@chakflying

There are a lot of changes here: -Fixed a lot of issues encountered during my testing -JSON path is evaluated BEFORE making comparisons (this was the true intended behavior by @chakflying) -Variable name changes (cosmetic) -Added != operator -Changed jsonQueryDescription (again)

mattv8 · 2024-06-07T00:53:47Z

Had some more time today to dive into this and more thoroughly test my feat against an HTTP(s) Json response. I made a few more commits (with lots of changes) to address the issues I encountered. I'm going to let the code settle for a bit now and hopefully folks will get a chance to review.

Admittedly I went a little overboard and there's a bit of scope change now from the original SNMP monitor. I hope this is acceptable. I'll remain open-minded and if we want to dial it back, let me know. If we need to break this into two PR's; one addressing #4617 and one adding the SNMP monitor I'm fine with that. As it stands, this PR now combines the two feats.

Cleanup of some things I missed yesterday...

No longer used.

CommanderStorm

Looks good to me

@chakflying would like a review from your side as well ^^

src/pages/EditMonitor.vue

CommanderStorm · 2024-06-12T18:02:18Z

src/pages/EditMonitor.vue

- <label for="hostname" class="form-label">{{ $t("Hostname") }}</label>
+ <!-- TCP Port / Ping / DNS / Steam / MQTT / Radius / Tailscale Ping / SNMP only -->
+ <div v-if="monitor.type === 'port' || monitor.type === 'ping' || monitor.type === 'dns' || monitor.type === 'steam' || monitor.type === 'gamedig' || monitor.type === 'mqtt' || monitor.type === 'radius' || monitor.type === 'tailscale-ping' || monitor.type === 'snmp'" class="my-3">
+ <label for="hostname" class="form-label">{{ $t("Hostname or IP Address") }}</label>


nit: I don't think this translation does make sense for every monitor here.
F.ex. we currently don't support IP-adresses for the dns monitor.

Likely does not matter in the real world.

Probably best for me to put it back to simply "Hostname" to limit number of potentially disruptive frontend changes from this PR. My intention was to avoid potential confusion from people not realizing it is also an "IP address" field. Very rarely would one use a hostname for SNMP (IMHO).

server/model/monitor.js

server/monitor-types/snmp.js

CommanderStorm · 2024-06-12T18:34:07Z

src/util.ts

+ response // The response from the server or result from initial json-query evaluation
+ };
+ } catch (err: any) {
+ throw new Error(`Error evaluating JSON query: ${err.message}. Response from server was: ${response}`);


response in this context may quite large.
If we just print it to the console, this could DOS the server => please at most print a truncated version.

Alternatively, just returning the error is likely fine too.

I noticed the potential for this issue in my own testing and wrote truncation code that was a bit clunky so I didn't commit it yet. Let me adjust and see what I can come up with.

CommanderStorm · 2024-06-12T18:46:26Z

src/lang/en.json

@@ -588,7 +588,7 @@
 "notificationDescription": "Notifications must be assigned to a monitor to function.",
 "keywordDescription": "Search keyword in plain HTML or JSON response. The search is case-sensitive.",
 "invertKeywordDescription": "Look for the keyword to be absent rather than present.",
- "jsonQueryDescription": "Do a json Query against the response and check for expected value (Return value will get converted into string for comparison). Check out {0} for the documentation about the query language. A playground can be found {1}.",
+ "jsonQueryDescription": "Parse and extract specific data from the server's JSON response using JSON query or use \"$\" for the raw response, if not expecting JSON. The result is then compared to the expected value, as strings. See {0} for documentation and use {1} to experiment with queries.",


We might want to remove the other translations.
This is a significantly different (read: better) message and needs to be re-translated anyway.

Thanks, I'll remove the other translations for this PR, and commit.

Co-authored-by: Frank Elsinga <[email protected]>

SNMP Initial Commits

d92003e

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).

mattv8 mentioned this pull request Apr 27, 2024

SNMP Value Retrieval #1675

Open

1 task

mattv8 changed the title ~~feat: Add SNMP Monitor~~ feat: Add SNMP Monitor (WIP) Apr 27, 2024

mattv8 added 3 commits April 29, 2024 13:58

Use net-snmp instead of snmp-native

a3cdd69

net-snmp over snmp-native is: -more robust -more popular -better documented -supports v3

Updated a comment

ff5890a

Further SNMP monitor development

4a882be

Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.

This comment has been minimized.

Sign in to view

Wrong variable used

99dc4cf

Thanks for pointing it out @CommanderStorm!

This comment was marked as resolved.

Sign in to view

mattv8 added 6 commits April 30, 2024 15:10

Update db migration: allow nulls

138075a

DB must allow nulls otherwise this will break other monitors.

Update db migration: down function

9c8024c

knex requires down function

Update bean model backend

9d28fcf

Updates appropriate values async when editing the SNMP monitor

Frontend input validation

4593afb

Minor frontend styling

9848ce4

Finalized SNMP monitor

704ffd3

mattv8 changed the title ~~feat: Add SNMP Monitor (WIP)~~ feat: Add SNMP Monitor Apr 30, 2024

mattv8 marked this pull request as ready for review April 30, 2024 21:46

Merge branch 'master' into snmp-monitor

b4bd003

This comment was marked as resolved.

Sign in to view

mattv8 and others added 6 commits April 30, 2024 17:54

Apply suggestions from code review

ba47aca

Co-authored-by: Frank Elsinga <[email protected]>

ES Lint Compliant

7459654

Corrected down function

e944492

https://github.com/louislam/uptime-kuma/pull/4717/files/b4bd003626986be1faab7142710f9443273d89fc#r1585590243

ES Lint Compliant

97a9094

Remove supurfluous log.debug

9ba0f68

Co-Authored-By: Frank Elsinga <[email protected]>

Delete .EditMonitor.vue.swp

ba84f01

Co-Authored-By: Frank Elsinga <[email protected]>

github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 23, 2024

This comment was marked as outdated.

Sign in to view

mattv8 added 4 commits June 5, 2024 15:37

Variable changes

7eee5db

-Reuse expected_value for snmpControlValue -Create jsonPathOperator for snmpCondition

Refactor line for conciseness

b2d76bc

Fix: a typo

2d2c186

Blend json-query and snmp monitors

efb1642

Utilizes the JSON Query library to handle comparison logic.

mattv8 added 4 commits June 6, 2024 10:04

Better type handling

36dc94b

Query json directly rather than with $.value

10d3188

-This is less abstract -Maximum compatibility with @chakflying's existing json-query monitor code.

Also return result of the evaluation

eaa935c

-Maximum compatibility with @chakflying's existing json-query monitor code.

mattv8 added 4 commits June 7, 2024 16:35

Error handling robustness

23f844d

Cleanup of some things I missed yesterday...

Fix: Cast to string then eval

8235291

Helpful error when query returns object or array

e2e8109

Update 2024-04-26-0000-snmp-monitor.js

43bd09b

mattv8 requested a review from CommanderStorm June 12, 2024 17:27

mattv8 and others added 2 commits June 12, 2024 11:46

Removed "Control Value"

69c22ed

No longer used.

Merge branch 'master' into snmp-monitor

5dc4bb6

github-actions bot removed the needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jun 12, 2024

CommanderStorm approved these changes Jun 12, 2024

View reviewed changes

Apply suggestions from code review

b5a73e5

Co-authored-by: Frank Elsinga <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SNMP Monitor #4717

feat: Add SNMP Monitor #4717

mattv8 commented Apr 27, 2024 •

edited

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattv8 commented Apr 30, 2024 •

edited

This comment was marked as resolved.

chakflying commented May 24, 2024

chakflying commented May 24, 2024

mattv8 commented May 28, 2024 •

edited

This comment was marked as outdated.

mattv8 commented Jun 5, 2024 •

edited

mattv8 commented Jun 7, 2024 •

edited

CommanderStorm left a comment

CommanderStorm Jun 12, 2024

mattv8 Jun 12, 2024 •

edited

CommanderStorm Jun 12, 2024

mattv8 Jun 12, 2024 •

edited

CommanderStorm Jun 12, 2024

mattv8 Jun 12, 2024 •

edited

feat: Add SNMP Monitor #4717

Are you sure you want to change the base?

feat: Add SNMP Monitor #4717

Conversation

mattv8 commented Apr 27, 2024 • edited

Description (Updated 06 June 2024 to reflect new scope)

Type of change

Checklist

Screenshots

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattv8 commented Apr 30, 2024 • edited

This comment was marked as resolved.

chakflying commented May 24, 2024

chakflying commented May 24, 2024

mattv8 commented May 28, 2024 • edited

This comment was marked as outdated.

mattv8 commented Jun 5, 2024 • edited

mattv8 commented Jun 7, 2024 • edited

CommanderStorm left a comment

Choose a reason for hiding this comment

CommanderStorm Jun 12, 2024

Choose a reason for hiding this comment

mattv8 Jun 12, 2024 • edited

Choose a reason for hiding this comment

CommanderStorm Jun 12, 2024

Choose a reason for hiding this comment

mattv8 Jun 12, 2024 • edited

Choose a reason for hiding this comment

CommanderStorm Jun 12, 2024

Choose a reason for hiding this comment

mattv8 Jun 12, 2024 • edited

Choose a reason for hiding this comment

mattv8 commented Apr 27, 2024 •

edited

mattv8 commented Apr 30, 2024 •

edited

mattv8 commented May 28, 2024 •

edited

mattv8 commented Jun 5, 2024 •

edited

mattv8 commented Jun 7, 2024 •

edited

mattv8 Jun 12, 2024 •

edited

mattv8 Jun 12, 2024 •

edited

mattv8 Jun 12, 2024 •

edited