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.

Sign up for GitHub

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

Jump to bottom

Support more snmpv3 authentication protocols #8850

Merged
merged 2 commits into from
Feb 24, 2021
Merged

Support more snmpv3 authentication protocols #8850

merged 2 commits into from
Feb 24, 2021

Conversation

Hipska
Copy link
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
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
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
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
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
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

@reimda reimda reimda approved these changes

@srebhan srebhan srebhan approved these changes

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

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