353: Netmask MIB enhancement #355
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@madhupalu will plz help in assigning the right reviewer? |
|
Could you also add sonic-mgmt new testcases? |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the NETMASK MIB (OID: 1.3.6.1.2.1.4.34.1.5.1) to the SNMP subagent, enabling SNMP clients to query the mapping between IP addresses, network IPs, netmasks, and interface indices.
- Added NetmaskUpdater class to parse and expose IP address/netmask information via SNMP
- Implemented ipNetToNetMask MIB entry in the IpMib class
- Added test coverage for the new NetmaskUpdater functionality
- Updated counter retrieval logic to return 0 for VLAN interfaces in both rfc1213.py and rfc2863.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| src/sonic_ax_impl/mibs/ietf/rfc1213.py | Implements NetmaskUpdater class with methods to parse interface IP addresses from APPL_DB and system commands, and exposes them via the ipNetToNetMask MIB entry |
| src/sonic_ax_impl/mibs/ietf/rfc2863.py | Adds VLAN interface check to return 0 for counter queries, consistent with management interface handling |
| tests/test_rfc1213.py | Adds TestNetmaskUpdater class with tests for _update_netmask_info, update_data, and get_next methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| interfaces = Namespace.dbs_keys(self.db_conn, mibs.APPL_DB, "INTF_TABLE:*") | ||
| for interface in interfaces: | ||
| ethTablePrefix = re.search(r"INTF_TABLE\:[A-Z][a-z0-9]+\:[0-9./]+", interface) |
There was a problem hiding this comment.
The regex pattern only matches interface names starting with uppercase letters followed by lowercase letters and digits (e.g., Eth0, Docker0). This won't match common SONiC interface naming patterns like "Ethernet8", "PortChannel1", or "Vlan100" which contain uppercase letters throughout. Consider updating the pattern to: r"INTF_TABLE:([A-Za-z][A-Za-z0-9]+):([0-9./]+)" to properly capture these interface names.
| process = os.popen('ip addr show eth0 | grep "\<inet\>" | awk \'{ print $2 }\' | awk -F "/" \'{ print $1 }\'') | ||
| mgmt_ip = process.read().strip() | ||
| if (len(mgmt_ip) != 0): | ||
| self._update_netmask_info("eth0", mgmt_ip) | ||
| process.close() | ||
|
|
||
| process = os.popen('ip addr show docker0 | grep "\<inet\>" | awk \'{ print $2 }\' | awk -F "/" \'{ print $1 }\'') | ||
| docker_inet = process.read().strip() | ||
| if (len(docker_inet) != 0): | ||
| self._update_netmask_info("docker0", docker_inet) | ||
| process.close() | ||
|
|
||
| process = os.popen('ip addr show docker0 | grep "\<inet\>" | awk \'{ print $4 }\' | awk -F "/" \'{ print $1 }\'') | ||
| docker_brd = process.read().strip() | ||
| if (len(docker_brd) != 0): | ||
| self._update_netmask_info("docker0", docker_brd) | ||
| process.close() |
There was a problem hiding this comment.
Using os.popen() is deprecated and can lead to resource leaks if the process handle is not properly managed. Additionally, it poses security risks if shell commands are constructed with untrusted input. Consider using subprocess.run() or subprocess.Popen() with proper error handling instead. For example: subprocess.run(['ip', 'addr', 'show', 'eth0'], capture_output=True, text=True) and then parse the output.
| else: | ||
| dev = ethTablePrefix.group().split(':')[1] | ||
| ip = ethTablePrefix.group().split(':')[2] |
There was a problem hiding this comment.
The else block following a continue statement is redundant. Since the 'if' block contains a 'continue', the code in the else block will always execute when the condition is False, making the else keyword unnecessary. The code can be unindented to improve readability.
| iptuple = ip2byte_tuple(ip) | ||
| subid = (4,) + iptuple | ||
|
|
||
| # Create map beteen subid and OID |
There was a problem hiding this comment.
The comment has a typo: "beteen" should be "between".
| if ip.find(".") != -1: | ||
| self._update_netmask_info(dev, ip) | ||
|
|
||
| process = os.popen('ip addr show eth0 | grep "\<inet\>" | awk \'{ print $2 }\' | awk -F "/" \'{ print $1 }\'') |
There was a problem hiding this comment.
The os.popen calls extract only the first IP address part using awk, but then pass it to _update_netmask_info which expects an IP address in CIDR format (with /prefix). The awk command 'print $1' after 'awk -F "/" removes the prefix length, so the resulting mgmt_ip will be like "192.168.1.1" instead of "192.168.1.1/24". This will cause the split('/') operations in _update_netmask_info to fail. The commands should preserve the full CIDR notation by removing the final awk command.
| self._update_netmask_info("docker0", docker_inet) | ||
| process.close() | ||
|
|
||
| process = os.popen('ip addr show docker0 | grep "\<inet\>" | awk \'{ print $4 }\' | awk -F "/" \'{ print $1 }\'') |
There was a problem hiding this comment.
The os.popen calls extract only the broadcast address part using awk, but then pass it to _update_netmask_info which expects an IP address in CIDR format (with /prefix). The awk command 'print $1' after 'awk -F "/" removes the prefix length, so the resulting docker_brd will be like "172.17.255.255" instead of "172.17.255.255/24". This will cause the split('/') operations in _update_netmask_info to fail. The commands should preserve the full CIDR notation by removing the final awk command.
| from unittest import TestCase | ||
| import pytest | ||
| from unittest.mock import MagicMock | ||
| from sonic_ax_impl.mibs.ietf.rfc1213 import NetmaskUpdater |
There was a problem hiding this comment.
The duplicate import of NetmaskUpdater on line 8 is unnecessary since it's already imported on line 18. Remove the duplicate import to keep the imports clean and organized.
| with mock.patch("sonic_ax_impl.mibs.ietf.rfc1213.Namespace.dbs_keys", mock_dbs_keys): | ||
| def popen_side_effect(cmd): | ||
| mock_proc = MagicMock() | ||
| if "Eth0" in cmd and "print $2" in cmd: |
There was a problem hiding this comment.
The test's mock for os.popen is checking for 'Eth0' in the command, but the actual command in the implementation uses 'eth0' (lowercase). This case sensitivity mismatch means the test won't properly validate the actual code path. The test should match the exact interface names used in the implementation.
| class TestNetmaskUpdater(TestCase): | ||
| @mock.patch("sonic_ax_impl.mibs.Namespace.init_namespace_dbs", return_value="mock_db_conn") | ||
| @mock.patch('sonic_ax_impl.mibs.get_index_from_str', return_value=1) | ||
| @mock.patch('ax_interface.util.ip2byte_tuple', side_effect=lambda ip: tuple(map(int, ip.split('.')))) | ||
| def test_update_netmask_info(self, mock_ip2byte, mock_get_index, mock_namespace): | ||
| updater = NetmaskUpdater() | ||
| updater._update_netmask_info("eth0", "192.168.1.1/24") | ||
|
|
||
| expected_key = (4, 192, 168, 1, 1) | ||
| self.assertIn(expected_key, updater.netmask_map) | ||
| self.assertIn(expected_key, updater.netmask_list) | ||
|
|
||
| @mock.patch("ax_interface.util.ip2byte_tuple", side_effect=lambda ip: tuple(map(int, ip.split('.')))) | ||
| @mock.patch("sonic_ax_impl.mibs.get_index_from_str", return_value=2) | ||
| @mock.patch("sonic_ax_impl.mibs.ietf.rfc1213.Namespace.init_namespace_dbs", return_value="mock_db_conn") | ||
| @mock.patch("sonic_ax_impl.mibs.ietf.rfc1213.os.popen") | ||
| def test_update_data(self, mock_popen, mock_init_ns, mock_get_index, mock_ip2byte): | ||
| mock_dbs_keys = mock.MagicMock(return_value=[ | ||
| "INTF_TABLE:Eth0:192.168.1.1/24", | ||
| "INTF_TABLE:Docker0:10.0.0.1/8" | ||
| ]) | ||
| mock_init_ns.return_value = "mock_db_conn" | ||
|
|
||
| with mock.patch("sonic_ax_impl.mibs.ietf.rfc1213.Namespace.dbs_keys", mock_dbs_keys): | ||
| def popen_side_effect(cmd): | ||
| mock_proc = MagicMock() | ||
| if "Eth0" in cmd and "print $2" in cmd: | ||
| mock_proc.read.return_value = "192.168.1.1/24\n" | ||
| elif "docker0" in cmd and "print $2" in cmd: | ||
| mock_proc.read.return_value = "172.17.0.1/24\n" | ||
| elif "docker0" in cmd and "print $4" in cmd: | ||
| mock_proc.read.return_value = "172.17.255.255/24\n" | ||
| else: | ||
| mock_proc.read.return_value = "192.168.1.1/24\n" | ||
| return mock_proc | ||
|
|
||
| mock_popen.side_effect = popen_side_effect | ||
|
|
||
| updater = NetmaskUpdater() | ||
| updater.update_data() | ||
|
|
||
| self.assertGreater(len(updater.netmask_map), 0) | ||
| self.assertTrue(all(isinstance(k, tuple) for k in updater.netmask_list)) | ||
|
|
||
| def test_get_next(self): | ||
| updater = NetmaskUpdater() | ||
| updater.netmask_list = [(4, 10, 0, 0, 1), (4, 192, 168, 1, 1), (4, 192, 168, 1, 2)] | ||
| self.assertEqual(updater.get_next((4, 10, 0, 0, 1)), (4, 192, 168, 1, 1)) | ||
| self.assertEqual(updater.get_next((4, 192, 168, 1, 2)), None) |
There was a problem hiding this comment.
Missing error handling test cases. The tests should verify that the NetmaskUpdater handles error conditions gracefully, such as: invalid IP addresses, missing prefix lengths, malformed database entries, or failed os.popen calls. Consider adding negative test cases to ensure robustness.
| import sonic_ax_impl | ||
| import sys | ||
| from unittest import TestCase | ||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
|
@ssithaia-ebay please address the Copilot review comments.thx |
5 participants
- What I did
Adding support of NETMASK MIB [ OID: 1.3.6.1.2.1.4.34.1.5.1] in snmp-subagent. This MIB returns the mapping between the assigned IP address and its network IP along with the netmask and IfIndex.
- How I did it
Updated rfc1213.py from snmp-subagent to accomodate NETMASK MIB implementation.
- How to verify it
$ snmpwalk -v2c -c public 192.168.1.1 1.3.6.1.2.1.4.34.1.5.1