Skip to content

Support more snmpv3 authentication protocols#8850

Merged
reimda merged 2 commits intoinfluxdata:masterfrom
Super-Visions:snmp/sha512
Feb 24, 2021
Merged

Support more snmpv3 authentication protocols#8850
reimda merged 2 commits intoinfluxdata:masterfrom
Super-Visions:snmp/sha512

Conversation

@Hipska
Copy link
Copy Markdown
Contributor

@Hipska Hipska commented Feb 12, 2021 •
edited
Loading

Fixes #8747

Required for all PRs:

  • Associated README.md updated.
  • Has appropriate unit tests.

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Feb 12, 2021
telegraf-tiger[bot]
telegraf-tiger bot approved these changes Feb 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

@Hipska Hipska added area/snmp feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed fix pr to fix corresponding bug labels Feb 12, 2021
@Hipska Hipska requested a review from reimda February 12, 2021 11:45
@Hipska
Copy link
Copy Markdown
Contributor Author

Hipska commented Feb 12, 2021

@reimda would this need to have extra unit tests?

@Hipska Hipska marked this pull request as ready for review February 12, 2021 14:33
srebhan
srebhan approved these changes Feb 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@srebhan srebhan 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 to me.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 14, 2021
@srebhan srebhan self-assigned this Feb 14, 2021
@Hipska Hipska assigned Hipska and reimda and unassigned srebhan and Hipska Feb 17, 2021
reimda
reimda approved these changes Feb 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

Thanks Thomas, this looks good to me. Have you done manual integration testing with these new auth protocols? I haven't looked closely at gosnmp's v3 compatibility lately but when it added v3 support there were bugs related to protocols. In the snmp_trap plugin we added support for new protocols initially but had to disable them. (I can't remember if they were auth or security protocols)

I don't think unit tests on each protocol are absolutely necessary. It might be nice to add integration tests for them using net-snmp, but I'm fine with that being in a separate future PR.

@reimda reimda merged commit 47e12d1 into influxdata:master Feb 24, 2021
@Hipska
Copy link
Copy Markdown
Contributor Author

Hipska commented Feb 25, 2021

The tests would mainly be testing the config itself. I also tested manually with an snmpsim instance.

I did not test traps, don't know how I could do that. Speaking of traps, maybe should the README for snmp_trap plugin be updated as well?

@Hipska Hipska deleted the snmp/sha512 branch February 25, 2021 08:55
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@Hipska
Support more snmpv3 authentication protocols (influxdata#8850)
ee8613a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srebhan srebhan srebhan approved these changes

@telegraf-tiger telegraf-tiger[bot] telegraf-tiger[bot] approved these changes

+1 more reviewer

@reimda reimda reimda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@reimda reimda

Labels

area/snmp feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SNMP Plugin supports SHA-512?

3 participants

@Hipska @reimda @srebhan