python: Make key-value matching strict by default.

Currently, if a key is not found in the decoder information, we use the
default decoder which typically returns a string.

This not only means we can go out of sync with the C code without
noticing but it's also error prone as malformed flows could be parsed
without warning.

Make KeyValue parsing strict, raising an error if a decoder is not found
for a key.
This behaviour can be turned off globally by running 'KVDecoders.strict
= False' but it's generally not recommended. Also, if a KVDecoder does
need this default behavior, it can be explicitly configured specifying
it's default decoder.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Acked-by: Mike Pattrick <mkp@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

4 files changed, 61 insertions, 19 deletions

diff --git a/python/ovs/flow/kv.py b/python/ovs/flow/kv.py
index 383d7ee78..32463254b 100644
--- a/python/ovs/flow/kv.py
+++ b/python/ovs/flow/kv.py
@@ -85,13 +85,17 @@ class KVDecoders(object):
     reason, the default_free decoder, must return both the key and value to be
     stored.
 
+    Globally defined "strict" variable controls what to do when decoders do not
+    contain a valid decoder for a key and a default function is not provided.
+    If set to True (default), a ParseError is raised.
+    If set to False, the value will be decoded as a string.
+
     Args:
         decoders (dict): Optional; A dictionary of decoders indexed by keyword.
         default (callable): Optional; A function to use if a match is not
             found in configured decoders. If not provided, the default behavior
-            is to try to decode the value into an integer and, if that fails,
-            just return the string as-is. The function must accept a the key
-            and the value and return the decoded (key, value) tuple back.
+            depends on "strict". The function must accept a the key and a value
+            and return the decoded (key, value) tuple back.
         default_free (callable): Optional; The decoder used if a match is not
             found in configured decoders and it's a free value (e.g:
             a value without a key) Defaults to returning the free value as
@@ -99,9 +103,11 @@ class KVDecoders(object):
             The callable must accept a string and return a key-value pair.
     """
 
+    strict = True
+
     def __init__(self, decoders=None, default=None, default_free=None):
         self._decoders = decoders or dict()
-        self._default = default or (lambda k, v: (k, decode_default(v)))
+        self._default = default
         self._default_free = default_free or self._default_free_decoder
 
     def decode(self, keyword, value_str):
@@ -127,9 +133,14 @@ class KVDecoders(object):
             return keyword, value
         else:
             if value_str:
-                return self._default(keyword, value_str)
-            else:
-                return self._default_free(keyword)
+                if self._default:
+                    return self._default(keyword, value_str)
+                if self.strict:
+                    raise ParseError(
+                        "Cannot parse key {}: No decoder found".format(keyword)
+                    )
+                return keyword, decode_default(value_str)
+            return self._default_free(keyword)
 
     @staticmethod
     def _default_free_decoder(key):
diff --git a/python/ovs/flow/list.py b/python/ovs/flow/list.py
index b1e9e3fca..bc466ef89 100644
--- a/python/ovs/flow/list.py
+++ b/python/ovs/flow/list.py
@@ -31,7 +31,12 @@ class ListDecoders(object):
             value_str (str): The value string to decode.
         """
         if index < 0 or index >= len(self._decoders):
-            return self._default_decoder(index, value_str)
+            if self._default_decoder:
+                return self._default_decoder(index, value_str)
+            else:
+                raise ParseError(
+                    f"Cannot decode element {index} in list: {value_str}"
+                )
 
         try:
             key = self._decoders[index][0]
diff --git a/python/ovs/tests/test_kv.py b/python/ovs/tests/test_kv.py
index c5b66de88..76887498a 100644
--- a/python/ovs/tests/test_kv.py
+++ b/python/ovs/tests/test_kv.py
@@ -1,6 +1,9 @@
 import pytest
 
-from ovs.flow.kv import KVParser, KeyValue
+from ovs.flow.kv import KVParser, KVDecoders, KeyValue
+from ovs.flow.decoders import decode_default
+
+decoders = KVDecoders(default=lambda k, v: (k, decode_default(v)))
 
 
 @pytest.mark.parametrize(
@@ -9,7 +12,7 @@ from ovs.flow.kv import KVParser, KeyValue
         (
             (
                 "cookie=0x0, duration=147566.365s, table=0, n_packets=39, n_bytes=2574, idle_age=65534, hard_age=65534",  # noqa: E501
-                None,
+                decoders,
             ),
             [
                 KeyValue("cookie", 0),
@@ -24,7 +27,7 @@ from ovs.flow.kv import KVParser, KeyValue
         (
             (
                 "load:0x4->NXM_NX_REG13[],load:0x9->NXM_NX_REG11[],load:0x8->NXM_NX_REG12[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],mod_dl_src:0a:58:a9:fe:00:02,resubmit(,8)",  # noqa: E501
-                None,
+                decoders,
             ),
             [
                 KeyValue("load", "0x4->NXM_NX_REG13[]"),
@@ -36,20 +39,17 @@ from ovs.flow.kv import KVParser, KeyValue
                 KeyValue("resubmit", ",8"),
             ],
         ),
+        (("l1(l2(l3(l4())))", decoders), [KeyValue("l1", "l2(l3(l4()))")]),
         (
-            ("l1(l2(l3(l4())))", None),
-            [KeyValue("l1", "l2(l3(l4()))")]
-        ),
-        (
-            ("l1(l2(l3(l4()))),foo:bar", None),
+            ("l1(l2(l3(l4()))),foo:bar", decoders),
             [KeyValue("l1", "l2(l3(l4()))"), KeyValue("foo", "bar")],
         ),
         (
-            ("enqueue:1:2,output=2", None),
+            ("enqueue:1:2,output=2", decoders),
             [KeyValue("enqueue", "1:2"), KeyValue("output", 2)],
         ),
         (
-            ("value_to_reg(100)->someReg[10],foo:bar", None),
+            ("value_to_reg(100)->someReg[10],foo:bar", decoders),
             [
                 KeyValue("value_to_reg", "(100)->someReg[10]"),
                 KeyValue("foo", "bar"),
diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py
index 389c4544a..328ab7285 100644
--- a/python/ovs/tests/test_ofp.py
+++ b/python/ovs/tests/test_ofp.py
@@ -2,7 +2,7 @@ import netaddr
 import pytest
 
 from ovs.flow.ofp import OFPFlow
-from ovs.flow.kv import KeyValue
+from ovs.flow.kv import KeyValue, ParseError
 from ovs.flow.decoders import EthMask, IPMask, decode_mask
 
 
@@ -509,11 +509,37 @@ from ovs.flow.decoders import EthMask, IPMask, decode_mask
                 ),
             ],
         ),
+        (
+            "actions=doesnotexist(1234)",
+            ParseError,
+        ),
+        (
+            "actions=learn(eth_type=nofield)",
+            ParseError,
+        ),
+        (
+            "actions=learn(nofield=eth_type)",
+            ParseError,
+        ),
+        (
+            "nofield=0x123 actions=drop",
+            ParseError,
+        ),
+        (
+            "actions=load:0x12334->NOFILED",
+            ParseError,
+        ),
     ],
 )
 def test_act(input_string, expected):
+    if isinstance(expected, type):
+        with pytest.raises(expected):
+            ofp = OFPFlow(input_string)
+        return
+
     ofp = OFPFlow(input_string)
     actions = ofp.actions_kv
+
     for i in range(len(expected)):
         assert expected[i].key == actions[i].key
         assert expected[i].value == actions[i].value