Skip to content

Add SNMPv3 to node-red-node-snmp#904

Merged
Steve-Mcl merged 4 commits intonode-red:masterfrom
Steve-Mcl:snmpv3_Add_smc
Apr 28, 2022
Merged

Add SNMPv3 to node-red-node-snmp#904
Steve-Mcl merged 4 commits intonode-red:masterfrom
Steve-Mcl:snmpv3_Add_smc

Conversation

@Steve-Mcl
Copy link
Copy Markdown
Contributor

Closes #705

This PR is based on #705 & addresses the concerns raised on the PR Review.
#705 is an abandoned PR for adding v3 support.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

  • Update to latest SNMP lib
  • Add support for SNMPv3
    • v3 username, authkey and privkey are moved to credentials
  • Improve DRY by encapsulating repeated code in helper function
  • Code tidy
    • replacing all vars to const+let
    • auto formatted for consistent spacing

Notes

  • Since a varbind error in SNMP.get node still sends a message, I added a property _error to each result that fails.
    Ideally, the get node would NOT output a msg if a varbind has an error but this would change the behaviour.
  • A future version of this node should move the majority of the repeated HTML to a config node. This would also fix the frustration of having to fill out the credentials every time.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

ASH3xC0d3 and others added 2 commits October 24, 2020 11:07
Added support for version 3.
All the existing issues with v1 and v2c are also with v3 e.g.
node-red#679
snmp walker will not return more than 1000 objects, all following objects cannot be accessed
Incorrect encryption (privacy) passphrase gives a timeout and not an error (protocol limitation?)
Incorrect community string gives a timeout and not an error (protocol limitation?)
Changes to the node are made with following details:
Node-red v1.2.2
Node.js v12.6.0
Windows_NT 10.0.19841 x64 LE
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Apr 21, 2022

CLA Not Signed

@Steve-Mcl Steve-Mcl requested a review from dceejay April 21, 2022 16:03
@Steve-Mcl
Copy link
Copy Markdown
Contributor Author

@knolleary @dceejay

I have signed the CLA - not sure why this is not picking up?

@hardillb
Copy link
Copy Markdown
Member

@Steve-Mcl It's not complain about you, it complaining about an update from @ASH3xC0d3

@Steve-Mcl
Copy link
Copy Markdown
Contributor Author

@dceejay Dave, are you OK to review this or should I request someone else? or are you happy for me to merge?
Thanks.

@dceejay
Copy link
Copy Markdown
Member

dceejay commented Apr 26, 2022

Hi @Steve-Mcl
yes - I don't have anything (that I know of) to test v3 against... but the menus / options seem fine to me.

@Steve-Mcl
Copy link
Copy Markdown
Contributor Author

@dceejay I have tested against a physical device and snmpd on Ubuntu.

Had to do a fair bit debugging to get past this issue -
markabrahams/node-net-snmp#202 (kindly fixed by the guys over at net-snmp) - so it has had a fair good looking at (from me)

Can we agree since it is a new feature and the original SNMP v1/2 are pretty much unchanged that we can merge it and see if anyone raises any issue?

@dceejay
Copy link
Copy Markdown
Member

dceejay commented Apr 27, 2022

I'm fine with that.
Also maybe version bump to 1.0.0 - so future minor changes get pulled in ok if folk specify ^.

@Steve-Mcl
Copy link
Copy Markdown
Contributor Author

Cool.
I'll bump and merge tomorrow.

Will let you know so that it can be published.

@Steve-Mcl Steve-Mcl merged commit c77f13a into node-red:master Apr 28, 2022
@Steve-Mcl Steve-Mcl mentioned this pull request Apr 28, 2022
6 tasks
@Steve-Mcl
Copy link
Copy Markdown
Contributor Author

@dceejay could you release to NPM please?

@dceejay
Copy link
Copy Markdown
Member

dceejay commented Apr 28, 2022

Done - Thanks all round

@Steve-Mcl Steve-Mcl deleted the snmpv3_Add_smc branch April 28, 2022 21:39
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.

4 participants