On Mon, Oct 17, 2022 at 9:18 AM Adrian Moreno <[email protected]> wrote: > > We were silently relying on some ofp actions to be decoded by the > default decoder which would yield decent string values. > > In order to be more safe and robust, add an explicit decoder for all > missing actions. > > This patch also reworks the learn action decoding to make it more > explicit and verify all the fields specified in the learn action are > actually valid fields. > > Signed-off-by: Adrian Moreno <[email protected]> > --- > python/ovs/flow/kv.py | 13 +++--- > python/ovs/flow/ofp.py | 51 +++++++++++++++------- > python/ovs/flow/ofp_act.py | 85 +++++++++++++++++++++++++----------- > python/ovs/tests/test_ofp.py | 6 +-- > 4 files changed, 106 insertions(+), 49 deletions(-) > > diff --git a/python/ovs/flow/kv.py b/python/ovs/flow/kv.py > index cceb95e43..383d7ee78 100644 > --- a/python/ovs/flow/kv.py > +++ b/python/ovs/flow/kv.py > @@ -87,10 +87,11 @@ class KVDecoders(object): > > Args: > decoders (dict): Optional; A dictionary of decoders indexed by > keyword. > - default (callable): Optional; A decoder used 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. > + 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. > 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 > @@ -100,7 +101,7 @@ class KVDecoders(object): > > def __init__(self, decoders=None, default=None, default_free=None): > self._decoders = decoders or dict() > - self._default = default or decode_default > + self._default = default or (lambda k, v: (k, decode_default(v))) > self._default_free = default_free or self._default_free_decoder > > def decode(self, keyword, value_str): > @@ -126,7 +127,7 @@ class KVDecoders(object): > return keyword, value > else: > if value_str: > - return keyword, self._default(value_str) > + return self._default(keyword, value_str) > else: > return self._default_free(keyword) > > diff --git a/python/ovs/flow/ofp.py b/python/ovs/flow/ofp.py > index 0bc110c57..4ebb97df0 100644 > --- a/python/ovs/flow/ofp.py > +++ b/python/ovs/flow/ofp.py > @@ -243,6 +243,7 @@ class OFPFlow(Flow): > **OFPFlow._fw_action_decoders_args(), > **OFPFlow._control_action_decoders_args(), > **OFPFlow._other_action_decoders_args(), > + **OFPFlow._instruction_action_decoders_args(), > } > clone_actions = OFPFlow._clone_actions_decoders_args(actions) > actions.update(clone_actions) > @@ -255,6 +256,7 @@ class OFPFlow(Flow): > "output": decode_output, > "drop": decode_flag, > "controller": decode_controller, > + "CONTROLLER": decode_controller,
This is beyond the scope of this patch, but I was surprised to see that format_CONTROLLER in ofp-actions.c is the only action formatter that can output the function name as all-caps. Should this be changed to always output "controller" ? More of a general comment. Acked-by: Mike Pattrick <[email protected]> > "enqueue": nested_list_decoder( > ListDecoders([("port", decode_default), ("queue", int)]), > delims=[",", ":"], > @@ -272,6 +274,8 @@ class OFPFlow(Flow): > "pop_vlan": decode_flag, > "strip_vlan": decode_flag, > "push_vlan": decode_default, > + "pop_mpls": decode_int, > + "push_mpls": decode_int, > "decap": decode_flag, > "encap": decode_encap, > } > @@ -286,8 +290,8 @@ class OFPFlow(Flow): > "set_mpls_ttl", > "mod_nw_tos", > "mod_nw_ecn", > - "mod_tcp_src", > - "mod_tcp_dst", > + "mod_tp_src", > + "mod_tp_dst", > ] > return { > "load": decode_load_field, > @@ -299,9 +303,15 @@ class OFPFlow(Flow): > "mod_dl_src": EthMask, > "mod_nw_dst": IPMask, > "mod_nw_src": IPMask, > + "mod_nw_ttl": decode_int, > + "mod_vlan_vid": decode_int, > + "set_vlan_vid": decode_int, > + "mod_vlan_pcp": decode_int, > + "set_vlan_pcp": decode_int, > "dec_ttl": decode_dec_ttl, > "dec_mpls_ttl": decode_flag, > "dec_nsh_ttl": decode_flag, > + "delete_field": decode_field, > "check_pkt_larger": decode_chk_pkt_larger, > **{field: decode_default for field in field_default_decoders}, > } > @@ -342,6 +352,14 @@ class OFPFlow(Flow): > ) > ), > "ct_clear": decode_flag, > + "fin_timeout": nested_kv_decoder( > + KVDecoders( > + { > + "idle_timeout": decode_time, > + "hard_timeout": decode_time, > + } > + ) > + ), > } > > @staticmethod > @@ -382,22 +400,13 @@ class OFPFlow(Flow): > actions. > """ > return { > - "learn": decode_learn( > - { > - **action_decoders, > - "fin_timeout": nested_kv_decoder( > - KVDecoders( > - { > - "idle_timeout": decode_time, > - "hard_timeout": decode_time, > - } > - ) > - ), > - } > - ), > + "learn": decode_learn(action_decoders), > "clone": functools.partial( > decode_exec, KVDecoders(action_decoders) > ), > + "write_actions": functools.partial( > + decode_exec, KVDecoders(action_decoders) > + ), > } > > @staticmethod > @@ -426,3 +435,15 @@ class OFPFlow(Flow): > ) > ), > } > + > + @staticmethod > + def _instruction_action_decoders_args(): > + """Generate the decoder arguments for instruction actions > + (see man(7) ovs-actions).""" > + return { > + "meter": decode_int, > + "clear_actions": decode_flag, > + # write_actions moved to _clone actions > + "write_metadata": decode_mask(64), > + "goto_table": decode_int, > + } > diff --git a/python/ovs/flow/ofp_act.py b/python/ovs/flow/ofp_act.py > index acb16cd9a..c481d6fc7 100644 > --- a/python/ovs/flow/ofp_act.py > +++ b/python/ovs/flow/ofp_act.py > @@ -9,9 +9,15 @@ from ovs.flow.decoders import ( > decode_flag, > decode_int, > ) > -from ovs.flow.kv import nested_kv_decoder, KVDecoders, KeyValue, KVParser > +from ovs.flow.kv import ( > + nested_kv_decoder, > + KVDecoders, > + KeyValue, > + KVParser, > + ParseError, > +) > from ovs.flow.list import nested_list_decoder, ListDecoders > -from ovs.flow.ofp_fields import field_decoders > +from ovs.flow.ofp_fields import field_decoders, field_aliases > > > def decode_output(value): > @@ -20,7 +26,9 @@ def decode_output(value): > Does not support field specification. > """ > if len(value.split(",")) > 1: > - return nested_kv_decoder()(value) > + return nested_kv_decoder( > + KVDecoders({"port": decode_default, "max_len": decode_int}) > + )(value) > try: > return {"port": int(value)} > except ValueError: > @@ -41,7 +49,17 @@ def decode_controller(value): > except ValueError: > pass > # controller(key[=val], ...) > - return nested_kv_decoder()(value) > + return nested_kv_decoder( > + KVDecoders( > + { > + "max_len": decode_int, > + "reason": decode_default, > + "id": decode_int, > + "userdata": decode_default, > + "pause": decode_flag, > + } > + ) > + )(value) > > > def decode_bundle_load(value): > @@ -141,6 +159,12 @@ def decode_field(value): > man page: > http://www.openvswitch.org/support/dist-docs/ovs-actions.7.txt."""; > parts = value.strip("]\n\r").split("[") > + if ( > + parts[0] not in field_decoders.keys() > + and parts[0] not in field_aliases.keys() > + ): > + raise ParseError("Field not supported: {}".format(parts[0])) > + > result = { > "field": parts[0], > } > @@ -269,31 +293,36 @@ def decode_learn(action_decoders): > action decoding. > """ > > - def decode_learn_field(decoder, value): > - """Generates a decoder to be used for the 'field' argument of the > - 'learn' action. > - > - The field can hold a value that should be decoded, either as a field, > - or as a the value (see man(7) ovs-actions). > - > - Args: > - decoder (callable): The decoder. > + def learn_field_decoding_kv(key, value): > + """Decodes a key, value pair from the learn action. > + The key must be a decodable field. The value can be either a value > + in the format defined for the field or another field. > """ > - if value in field_decoders.keys(): > - # It's a field > - return value > - else: > - return decoder(value) > - > - learn_field_decoders = { > - field: functools.partial(decode_learn_field, decoder) > - for field, decoder in field_decoders.items() > - } > + key_field = decode_field(key) > + try: > + return key, decode_field(value) > + except ParseError: > + return key, field_decoders.get(key_field.get("field"))(value) > + > + def learn_field_decoding_free(key): > + """Decodes the free fields found in the learn action. > + Free fields indicate that the filed is to be copied from the > original. > + In order to express that in a dictionary, return the fieldspec as > + value. So, the free fild NXM_OF_IP_SRC[], is encoded as: > + "NXM_OF_IP_SRC[]": { > + "field": "NXM_OF_IP_SRC" > + } > + That way we also ensure the actual free key is correct. > + """ > + key_field = decode_field(key) > + return key, key_field > + > learn_decoders = { > **action_decoders, > - **learn_field_decoders, > "idle_timeout": decode_time, > "hard_timeout": decode_time, > + "fin_idle_timeout": decode_time, > + "fin_hard_timeout": decode_time, > "priority": decode_int, > "cookie": decode_int, > "send_flow_rem": decode_flag, > @@ -303,4 +332,10 @@ def decode_learn(action_decoders): > "result_dst": decode_field, > } > > - return functools.partial(decode_exec, KVDecoders(learn_decoders)) > + learn_decoder = KVDecoders( > + learn_decoders, > + default=learn_field_decoding_kv, > + default_free=learn_field_decoding_free, > + ) > + > + return functools.partial(decode_exec, learn_decoder) > diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py > index 7a93b2fd4..389c4544a 100644 > --- a/python/ovs/tests/test_ofp.py > +++ b/python/ovs/tests/test_ofp.py > @@ -331,12 +331,12 @@ from ovs.flow.decoders import EthMask, IPMask, > decode_mask > {"table": 69}, > {"delete_learned": True}, > {"cookie": 3664728752}, > - {"OXM_OF_METADATA[]": True}, > + {"OXM_OF_METADATA[]": {"field": "OXM_OF_METADATA"}}, > {"eth_type": 2048}, > - {"NXM_OF_IP_SRC[]": True}, > + {"NXM_OF_IP_SRC[]": {"field": "NXM_OF_IP_SRC"}}, > {"ip_dst": IPMask("172.30.204.105/32")}, > {"nw_proto": 6}, > - {"NXM_OF_TCP_SRC[]": "NXM_OF_TCP_DST[]"}, > + {"NXM_OF_TCP_SRC[]": {"field": "NXM_OF_TCP_DST"}}, > { > "load": { > "value": 1, > -- > 2.37.3 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
