From e21db16de7e095a15874a64d12afb8e770325434 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Wed, 28 Aug 2024 18:25:34 -0700 Subject: [PATCH] [BFD]Fix BFD blackout issue (#150) Fixes sonic-net/sonic-buildimage#19762 When caclmgrd processes any ACL table change, this results in rules flushed and readded which was introduced in #114 . However in this flow the BFD and vxlan rules were added at the end which may result in traffic loss because of the DROP rule for ip2me installed. To overcome this added BFD and vxlan rules at the very start so that there is no drop seen for BFD. Existing UT should verify the flows. Additionally manually verified --- scripts/caclmgrd | 38 +++++++++++++++++++---------- tests/caclmgrd/caclmgrd_bfd_test.py | 22 +++++++++++++++-- tests/caclmgrd/test_bfd_vectors.py | 10 ++++++++ 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/scripts/caclmgrd b/scripts/caclmgrd index 54203d54..90aff6a0 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -586,6 +586,14 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', '127.0.0.1', '-i', 'lo', '-j', 'ACCEPT']) iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', '::1', '-i', 'lo', '-j', 'ACCEPT']) + + if self.bfdAllowed: + iptables_cmds += self.get_bfd_iptable_commands(namespace) + + if self.VxlanAllowed: + fvs = swsscommon.FieldValuePairs([("src_ip", self.VxlanSrcIP)]) + iptables_cmds += self.get_vxlan_port_iptable_commands(namespace, fvs) + # Add iptables commands to allow internal docker traffic iptables_cmds += self.generate_allow_internal_docker_ip_traffic_commands(namespace) @@ -813,12 +821,6 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): self.log_info(" " + ' '.join(cmd)) self.run_commands(iptables_cmds) - if self.bfdAllowed: - self.allow_bfd_protocol(namespace) - if self.VxlanAllowed: - fvs = swsscommon.FieldValuePairs([("src_ip", self.VxlanSrcIP)]) - self.allow_vxlan_port(namespace, fvs) - self.update_control_plane_nat_acls(namespace, service_to_source_ip_map, config_db_connector) @@ -886,14 +888,21 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): finally: new_config_db_connector.close("CONFIG_DB") - def allow_bfd_protocol(self, namespace): + def get_bfd_iptable_commands(self, namespace): iptables_cmds = [] # Add iptables/ip6tables commands to allow all BFD singlehop and multihop sessions iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT']) iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT']) - self.run_commands(iptables_cmds) + return iptables_cmds - def allow_vxlan_port(self, namespace, data): + def allow_bfd_protocol(self, namespace): + iptables_cmds = self.get_bfd_iptable_commands(namespace) + if iptables_cmds: + self.run_commands(iptables_cmds) + + + def get_vxlan_port_iptable_commands(self, namespace, data): + iptables_cmds = [] for fv in data: if (fv[0] == "src_ip"): self.VxlanSrcIP = fv[1] @@ -901,9 +910,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): if not self.VxlanSrcIP: self.log_info("Received vxlan tunnel configuration without source ip") - return False - - iptables_cmds = [] + return iptables_cmds # Add iptables/ip6tables commands to allow VxLAN packets ip_addr = ipaddress.ip_address(self.VxlanSrcIP) @@ -914,10 +921,15 @@ class ControlPlaneAclManager(daemon_base.DaemonBase): iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-d', self.VxlanSrcIP, '--dport', '4789', '-j', 'ACCEPT']) + return iptables_cmds + + def allow_vxlan_port(self, namespace, data): + iptables_cmds = self.get_vxlan_port_iptable_commands(namespace, data) + if not iptables_cmds: + return False self.run_commands(iptables_cmds) self.log_info("Enabled vxlan port for source ip " + self.VxlanSrcIP) self.VxlanAllowed = True - return True def block_vxlan_port(self, namespace): if not self.VxlanSrcIP: diff --git a/tests/caclmgrd/caclmgrd_bfd_test.py b/tests/caclmgrd/caclmgrd_bfd_test.py index dd577fe0..14da370a 100644 --- a/tests/caclmgrd/caclmgrd_bfd_test.py +++ b/tests/caclmgrd/caclmgrd_bfd_test.py @@ -47,10 +47,28 @@ def test_caclmgrd_bfd(self, test_name, test_data, fs): caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd") caclmgrd_daemon.allow_bfd_protocol('') - mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True) + mocked_subprocess.Popen.assert_has_calls(test_data["expected_bfd_subprocess_calls"], any_order=True) caclmgrd_daemon.bfdAllowed = True mocked_subprocess.Popen.reset_mock() caclmgrd_daemon.num_changes[''] = 1 caclmgrd_daemon.check_and_update_control_plane_acls('', 1) - mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True) + + #Ensure BFD rules are installed before ip2me rules to avoid traffic loss during update of control plane acl rules + bfd_ipv4_idx = 0 + bfd_ipv6_idx = 0 + ip2me_ipv4_idx = 0 + ip2me_ipv6_idx = 0 + idx = 0 + for call in mocked_subprocess.Popen.call_args_list: + if test_data["expected_subprocess_calls"][0] == call: + bfd_ipv4_idx = idx + elif test_data["expected_subprocess_calls"][1] == call: + bfd_ipv6_idx = idx + elif test_data["expected_subprocess_calls"][2] == call: + ip2me_ipv4_idx = idx + elif test_data["expected_subprocess_calls"][3] == call: + ip2me_ipv6_idx = idx + idx = idx+1 + assert (bfd_ipv4_idx != 0 and bfd_ipv6_idx != 0 and ip2me_ipv4_idx !=0 and ip2me_ipv6_idx != 0) + assert (bfd_ipv4_idx < ip2me_ipv4_idx and bfd_ipv6_idx < ip2me_ipv6_idx) diff --git a/tests/caclmgrd/test_bfd_vectors.py b/tests/caclmgrd/test_bfd_vectors.py index 8a0e5b98..9211f35a 100644 --- a/tests/caclmgrd/test_bfd_vectors.py +++ b/tests/caclmgrd/test_bfd_vectors.py @@ -21,8 +21,18 @@ "state": "enabled", } }, + "LOOPBACK_INTERFACE": { + "Loopback0|2.2.2.1/32": {}, + "Loopback0|2001:db8:10::/64": {} + }, }, "expected_subprocess_calls": [ + call(['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE), + call(['ip6tables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE), + call(['iptables', '-A', 'INPUT', '-d', '2.2.2.1/32', '-j', 'DROP'],universal_newlines=True, stdout=subprocess.PIPE), + call(['ip6tables', '-A', 'INPUT', '-d', '2001:db8:10::/128', '-j', 'DROP'],universal_newlines=True, stdout=subprocess.PIPE) + ], + "expected_bfd_subprocess_calls": [ call(['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE), call(['ip6tables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE) ],