From f008ad7d135d6fb1d02347516585d45bb1d2303f Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Mon, 8 Nov 2021 11:53:24 -0300 Subject: [PATCH 01/10] Augmented _add_flow_store to overwrite overlapping flows --- main.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/main.py b/main.py index 7d0c9268..733bc4af 100644 --- a/main.py +++ b/main.py @@ -315,9 +315,8 @@ def _del_matched_flows_store(self, flow_dict, switch): del stored_flows_box["id"] self.stored_flows = deepcopy(stored_flows_box) - # pylint: disable=fixme def _add_flow_store(self, flow_dict, switch): - """Try to add a flow dict in the store.""" + """Try to add a flow dict in the store idempotently.""" installed_flow = {} installed_flow["flow"] = flow_dict installed_flow["created_at"] = now().strftime("%Y-%m-%dT%H:%M:%S") @@ -327,11 +326,23 @@ def _add_flow_store(self, flow_dict, switch): if switch.id not in stored_flows_box: stored_flows_box[switch.id] = OrderedDict() - # TODO handle issue 23 (overlapping FlowMod add) if not stored_flows_box[switch.id].get(cookie): stored_flows_box[switch.id][cookie] = [installed_flow] else: - stored_flows_box[switch.id][cookie].append(installed_flow) + version = switch.connection.protocol.version + stored_flows = stored_flows_box[switch.id].get(cookie, []) + for i in range(len(stored_flows)): + if all( + ( + stored_flows[i]["flow"].get("priority", 0) + == flow_dict.get("priority", 0), + match_flow(flow_dict, version, stored_flows[i]["flow"]), + ) + ): + stored_flows_box[switch.id][cookie][i] = installed_flow + break + else: + stored_flows_box[switch.id][cookie].append(installed_flow) stored_flows_box["id"] = "flow_persistence" self.storehouse.save_flow(stored_flows_box) From e9fd7f807679d155d48b0ca8028f566180c94e20 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Mon, 8 Nov 2021 11:54:16 -0300 Subject: [PATCH 02/10] Added unit test for overlapping flows --- tests/unit/test_main.py | 81 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index 0addd547..25f476dd 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -405,6 +405,87 @@ def test_check_switch_consistency_add(self, *args): self.napp.check_switch_consistency(switch) mock_install_flows.assert_called() + @patch("napps.kytos.flow_manager.storehouse.StoreHouse.save_flow") + def test_add_overlapping_flow(self, *args): + """Test add an overlapping flow.""" + dpid = "00:00:00:00:00:00:00:01" + switch = get_switch_mock(dpid, 0x04) + switch.id = dpid + cookie = 0x20 + + self.napp.stored_flows = { + dpid: { + cookie: [ + { + "flow": { + "priority": 10, + "cookie": 84114904, + "match": { + "ipv4_src": "192.168.1.1", + "ipv4_dst": "192.168.0.2", + }, + "actions": [{"action_type": "output", "port": 2}], + } + } + ] + } + } + + new_actions = [{"action_type": "output", "port": 3}] + flow_dict = { + "priority": 10, + "cookie": cookie, + "match": { + "ipv4_src": "192.168.1.1", + "ipv4_dst": "192.168.0.2", + }, + "actions": new_actions, + } + + self.napp._add_flow_store(flow_dict, switch) + assert len(self.napp.stored_flows[dpid]) == 1 + assert self.napp.stored_flows[dpid][0x20][0]["flow"]["actions"] == new_actions + + @patch("napps.kytos.flow_manager.storehouse.StoreHouse.save_flow") + def test_add_overlapping_flow_diff_priority(self, *args): + """Test that a different priority wouldn't overlap.""" + dpid = "00:00:00:00:00:00:00:01" + switch = get_switch_mock(dpid, 0x04) + switch.id = dpid + + cookie = 0x20 + self.napp.stored_flows = { + dpid: { + cookie: [ + { + "flow": { + "priority": 10, + "cookie": 84114904, + "match": { + "ipv4_src": "192.168.1.1", + "ipv4_dst": "192.168.0.2", + }, + "actions": [{"action_type": "output", "port": 2}], + } + } + ] + } + } + + new_actions = [{"action_type": "output", "port": 3}] + flow_dict = { + "priority": 11, + "cookie": cookie, + "match": { + "ipv4_src": "192.168.1.1", + "ipv4_dst": "192.168.0.2", + }, + "actions": new_actions, + } + + self.napp._add_flow_store(flow_dict, switch) + assert len(self.napp.stored_flows[dpid][cookie]) == 2 + @patch("napps.kytos.flow_manager.main.Main._install_flows") @patch("napps.kytos.flow_manager.main.FlowFactory.get_class") def test_check_switch_flow_not_missing(self, *args): From ef57e4254c51a6b171872c96cedd788fc9f4d41e Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Mon, 8 Nov 2021 12:03:45 -0300 Subject: [PATCH 03/10] Bumped 5.1.1 --- CHANGELOG.rst | 7 +++++++ kytos.json | 2 +- setup.py | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4140d722..b2008dbd 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -21,6 +21,13 @@ Removed Security ======== +[5.1.1] - 2021-11.08 +******************** + +Added +===== +- Augmented ``_add_flow_store`` to overwrite overlapping flows + [5.1.0] - 2021-11.05 ******************** diff --git a/kytos.json b/kytos.json index ad0072ec..3f264733 100644 --- a/kytos.json +++ b/kytos.json @@ -3,7 +3,7 @@ "username": "kytos", "name": "flow_manager", "description": "Manage switches' flows through a REST API.", - "version": "5.1.0", + "version": "5.1.1", "napp_dependencies": ["kytos/of_core", "kytos/storehouse"], "license": "MIT", "url": "https://github.com/kytos/flow_manager.git", diff --git a/setup.py b/setup.py index 2032e703..f1a653db 100644 --- a/setup.py +++ b/setup.py @@ -24,7 +24,7 @@ BASE_ENV = Path(os.environ.get("VIRTUAL_ENV", "/")) NAPP_NAME = "flow_manager" -NAPP_VERSION = "5.1.0" +NAPP_VERSION = "5.1.1" # Kytos var folder VAR_PATH = BASE_ENV / "var" / "lib" / "kytos" From fdc340df9989e70587e881222b346a080e4fad46 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Mon, 8 Nov 2021 12:24:07 -0300 Subject: [PATCH 04/10] Linter fixes --- main.py | 2 +- tests/unit/test_main.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/main.py b/main.py index 733bc4af..ad3464b2 100644 --- a/main.py +++ b/main.py @@ -331,7 +331,7 @@ def _add_flow_store(self, flow_dict, switch): else: version = switch.connection.protocol.version stored_flows = stored_flows_box[switch.id].get(cookie, []) - for i in range(len(stored_flows)): + for i, _ in enumerate(stored_flows): if all( ( stored_flows[i]["flow"].get("priority", 0) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index 25f476dd..9b2aaae4 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -2,6 +2,9 @@ from unittest import TestCase from unittest.mock import MagicMock, patch +# pylint: disable=too-many-lines,fixme +# TODO split this test suite in smaller ones + from kytos.core.helpers import now from kytos.lib.helpers import ( get_connection_mock, @@ -408,6 +411,7 @@ def test_check_switch_consistency_add(self, *args): @patch("napps.kytos.flow_manager.storehouse.StoreHouse.save_flow") def test_add_overlapping_flow(self, *args): """Test add an overlapping flow.""" + (_,) = args dpid = "00:00:00:00:00:00:00:01" switch = get_switch_mock(dpid, 0x04) switch.id = dpid @@ -449,6 +453,7 @@ def test_add_overlapping_flow(self, *args): @patch("napps.kytos.flow_manager.storehouse.StoreHouse.save_flow") def test_add_overlapping_flow_diff_priority(self, *args): """Test that a different priority wouldn't overlap.""" + (_,) = args dpid = "00:00:00:00:00:00:00:01" switch = get_switch_mock(dpid, 0x04) switch.id = dpid From 5b667e9290a10ed4c95f2e991570a84f99defb12 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Mon, 29 Nov 2021 13:56:46 -0300 Subject: [PATCH 05/10] Enhanced enumerate usage --- main.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/main.py b/main.py index 1a2e98f2..f7645d46 100644 --- a/main.py +++ b/main.py @@ -330,12 +330,12 @@ def _add_flow_store(self, flow_dict, switch): else: version = switch.connection.protocol.version stored_flows = stored_flows_box[switch.id].get(cookie, []) - for i, _ in enumerate(stored_flows): + for i, stored_flow in enumerate(stored_flows): if all( ( - stored_flows[i]["flow"].get("priority", 0) + stored_flow["flow"].get("priority", 0) == flow_dict.get("priority", 0), - match_flow(flow_dict, version, stored_flows[i]["flow"]), + match_flow(flow_dict, version, stored_flow["flow"]), ) ): stored_flows_box[switch.id][cookie][i] = installed_flow From 32c9e5f1598bfb5d4c2e55aac0c4a7d251181753 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Thu, 2 Dec 2021 11:14:41 -0300 Subject: [PATCH 06/10] Added match13_strict --- match.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/match.py b/match.py index 71aa97bf..f86cce96 100644 --- a/match.py +++ b/match.py @@ -131,3 +131,36 @@ def match13_no_strict(flow_to_install, stored_flow_dict): return False return stored_flow_dict + + +def match13_strict(flow_to_install, stored_flow_dict): + """Match a flow strictly (OF1.3). + + Return the flow if all fields match, otherwise, return False. + """ + cookie = flow_to_install.get("cookie", 0) & flow_to_install.get("cookie_mask", 0) + cookie_stored = stored_flow_dict.get("cookie", 0) & flow_to_install.get( + "cookie_mask", 0 + ) + if cookie and cookie != cookie_stored: + return False + if flow_to_install.get("priority", 0) != stored_flow_dict.get("priority", 0): + return False + + if "match" not in flow_to_install and "match" not in stored_flow_dict: + return stored_flow_dict + if "match" not in flow_to_install and "match" in stored_flow_dict: + return False + if "match" in flow_to_install and "match" not in stored_flow_dict: + return False + + if len(flow_to_install["match"]) != len(stored_flow_dict["match"]): + return False + + for key, value in flow_to_install.get("match").items(): + if key not in stored_flow_dict["match"]: + return False + if value != stored_flow_dict["match"].get(key): + return False + + return stored_flow_dict From 59b155f7d9c87e3028b777aa38dcc6d28164cc42 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Thu, 2 Dec 2021 11:54:01 -0300 Subject: [PATCH 07/10] Added unit test test_add_overlapping_flow_multiple_stored --- tests/unit/test_main.py | 67 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index 9b2aaae4..2f92843e 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -450,6 +450,73 @@ def test_add_overlapping_flow(self, *args): assert len(self.napp.stored_flows[dpid]) == 1 assert self.napp.stored_flows[dpid][0x20][0]["flow"]["actions"] == new_actions + @patch("napps.kytos.flow_manager.storehouse.StoreHouse.save_flow") + def test_add_overlapping_flow_multiple_stored(self, *args): + """Test add an overlapping flow with multiple flows stored.""" + (_,) = args + dpid = "00:00:00:00:00:00:00:01" + switch = get_switch_mock(dpid, 0x04) + switch.id = dpid + cookie = 0x20 + + stored_flows_list = [ + { + "flow": { + "actions": [{"action_type": "output", "port": 2}], + "match": {"dl_vlan": 100, "in_port": 1}, + "priority": 10, + }, + }, + { + "flow": { + "actions": [{"action_type": "output", "port": 3}], + "match": {"dl_vlan": 200, "in_port": 1}, + "priority": 10, + }, + }, + { + "flow": { + "actions": [{"action_type": "output", "port": 4}], + "match": {"dl_vlan": 300, "in_port": 1}, + "priority": 10, + }, + }, + { + "flow": { + "actions": [{"action_type": "output", "port": 4}], + "match": {"in_port": 1}, + "priority": 10, + }, + }, + ] + + self.napp.stored_flows = {dpid: {cookie: list(stored_flows_list)}} + + new_actions = [{"action_type": "output", "port": 3}] + overlapping_flow = { + "priority": 10, + "cookie": cookie, + "match": { + "in_port": 1, + }, + "actions": new_actions, + } + + self.napp._add_flow_store(overlapping_flow, switch) + assert len(self.napp.stored_flows[dpid][cookie]) == len(stored_flows_list) + + # only the last flow is expected to be strictly matched + self.assertDictEqual( + self.napp.stored_flows[dpid][cookie][len(stored_flows_list) - 1]["flow"], + overlapping_flow, + ) + + # all flows except the last one should still be the same + for i in range(0, len(stored_flows_list) - 1): + self.assertDictEqual( + self.napp.stored_flows[dpid][cookie][i], stored_flows_list[i] + ) + @patch("napps.kytos.flow_manager.storehouse.StoreHouse.save_flow") def test_add_overlapping_flow_diff_priority(self, *args): """Test that a different priority wouldn't overlap.""" From 24762459e50d88db2ad3039c29f0a35c23415cf8 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Thu, 2 Dec 2021 11:54:43 -0300 Subject: [PATCH 08/10] Replaced add flows call site to use match_strict_flow --- main.py | 4 ++-- match.py | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/main.py b/main.py index f7645d46..98673f71 100644 --- a/main.py +++ b/main.py @@ -7,7 +7,7 @@ from threading import Lock from flask import jsonify, request -from napps.kytos.flow_manager.match import match_flow +from napps.kytos.flow_manager.match import match_flow, match_strict_flow from napps.kytos.flow_manager.storehouse import StoreHouse from napps.kytos.of_core.flow import FlowFactory from napps.kytos.of_core.settings import STATS_INTERVAL @@ -335,7 +335,7 @@ def _add_flow_store(self, flow_dict, switch): ( stored_flow["flow"].get("priority", 0) == flow_dict.get("priority", 0), - match_flow(flow_dict, version, stored_flow["flow"]), + match_strict_flow(flow_dict, version, stored_flow["flow"]), ) ): stored_flows_box[switch.id][cookie][i] = installed_flow diff --git a/match.py b/match.py index f86cce96..0988e9e1 100644 --- a/match.py +++ b/match.py @@ -21,6 +21,17 @@ def match_flow(flow_to_install, version, stored_flow_dict): raise NotImplementedError(f"Unsupported OpenFlow version {version}") +def match_strict_flow(flow_to_install, version, stored_flow_dict) -> None: + """Match the flow strictly. + + It has support for only for (OF 1.3) flows. + If all fields match, return the flow, otherwise return False. + """ + if version != 0x04: + raise NotImplementedError(f"Unsupported OpenFlow version {version}") + return match13_strict(flow_to_install, stored_flow_dict) + + def _get_match_fields(flow_dict): """Generate match fields.""" match_fields = {} From b8fc4e55409292acdbd6342b0656beeb3c289dfe Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Thu, 2 Dec 2021 12:22:53 -0300 Subject: [PATCH 09/10] Extracted cookie match in a func --- match.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/match.py b/match.py index 0988e9e1..bb81fc8c 100644 --- a/match.py +++ b/match.py @@ -116,17 +116,24 @@ def match10_no_strict(flow_dict, args): return flow_dict -def match13_no_strict(flow_to_install, stored_flow_dict): - """Match a flow that is either exact or more specific (non-strict) (OF1.3). - - Return the flow if any fields match, otherwise, return False. - """ +def _match_cookie(flow_to_install, stored_flow_dict): + """Check if a the cookie and its mask matches between the flows.""" cookie = flow_to_install.get("cookie", 0) & flow_to_install.get("cookie_mask", 0) cookie_stored = stored_flow_dict.get("cookie", 0) & flow_to_install.get( "cookie_mask", 0 ) if cookie and cookie != cookie_stored: return False + return True + + +def match13_no_strict(flow_to_install, stored_flow_dict): + """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 not _match_cookie(flow_to_install, stored_flow_dict): + return False if "match" not in flow_to_install or "match" not in stored_flow_dict: return stored_flow_dict @@ -149,11 +156,7 @@ def match13_strict(flow_to_install, stored_flow_dict): Return the flow if all fields match, otherwise, return False. """ - cookie = flow_to_install.get("cookie", 0) & flow_to_install.get("cookie_mask", 0) - cookie_stored = stored_flow_dict.get("cookie", 0) & flow_to_install.get( - "cookie_mask", 0 - ) - if cookie and cookie != cookie_stored: + if not _match_cookie(flow_to_install, stored_flow_dict): return False if flow_to_install.get("priority", 0) != stored_flow_dict.get("priority", 0): return False From 2fe09ea1367576346447037d7a04317ac11be760 Mon Sep 17 00:00:00 2001 From: Vinicius Arcanjo Date: Thu, 2 Dec 2021 12:34:39 -0300 Subject: [PATCH 10/10] Extracted _match_keys in a func --- match.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/match.py b/match.py index bb81fc8c..2cc84f05 100644 --- a/match.py +++ b/match.py @@ -127,6 +127,16 @@ def _match_cookie(flow_to_install, stored_flow_dict): return True +def _match_keys(flow_to_install, stored_flow_dict, flow_to_install_keys): + """Check if certain keys on flow_to_install match on stored_flow_dict.""" + for key in flow_to_install_keys: + if key not in stored_flow_dict["match"]: + return False + if flow_to_install["match"][key] != stored_flow_dict["match"].get(key): + return False + return True + + def match13_no_strict(flow_to_install, stored_flow_dict): """Match a flow that is either exact or more specific (non-strict) (OF1.3). @@ -142,12 +152,10 @@ def match13_no_strict(flow_to_install, 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 key not in stored_flow_dict["match"]: - return False - if value != stored_flow_dict["match"].get(key): - return False - + if not _match_keys( + flow_to_install, stored_flow_dict, flow_to_install["match"].keys() + ): + return False return stored_flow_dict @@ -171,10 +179,8 @@ def match13_strict(flow_to_install, 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 key not in stored_flow_dict["match"]: - return False - if value != stored_flow_dict["match"].get(key): - return False - + if not _match_keys( + flow_to_install, stored_flow_dict, flow_to_install["match"].keys() + ): + return False return stored_flow_dict