From 571f3fd5132f72eef174663570f96f474751baeb Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Tue, 31 Oct 2023 22:26:31 +0000 Subject: [PATCH 1/4] [DualToR][caclmgrd] Fix IPtables rules for multiple vlan interfaces for DualToR config Signed-off-by: vaibhav-dahiya --- scripts/caclmgrd | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/scripts/caclmgrd b/scripts/caclmgrd index d914b493..4c583682 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 ============================== @@ -353,25 +354,45 @@ 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_table = self.config_db_map[DEFAULT_NAMESPACE].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']) + "iptables -t nat --flush POSTROUTING") if loopback_address is not None: - mux_table = config_db_connector.get_table(self.CONFIG_MUX_CABLE) + mux_table = self.config_db_map[DEFAULT_NAMESPACE].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', 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 {} --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 + 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 From 43e8d535ab13590719c9f3c4827f892fc91058c6 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Wed, 1 Nov 2023 00:12:55 +0000 Subject: [PATCH 2/4] fix changes Signed-off-by: vaibhav-dahiya --- scripts/caclmgrd | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/caclmgrd b/scripts/caclmgrd index 4c583682..0f1b07ac 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -365,7 +365,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): loopback_address = loopback_networks[0].network_address vlan_name = 'Vlan' - vlan_table = self.config_db_map[DEFAULT_NAMESPACE].get_table(self.VLAN_INTF_TABLE) + 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: @@ -373,10 +373,10 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): return fwd_dualtor_grpc_traffic_from_host_to_soc_cmds fwd_dualtor_grpc_traffic_from_host_to_soc_cmds.append(self.iptables_cmd_ns_prefix[namespace] + - "iptables -t nat --flush POSTROUTING") + ['iptables', '-t', 'nat', '--flush', 'POSTROUTING']) if loopback_address is not None: - mux_table = self.config_db_map[DEFAULT_NAMESPACE].get_table(self.CONFIG_MUX_CABLE) + 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) From 512f145626e4d04498f01dc3329f4bc3ec8f9a93 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Thu, 2 Nov 2023 22:02:00 +0000 Subject: [PATCH 3/4] fix ut Signed-off-by: vaibhav-dahiya --- scripts/caclmgrd | 4 ++-- tests/caclmgrd/caclmgrd_soc_rules_test.py | 10 +++++----- tests/caclmgrd/test_soc_rules_vectors.py | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/scripts/caclmgrd b/scripts/caclmgrd index 0f1b07ac..a2eb6e89 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -359,7 +359,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): 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) + 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 @@ -388,7 +388,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): 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))) + ['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 diff --git a/tests/caclmgrd/caclmgrd_soc_rules_test.py b/tests/caclmgrd/caclmgrd_soc_rules_test.py index 5f5ee4d8..3668f750 100644 --- a/tests/caclmgrd/caclmgrd_soc_rules_test.py +++ b/tests/caclmgrd/caclmgrd_soc_rules_test.py @@ -4,7 +4,7 @@ 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 @@ -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,11 @@ 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): + 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..98ea2d7e 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'), From 71f5370181108b6ab0cc8d95a9336d85c43e768a Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Thu, 2 Nov 2023 23:08:51 +0000 Subject: [PATCH 4/4] add coverage Signed-off-by: vaibhav-dahiya --- tests/caclmgrd/caclmgrd_soc_rules_test.py | 53 ++++++++++++++++++++++- tests/caclmgrd/test_soc_rules_vectors.py | 40 +++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/tests/caclmgrd/caclmgrd_soc_rules_test.py b/tests/caclmgrd/caclmgrd_soc_rules_test.py index 3668f750..ef40292a 100644 --- a/tests/caclmgrd/caclmgrd_soc_rules_test.py +++ b/tests/caclmgrd/caclmgrd_soc_rules_test.py @@ -8,7 +8,7 @@ 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 @@ -51,6 +51,57 @@ 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) + + @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 diff --git a/tests/caclmgrd/test_soc_rules_vectors.py b/tests/caclmgrd/test_soc_rules_vectors.py index 98ea2d7e..b339a32d 100644 --- a/tests/caclmgrd/test_soc_rules_vectors.py +++ b/tests/caclmgrd/test_soc_rules_vectors.py @@ -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, + } + ] +]