Merge pull request #25 from viniarck/fix/issue_13

Fix/issue 13

CHANGELOG.rst

@@ -21,6 +21,18 @@ Removed
 Security
 ========
 
+[4.1] - 2021-10.22
+******************
+
+Changed
+=======
+- Removed 'delete' command persistency
+- Faster filtering when removing stored flows
+
+Fixed
+=====
+- Fixed ``match13_no_strict`` issue 13
+
 
 [4.0] - 2021-05-27
 ******************

kytos.json

@@ -3,7 +3,7 @@
   "username": "kytos",
   "name": "flow_manager",
   "description": "Manage switches' flows through a REST API.",
-  "version": "4.0",
+  "version": "4.1",
   "napp_dependencies": ["kytos/of_core", "kytos/storehouse"],
   "license": "MIT",
   "url": "https://github.com/kytos/flow_manager.git",

main.py

@@ -199,12 +199,6 @@ def check_switch_consistency(self, switch):
                     self._install_flows(command, flow, [switch], save=False)
                     log.info(f'Flow forwarded to switch {dpid} to be '
                              'installed.')
-            elif command == 'delete':
-                log.info('A consistency problem was detected in '
-                         f'switch {dpid}.')
-                command = 'delete_strict'
-                self._install_flows(command, flow, [switch], save=False)
-                log.info(f'Flow forwarded to switch {dpid} to be deleted.')
 
     def check_storehouse_consistency(self, switch):
         """Check consistency of installed flows for a specific switch."""
@@ -266,22 +260,22 @@ def _store_changed_flows(self, command, flow, switch):
                      f'have not been specified: {switch}')
             return
         installed_flow = {}
-        flow_list = []
         installed_flow['command'] = command
         installed_flow['flow'] = flow
-        deleted_flows = []
+        should_persist_flow = command == "add"
+        deleted_flows_idxs = set()
 
         serializer = FlowFactory.get_class(switch)
         installed_flow_obj = serializer.from_dict(flow, switch)
 
         if switch.id not in stored_flows_box:
             # Switch not stored, add to box.
-            flow_list.append(installed_flow)
-            stored_flows_box[switch.id] = {'flow_list': flow_list}
+            if should_persist_flow:
+                stored_flows_box[switch.id] = {'flow_list': [installed_flow]}
         else:
             stored_flows = stored_flows_box[switch.id].get('flow_list', [])
             # Check if flow already stored
-            for stored_flow in stored_flows:
+            for i, stored_flow in enumerate(stored_flows):
                 stored_flow_obj = serializer.from_dict(stored_flow['flow'],
                                                        switch)
 
@@ -290,7 +284,7 @@ def _store_changed_flows(self, command, flow, switch):
                 if installed_flow['command'] == 'delete':
                     # No strict match
                     if match_flow(flow, version, stored_flow['flow']):
-                        deleted_flows.append(stored_flow)
+                        deleted_flows_idxs.add(i)
 
                 elif installed_flow_obj == stored_flow_obj:
                     if stored_flow['command'] == installed_flow['command']:
@@ -302,13 +296,17 @@ def _store_changed_flows(self, command, flow, switch):
                     # is to remove it. In this case, the old instruction is
                     # removed and the new one is stored.
                     stored_flow['command'] = installed_flow.get('command')
-                    deleted_flows.append(stored_flow)
+                    deleted_flows_idxs.add(i)
                     break
 
-            # if installed_flow['command'] != 'delete':
-            stored_flows.append(installed_flow)
-            for i in deleted_flows:
-                stored_flows.remove(i)
+            if deleted_flows_idxs:
+                stored_flows = [
+                    flow
+                    for i, flow in enumerate(stored_flows)
+                    if i not in deleted_flows_idxs
+                ]
+            if should_persist_flow:
+                stored_flows.append(installed_flow)
             stored_flows_box[switch.id]['flow_list'] = stored_flows
 
         stored_flows_box['id'] = 'flow_persistence'

match.py

@@ -108,33 +108,30 @@ def match10_no_strict(flow_dict, args):
 
 
 def match13_no_strict(flow_to_install, stored_flow_dict):
-    """Match a packet againts the stored flow (OF 1.3).
+    """Match a flow that is either exact or more specific (non-strict) (OF1.3).
 
     Return the flow if any fields match, otherwise, return False.
     """
-    if flow_to_install.get('cookie_mask') and 'cookie' in stored_flow_dict:
-        cookie = flow_to_install['cookie'] & flow_to_install['cookie_mask']
-        stored_cookie = (stored_flow_dict['cookie'] &
-                         flow_to_install['cookie_mask'])
-        if cookie == stored_cookie:
-            return stored_flow_dict
-        return False
-    if 'match' not in flow_to_install:
+    if 'match' not in flow_to_install or 'match' not in stored_flow_dict:
+        return stored_flow_dict
+    if not flow_to_install['match']:
+        return stored_flow_dict
+    if len(flow_to_install['match']) > len(stored_flow_dict['match']):
         return False
 
     for key, value in flow_to_install.get('match').items():
-        if 'match' not in stored_flow_dict:
-            return False
-        if key not in ('ipv4_src', 'ipv4_dst', 'ipv6_src', 'ipv6_dst'):
-            if value == stored_flow_dict['match'].get(key):
-                return stored_flow_dict
-        else:
-            field = stored_flow_dict['match'].get(key)
-            if not field:
+        if key not in stored_flow_dict['match']:
+            return False
+        if value != stored_flow_dict['match'].get(key):
+            return False
+
+    key_masks = {"cookie": "cookie_mask"}
+    for key, key_mask in key_masks.items():
+        if flow_to_install.get(key_mask) and key in stored_flow_dict:
+            value = flow_to_install[key] & flow_to_install[key_mask]
+            stored_value = (stored_flow_dict[key] &
+                            flow_to_install[key_mask])
+            if value != stored_value:
                 return False
-            masked_ip_addr = ipaddress.ip_network(value, False)
-            field_mask = field + "/" + str(masked_ip_addr.netmask)
-            masked_stored_ip = ipaddress.ip_network(field_mask, False)
-            if masked_ip_addr == masked_stored_ip:
-                return stored_flow_dict
-    return False
+
+    return stored_flow_dict

setup.py

@@ -22,7 +22,7 @@
 BASE_ENV = Path(os.environ.get('VIRTUAL_ENV', '/'))
 
 NAPP_NAME = 'flow_manager'
-NAPP_VERSION = '4.0'
+NAPP_VERSION = '4.1'
 
 # Kytos var folder
 VAR_PATH = BASE_ENV / 'var' / 'lib' / 'kytos'

tests/unit/test_main.py

@@ -408,7 +408,7 @@ def test_check_switch_consistency_delete(self, *args):
         mock_flow_factory.return_value = serializer
         self.napp.stored_flows = {dpid: {"flow_list": flow_list}}
         self.napp.check_switch_consistency(switch)
-        mock_install_flows.assert_called()
+        mock_install_flows.assert_not_called()
 
     @patch('napps.kytos.flow_manager.main.Main._install_flows')
     @patch('napps.kytos.flow_manager.main.FlowFactory.get_class')
@@ -496,7 +496,7 @@ def test_no_strict_delete_with_ipv4(self, *args):
                 "priority": 10,
                 "cookie": 84114904,
                 "match": {
-                    "ipv4_src": "192.168.1.120",
+                    "ipv4_src": "192.168.1.1",
                     "ipv4_dst": "192.168.0.2",
                 },
                 "actions": [],
@@ -510,14 +510,141 @@ def test_no_strict_delete_with_ipv4(self, *args):
                 "match": {"in_port": 2},
             },
         }
-        flow_to_install = {"match": {"ipv4_src": '192.168.1.1/24'}}
+        flow_to_install = {"match": {"ipv4_src": '192.168.1.1'}}
         flow_list = {"flow_list": [stored_flow, stored_flow2]}
         command = "delete"
         self.napp.stored_flows = {dpid: flow_list}
 
         self.napp._store_changed_flows(command, flow_to_install, switch)
         mock_save_flow.assert_called()
-        self.assertEqual(len(self.napp.stored_flows[dpid]['flow_list']), 2)
+        expected_stored = {
+          "flow_list": [
+            {
+                "command": "add",
+                "flow": {
+                    "actions": [],
+                    "cookie": 4961162389751548787,
+                    "match": {"in_port": 2},
+                },
+            }
+          ]
+        }
+        self.assertDictEqual(self.napp.stored_flows[dpid], expected_stored)
+
+    @patch('napps.kytos.flow_manager.main.Main._install_flows')
+    @patch('napps.kytos.flow_manager.main.FlowFactory.get_class')
+    @patch("napps.kytos.flow_manager.main.StoreHouse.save_flow")
+    def test_no_strict_delete_in_port(self, *args):
+        """Test the non-strict matching method.
+
+        Test non-strict matching to delete a Flow matching in_port.
+        """
+        (mock_save_flow, _, _) = args
+        dpid = "00:00:00:00:00:00:00:01"
+        switch = get_switch_mock(dpid, 0x04)
+        switch.id = dpid
+        flow_to_install = {"match": {"in_port": 1}}
+        stored_flow = {"flow_list": [
+            {
+                "command": "add",
+                "flow": {
+                    "priority": 10,
+                    "cookie": 84114904,
+                    "match": {
+                        "in_port": 1,
+                        "dl_vlan": 100,
+                    },
+                    "actions": [],
+                },
+            },
+            {
+                "command": "add",
+                "flow": {
+                    "actions": [],
+                    "match": {"in_port": 2},
+                },
+            },
+            {
+                "command": "add",
+                "flow": {
+                    "priority": 20,
+                    "cookie": 84114904,
+                    "match": {
+                        "in_port": 1,
+                        "dl_vlan": 102,
+                    },
+                    "actions": [],
+                },
+            },
+        ]}
+        command = "delete"
+        self.napp.stored_flows = {dpid: stored_flow}
+
+        self.napp._store_changed_flows(command, flow_to_install, switch)
+        mock_save_flow.assert_called()
+
+        expected_stored = {
+          "flow_list": [
+            {
+              "command": "add",
+              "flow": {
+                "actions": [],
+                "match": {
+                  "in_port": 2
+                }
+              }
+            }
+          ]
+        }
+        self.assertDictEqual(self.napp.stored_flows[dpid], expected_stored)
+
+    @patch('napps.kytos.flow_manager.main.Main._install_flows')
+    @patch('napps.kytos.flow_manager.main.FlowFactory.get_class')
+    @patch("napps.kytos.flow_manager.main.StoreHouse.save_flow")
+    def test_no_strict_delete_all_if_empty_match(self, *args):
+        """Test the non-strict matching method.
+
+        Test non-strict matching to delete all if empty match is given.
+        """
+        (mock_save_flow, _, _) = args
+        dpid = "00:00:00:00:00:00:00:01"
+        switch = get_switch_mock(dpid, 0x04)
+        switch.id = dpid
+        flow_to_install = {"match": {}}
+        stored_flow = {"flow_list": [
+            {
+                "command": "add",
+                "flow": {
+                    "priority": 10,
+                    "cookie": 84114904,
+                    "match": {
+                        "in_port": 1,
+                        "dl_vlan": 100,
+                    },
+                    "actions": [],
+                },
+            },
+            {
+                "command": "add",
+                "flow": {
+                    "priority": 20,
+                    "cookie": 84114904,
+                    "match": {
+                        "in_port": 1,
+                        "dl_vlan": 102,
+                    },
+                    "actions": [],
+                },
+            },
+        ]}
+        command = "delete"
+        self.napp.stored_flows = {dpid: stored_flow}
+
+        self.napp._store_changed_flows(command, flow_to_install, switch)
+        mock_save_flow.assert_called()
+
+        expected_stored = {"flow_list": []}
+        self.assertDictEqual(self.napp.stored_flows[dpid], expected_stored)
 
     @patch('napps.kytos.flow_manager.main.Main._install_flows')
     @patch('napps.kytos.flow_manager.main.FlowFactory.get_class')
@@ -551,14 +678,38 @@ def test_no_strict_delete_with_ipv4_fail(self, *args):
                 "match": {"in_port": 2},
             },
         }
-        flow_to_install = {"match": {"ipv4_src": '192.168.1.1/24'}}
+        flow_to_install = {"match": {"ipv4_src": '192.168.20.20'}}
         flow_list = {"flow_list": [stored_flow, stored_flow2]}
         command = "delete"
         self.napp.stored_flows = {dpid: flow_list}
 
         self.napp._store_changed_flows(command, flow_to_install, switch)
         mock_save_flow.assert_called()
-        self.assertEqual(len(self.napp.stored_flows[dpid]['flow_list']), 3)
+        expected_stored = {
+          "flow_list": [
+            {
+                "command": "add",
+                "flow": {
+                    "priority": 10,
+                    "cookie": 84114904,
+                    "match": {
+                        "ipv4_src": "192.168.2.1",
+                        "ipv4_dst": "192.168.0.2",
+                    },
+                    "actions": [],
+                },
+            },
+            {
+                "command": "add",
+                "flow": {
+                    "actions": [],
+                    "cookie": 4961162389751548787,
+                    "match": {"in_port": 2},
+                },
+            }
+          ]
+        }
+        self.assertDictEqual(self.napp.stored_flows[dpid], expected_stored)
 
     @patch('napps.kytos.flow_manager.main.Main._install_flows')
     @patch('napps.kytos.flow_manager.main.FlowFactory.get_class')
@@ -625,7 +776,7 @@ def test_no_strict_delete_of10(self, *args):
 
         self.napp._store_changed_flows(command, flow_to_install, switch)
         mock_save_flow.assert_called()
-        self.assertEqual(len(self.napp.stored_flows[dpid]['flow_list']), 1)
+        self.assertEqual(len(self.napp.stored_flows[dpid]['flow_list']), 0)
 
     @patch('napps.kytos.flow_manager.main.Main._install_flows')
     @patch('napps.kytos.flow_manager.main.FlowFactory.get_class')