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

feat: Add SNMP Monitor #4717

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

feat: Add SNMP Monitor #4717

wants to merge 48 commits into from

Conversation

mattv8
Copy link

@mattv8 mattv8 commented Apr 27, 2024

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

This PR introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol). The following changes have been made:

  • Added new fields for SNMP configuration, including community string, OID (Object Identifier), SNMP version, control value, and condition.
  • Updates the database schema to include SNMP-related columns.
  • Implemented SNMP check logic to determine device status based on SNMP values and configured conditions.
  • Utilizes net-snmp.

Addresses #1675

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

image

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).
@mattv8 mattv8 mentioned this pull request Apr 27, 2024
1 task
@mattv8 mattv8 changed the title feat: Add SNMP Monitor feat: Add SNMP Monitor (WIP) Apr 27, 2024
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.
@mattv8

This comment has been minimized.

@CommanderStorm

This comment has been minimized.

Thanks for pointing it out @CommanderStorm!
@mattv8

This comment was marked as resolved.

@CommanderStorm

This comment was marked as resolved.

@mattv8

This comment was marked as resolved.

@CommanderStorm

This comment was marked as resolved.

DB must allow nulls otherwise this will break other monitors.
knex requires down function
Updates appropriate values async when editing the SNMP monitor
@mattv8 mattv8 changed the title feat: Add SNMP Monitor (WIP) feat: Add SNMP Monitor Apr 30, 2024
@mattv8 mattv8 marked this pull request as ready for review April 30, 2024 21:46
@mattv8
Copy link
Author

mattv8 commented Apr 30, 2024

@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.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Please have a look at https://github.com/louislam/uptime-kuma/pull/4717/files or https://github.com/louislam/uptime-kuma/actions/runs/8902151438/job/24447507847?pr=4717 and fix the linting mistakes.
I don't know why your local environment is producing different linting results.

I am certain that a review at this time will miss too many things and will thus require another review cycle. Nevertheless, I have attached a few comments.

src/pages/.EditMonitor.vue.swp Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
db/knex_migrations/2024-04-26-0000-snmp-monitor.js Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
Comment on lines 61 to 90
switch (monitor.snmpCondition) {
case '>':
heartbeat.status = numericValue > monitor.snmpControlValue ? UP : DOWN;
break;
case '>=':
heartbeat.status = numericValue >= monitor.snmpControlValue ? UP : DOWN;
break;
case '<':
heartbeat.status = numericValue < monitor.snmpControlValue ? UP : DOWN;
break;
case '<=':
heartbeat.status = numericValue <= monitor.snmpControlValue ? UP : DOWN;
break;
case '==':
if (!isNaN(value) && !isNaN(monitor.snmpControlValue)) {
// Both values are numeric, parse them as numbers
heartbeat.status = parseFloat(value) === parseFloat(monitor.snmpControlValue) ? UP : DOWN;
} else {
// At least one of the values is not numeric, compare them as strings
heartbeat.status = value.toString() === monitor.snmpControlValue.toString() ? UP : DOWN;
}
break;
case 'contains':
heartbeat.status = stringValue.includes(monitor.snmpControlValue) ? UP : DOWN;
break;
default:
heartbeat.status = DOWN;
heartbeat.msg = `Invalid condition: ${monitor.snmpCondition}`;
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

About this part @chakflying has commented in #1675 (comment)

Sounds pretty good, but just in case you didn't know, you should take a look at how the json-query monitor works.

Ideally in the long term, we would want all value comparisons to work with the jsonata syntax, and reuse the database fields for better compatibility (see #3919). I don't think it's worth implementing custom value comparison functionality just for this monitor.

=> This needs to be compatible with #4617 and #3919

@chakflying what do you think is the best course forward?

Copy link
Author

@mattv8 mattv8 May 2, 2024

Choose a reason for hiding this comment

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

@chakflying & @CommanderStorm I propose waiting until after #4617 is committed and I will re-factor & maintain the SNMP monitor after the fact.

server/monitor-types/snmp.js Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
@mattv8
Copy link
Author

mattv8 commented May 6, 2024

I would feel more comfortable with and think it might be better if we restrict this to just being an equality check for the moment and make this PR not depend on other PRs. This being json-query based would also be fine by me.

I strongly argue SNMP is its own protocol; therefore, it should have its own monitor type. It would confuse users to mix this into a JSON Query monitor. Also, what if in the future we want to add an OID discovery tool as discussed here #1675 (comment)? We would need room in the UI for this.

Furthermore, restricting this to an equality check would defeat the purpose entirely since users in #1675 are asking for a way to statefully determine whether something is up or down based on discrete SNMP responses (i.e. CPU usage is >70:, status should be DOWN and sending an alert).

@CommanderStorm
Copy link
Collaborator

Re: #4717 (comment)

There has been another miscommunication.
I did not ask to extend the json-query monitor. I asked it to be json-query based. (I did NOT ask for is to remove the monitoring type smnp)

What I mean is that the way the comparisons being made should be the same as either the keyword or the json-query monitor

=> This PR should not add another thing we need to migrate in #3919

Currently, you have addeed a third way of comparing them which is somewhere in between them
=> if you want to keep functionality, I would suggest using the same logic as json-query. This being strict equality and extending it in the future is fine by me too.

The logic is here

let data = res.data;
// convert data to object
if (typeof data === "string" && res.headers["content-type"] !== "application/json") {
try {
data = JSON.parse(data);
} catch (_) {
// Failed to parse as JSON, just process it as a string
}
}
let expression = jsonata(this.jsonPath);
let result = await expression.evaluate(data);
if (result.toString() === this.expectedValue) {
bean.msg += ", expected value is found";
bean.status = UP;
} else {
throw new Error(bean.msg + ", but value is not equal to expected value, value was: [" + result + "]");
}

@mattv8
Copy link
Author

mattv8 commented May 7, 2024

I apologize for misunderstanding. :) Thank you for explaining what you mean by using the json-query logic.

if you want to keep functionality, I would suggest using the same logic as json-query. This being strict equality and extending it in the future is fine by me too.

=> May I ask simply: why? What are we achieving right now by limiting to only strict equality?? Are we aiming to enhance security, streamline code consistency, or address other technical considerations?

I'm pushing back because the SNMP monitor is ready to ship as it stands with its own organized, robust switch case logic and a user-friendly UI that will not interfere with any other monitor functionality (except for perhaps adding more DB keys). And as I stated in #4717 (comment) limiting to strict equality would not achieve what users are requesting in #1675.

Reading back, this sounds argumentative-- don't take it that way. I'm merely trying to determine the logic behind holding off on a merge. Also, I'm an engineer. ;)

@CommanderStorm
Copy link
Collaborator

why?

Having another custom way of doing comparisons is simply not maintainable.
As stated above, this needs to be compatible with the other changes.
Yes, "Is how this works something we want to maintain forever" is quite subjective, but I am drawing the line here.

=> reusing the json-query/keyword comparison logic is maintainable, implementing another way of comparing is not.

CommanderStorm

This comment was marked as resolved.

Co-authored-by: Frank Elsinga <frank@elsinga.de>
@mattv8
Copy link
Author

mattv8 commented May 8, 2024

Having another custom way of doing comparisons is simply not maintainable ...

Understandable. Let me play around with json-query and see if I can come up with something that functions equivalently to what I've constructed already for this SNMP monitor.

=>Based on #4717 (comment), would it be acceptable to maintain the condition/control value UI as it stands (as in my screenshot) and refactor my switch case logic in snmp.js to use JSON query expression(s)? Basically, we're constructing the json-query expressions for the user.

Something very loosely like this in server/monitor-types/snmp.js:

let jsonQueryExpression;
switch (monitor.snmpCondition) {
    case ">":
    case ">=":
    case "<":
    case "<=":
        jsonQueryExpression = `$.value ${monitor.snmpCondition} $.control`;
        break;
    case "==":
        jsonQueryExpression = "$string($.value) = $string($.control)";
        break;
    case "contains":
        jsonQueryExpression = "$contains($string($.value), $string($.control))";
        break;
    default:
        throw new Error(`Invalid condition ${monitor.snmpCondition}`);
}

const expression = jsonata(jsonQueryExpression);
const result = await expression.evaluate({
    value: snmpValue,
    control: monitor.snmpControlValue
});
heartbeat.status = result ? UP : DOWN;

image

@@ -265,18 +265,18 @@
<!-- SNMP Monitor Type -->
<div v-if="monitor.type === 'snmp'" class="my-3">
<label for="snmp_community_string" class="form-label">{{ $t("Community String") }}</label>
<input id="snmp_community_string" v-model="monitor.snmpCommunityString" type="text" class="form-control" required placeholder="public">
<!-- TODO: Rename monitor.radiusPassword to monitor.password for general use -->
Copy link
Author

@mattv8 mattv8 May 8, 2024

Choose a reason for hiding this comment

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

Do you want me to address this TO DO in this PR?

Equivalent functionality as before, but we're now building json-query expressions for the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants