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 <[email protected]> --- python/ovs/flow/kv.py | 25 ++++++++++++++++++------- python/ovs/flow/list.py | 7 ++++++- python/ovs/tests/test_kv.py | 20 ++++++++++---------- python/ovs/tests/test_ofp.py | 28 +++++++++++++++++++++++++++- 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 -- 2.37.3 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
