From dfe45212eec134ad9e24216929424a23c460662f Mon Sep 17 00:00:00 2001 From: vdahiya12 <67608553+vdahiya12@users.noreply.github.com> Date: Mon, 6 Nov 2023 10:08:56 -0800 Subject: [PATCH] [DualToR][caclmgrd] Fix IPtables rules for multiple vlan interfaces for DualToR config (#17093) * [DualToR][caclmgrd] Fix IPtables rules for multiple vlan interfaces for DualToR config Signed-off-by: vaibhav-dahiya --- src/sonic-host-services/scripts/caclmgrd | 43 +++++++++---- .../tests/caclmgrd/caclmgrd_soc_rules_test.py | 61 +++++++++++++++++-- .../tests/caclmgrd/test_soc_rules_vectors.py | 48 +++++++++++++-- 3 files changed, 130 insertions(+), 22 deletions(-) diff --git a/src/sonic-host-services/scripts/caclmgrd b/src/sonic-host-services/scripts/caclmgrd index 10ab7425485f..82c8097cf5c2 100755 --- a/src/sonic-host-services/scripts/caclmgrd +++ b/src/sonic-host-services/scripts/caclmgrd @@ -41,8 +41,9 @@ def _ip_prefix_in_key(key): """ return (isinstance(key, tuple)) -def get_ip_from_interface_table(table, intf_name): - +def get_ipv4_networks_from_interface_table(table, intf_name): + + addresses = [] if table: for key, _ in table.items(): if not _ip_prefix_in_key(key): @@ -51,12 +52,11 @@ def get_ip_from_interface_table(table, intf_name): iface_name, iface_cidr = key if iface_name.startswith(intf_name): - ip_str = iface_cidr.split("/")[0] - ip_addr = ipaddress.ip_address(ip_str) - if isinstance(ip_addr, ipaddress.IPv4Address): - return ip_addr + ip_ntwrk = ipaddress.ip_network(iface_cidr, strict=False) + if isinstance(ip_ntwrk, ipaddress.IPv4Network): + addresses.append(ip_ntwrk) - return None + return addresses # ============================== Classes ============================== @@ -342,22 +342,41 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): if self.DualToR: loopback_table = config_db_connector.get_table(self.LOOPBACK_TABLE) loopback_name = 'Loopback3' - loopback_address = get_ip_from_interface_table(loopback_table, loopback_name) + loopback_networks = get_ipv4_networks_from_interface_table(loopback_table, loopback_name) + if len(loopback_networks) == 0: + self.log_warning("Loopback 3 IP not available from DualToR active-active config") + return fwd_dualtor_grpc_traffic_from_host_to_soc_cmds + + if not isinstance(loopback_networks[0], ipaddress.IPv4Network): + self.log_warning("Loopback 3 IP Network not available from DualToR active-active config") + return fwd_dualtor_grpc_traffic_from_host_to_soc_cmds + + loopback_address = loopback_networks[0].network_address vlan_name = 'Vlan' vlan_table = config_db_connector.get_table(self.VLAN_INTF_TABLE) + vlan_networks = get_ipv4_networks_from_interface_table(vlan_table, vlan_name) + + if len(vlan_networks) == 0: + self.log_warning("Vlan IP not available from DualToR active-active config") + return fwd_dualtor_grpc_traffic_from_host_to_soc_cmds - vlan_address = get_ip_from_interface_table(vlan_table, vlan_name) fwd_dualtor_grpc_traffic_from_host_to_soc_cmds.append(self.iptables_cmd_ns_prefix[namespace] + "iptables -t nat --flush POSTROUTING") - + if loopback_address is not None: mux_table = config_db_connector.get_table(self.CONFIG_MUX_CABLE) mux_table_keys = mux_table.keys() for key in mux_table_keys: kvp = mux_table.get(key) if 'cable_type' in kvp and kvp['cable_type'] == 'active-active': - fwd_dualtor_grpc_traffic_from_host_to_soc_cmds.append(self.iptables_cmd_ns_prefix[namespace] + - "iptables -t nat -A POSTROUTING --destination {} --source {} -j SNAT --to-source {}".format(kvp['soc_ipv4'], vlan_address, loopback_address)) + soc_ipv4_str = kvp['soc_ipv4'].split("/")[0] + soc_ipv4_addr = ipaddress.ip_address(soc_ipv4_str) + for ip_network in vlan_networks: + # Only add the vlan source IP specific soc IP address to IPtables + if soc_ipv4_addr in ip_network: + vlan_address = ip_network.network_address + fwd_dualtor_grpc_traffic_from_host_to_soc_cmds.append(self.iptables_cmd_ns_prefix[namespace] + + "iptables -t nat -A POSTROUTING --destination {} --source {} -j SNAT --to-source {}".format(str(soc_ipv4_addr), str(vlan_address), str(loopback_address))) return fwd_dualtor_grpc_traffic_from_host_to_soc_cmds diff --git a/src/sonic-host-services/tests/caclmgrd/caclmgrd_soc_rules_test.py b/src/sonic-host-services/tests/caclmgrd/caclmgrd_soc_rules_test.py index 558b38f199bf..880798aecf4e 100644 --- a/src/sonic-host-services/tests/caclmgrd/caclmgrd_soc_rules_test.py +++ b/src/sonic-host-services/tests/caclmgrd/caclmgrd_soc_rules_test.py @@ -4,11 +4,11 @@ from parameterized import parameterized from sonic_py_common.general import load_module_from_source -from ipaddress import IPv4Address +from ipaddress import IPv4Address, IPv4Network from unittest import TestCase, mock from pyfakefs.fake_filesystem_unittest import patchfs -from .test_soc_rules_vectors import CACLMGRD_SOC_TEST_VECTOR +from .test_soc_rules_vectors import CACLMGRD_SOC_TEST_VECTOR, CACLMGRD_SOC_TEST_VECTOR_EMPTY from tests.common.mock_configdb import MockConfigDb from unittest.mock import MagicMock, patch @@ -29,7 +29,7 @@ def setUp(self): @parameterized.expand(CACLMGRD_SOC_TEST_VECTOR) @patchfs - @patch('caclmgrd.get_ip_from_interface_table', MagicMock(return_value="10.10.10.10")) + @patch('caclmgrd.get_ipv4_networks_from_interface_table', MagicMock(return_value=[IPv4Network('10.10.10.18/24', strict=False), IPv4Network('10.10.11.18/24', strict=False)])) def test_caclmgrd_soc(self, test_name, test_data, fs): if not os.path.exists(DBCONFIG_PATH): fs.create_file(DBCONFIG_PATH) # fake database_config.json @@ -50,11 +50,60 @@ def test_caclmgrd_soc(self, test_name, test_data, fs): caclmgrd_daemon.update_control_plane_nat_acls('', {}, MockConfigDb()) mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True) - def test_get_ip_from_interface_table(self): + + @parameterized.expand(CACLMGRD_SOC_TEST_VECTOR_EMPTY) + @patchfs + @patch('caclmgrd.get_ipv4_networks_from_interface_table', MagicMock(return_value=[])) + def test_caclmgrd_soc_no_ips(self, test_name, test_data, fs): + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) # fake database_config.json + + MockConfigDb.set_config_db(test_data["config_db"]) + + with mock.patch("caclmgrd.subprocess") as mocked_subprocess: + popen_mock = mock.Mock() + popen_attrs = test_data["popen_attributes"] + popen_mock.configure_mock(**popen_attrs) + mocked_subprocess.Popen.return_value = popen_mock + mocked_subprocess.PIPE = -1 + + call_rc = test_data["call_rc"] + mocked_subprocess.call.return_value = call_rc + + caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd") + caclmgrd_daemon.update_control_plane_nat_acls('', {}, MockConfigDb()) + mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True) + + + @parameterized.expand(CACLMGRD_SOC_TEST_VECTOR_EMPTY) + @patchfs + @patch('caclmgrd.get_ipv4_networks_from_interface_table', MagicMock(return_value=['10.10.10.10'])) + def test_caclmgrd_soc_ip_string(self, test_name, test_data, fs): + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) # fake database_config.json + + MockConfigDb.set_config_db(test_data["config_db"]) + + with mock.patch("caclmgrd.subprocess") as mocked_subprocess: + popen_mock = mock.Mock() + popen_attrs = test_data["popen_attributes"] + popen_mock.configure_mock(**popen_attrs) + mocked_subprocess.Popen.return_value = popen_mock + mocked_subprocess.PIPE = -1 + + call_rc = test_data["call_rc"] + mocked_subprocess.call.return_value = call_rc + + caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd") + caclmgrd_daemon.update_control_plane_nat_acls('', {}, MockConfigDb()) + mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True) + + + def test_get_ipv4_networks_from_interface_table(self): if not os.path.exists(DBCONFIG_PATH): fs.create_file(DBCONFIG_PATH) # fake database_config.json table = {("Vlan1000","10.10.10.1/32"): "val"} - ip_addr = self.caclmgrd.get_ip_from_interface_table(table, "Vlan") + ip_addr = self.caclmgrd.get_ipv4_networks_from_interface_table(table, "Vlan") - assert (ip_addr == IPv4Address('10.10.10.1')) + assert (ip_addr == [IPv4Network('10.10.10.1/32')]) diff --git a/src/sonic-host-services/tests/caclmgrd/test_soc_rules_vectors.py b/src/sonic-host-services/tests/caclmgrd/test_soc_rules_vectors.py index d8016dbe0672..9d969b9cfeed 100644 --- a/src/sonic-host-services/tests/caclmgrd/test_soc_rules_vectors.py +++ b/src/sonic-host-services/tests/caclmgrd/test_soc_rules_vectors.py @@ -18,11 +18,11 @@ "MUX_CABLE": { "Ethernet4": { "cable_type": "active-active", - "soc_ipv4": "192.168.1.0/32", + "soc_ipv4": "10.10.11.7/32", } }, "VLAN_INTERFACE": { - "Vlan1000|10.10.2.2/23": { + "Vlan1000|10.10.10.3/24": { "NULL": "NULL", } }, @@ -35,8 +35,48 @@ }, }, "expected_subprocess_calls": [ - call("iptables -t nat -A POSTROUTING --destination 192.168.1.0/32 --source 10.10.10.10 -j SNAT --to-source 10.10.10.10",shell=True, universal_newlines=True, stdout=-1) - ], + call('iptables -t nat -A POSTROUTING --destination 10.10.11.7 --source 10.10.11.0 -j SNAT --to-source 10.10.10.0', shell=True, universal_newlines=True, stdout=-1) + ], + "popen_attributes": { + 'communicate.return_value': ('output', 'error'), + }, + "call_rc": 0, + } + ] +] + + +CACLMGRD_SOC_TEST_VECTOR_EMPTY = [ + [ + "SOC_SESSION_TEST", + { + "config_db": { + "DEVICE_METADATA": { + "localhost": { + "subtype": "DualToR", + "type": "ToRRouter", + } + }, + "MUX_CABLE": { + "Ethernet4": { + "cable_type": "active-active", + "soc_ipv4": "10.10.11.7/32", + } + }, + "VLAN_INTERFACE": { + "Vlan1000|10.10.10.3/24": { + "NULL": "NULL", + } + }, + "LOOPBACK_INTERFACE": { + "Loopback3|10.10.10.10/32": { + "NULL": "NULL", + } + }, + "FEATURE": { + }, + }, + "expected_subprocess_calls": [], "popen_attributes": { 'communicate.return_value': ('output', 'error'), },