Merge pull request #151 from kytos-ng/feat/cookie_basic_validation

feat: basic validation for ``cookie`` and ``cookie_mask`` when installing or removing flows.

CHANGELOG.rst

@@ -13,6 +13,7 @@ Added
 - Added support for VLAN with mask. ``dl_vlan`` now also supports a string ``"vlan/mask"``.
 - Added two new fields to the collection ``flows``. ``owner`` has the name of the NApp that created the flow. ``table_group`` is the classification of a flow, for example: ``epl``, ``base`` and ``evpl``.
 - Added new script ``pipeline_related.py`` to add new fields ``owner`` and ``table_group`` to the flows on the collection ``flows`` on MongoDB
+- Added basic validation for ``cookie`` and ``cookie_mask`` when installing or removing flows.
 
 Changed
 =======

exceptions.py

@@ -5,6 +5,16 @@ class InvalidCommandError(Exception):
     """Command has an invalid value."""
 
 
+class FlowSerializerError(Exception):
+    """FlowSerializerError."""
+
+    def __init__(self, message, flow_dict=None):
+        """Constructor."""
+        self.message = message
+        self.flow_dict = flow_dict
+        super().__init__(message)
+
+
 class SwitchNotConnectedError(Exception):
     """Exception raised when a switch's connection isn't connected."""
 

main.py

@@ -29,7 +29,11 @@
 from .barrier_request import new_barrier_request
 from .controllers import FlowController
 from .db.models import FlowEntryState
-from .exceptions import InvalidCommandError, SwitchNotConnectedError
+from .exceptions import (
+    FlowSerializerError,
+    InvalidCommandError,
+    SwitchNotConnectedError,
+)
 from .settings import (
     CONN_ERR_MAX_RETRIES,
     CONN_ERR_MIN_WAIT,
@@ -50,6 +54,7 @@
     get_min_wait_diff,
     is_ignored,
     merge_cookie_ranges,
+    validate_cookies_and_masks,
 )
 
 
@@ -540,7 +545,17 @@ def handle_flows_install_delete(self, event):
             command = "delete"
         else:
             msg = f'Invalid event "{event.name}", should be install|delete'
-            raise ValueError(msg)
+            log.error(msg)
+            return
+
+        try:
+            validate_cookies_and_masks(flow_dict, command)
+        except ValueError as exc:
+            log.error(str(exc))
+            return
+        except TypeError as exc:
+            log.error(f"{str(exc)} for flow_dict {flow_dict} ")
+            return
 
         force = bool(event.content.get("force", False))
         switch = self.controller.get_switch_by_dpid(dpid)
@@ -600,6 +615,11 @@ def _send_flow_mods_from_request(
             result = "The request body doesn't have any flows"
             raise HTTPException(400, detail=result)
 
+        try:
+            validate_cookies_and_masks(flows, command)
+        except ValueError as exc:
+            raise HTTPException(400, detail=str(exc))
+
         force = bool(flows_dict.get("force", False))
         log.info(
             f"Send FlowMod from request dpid: {dpid}, command: {command}, "
@@ -631,6 +651,8 @@ def _send_flow_mods_from_request(
             raise HTTPException(424, detail=str(error))
         except PackException as error:
             raise HTTPException(400, detail=str(error))
+        except FlowSerializerError as error:
+            raise HTTPException(400, detail=str(error))
 
     def _install_flows(
         self,
@@ -656,7 +678,14 @@ def _install_flows(
             serializer = FlowFactory.get_class(switch, Flow04)
             flows_list = flows_dict.get("flows", [])
             for flow_dict in flows_list:
-                flow = serializer.from_dict(flow_dict, switch)
+                try:
+                    flow = serializer.from_dict(flow_dict, switch)
+                except (TypeError, KeyError) as exc:
+                    raise FlowSerializerError(
+                        f"It couldn't serialize flow_dict: {flow_dict}. "
+                        f"Exception type: {type(exc)} "
+                        f"Error: {str(exc)} "
+                    )
                 flow_mod = build_flow_mod_from_command(flow, command)
                 flow_mod.pack()
                 flow_mods.append(flow_mod)

tests/unit/test_main.py

@@ -231,16 +231,63 @@ async def test_rest_add_and_delete_without_dpid(
 
         assert mock_install_flows.call_count == 2
 
-    async def test_rest_add_pack_exc(self, event_loop):
+    @pytest.mark.parametrize("cookie", [27115650311270694912, "a", -1])
+    async def test_rest_add_pack_exc(self, cookie, event_loop):
         """Test add pack exception."""
         self.napp.controller.loop = event_loop
-        body = {"flows": [{"cookie": 27115650311270694912}]}
+        body = {"flows": [{"cookie": cookie}]}
         endpoint = f"{self.base_endpoint}/flows"
         response = await self.api_client.post(endpoint, json=body)
         assert response.status_code == 400
         data = response.json()
         assert "FlowMod.cookie" in data["description"]
 
+    async def test_rest_add_flow_serializer_type_error(self, event_loop):
+        """Test add flow serializer type error exception."""
+        self.napp.controller.loop = event_loop
+        body = {
+            "flows": [
+                {
+                    "priority": 101,
+                    "match": {"in_port": 1},
+                    "actions": [{"action_type": "output", "portx": 1}],
+                }
+            ],
+        }
+        endpoint = f"{self.base_endpoint}/flows"
+        response = await self.api_client.post(endpoint, json=body)
+        assert response.status_code == 400
+        data = response.json()
+        assert "portx" in data["description"]
+
+    async def test_rest_add_flow_serializer_key_error(self, event_loop):
+        """Test add flow serializer key error exception."""
+        self.napp.controller.loop = event_loop
+        body = {
+            "flows": [
+                {
+                    "priority": 101,
+                    "match": {"in_port": 1},
+                    "actions": [{"what": "1"}],
+                }
+            ],
+        }
+        endpoint = f"{self.base_endpoint}/flows"
+        response = await self.api_client.post(endpoint, json=body)
+        assert response.status_code == 400
+        data = response.json()
+        assert "what" in data["description"]
+
+    async def test_rest_del_missing_cookie_mask(self, event_loop):
+        """Test del missing cookie_mask."""
+        self.napp.controller.loop = event_loop
+        body = {"flows": [{"cookie": 0x64}]}
+        endpoint = f"{self.base_endpoint}/flows"
+        response = await self.api_client.request("DELETE", endpoint, json=body)
+        assert response.status_code == 400
+        data = response.json()
+        assert "cookie_mask should be set too" in data["description"]
+
     @patch("napps.kytos.flow_manager.main.Main._install_flows")
     async def test_rest_add_and_delete_with_dpi_fail(
         self, mock_install_flows, event_loop
@@ -464,15 +511,31 @@ def test_handle_flows_install_delete_fail(self, *args):
             content={},
         )
         self.napp.handle_flows_install_delete(event)
-        mock_log.error.assert_called()
+        assert mock_log.error.call_count == 1
 
         # invalid command
         event = get_kytos_event_mock(
             name="kytos.flow_manager.flows.xpto",
             content={"dpid": dpid, "flow_dict": mock_flow_dict},
         )
-        with pytest.raises(ValueError):
-            self.napp.handle_flows_install_delete(event)
+        self.napp.handle_flows_install_delete(event)
+        assert mock_log.error.call_count == 2
+
+        # missing cookie_mask
+        event = get_kytos_event_mock(
+            name="kytos.flow_manager.flows.delete",
+            content={"dpid": dpid, "flow_dict": [{"cookie": 1}]},
+        )
+        self.napp.handle_flows_install_delete(event)
+        assert mock_log.error.call_count == 3
+
+        # type error
+        event = get_kytos_event_mock(
+            name="kytos.flow_manager.flows.delete",
+            content={"dpid": dpid, "flow_dict": 1},
+        )
+        self.napp.handle_flows_install_delete(event)
+        assert mock_log.error.call_count == 4
 
         # install_flow exceptions
         event = get_kytos_event_mock(

tests/unit/test_utils.py

@@ -13,6 +13,8 @@
     build_flow_mod_from_command,
     get_min_wait_diff,
     merge_cookie_ranges,
+    validate_cookies_add,
+    validate_cookies_del,
 )
 from pyof.v0x04.controller2switch.flow_mod import FlowModCommand
 
@@ -26,9 +28,11 @@
         (FlowModCommand.OFPFC_MODIFY.value, str(FlowModCommand.OFPFC_MODIFY.value)),
     ],
 )
-def tet_build_command_from_flow_mod(value, expected):
+def test_build_command_from_flow_mod(value, expected):
     """Test build_command_from_flow_mod."""
-    assert build_command_from_flow_mod(value) == expected
+    mock = MagicMock()
+    mock.command.value = value
+    assert build_command_from_flow_mod(mock) == expected
 
 
 def test_build_flow_mod_from_command_exc():
@@ -131,6 +135,56 @@ def test_build_flow_mod_from_command(command, mock_method):
     assert getattr(mock, mock_method).call_count == 1
 
 
+@pytest.mark.parametrize(
+    "cookie,cookie_mask,should_raise",
+    [
+        (0x64, 0xFF, False),
+        (0x64, None, True),
+        (None, 0xFF, True),
+        (None, None, False),
+    ],
+)
+def test_validate_cookies_del(cookie, cookie_mask, should_raise):
+    """Test validate_cookies_del."""
+    flow = {}
+    if cookie:
+        flow["cookie"] = cookie
+    if cookie_mask:
+        flow["cookie_mask"] = cookie_mask
+
+    if should_raise:
+        with pytest.raises(ValueError) as exc:
+            validate_cookies_del([flow])
+        assert str(exc)
+    else:
+        assert not validate_cookies_del([flow])
+
+
+@pytest.mark.parametrize(
+    "cookie,cookie_mask,should_raise",
+    [
+        (0x64, 0xFF, True),
+        (0x64, None, False),
+        (None, 0xFF, True),
+        (None, None, False),
+    ],
+)
+def test_validate_cookies_add(cookie, cookie_mask, should_raise):
+    """Test validate_cookies_add."""
+    flow = {}
+    if cookie:
+        flow["cookie"] = cookie
+    if cookie_mask:
+        flow["cookie_mask"] = cookie_mask
+
+    if should_raise:
+        with pytest.raises(ValueError) as exc:
+            validate_cookies_add([flow])
+        assert str(exc)
+    else:
+        assert not validate_cookies_add([flow])
+
+
 class TestUtils(TestCase):
     """Test Utils."""
 

utils.py

@@ -119,14 +119,14 @@ def _valid_consistency_ignored(consistency_ignored_list):
             try:
                 _validate_range(consistency_ignored)
             except (TypeError, ValueError) as error:
-                log.warn(msg, error)
+                log.warning(msg, error)
                 return False
         elif not isinstance(consistency_ignored, (int, tuple)):
             error_msg = (
                 "The elements must be of class int or tuple"
                 f" but they are: {type(consistency_ignored)}"
             )
-            log.warn(msg, error_msg)
+            log.warning(msg, error_msg)
             return False
     return True
 
@@ -140,3 +140,38 @@ def get_min_wait_diff(datetime_t2, datetime_t1, min_wait):
     if min_wait_diff <= 0:
         return 0
     return min_wait_diff
+
+
+def validate_cookies_and_masks(flows: list[dict], command: str) -> None:
+    """Validate cookies and masks."""
+    validators = {"add": validate_cookies_add, "delete": validate_cookies_del}
+    if command in validators:
+        validators[command](flows)
+
+
+def validate_cookies_add(flows: list[dict]) -> None:
+    """Validate cookies add."""
+    for flow in flows:
+        if "cookie_mask" in flow:
+            raise ValueError(
+                f"cookie_mask shouldn't be set when adding flows. flow: {flow}"
+            )
+
+
+def validate_cookies_del(flows: list[dict]) -> None:
+    """Validate cookies del."""
+    for flow in flows:
+        has_cookie, has_mask = "cookie" in flow, "cookie_mask" in flow
+
+        if not has_cookie and not has_mask:
+            continue
+        if has_cookie and not has_mask:
+            raise ValueError(
+                "cookie is set, cookie_mask should be set too "
+                f"when deleting flows. flow: {flow}"
+            )
+        if not has_cookie and has_mask:
+            raise ValueError(
+                "cookie_mask is set, cookie should be set too"
+                f"when deleting flows. flow: {flow}"
+            )