From beb8bbe9f40dae3e3af27de989b8a4ab899ba801 Mon Sep 17 00:00:00 2001 From: vdahiya12 <67608553+vdahiya12@users.noreply.github.com> Date: Thu, 2 Nov 2023 17:08:07 -0700 Subject: [PATCH] [DualToR][caclmgrd] Fix IPtables rules for multiple vlan interfaces for DualToR config (#82) This PR is a required for changing the L3 IP forwarding Behavior to SoC in active-active toplogy. Basically a src IP is added to the SNAT rule so that only packets originating from ToR with src IP as vlan IP get natted by the rule and change the src IP to LoopBack IP However if there are mutiple vlan IP's we only add the source IP as vlan IP, for which the SoC IP belongs to, this PR adds that change. How I did it check the config DB if the ToR is a DualToR and has an SoC IP assigned. put an iptable rule iptables -t nat -A POSTROUTING --destination -j SNAT --to-source " Signed-off-by: vaibhav-dahiya --- scripts/caclmgrd | 41 +++++++++++---- tests/caclmgrd/caclmgrd_soc_rules_test.py | 63 ++++++++++++++++++++--- tests/caclmgrd/test_soc_rules_vectors.py | 46 +++++++++++++++-- 3 files changed, 131 insertions(+), 19 deletions(-) diff --git a/scripts/caclmgrd b/scripts/caclmgrd index 1f044ad1..1b53c3a2 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -41,8 +41,10 @@ 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 +53,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 ============================== @@ -359,11 +360,24 @@ 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']) @@ -373,11 +387,18 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): 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', kvp['soc_ipv4'], '--source', vlan_address, '-j', 'SNAT', '--to-source', 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', str(soc_ipv4_addr), '--source', str(vlan_address), '-j', 'SNAT', '--to-source', str(loopback_address)]) return fwd_dualtor_grpc_traffic_from_host_to_soc_cmds + def generate_fwd_traffic_from_namespace_to_host_commands(self, namespace, acl_source_ip_map): """ The below SNAT and DNAT rules are added in asic namespace in multi-ASIC platforms. It helps to forward request coming diff --git a/tests/caclmgrd/caclmgrd_soc_rules_test.py b/tests/caclmgrd/caclmgrd_soc_rules_test.py index 5f5ee4d8..ef40292a 100644 --- a/tests/caclmgrd/caclmgrd_soc_rules_test.py +++ b/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 @@ -51,11 +51,62 @@ 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.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'): + 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.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'): + 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/tests/caclmgrd/test_soc_rules_vectors.py b/tests/caclmgrd/test_soc_rules_vectors.py index c3b871c7..b339a32d 100644 --- a/tests/caclmgrd/test_soc_rules_vectors.py +++ b/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,7 +35,7 @@ }, }, "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'], 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'], universal_newlines=True, stdout=-1) ], "popen_attributes": { 'communicate.return_value': ('output', 'error'), @@ -44,3 +44,43 @@ } ] ] + + +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'), + }, + "call_rc": 0, + } + ] +]