python: Add explicit decoders for all ofp actions.

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 <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

4 files changed, 105 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..3d79ed6ad 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)
@@ -272,6 +273,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 +289,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 +302,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 +351,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 +399,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 +434,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,