Merge "Add read&write SNMP community names to snmp driver"

doc/source/admin/drivers/snmp.rst

@@ -127,8 +127,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.

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"),
@@ -947,11 +968,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 "
+                "SNMP driver requires `snmp_community` or "
-                "%s.") % snmp_info['version'])
+                "`snmp_community_read`/`snmp_community_write` properties "
-        snmp_info['community'] = info.get('snmp_community')
+                "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))

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',

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")

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):
+    def test__get_auth_v1_read(self, mock_community, mock_cmdgen):
-        client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V1)
+        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.

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.