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.