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

Support SNMPv3 and asyncio in snmp sensor #14753

Merged
merged 4 commits into from
Sep 7, 2018
Merged

Support SNMPv3 and asyncio in snmp sensor #14753

merged 4 commits into from
Sep 7, 2018

Conversation

mtdcr
Copy link
Contributor

@mtdcr mtdcr commented Jun 2, 2018

Instantiate pysnmp objects once during setup and reuse them. Requires asyncio, because SnmpEngine objects can't be shared between threads. This speeds up repeated queries significantly and simplifies support for different authentication types.

Related to #6973 and #11370.

For SNMPv3, the following new options were modified or added:

  • version
    Changed to accept version '3'.
  • username
    <string>
  • auth_key
    <string>
  • auth_protocol
    <none|hmac-md5|hmac-sha|hmac128-sha224|hmac192-sha256|hmac256-sha384|hmac384-sha512>
  • priv_key
    <string>
  • priv_protocol
    <none|des|3des-ede|aes-cfb-128|aes-cfb-192|aes-cfb-256>

Note:

  • auth_protocol defaults to none.
  • priv_protocol defaults to none.
  • Setting no auth_key forces auth_protocol to none.
  • Setting no priv_key forces priv_protocol to none.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5484

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.

If user exposed functionality or configuration variables are added/changed:

No new files or dependencies were introduced.

@homeassistant
Copy link
Contributor

Hi @mtdcr,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

authProtocol=auth_protocols[authproto] if authkey
else usmNoAuthProtocol,
provProtocol=priv_protocols[privproto] if privkey
else usmNoPrivProtocol,

Choose a reason for hiding this comment

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

continuation line unaligned for hanging indent

authKey=authkey or None,
privKey=privkey or None,
authProtocol=auth_protocols[authproto] if authkey
else usmNoAuthProtocol,

Choose a reason for hiding this comment

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

continuation line unaligned for hanging indent

@mtdcr
Copy link
Contributor Author

mtdcr commented Jun 2, 2018

TOXENV=pylint fails on Travis CI:

E:161,40: No value for argument 'transportTarget' in function call (no-value-for-parameter)
E:161,40: No value for argument 'contextData' in function call (no-value-for-parameter)

I'd say that's not true. And it doesn't occur when I run the tests locally.

@mtdcr
Copy link
Contributor Author

mtdcr commented Jun 2, 2018

I managed to reproduce the warning locally and successively replaced the code in question with functionally equivalent code accepted by pylint.

@dgomes
Copy link
Contributor

dgomes commented Jun 2, 2018

We have adopted PEP 492, please update accordingly, eg:

@asyncio.coroutine
def function():`

to

async def function

yield from becomes await

vol.Optional(CONF_AUTH_KEY): cv.string,
vol.Optional(CONF_AUTH_PROTOCOL, default=DEFAULT_AUTH_PROTOCOL):
vol.In(AUTH_PROTOCOLS),
vol.Optional(CONF_PRIV_KEY): cv.string,
Copy link
Member

Choose a reason for hiding this comment

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

is that a file, use cv.isfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's a string.

@mtdcr
Copy link
Contributor Author

mtdcr commented Jun 6, 2018

@dgomes: I've updated my branch.

UsmUserData(
username,
authKey=authkey or None,
privKey=privkey or None,
Copy link
Member

Choose a reason for hiding this comment

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

Need not set to None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces en empty string with None, which makes a difference in UsmUserData. Are you implying that authkey=="" is guaranteed not to appear in this codepath?

@pvizeli
Copy link
Member

pvizeli commented Jun 15, 2018

Use on top:

MAP_XY = {
   'hmac-md5': 'usmHMACMD5AuthProtocol',
}
...
vol.In(MAP_XY.keys())
...

import pysnmp.hlapi.asyncio as snmp
...
dynval = getattr(snmp, MAP_XY[bla])

@mtdcr
Copy link
Contributor Author

mtdcr commented Jun 25, 2018

@pvizeli: Thanks for your suggestions! I've updated my branch.

vol.Optional(CONF_USERNAME): cv.string,
vol.Optional(CONF_AUTH_KEY): cv.string,
vol.Optional(CONF_AUTH_PROTOCOL, default=DEFAULT_AUTH_PROTOCOL):
vol.In(MAP_AUTH_PROTOCOLS.keys()),
Copy link
Member

Choose a reason for hiding this comment

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

.keys() is not needed, it's the default.

vol.In(MAP_AUTH_PROTOCOLS.keys()),
vol.Optional(CONF_PRIV_KEY): cv.string,
vol.Optional(CONF_PRIV_PROTOCOL, default=DEFAULT_PRIV_PROTOCOL):
vol.In(MAP_PRIV_PROTOCOLS.keys()),
Copy link
Member

Choose a reason for hiding this comment

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

Dito

@thrawnarn
Copy link
Contributor

Any progress on this PR?

@mtdcr
Copy link
Contributor Author

mtdcr commented Jul 31, 2018

I rebased this branch on dev again and removed keys(), even though using keys() was suggested to use before. Contributing to this project is fun, really.

Copy link
Member

@MartinHjelmare MartinHjelmare 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.
@pvizeli @fabaff ?

@ghost ghost assigned fabaff Sep 7, 2018
fabaff pushed a commit to home-assistant/home-assistant.io that referenced this pull request Sep 7, 2018
* Update sensor.snmp.markdown

Depends on home-assistant/core#14753

* ✏️ Minor tweaks

* Minor changes
@fabaff fabaff merged commit 50266e9 into home-assistant:dev Sep 7, 2018
@ghost ghost removed the in progress label Sep 7, 2018
@balloob balloob mentioned this pull request Sep 17, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
@mtdcr mtdcr deleted the snmpv3-asyncio-sensor branch April 10, 2021 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants