[BFD]Fix BFD blackout issue (#150)

Fixes sonic-net#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

Author: dgsudharsan

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,24 +888,29 @@ 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]
                 break
 
         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:

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)
 

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)
             ],