From 876b22095d39cb4cfb24d48bc91135e2219cc08a Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Fri, 29 Jun 2018 15:51:54 +0200 Subject: [PATCH] Add read&write SNMP community names to `snmp` driver SNMP agents are sometimes configured to use different SNMP community names for read and write operations. With ironic `snmp` driver it is currently impossible to configure its SNMP manager to use different SNMP community names for SNMP GET and SET commands. This patch fixes that by introducing new optional node properties: `snmp_community_read` and `snmp_community_write`. Change-Id: Idb726f072f031a819b48fdd1ae66369cffb73841 Story: 1751748 Task: 10663 --- doc/source/admin/drivers/snmp.rst | 8 ++- ironic/drivers/modules/snmp.py | 54 ++++++++++++--- ironic/tests/unit/conductor/test_manager.py | 4 +- ironic/tests/unit/db/utils.py | 4 ++ .../tests/unit/drivers/modules/test_snmp.py | 68 ++++++++++++++++--- ...rite-community-names-7589a8d1899c142c.yaml | 8 +++ 6 files changed, 125 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/add-snmp-read-write-community-names-7589a8d1899c142c.yaml diff --git a/doc/source/admin/drivers/snmp.rst b/doc/source/admin/drivers/snmp.rst index 92a730f406..7208148ebc 100644 --- a/doc/source/admin/drivers/snmp.rst +++ b/doc/source/admin/drivers/snmp.rst @@ -126,8 +126,14 @@ The following property values have to be added to the node's - ``snmp_version``: (optional) SNMP protocol version (permitted values ``1``, ``2c`` or ``3``). If not specified, SNMPv1 is chosen. -- ``snmp_community``: (Required for SNMPv1/SNMPv2c) SNMP community +- ``snmp_community``: (Required for SNMPv1/SNMPv2c unless + ``snmp_community_read`` and/or ``snmp_community_write`` properties are + present in which case the latter take over) SNMP community name parameter for reads and writes to the PDU. +- ``snmp_community_read``: SNMP community name parameter for reads + to the PDU. Takes precedence over the ``snmp_community`` property. +- ``snmp_community_write``: SNMP community name parameter for writes + to the PDU. Takes precedence over the ``snmp_community`` property. - ``snmp_user``: (Required for SNMPv3) SNMPv3 User-based Security Model (USM) user name. Synonym for now obsolete ``snmp_security`` parameter. - ``snmp_auth_protocol``: SNMPv3 message authentication protocol ID. diff --git a/ironic/drivers/modules/snmp.py b/ironic/drivers/modules/snmp.py index 8708aeafce..4180286364 100644 --- a/ironic/drivers/modules/snmp.py +++ b/ironic/drivers/modules/snmp.py @@ -122,7 +122,20 @@ OPTIONAL_PROPERTIES = { 'snmp_port': _("SNMP port, default %(port)d.") % {"port": SNMP_PORT}, 'snmp_community': - _("SNMP community. Required for versions %(v1)s and %(v2c)s.") + _("SNMP community name to use for read and/or write class SNMP " + "commands unless `snmp_community_read` and/or " + "`snmp_community_write` properties are present in which case the " + "latter takes over. Applicable only to versions %(v1)s and %(v2c)s.") + % {"v1": SNMP_V1, "v2c": SNMP_V2C}, + 'snmp_community_read': + _("SNMP community name to use for read class SNMP commands. " + "Takes precedence over the `snmp_community` property. " + "Applicable only to versions %(v1)s and %(v2c)s.") + % {"v1": SNMP_V1, "v2c": SNMP_V2C}, + 'snmp_community_write': + _("SNMP community name to use for write class SNMP commands. " + "Takes precedence over the `snmp_community` property. " + "Applicable only to versions %(v1)s and %(v2c)s.") % {"v1": SNMP_V1, "v2c": SNMP_V2C}, 'snmp_user': _("SNMPv3 User-based Security Model (USM) username. " @@ -180,7 +193,8 @@ class SNMPClient(object): interaction with PySNMP to simplify dynamic importing and unit testing. """ - def __init__(self, address, port, version, community=None, + def __init__(self, address, port, version, + read_community=None, write_community=None, user=None, auth_proto=None, auth_key=None, priv_proto=None, priv_key=None, context_engine_id=None, context_name=None): @@ -202,13 +216,16 @@ class SNMPClient(object): self.context_engine_id = context_engine_id self.context_name = context_name or '' else: - self.community = community + self.read_community = read_community + self.write_community = write_community self.cmd_gen = cmdgen.CommandGenerator() - def _get_auth(self): + def _get_auth(self, write_mode=False): """Return the authorization data for an SNMP request. + :param write_mode: `True` if write class SNMP command is + executed. Default is `False`. :returns: Either :class:`pysnmp.entity.rfc3413.oneliner.cmdgen.CommunityData` or :class:`pysnmp.entity.rfc3413.oneliner.cmdgen.UsmUserData` @@ -226,7 +243,10 @@ class SNMPClient(object): ) else: mp_model = 1 if self.version == SNMP_V2C else 0 - return cmdgen.CommunityData(self.community, mpModel=mp_model) + return cmdgen.CommunityData( + self.write_community if write_mode else self.read_community, + mpModel=mp_model + ) def _get_transport(self): """Return the transport target for an SNMP request. @@ -309,7 +329,7 @@ class SNMPClient(object): :raises: SNMPFailure if an SNMP request fails. """ try: - results = self.cmd_gen.setCmd(self._get_auth(), + results = self.cmd_gen.setCmd(self._get_auth(write_mode=True), self._get_transport(), (oid, value)) except snmp_error.PySnmpError as e: @@ -337,7 +357,8 @@ def _get_client(snmp_info): return SNMPClient(snmp_info["address"], snmp_info["port"], snmp_info["version"], - snmp_info.get("community"), + snmp_info.get("read_community"), + snmp_info.get("write_community"), snmp_info.get("user"), snmp_info.get("auth_proto"), snmp_info.get("auth_key"), @@ -931,11 +952,22 @@ def _parse_driver_info(node): # Extract version-dependent required parameters if snmp_info['version'] in (SNMP_V1, SNMP_V2C): - if 'snmp_community' not in info: + read_community = info.get('snmp_community_read') + if read_community is None: + read_community = info.get('snmp_community') + + write_community = info.get('snmp_community_write') + if write_community is None: + write_community = info.get('snmp_community') + + if not read_community or not write_community: raise exception.MissingParameterValue(_( - "SNMP driver requires snmp_community to be set for version " - "%s.") % snmp_info['version']) - snmp_info['community'] = info.get('snmp_community') + "SNMP driver requires `snmp_community` or " + "`snmp_community_read`/`snmp_community_write` properties " + "to be set for version %s.") % snmp_info['version']) + snmp_info['read_community'] = read_community + snmp_info['write_community'] = write_community + elif snmp_info['version'] == SNMP_V3: snmp_info.update(_parse_driver_info_snmpv3_user(node, info)) snmp_info.update(_parse_driver_info_snmpv3_crypto(node, info)) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 2e58968438..8769980813 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -5759,7 +5759,9 @@ class ManagerTestProperties(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): expected = ['deploy_kernel', 'deploy_ramdisk', 'force_persistent_boot_device', 'snmp_driver', 'snmp_address', 'snmp_port', 'snmp_version', - 'snmp_community', 'snmp_security', 'snmp_outlet', + 'snmp_community', + 'snmp_community_read', 'snmp_community_write', + 'snmp_security', 'snmp_outlet', 'snmp_user', 'snmp_context_engine_id', 'snmp_context_name', 'snmp_auth_key', 'snmp_auth_protocol', diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 6363ab2bfd..b9da90741d 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -137,6 +137,10 @@ def get_test_snmp_info(**kw): } if result["snmp_version"] in ("1", "2c"): result["snmp_community"] = kw.get("snmp_community", "public") + if "snmp_community_read" in kw: + result["snmp_community_read"] = kw["snmp_community_read"] + if "snmp_community_write" in kw: + result["snmp_community_write"] = kw["snmp_community_write"] elif result["snmp_version"] == "3": result["snmp_user"] = kw.get( "snmp_user", kw.get("snmp_security", "snmpuser") diff --git a/ironic/tests/unit/drivers/modules/test_snmp.py b/ironic/tests/unit/drivers/modules/test_snmp.py index e31f2e15ee..183566760b 100644 --- a/ironic/tests/unit/drivers/modules/test_snmp.py +++ b/ironic/tests/unit/drivers/modules/test_snmp.py @@ -51,16 +51,30 @@ class SNMPClientTestCase(base.TestCase): self.assertEqual(self.address, client.address) self.assertEqual(self.port, client.port) self.assertEqual(snmp.SNMP_V1, client.version) - self.assertIsNone(client.community) + self.assertIsNone(client.read_community) + self.assertIsNone(client.write_community) self.assertNotIn('user', client.__dict__) self.assertEqual(mock_cmdgen.return_value, client.cmd_gen) @mock.patch.object(cmdgen, 'CommunityData', autospec=True) - def test__get_auth_v1(self, mock_community, mock_cmdgen): - client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V1) + def test__get_auth_v1_read(self, mock_community, mock_cmdgen): + client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V1, + read_community='public', + write_community='private') client._get_auth() mock_cmdgen.assert_called_once_with() - mock_community.assert_called_once_with(client.community, mpModel=0) + mock_community.assert_called_once_with(client.read_community, + mpModel=0) + + @mock.patch.object(cmdgen, 'CommunityData', autospec=True) + def test__get_auth_v1_write(self, mock_community, mock_cmdgen): + client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V1, + read_community='public', + write_community='private') + client._get_auth(write_mode=True) + mock_cmdgen.assert_called_once_with() + mock_community.assert_called_once_with(client.write_community, + mpModel=0) @mock.patch.object(cmdgen, 'UsmUserData', autospec=True) def test__get_auth_v3(self, mock_user, mock_cmdgen): @@ -244,7 +258,8 @@ class SNMPValidateParametersTestCase(db_base.DbTestCase): self.assertEqual(INFO_DICT['snmp_port'], str(info['port'])) self.assertEqual(INFO_DICT['snmp_outlet'], str(info['outlet'])) self.assertEqual(INFO_DICT['snmp_version'], info['version']) - self.assertEqual(INFO_DICT['snmp_community'], info['community']) + self.assertEqual(INFO_DICT['snmp_community'], info['read_community']) + self.assertEqual(INFO_DICT['snmp_community'], info['write_community']) self.assertNotIn('user', info) def test__parse_driver_info_apc(self): @@ -310,7 +325,8 @@ class SNMPValidateParametersTestCase(db_base.DbTestCase): node = self._get_test_node(info) info = snmp._parse_driver_info(node) self.assertEqual('1', info['version']) - self.assertEqual('public', info['community']) + self.assertEqual('public', info['read_community']) + self.assertEqual('public', info['write_community']) def test__parse_driver_info_snmp_v2c(self): # Make sure SNMPv2c is parsed with a community string. @@ -319,7 +335,42 @@ class SNMPValidateParametersTestCase(db_base.DbTestCase): node = self._get_test_node(info) info = snmp._parse_driver_info(node) self.assertEqual('2c', info['version']) - self.assertEqual('private', info['community']) + self.assertEqual('private', info['read_community']) + self.assertEqual('private', info['write_community']) + + def test__parse_driver_info_read_write_community(self): + # Make sure separate read/write community name take precedence + info = db_utils.get_test_snmp_info(snmp_version='1', + snmp_community='impossible', + snmp_community_read='public', + snmp_community_write='private') + node = self._get_test_node(info) + info = snmp._parse_driver_info(node) + self.assertEqual('1', info['version']) + self.assertEqual('public', info['read_community']) + self.assertEqual('private', info['write_community']) + + def test__parse_driver_info_read_community(self): + # Make sure separate read community name take precedence + info = db_utils.get_test_snmp_info(snmp_version='1', + snmp_community='foo', + snmp_community_read='bar') + node = self._get_test_node(info) + info = snmp._parse_driver_info(node) + self.assertEqual('1', info['version']) + self.assertEqual('bar', info['read_community']) + self.assertEqual('foo', info['write_community']) + + def test__parse_driver_info_write_community(self): + # Make sure separate write community name take precedence + info = db_utils.get_test_snmp_info(snmp_version='1', + snmp_community='foo', + snmp_community_write='bar') + node = self._get_test_node(info) + info = snmp._parse_driver_info(node) + self.assertEqual('1', info['version']) + self.assertEqual('foo', info['read_community']) + self.assertEqual('bar', info['write_community']) def test__parse_driver_info_snmp_v3(self): # Make sure SNMPv3 is parsed with user string. @@ -539,7 +590,8 @@ class SNMPValidateParametersTestCase(db_base.DbTestCase): node = self._get_test_node(info) info = snmp._parse_driver_info(node) self.assertEqual('1', info['version']) - self.assertEqual(INFO_DICT['snmp_community'], info['community']) + self.assertEqual(INFO_DICT['snmp_community'], info['read_community']) + self.assertEqual(INFO_DICT['snmp_community'], info['write_community']) def test__parse_driver_info_invalid_version(self): # Make sure exception is raised when version is invalid. diff --git a/releasenotes/notes/add-snmp-read-write-community-names-7589a8d1899c142c.yaml b/releasenotes/notes/add-snmp-read-write-community-names-7589a8d1899c142c.yaml new file mode 100644 index 0000000000..031463d8cb --- /dev/null +++ b/releasenotes/notes/add-snmp-read-write-community-names-7589a8d1899c142c.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + Adds new optional ``snmp_community_read`` and ``snmp_community_write`` + properties to ``snmp`` driver configuration (specified via a node's + ``driver_info`` field). If present, the value(s) + will be used respectively for SNMP reads and/or writes to the PDU. + When not present, ``snmp_community`` value will be used instead.