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

@@ -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.

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 "
+                "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.