diff mbox series

[ovs-dev,v2,07/18] python: introduce OpenFlow Flow parsing

Message ID 20220128160441.23477-8-amorenoz@redhat.com
State Superseded
Headers show
Series python: add flow parsing library | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail

Commit Message

Adrian Moreno Jan. 28, 2022, 4:04 p.m. UTC
Introduce OFPFlow class and all its decoders.

Most of the decoders are generic (from decoders.py). Some have special
syntax and need a specific implementation.

Decoders for nat are moved to the common decoders.py because it's syntax
is shared with other types of flows (e.g: dpif flows).

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 python/automake.mk           |   2 +
 python/ovs/flows/decoders.py | 108 +++++++++
 python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
 python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
 4 files changed, 806 insertions(+)
 create mode 100644 python/ovs/flows/ofp.py
 create mode 100644 python/ovs/flows/ofp_act.py

Comments

0-day Robot Jan. 28, 2022, 5:30 p.m. UTC | #1
Bleep bloop.  Greetings Adrian Moreno, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: corrupt patch at line 147
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 python: introduce OpenFlow Flow parsing
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ilya Maximets Jan. 28, 2022, 9:18 p.m. UTC | #2
On 1/28/22 17:04, Adrian Moreno wrote:
> Introduce OFPFlow class and all its decoders.
> 
> Most of the decoders are generic (from decoders.py). Some have special
> syntax and need a specific implementation.
> 
> Decoders for nat are moved to the common decoders.py because it's syntax
> is shared with other types of flows (e.g: dpif flows).
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  python/automake.mk           |   2 +
>  python/ovs/flows/decoders.py | 108 +++++++++
>  python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>  python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>  4 files changed, 806 insertions(+)
>  create mode 100644 python/ovs/flows/ofp.py
>  create mode 100644 python/ovs/flows/ofp_act.py
> 
> diff --git a/python/automake.mk b/python/automake.mk
> index b7debfbd9..7b6d6596f 100644
> --- a/python/automake.mk
> +++ b/python/automake.mk
> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>  	python/ovs/flows/flow.py \
>  	python/ovs/flows/kv.py \
>  	python/ovs/flows/list.py \
> +	python/ovs/flows/ofp.py \
> +	python/ovs/flows/ofp_act.py \
>  	python/ovs/json.py \
>  	python/ovs/jsonrpc.py \
>  	python/ovs/ovsuuid.py \
> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
> index 2f8e5bd0a..1462b0b9d 100644
> --- a/python/ovs/flows/decoders.py
> +++ b/python/ovs/flows/decoders.py
> @@ -6,6 +6,7 @@ object.
>  """
>  
>  import netaddr
> +import re
>  
>  
>  class Decoder(object):
> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>  
>      def to_json(self):
>          return str(self)
> +
> +
> +def decode_free_output(value):
> +    """The value of the output action can be found free, i.e: without the
> +    'output' keyword. This decoder decodes its value when found this way."""
> +    try:
> +        return "output", {"port": int(value)}
> +    except ValueError:
> +        return "output", {"port": value.strip('"')}
> +
> +
> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
> +"""
> +The following IPv6 regexp is a modified version of the one in:
> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
> +
> +It matches all these types of ipv6 addresses:
> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
> +fe80:0:0:0:204:61ff:fe9d:f156
> +fe80::204:61ff:fe9d:f156
> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
> +fe80:0:0:0:0204:61ff:254.157.241.86
> +fe80::204:61ff:254.157.241.86
> +::1
> +2001::
> +fe80::
> +"""
> +ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4
 ]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501

This line seems to be corrupted the patch in patchwork.

In any case, can we not have this line in the patch?
It doesn't look maintainable.

Can we use something like 'ipaddress' module to parse
ip addresses instead?

Bets regards, Ilya Maximets.
Adrian Moreno Jan. 31, 2022, 6:20 a.m. UTC | #3
On 1/28/22 22:18, Ilya Maximets wrote:
> On 1/28/22 17:04, Adrian Moreno wrote:
>> Introduce OFPFlow class and all its decoders.
>>
>> Most of the decoders are generic (from decoders.py). Some have special
>> syntax and need a specific implementation.
>>
>> Decoders for nat are moved to the common decoders.py because it's syntax
>> is shared with other types of flows (e.g: dpif flows).
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>   python/automake.mk           |   2 +
>>   python/ovs/flows/decoders.py | 108 +++++++++
>>   python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>>   python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>>   4 files changed, 806 insertions(+)
>>   create mode 100644 python/ovs/flows/ofp.py
>>   create mode 100644 python/ovs/flows/ofp_act.py
>>
>> diff --git a/python/automake.mk b/python/automake.mk
>> index b7debfbd9..7b6d6596f 100644
>> --- a/python/automake.mk
>> +++ b/python/automake.mk
>> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>>   	python/ovs/flows/flow.py \
>>   	python/ovs/flows/kv.py \
>>   	python/ovs/flows/list.py \
>> +	python/ovs/flows/ofp.py \
>> +	python/ovs/flows/ofp_act.py \
>>   	python/ovs/json.py \
>>   	python/ovs/jsonrpc.py \
>>   	python/ovs/ovsuuid.py \
>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>> index 2f8e5bd0a..1462b0b9d 100644
>> --- a/python/ovs/flows/decoders.py
>> +++ b/python/ovs/flows/decoders.py
>> @@ -6,6 +6,7 @@ object.
>>   """
>>   
>>   import netaddr
>> +import re
>>   
>>   
>>   class Decoder(object):
>> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>>   
>>       def to_json(self):
>>           return str(self)
>> +
>> +
>> +def decode_free_output(value):
>> +    """The value of the output action can be found free, i.e: without the
>> +    'output' keyword. This decoder decodes its value when found this way."""
>> +    try:
>> +        return "output", {"port": int(value)}
>> +    except ValueError:
>> +        return "output", {"port": value.strip('"')}
>> +
>> +
>> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
>> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
>> +"""
>> +The following IPv6 regexp is a modified version of the one in:
>> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
>> +
>> +It matches all these types of ipv6 addresses:
>> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
>> +fe80:0:0:0:204:61ff:fe9d:f156
>> +fe80::204:61ff:fe9d:f156
>> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
>> +fe80:0:0:0:0204:61ff:254.157.241.86
>> +fe80::204:61ff:254.157.241.86
>> +::1
>> +2001::
>> +fe80::
>> +"""
>> +ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501
> 
> This line seems to be corrupted the patch in patchwork.
> 
> In any case, can we not have this line in the patch?
> It doesn't look maintainable.
> 
> Can we use something like 'ipaddress' module to parse
> ip addresses instead?
> 

I agree having this huge blob is not very maintainable, maybe we can roll back 
to the previous, more simplistic, regexp.

The goal of this regexp is not to parse the address but to extract it from the 
ip/port range where we can have [ip_start:ip_end]:port_start:port_end. After the 
substrings are extracted we use 'netaddr' module to extract it.


> Bets regards, Ilya Maximets.
>
James Troup Jan. 31, 2022, 9:28 p.m. UTC | #4
Adrian Moreno <amorenoz@redhat.com> writes:

> diff --git a/python/ovs/flows/ofp.py b/python/ovs/flows/ofp.py
> new file mode 100644
> index 000000000..1af06fa01
> --- /dev/null
> +++ b/python/ovs/flows/ofp.py
> @@ -0,0 +1,453 @@

[...]

> +
> +class OFPFlow(Flow):

[...]

> +    def __init__(self, ofp_string, id=None):
> +        """Create a OFPFlow from a flow string.
> +
> +        The string is expected to have the follwoing format:

following


[...]

> +    @staticmethod
> +    def _field_decoder_args():
> +        """Returns the KVDecoder arguments needed to decode match fields
> +        decoding match fields.

Huh?

> diff --git a/python/ovs/flows/ofp_act.py b/python/ovs/flows/ofp_act.py
> new file mode 100644
> index 000000000..9e04445ba
> --- /dev/null
> +++ b/python/ovs/flows/ofp_act.py
> @@ -0,0 +1,243 @@
> +""" Defines decoders for openflow actions.

Leading whitespace, and 'OpenFlow' for consistency?


[...]

> +def decode_learn(action_decoders):

[...]

> +    For this we need to create a wrapper of field_decoders that, for each
> +    "field=X" key-value we check if X is a field_name or if it's acually

actually
Mark Michelson Feb. 5, 2022, 2:55 a.m. UTC | #5
On 1/31/22 01:20, Adrian Moreno wrote:
> 
> 
> On 1/28/22 22:18, Ilya Maximets wrote:
>> On 1/28/22 17:04, Adrian Moreno wrote:
>>> Introduce OFPFlow class and all its decoders.
>>>
>>> Most of the decoders are generic (from decoders.py). Some have special
>>> syntax and need a specific implementation.
>>>
>>> Decoders for nat are moved to the common decoders.py because it's syntax
>>> is shared with other types of flows (e.g: dpif flows).
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>   python/automake.mk           |   2 +
>>>   python/ovs/flows/decoders.py | 108 +++++++++
>>>   python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>>>   python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>>>   4 files changed, 806 insertions(+)
>>>   create mode 100644 python/ovs/flows/ofp.py
>>>   create mode 100644 python/ovs/flows/ofp_act.py
>>>
>>> diff --git a/python/automake.mk b/python/automake.mk
>>> index b7debfbd9..7b6d6596f 100644
>>> --- a/python/automake.mk
>>> +++ b/python/automake.mk
>>> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>>>       python/ovs/flows/flow.py \
>>>       python/ovs/flows/kv.py \
>>>       python/ovs/flows/list.py \
>>> +    python/ovs/flows/ofp.py \
>>> +    python/ovs/flows/ofp_act.py \
>>>       python/ovs/json.py \
>>>       python/ovs/jsonrpc.py \
>>>       python/ovs/ovsuuid.py \
>>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>>> index 2f8e5bd0a..1462b0b9d 100644
>>> --- a/python/ovs/flows/decoders.py
>>> +++ b/python/ovs/flows/decoders.py
>>> @@ -6,6 +6,7 @@ object.
>>>   """
>>>   import netaddr
>>> +import re
>>>   class Decoder(object):
>>> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>>>       def to_json(self):
>>>           return str(self)
>>> +
>>> +
>>> +def decode_free_output(value):
>>> +    """The value of the output action can be found free, i.e: 
>>> without the
>>> +    'output' keyword. This decoder decodes its value when found this 
>>> way."""
>>> +    try:
>>> +        return "output", {"port": int(value)}
>>> +    except ValueError:
>>> +        return "output", {"port": value.strip('"')}
>>> +
>>> +
>>> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
>>> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
>>> +"""
>>> +The following IPv6 regexp is a modified version of the one in:
>>> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976 
>>>
>>> +
>>> +It matches all these types of ipv6 addresses:
>>> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
>>> +fe80:0:0:0:204:61ff:fe9d:f156
>>> +fe80::204:61ff:fe9d:f156
>>> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
>>> +fe80:0:0:0:0204:61ff:254.157.241.86
>>> +fe80::204:61ff:254.157.241.86
>>> +::1
>>> +2001::
>>> +fe80::
>>> +"""
>>> +ipv6 = 
>>> r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  
>>> # noqa: E501
>>
>> This line seems to be corrupted the patch in patchwork.
>>
>> In any case, can we not have this line in the patch?
>> It doesn't look maintainable.
>>
>> Can we use something like 'ipaddress' module to parse
>> ip addresses instead?
>>
> 
> I agree having this huge blob is not very maintainable, maybe we can 
> roll back to the previous, more simplistic, regexp.
> 
> The goal of this regexp is not to parse the address but to extract it 
> from the ip/port range where we can have 
> [ip_start:ip_end]:port_start:port_end. After the substrings are 
> extracted we use 'netaddr' module to extract it.
I think that regex is not a good choice here. There are too many 
variables (IPv4 vs IPv6) and optional parameters (ports may or may not 
be present, range may only have one value) that regexes become complex 
very quickly. Throw in the fact that ":" is the separator for IPv6 
address sections AND is the separator between IP addresses and ports, 
plus the fact that the presence of a port changes how to express an IPv6 
address, and you're talking about some unmaintainable regexes, even if 
they're not as monstrous as the current IPv6 one.

> 
> 
>> Bets regards, Ilya Maximets.
>>
>
Adrian Moreno Feb. 10, 2022, 7:31 a.m. UTC | #6
Hi Mark

On 2/5/22 03:55, Mark Michelson wrote:
> On 1/31/22 01:20, Adrian Moreno wrote:
>>
>>
>> On 1/28/22 22:18, Ilya Maximets wrote:
>>> On 1/28/22 17:04, Adrian Moreno wrote:
>>>> Introduce OFPFlow class and all its decoders.
>>>>
>>>> Most of the decoders are generic (from decoders.py). Some have special
>>>> syntax and need a specific implementation.
>>>>
>>>> Decoders for nat are moved to the common decoders.py because it's syntax
>>>> is shared with other types of flows (e.g: dpif flows).
>>>>
>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>> ---
>>>>   python/automake.mk           |   2 +
>>>>   python/ovs/flows/decoders.py | 108 +++++++++
>>>>   python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>>>>   python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>>>>   4 files changed, 806 insertions(+)
>>>>   create mode 100644 python/ovs/flows/ofp.py
>>>>   create mode 100644 python/ovs/flows/ofp_act.py
>>>>
>>>> diff --git a/python/automake.mk b/python/automake.mk
>>>> index b7debfbd9..7b6d6596f 100644
>>>> --- a/python/automake.mk
>>>> +++ b/python/automake.mk
>>>> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>>>>       python/ovs/flows/flow.py \
>>>>       python/ovs/flows/kv.py \
>>>>       python/ovs/flows/list.py \
>>>> +    python/ovs/flows/ofp.py \
>>>> +    python/ovs/flows/ofp_act.py \
>>>>       python/ovs/json.py \
>>>>       python/ovs/jsonrpc.py \
>>>>       python/ovs/ovsuuid.py \
>>>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>>>> index 2f8e5bd0a..1462b0b9d 100644
>>>> --- a/python/ovs/flows/decoders.py
>>>> +++ b/python/ovs/flows/decoders.py
>>>> @@ -6,6 +6,7 @@ object.
>>>>   """
>>>>   import netaddr
>>>> +import re
>>>>   class Decoder(object):
>>>> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>>>>       def to_json(self):
>>>>           return str(self)
>>>> +
>>>> +
>>>> +def decode_free_output(value):
>>>> +    """The value of the output action can be found free, i.e: without the
>>>> +    'output' keyword. This decoder decodes its value when found this way."""
>>>> +    try:
>>>> +        return "output", {"port": int(value)}
>>>> +    except ValueError:
>>>> +        return "output", {"port": value.strip('"')}
>>>> +
>>>> +
>>>> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
>>>> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
>>>> +"""
>>>> +The following IPv6 regexp is a modified version of the one in:
>>>> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976 
>>>>
>>>> +
>>>> +It matches all these types of ipv6 addresses:
>>>> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
>>>> +fe80:0:0:0:204:61ff:fe9d:f156
>>>> +fe80::204:61ff:fe9d:f156
>>>> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
>>>> +fe80:0:0:0:0204:61ff:254.157.241.86
>>>> +fe80::204:61ff:254.157.241.86
>>>> +::1
>>>> +2001::
>>>> +fe80::
>>>> +"""
>>>> +ipv6 = 
>>>> r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))" 
>>>> # noqa: E501
>>>
>>> This line seems to be corrupted the patch in patchwork.
>>>
>>> In any case, can we not have this line in the patch?
>>> It doesn't look maintainable.
>>>
>>> Can we use something like 'ipaddress' module to parse
>>> ip addresses instead?
>>>
>>
>> I agree having this huge blob is not very maintainable, maybe we can roll back 
>> to the previous, more simplistic, regexp.
>>
>> The goal of this regexp is not to parse the address but to extract it from the 
>> ip/port range where we can have [ip_start:ip_end]:port_start:port_end. After 
>> the substrings are extracted we use 'netaddr' module to extract it.
> I think that regex is not a good choice here. There are too many variables (IPv4 
> vs IPv6) and optional parameters (ports may or may not be present, range may 
> only have one value) that regexes become complex very quickly. Throw in the fact 
> that ":" is the separator for IPv6 address sections AND is the separator between 
> IP addresses and ports, plus the fact that the presence of a port changes how to 
> express an IPv6 address, and you're talking about some unmaintainable regexes, 
> even if they're not as monstrous as the current IPv6 one.
> 

If we go back to the previous approach I think it doesn't get _too_ complicated 
since we only need to extract where the addresses are and leave the actual 
address parsing to netaddr. So this code:

ipv4 = r"[\d\.]+"
ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
ipv6 = r"[\w:\.]+"
ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
port_range = r":(\d+)(?:-(\d+))?"

ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
ipv4_port_regex = re.compile(
     ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
)
ipv6_port_regex = re.compile(
     ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
)

... properly captures the following ip-port ranges:

("192.168.0.0:1000")
("192.168.0.0:1000-2000"),
("192.168.0.0-192.168.0.200:1000-2000"),
("fe80:0000:0000:0000:0204:61ff:fe9d:f156")
("fe80:0000:0000:0000:0204:61ff:fe9d:f150-fe80:0000:0000:0000:0204:61ff:fe9d:f15f")
("[fe80:0000:0000:0000:0204:61ff:fe9d:f150]-[fe80:0000:0000:0000:0204:61ff:fe9d:f15f]:255")
("[fe80::f150]-[fe80::f15f]:255-300")
("[fe80::204:61ff:254.157.241.86]-[fe80::204:61ff:254.157.241.100]:255-300")

... plus all of the ip/port ranges used in the ofproto unit tests since they 
pass with patch 11/18.

Do you think it's worth replacing this with some heuristics in order to 
determine what's the format of the ip-range string?
Eelco Chaudron Feb. 11, 2022, 2:12 p.m. UTC | #7
On 28 Jan 2022, at 17:04, Adrian Moreno wrote:

> Introduce OFPFlow class and all its decoders.
>
> Most of the decoders are generic (from decoders.py). Some have special
> syntax and need a specific implementation.
>
> Decoders for nat are moved to the common decoders.py because it's syntax
> is shared with other types of flows (e.g: dpif flows).
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

Some small comments below…

> ---
>  python/automake.mk           |   2 +
>  python/ovs/flows/decoders.py | 108 +++++++++
>  python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>  python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>  4 files changed, 806 insertions(+)
>  create mode 100644 python/ovs/flows/ofp.py
>  create mode 100644 python/ovs/flows/ofp_act.py
>
> diff --git a/python/automake.mk b/python/automake.mk
> index b7debfbd9..7b6d6596f 100644
> --- a/python/automake.mk
> +++ b/python/automake.mk
> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>  	python/ovs/flows/flow.py \
>  	python/ovs/flows/kv.py \
>  	python/ovs/flows/list.py \
> +	python/ovs/flows/ofp.py \
> +	python/ovs/flows/ofp_act.py \
>  	python/ovs/json.py \
>  	python/ovs/jsonrpc.py \
>  	python/ovs/ovsuuid.py \
> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
> index 2f8e5bd0a..1462b0b9d 100644
> --- a/python/ovs/flows/decoders.py
> +++ b/python/ovs/flows/decoders.py
> @@ -6,6 +6,7 @@ object.
>  """
>
>  import netaddr
> +import re
>
>
>  class Decoder(object):
> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>
>      def to_json(self):
>          return str(self)
> +
> +
> +def decode_free_output(value):
> +    """The value of the output action can be found free, i.e: without the
> +    'output' keyword. This decoder decodes its value when found this way."""
> +    try:
> +        return "output", {"port": int(value)}
> +    except ValueError:
> +        return "output", {"port": value.strip('"')}
> +
> +
> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
> +"""
> +The following IPv6 regexp is a modified version of the one in:
> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
> +
> +It matches all these types of ipv6 addresses:
> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
> +fe80:0:0:0:204:61ff:fe9d:f156
> +fe80::204:61ff:fe9d:f156
> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
> +fe80:0:0:0:0204:61ff:254.157.241.86
> +fe80::204:61ff:254.157.241.86
> +::1
> +2001::
> +fe80::
> +"""
> +ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501
> +ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
> +port_range = r":(\d+)(?:-(\d+))?"
> +ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
> +ipv4_port_regex = re.compile(
> +    ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
> +)
> +ipv6_port_regex = re.compile(
> +    ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
> +)
> +
> +
> +def decode_ip_port_range(value):
> +    """
> +    Decodes an IP and port range:
> +        {ip_start}-{ip-end}:{port_start}-{port_end}
> +
> +    IPv6 addresses are surrounded by "[" and "]" if port ranges are also
> +    present
> +
> +    Returns the following dictionary:
> +        {
> +            "addrs": {
> +                "start": {ip_start}
> +                "end": {ip_end}
> +            }
> +            "ports": {
> +                "start": {port_start},
> +                "end": {port_end}
> +        }
> +        (the "ports" key might be omitted)
> +    """
> +    if value.count(":") > 1:
> +        match = ipv6_port_regex.match(value)
> +    else:
> +        match = ipv4_port_regex.match(value)
> +
> +    ip_start = match.group(1)
> +    ip_end = match.group(2)
> +    port_start = match.group(3)
> +    port_end = match.group(4)
> +
> +    result = {
> +        "addrs": {
> +            "start": netaddr.IPAddress(ip_start),
> +            "end": netaddr.IPAddress(ip_end or ip_start),
> +        }
> +    }
> +    if port_start:
> +        result["ports"] = {
> +            "start": int(port_start),
> +            "end": int(port_end or port_start),
> +        }
> +
> +    return result
> +
> +
> +def decode_nat(value):
> +    """Decodes the 'nat' keyword of the ct action"""

You were going to add an example of what the decode would look like?

> +    if not value:
> +        return True

You where going to add a comment here why return true:

“””

> Why returning True is no data is present? I would expect None.

In general keys without values are decoded as key: True. Other places in the code calls them "flags", e.g: "drop" action is decoded as {"drop": True}. In this case "ct" without a value is a flag.

But I can see how this can be confusing. I'll add a comment to clarify.

“””

> +
> +    result = dict()
> +    type_parts = value.split("=")
> +    result["type"] = type_parts[0]
> +
> +    if len(type_parts) > 1:
> +        value_parts = type_parts[1].split(",")
> +        if len(type_parts) != 2:
> +            raise ValueError("Malformed nat action: %s" % value)
> +
> +        ip_port_range = decode_ip_port_range(value_parts[0])
> +
> +        result = {"type": type_parts[0], **ip_port_range}
> +
> +        for flag in value_parts[1:]:
> +            result[flag] = True
> +
> +    return result
> diff --git a/python/ovs/flows/ofp.py b/python/ovs/flows/ofp.py
> new file mode 100644
> index 000000000..1af06fa01
> --- /dev/null
> +++ b/python/ovs/flows/ofp.py
> @@ -0,0 +1,453 @@
> +""" Defines the parsers needed to parse ofproto flows.
> +"""
> +
> +import functools
> +
> +from ovs.flows.kv import KVParser, KVDecoders, nested_kv_decoder
> +from ovs.flows.ofp_fields import field_decoders
> +from ovs.flows.flow import Flow, Section
> +from ovs.flows.list import ListDecoders, nested_list_decoder
> +from ovs.flows.decoders import (
> +    decode_default,
> +    decode_flag,
> +    decode_int,
> +    decode_time,
> +    decode_mask,
> +    IPMask,
> +    EthMask,
> +    decode_free_output,
> +    decode_nat,
> +)
> +from ovs.flows.ofp_act import (
> +    decode_output,
> +    decode_field,
> +    decode_controller,
> +    decode_bundle,
> +    decode_bundle_load,
> +    decode_encap_ethernet,
> +    decode_load_field,
> +    decode_set_field,
> +    decode_move_field,
> +    decode_dec_ttl,
> +    decode_chk_pkt_larger,
> +    decode_zone,
> +    decode_exec,
> +    decode_learn,
> +)
> +
> +
> +class OFPFlow(Flow):
> +    """OFPFLow represents an OpenFlow Flow.
> +
> +    Attributes:
> +        info: The info section.
> +        match: The match section.
> +        actions: The actions section.
> +        id: The id object given at construction time.
> +    """
> +
> +    """
> +    These class variables are used to cache the KVDecoders instances. This
> +    will speed up subsequent flow parsings.
> +    """
> +    _info_decoders = None
> +    _match_decoders = None
> +    _action_decoders = None
> +
> +    @staticmethod
> +    def info_decoders():
> +        """Return the KVDecoders instance to parse the info section.
> +
> +        Uses the cached version if available.
> +        """
> +        if not OFPFlow._info_decoders:
> +            OFPFlow._info_decoders = OFPFlow._gen_info_decoders()
> +        return OFPFlow._info_decoders
> +
> +    @staticmethod
> +    def match_decoders():
> +        """Return the KVDecoders instance to parse the match section.
> +
> +        Uses the cached version if available.
> +        """
> +        if not OFPFlow._match_decoders:
> +            OFPFlow._match_decoders = OFPFlow._gen_match_decoders()
> +        return OFPFlow._match_decoders
> +
> +    @staticmethod
> +    def action_decoders():
> +        """Return the KVDecoders instance to parse the actions section.
> +
> +        Uses the cached version if available.
> +        """
> +        if not OFPFlow._action_decoders:
> +            OFPFlow._action_decoders = OFPFlow._gen_action_decoders()
> +        return OFPFlow._action_decoders
> +
> +    def __init__(self, ofp_string, id=None):
> +        """Create a OFPFlow from a flow string.
> +
> +        The string is expected to have the follwoing format:

following

> +
> +            [flow data] [match] actions=[actions]
> +
> +        Args:
> +            ofp_string(str): An OpenFlow flow string.
> +            id(Any): Optional; any object used to uniquely identify this flow
> +                from the rest.
> +
> +        Returns
> +            An OFPFlow with the content of the flow string or None if there is
> +            no flow information but the string is expected to be found in a a

a a

> +            flow dump.
> +
> +        Raises
> +            ValueError if the string is malformed.
> +            ParseError if an error in parsing occurs.
> +        """
> +        if " reply " in ofp_string:
> +            return None
> +
> +        sections = list()
> +        parts = ofp_string.split("actions=")
> +        if len(parts) != 2:
> +            raise ValueError("malformed ofproto flow: %s" % ofp_string)
> +
> +        actions = parts[1]
> +
> +        field_parts = parts[0].rstrip(" ").rpartition(" ")
> +        if len(field_parts) != 3:
> +            raise ValueError("malformed ofproto flow: %s" % ofp_string)
> +
> +        info = field_parts[0]
> +        match = field_parts[2]
> +
> +        iparser = KVParser(info, OFPFlow.info_decoders())
> +        iparser.parse()
> +        isection = Section(
> +            name="info",
> +            pos=ofp_string.find(info),
> +            string=info,
> +            data=iparser.kv(),
> +        )
> +        sections.append(isection)
> +
> +        mparser = KVParser(match, OFPFlow.match_decoders())
> +        mparser.parse()
> +        msection = Section(
> +            name="match",
> +            pos=ofp_string.find(match),
> +            string=match,
> +            data=mparser.kv(),
> +        )
> +        sections.append(msection)
> +
> +        aparser = KVParser(actions, OFPFlow.action_decoders())
> +        aparser.parse()
> +        asection = Section(
> +            name="actions",
> +            pos=ofp_string.find(actions),
> +            string=actions,
> +            data=aparser.kv(),
> +            is_list=True,
> +        )
> +        sections.append(asection)
> +
> +        super(OFPFlow, self).__init__(sections, ofp_string, id)
> +
> +    def __str__(self):
> +        if self._orig:
> +            return self._orig
> +        else:
> +            return self.to_string()
> +
> +    def to_string(self):
> +        """Return a text representation of the flow."""
> +        string = "Info: {} | ".format(self.info)
> +        string += "Match : {} | ".format(self.match)
> +        string += "Actions: {}".format(self.actions)
> +        return string
> +
> +    @staticmethod
> +    def _gen_info_decoders():
> +        """Generate the info KVDecoders."""
> +        args = {
> +            "table": decode_int,
> +            "duration": decode_time,
> +            "n_packet": decode_int,
> +            "n_bytes": decode_int,
> +            "cookie": decode_int,
> +            "idle_timeout": decode_time,
> +            "hard_timeout": decode_time,
> +            "hard_age": decode_time,
> +        }
> +        return KVDecoders(args)
> +
> +    @staticmethod
> +    def _gen_match_decoders():
> +        """Generate the match KVDecoders."""
> +        args = {
> +            **OFPFlow._field_decoder_args(),
> +            **OFPFlow._extra_match_decoder_args(),
> +        }
> +
> +        return KVDecoders(args)
> +
> +    @staticmethod
> +    def _extra_match_decoder_args():
> +        """Returns the extra KVDecoder arguments needed to decode the match
> +        part of a flow (apart from the fields)."""
> +        return {
> +            "priority": decode_int,
> +        }
> +
> +    @staticmethod
> +    def _field_decoder_args():
> +        """Returns the KVDecoder arguments needed to decode match fields
> +        decoding match fields.
> +        """
> +        shorthands = [
> +            "eth",
> +            "ip",
> +            "ipv6",
> +            "icmp",
> +            "icmp6",
> +            "tcp",
> +            "tcp6",
> +            "udp",
> +            "udp6",
> +            "sctp",
> +            "arp",
> +            "rarp",
> +            "mpls",
> +            "mplsm",
> +        ]
> +
> +        fields = {**field_decoders, **{key: decode_flag for key in shorthands}}
> +
> +        # vlan_vid field is special. Although it is technically 12 bit wide,
> +        # bit 12 is allowed to be set to 1 to indicate that the vlan header is
> +        # present (see section VLAN FIELDS in
> +        # http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt)
> +        # Therefore, override the generated vlan_vid field size.
> +        fields["vlan_vid"] = decode_mask(13)
> +        return fields
> +
> +    @staticmethod
> +    def _gen_action_decoders():
> +        """Generate the actions decoders."""
> +
> +        actions = {
> +            **OFPFlow._output_actions_decoders_args(),
> +            **OFPFlow._encap_actions_decoders_args(),
> +            **OFPFlow._field_action_decoders_args(),
> +            **OFPFlow._meta_action_decoders_args(),
> +            **OFPFlow._fw_action_decoders_args(),
> +            **OFPFlow._control_action_decoders_args(),
> +            **OFPFlow._other_action_decoders_args(),
> +        }
> +        clone_actions = OFPFlow._clone_actions_decoders_args(actions)
> +        actions.update(clone_actions)
> +        return KVDecoders(actions, default_free=decode_free_output)
> +
> +    @staticmethod
> +    def _output_actions_decoders_args():
> +        """Returns the decoder arguments for the output actions."""
> +        return {
> +            "output": decode_output,
> +            "drop": decode_flag,
> +            "controller": decode_controller,
> +            "enqueue": nested_list_decoder(
> +                ListDecoders([("port", decode_default), ("queue", int)]),
> +                delims=[",", ":"],
> +            ),
> +            "bundle": decode_bundle,
> +            "bundle_load": decode_bundle_load,
> +            "group": decode_default,
> +        }
> +
> +    @staticmethod
> +    def _encap_actions_decoders_args():
> +        """Returns the decoders arguments for the encap actions."""
> +
> +        return {
> +            "pop_vlan": decode_flag,
> +            "strip_vlan": decode_flag,
> +            "push_vlan": decode_default,
> +            "decap": decode_flag,
> +            "encap": nested_kv_decoder(
> +                KVDecoders(
> +                    {
> +                        "nsh": nested_kv_decoder(
> +                            KVDecoders(
> +                                {
> +                                    "md_type": decode_default,
> +                                    "tlv": nested_list_decoder(
> +                                        ListDecoders(
> +                                            [
> +                                                ("class", decode_int),
> +                                                ("type", decode_int),
> +                                                ("value", decode_int),
> +                                            ]
> +                                        )
> +                                    ),
> +                                }
> +                            )
> +                        ),
> +                    },
> +                    default=None,
> +                    default_free=decode_encap_ethernet,
> +                )
> +            ),
> +        }
> +
> +    @staticmethod
> +    def _field_action_decoders_args():
> +        """Returns the decoders arguments for field-modification actions."""
> +        # Field modification actions
> +        field_default_decoders = [
> +            "set_mpls_label",
> +            "set_mpls_tc",
> +            "set_mpls_ttl",
> +            "mod_nw_tos",
> +            "mod_nw_ecn",
> +            "mod_tcp_src",
> +            "mod_tcp_dst",
> +        ]
> +        return {
> +            "load": decode_load_field,
> +            "set_field": functools.partial(
> +                decode_set_field, KVDecoders(OFPFlow._field_decoder_args())
> +            ),
> +            "move": decode_move_field,
> +            "mod_dl_dst": EthMask,
> +            "mod_dl_src": EthMask,
> +            "mod_nw_dst": IPMask,
> +            "mod_nw_src": IPMask,
> +            "dec_ttl": decode_dec_ttl,
> +            "dec_mpls_ttl": decode_flag,
> +            "dec_nsh_ttl": decode_flag,
> +            "check_pkt_larger": decode_chk_pkt_larger,
> +            **{field: decode_default for field in field_default_decoders},
> +        }
> +
> +    @staticmethod
> +    def _meta_action_decoders_args():
> +        """Returns the decoders arguments for the metadata actions."""
> +        meta_default_decoders = ["set_tunnel", "set_tunnel64", "set_queue"]
> +        return {
> +            "pop_queue": decode_flag,
> +            **{field: decode_default for field in meta_default_decoders},
> +        }
> +
> +    @staticmethod
> +    def _fw_action_decoders_args():
> +        """Returns the decoders arguments for the firewalling actions."""
> +        return {
> +            "ct": nested_kv_decoder(
> +                KVDecoders(
> +                    {
> +                        "commit": decode_flag,
> +                        "zone": decode_zone,
> +                        "table": decode_int,
> +                        "nat": decode_nat,
> +                        "force": decode_flag,
> +                        "exec": functools.partial(
> +                            decode_exec,
> +                            KVDecoders(
> +                                {
> +                                    **OFPFlow._encap_actions_decoders_args(),
> +                                    **OFPFlow._field_action_decoders_args(),
> +                                    **OFPFlow._meta_action_decoders_args(),
> +                                }
> +                            ),
> +                        ),
> +                        "alg": decode_default,
> +                    }
> +                )
> +            ),
> +            "ct_clear": decode_flag,
> +        }
> +
> +    @staticmethod
> +    def _control_action_decoders_args():
> +        return {
> +            "resubmit": nested_list_decoder(
> +                ListDecoders(
> +                    [
> +                        ("port", decode_default),
> +                        ("table", decode_int),
> +                        ("ct", decode_flag),
> +                    ]
> +                )
> +            ),
> +            "push": decode_field,
> +            "pop": decode_field,
> +            "exit": decode_flag,
> +            "multipath": nested_list_decoder(
> +                ListDecoders(
> +                    [
> +                        ("fields", decode_default),
> +                        ("basis", decode_int),
> +                        ("algorithm", decode_default),
> +                        ("n_links", decode_int),
> +                        ("arg", decode_int),
> +                        ("dst", decode_field),
> +                    ]
> +                )
> +            ),
> +        }
> +
> +    @staticmethod
> +    def _clone_actions_decoders_args(action_decoders):
> +        """Generate the decoder arguments for the clone actions.
> +
> +        Args:
> +            action_decoders (dict): The decoders of the supported nested
> +            actions.
> +        """
> +        return {
> +            "learn": decode_learn(
> +                {
> +                    **action_decoders,
> +                    "fin_timeout": nested_kv_decoder(
> +                        KVDecoders(
> +                            {
> +                                "idle_timeout": decode_time,
> +                                "hard_timeout": decode_time,
> +                            }
> +                        )
> +                    ),
> +                }
> +            ),
> +            "clone": functools.partial(
> +                decode_exec, KVDecoders(action_decoders)
> +            ),
> +        }
> +
> +    @staticmethod
> +    def _other_action_decoders_args():
> +        """Generate the decoder arguments for other actions
> +        (see man(7) ovs-actions)."""
> +        return {
> +            "conjunction": nested_list_decoder(
> +                ListDecoders(
> +                    [("id", decode_int), ("k", decode_int), ("n", decode_int)]
> +                ),
> +                delims=[",", "/"],
> +            ),
> +            "note": decode_default,
> +            "sample": nested_kv_decoder(
> +                KVDecoders(
> +                    {
> +                        "probability": decode_int,
> +                        "collector_set_id": decode_int,
> +                        "obs_domain_id": decode_int,
> +                        "obs_point_id": decode_int,
> +                        "sampling_port": decode_default,
> +                        "ingress": decode_flag,
> +                        "egress": decode_flag,
> +                    }
> +                )
> +            ),
> +        }
> diff --git a/python/ovs/flows/ofp_act.py b/python/ovs/flows/ofp_act.py
> new file mode 100644
> index 000000000..9e04445ba
> --- /dev/null
> +++ b/python/ovs/flows/ofp_act.py
> @@ -0,0 +1,243 @@
> +""" Defines decoders for openflow actions.
> +"""
> +
> +import functools
> +
> +from ovs.flows.kv import nested_kv_decoder, KVDecoders, KeyValue, KVParser
> +from ovs.flows.decoders import (
> +    decode_default,
> +    decode_time,
> +    decode_flag,
> +    decode_int,
> +)
> +from ovs.flows.ofp_fields import field_decoders
> +
> +
> +def decode_output(value):
> +    """Decodes the output value.
> +
> +    Does not support field specification.
> +    """
> +    if len(value.split(",")) > 1:
> +        return nested_kv_decoder()(value)
> +    try:
> +        return {"port": int(value)}
> +    except ValueError:
> +        return {"port": value.strip('"')}
> +
> +
> +def decode_controller(value):
> +    """Decodes the controller action."""
> +    if not value:
> +        return KeyValue("output", "controller")
> +    else:
> +        # Try controller:max_len
> +        try:
> +            max_len = int(value)
> +            return {
> +                "max_len": max_len,
> +            }
> +        except ValueError:
> +            pass
> +        # controller(key[=val], ...)
> +        return nested_kv_decoder()(value)
> +
> +
> +def decode_bundle_load(value):
> +    return decode_bundle(value, True)
> +
> +
> +def decode_bundle(value, load=False):
> +    """Decode bundle action."""
> +    result = {}
> +    keys = ["fields", "basis", "algorithm", "ofport"]
> +    if load:
> +        keys.append("dst")
> +
> +    for key in keys:
> +        parts = value.partition(",")
> +        nvalue = parts[0]
> +        value = parts[2]
> +        if key == "ofport":
> +            continue
> +        result[key] = decode_default(nvalue)
> +
> +    # Handle members:
> +    mvalues = value.split("members:")
> +    result["members"] = [int(port) for port in mvalues[1].split(",")]
> +    return result
> +
> +
> +def decode_encap_ethernet(value):
> +    """Decodes encap ethernet value."""
> +    return "ethernet", int(value, 0)
> +
> +
> +def decode_field(value):
> +    """Decodes a field as defined in the 'Field Specification' of the actions
> +    man page:
> +    http://www.openvswitch.org/support/dist-docs/ovs-actions.7.txt."""
> +    parts = value.strip("]\n\r").split("[")
> +    result = {
> +        "field": parts[0],
> +    }
> +
> +    if len(parts) > 1 and parts[1]:
> +        field_range = parts[1].split("..")
> +        start = field_range[0]
> +        end = field_range[1] if len(field_range) > 1 else start
> +        if start:
> +            result["start"] = int(start)
> +        if end:
> +            result["end"] = int(end)
> +
> +    return result
> +
> +
> +def decode_load_field(value):
> +    """Decodes LOAD actions such as: 'load:value->dst'."""
> +    parts = value.split("->")
> +    if len(parts) != 2:
> +        raise ValueError("Malformed load action : %s" % value)
> +
> +    # If the load action is performed within a learn() action,
> +    # The value can be specified as another field.
> +    try:
> +        return {"value": int(parts[0], 0), "dst": decode_field(parts[1])}
> +    except ValueError:
> +        return {"src": decode_field(parts[0]), "dst": decode_field(parts[1])}
> +
> +
> +def decode_set_field(field_decoders, value):
> +    """Decodes SET_FIELD actions such as: 'set_field:value/mask->dst'.
> +
> +    The value is decoded by field_decoders which is a KVDecoders instance.
> +    Args:
> +        field_decoders(KVDecoders): The KVDecoders to be used to decode the
> +            field.
> +    """
> +    parts = value.split("->")
> +    if len(parts) != 2:
> +        raise ValueError("Malformed set_field action : %s" % value)
> +
> +    val = parts[0]
> +    dst = parts[1]
> +
> +    val_result = field_decoders.decode(dst, val)
> +
> +    return {
> +        "value": {val_result[0]: val_result[1]},
> +        "dst": decode_field(dst),
> +    }
> +
> +
> +def decode_move_field(value):
> +    """Decodes MOVE actions such as 'move:src->dst'."""
> +    parts = value.split("->")
> +    if len(parts) != 2:
> +        raise ValueError("Malformed move action : %s" % value)
> +
> +    return {
> +        "src": decode_field(parts[0]),
> +        "dst": decode_field(parts[1]),
> +    }
> +
> +
> +def decode_dec_ttl(value):
> +    """Decodes dec_ttl and dec_ttl(id, id[2], ...) actions."""
> +    if not value:
> +        return True
> +    return [int(idx) for idx in value.split(",")]
> +
> +
> +def decode_chk_pkt_larger(value):
> +    """Decodes 'check_pkt_larger(pkt_len)->dst' actions."""
> +    parts = value.split("->")
> +    if len(parts) != 2:
> +        raise ValueError("Malformed check_pkt_larger action : %s" % value)
> +
> +    pkt_len = int(parts[0].strip("()"))
> +    dst = decode_field(parts[1])
> +    return {"pkt_len": pkt_len, "dst": dst}
> +
> +
> +# CT decoders
> +def decode_zone(value):
> +    """Decodes the value of the 'zone' keyword (part of the ct action)."""
> +    try:
> +        return int(value, 0)
> +    except ValueError:
> +        pass
> +    return decode_field(value)
> +
> +
> +def decode_exec(action_decoders, value):
> +    """Decodes the value of the 'exec' keyword (part of the ct action).
> +
> +    Args:
> +        decode_actions (KVDecoders): The decoders to be used to decode the
> +            nested exec.
> +        value (string): The string to be decoded.
> +    """
> +    exec_parser = KVParser(value, action_decoders)
> +    exec_parser.parse()
> +    return [{kv.key: kv.value} for kv in exec_parser.kv()]
> +
> +
> +def decode_learn(action_decoders):
> +    """Create the decoder to be used to decode the 'learn' action.
> +
> +    The learn action has two added complexities:
> +    1) It can hold any valid action key-value. Therefore we must take
> +    the precalculated action_decoders and use them. That's why we require
> +    them as argument.
> +
> +    2) The way fields can be specified is augmented. Not only we have
> +    'field=value', but we also have:
> +        - 'field=_src_' (where _src_ is another field name)
> +        - and just 'field'
> +    For this we need to create a wrapper of field_decoders that, for each
> +    "field=X" key-value we check if X is a field_name or if it's acually
> +    a value that we need to send to the appropriate field_decoder to
> +    process.
> +
> +    Args:
> +        action_decoders (dict): Dictionary of decoders to be used in nested
> +            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.
> +        """
> +        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()
> +    }
> +    learn_decoders = {
> +        **action_decoders,
> +        **learn_field_decoders,
> +        "idle_timeout": decode_time,
> +        "hard_timeout": decode_time,
> +        "priority": decode_int,
> +        "cookie": decode_int,
> +        "send_flow_rem": decode_flag,
> +        "table": decode_int,
> +        "delete_learned": decode_flag,
> +        "limit": decode_int,
> +        "result_dst": decode_field,
> +    }
> +
> +    return functools.partial(decode_exec, KVDecoders(learn_decoders))
> -- 
> 2.34.1
Adrian Moreno Feb. 16, 2022, 11:19 a.m. UTC | #8
On 2/11/22 15:12, Eelco Chaudron wrote:
>> +def decode_nat(value):
>> +    """Decodes the 'nat' keyword of the ct action"""
> You were going to add an example of what the decode would look like?
> 
>> +    if not value:
>> +        return True
> You where going to add a comment here why return true:
> 
> “””
> 
>> Why returning True is no data is present? I would expect None.
> In general keys without values are decoded as key: True. Other places in the code calls them "flags", e.g: "drop" action is decoded as {"drop": True}. In this case "ct" without a value is a flag.
> 
> But I can see how this can be confusing. I'll add a comment to clarify.
> 
> “””
> 

Sorry I missed it, will send another version.
Eelco Chaudron Feb. 16, 2022, 11:23 a.m. UTC | #9
On 16 Feb 2022, at 12:19, Adrian Moreno wrote:

> On 2/11/22 15:12, Eelco Chaudron wrote:
>>> +def decode_nat(value):
>>> +    """Decodes the 'nat' keyword of the ct action"""
>> You were going to add an example of what the decode would look like?
>>
>>> +    if not value:
>>> +        return True
>> You where going to add a comment here why return true:
>>
>> “””
>>
>>> Why returning True is no data is present? I would expect None.
>> In general keys without values are decoded as key: True. Other places in the code calls them "flags", e.g: "drop" action is decoded as {"drop": True}. In this case "ct" without a value is a flag.
>>
>> But I can see how this can be confusing. I'll add a comment to clarify.
>>
>> “””
>>
>
> Sorry I missed it, will send another version.

Don’t hurry too much, will try to finish the v2 review next week, as I’m on training this week.
Adrian Moreno Feb. 28, 2022, 9:03 a.m. UTC | #10
On 2/11/22 15:12, Eelco Chaudron wrote:
> 
> On 28 Jan 2022, at 17:04, Adrian Moreno wrote:
> 
>> Introduce OFPFlow class and all its decoders.
>>
>> Most of the decoders are generic (from decoders.py). Some have special
>> syntax and need a specific implementation.
>>
>> Decoders for nat are moved to the common decoders.py because it's syntax
>> is shared with other types of flows (e.g: dpif flows).
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> 
> Some small comments below…
> 
>> ---
>>   python/automake.mk           |   2 +
>>   python/ovs/flows/decoders.py | 108 +++++++++
>>   python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>>   python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>>   4 files changed, 806 insertions(+)
>>   create mode 100644 python/ovs/flows/ofp.py
>>   create mode 100644 python/ovs/flows/ofp_act.py
>>
>> diff --git a/python/automake.mk b/python/automake.mk
>> index b7debfbd9..7b6d6596f 100644
>> --- a/python/automake.mk
>> +++ b/python/automake.mk
>> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>>   	python/ovs/flows/flow.py \
>>   	python/ovs/flows/kv.py \
>>   	python/ovs/flows/list.py \
>> +	python/ovs/flows/ofp.py \
>> +	python/ovs/flows/ofp_act.py \
>>   	python/ovs/json.py \
>>   	python/ovs/jsonrpc.py \
>>   	python/ovs/ovsuuid.py \
>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>> index 2f8e5bd0a..1462b0b9d 100644
>> --- a/python/ovs/flows/decoders.py
>> +++ b/python/ovs/flows/decoders.py
>> @@ -6,6 +6,7 @@ object.
>>   """
>>
>>   import netaddr
>> +import re
>>
>>
>>   class Decoder(object):
>> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>>
>>       def to_json(self):
>>           return str(self)
>> +
>> +
>> +def decode_free_output(value):
>> +    """The value of the output action can be found free, i.e: without the
>> +    'output' keyword. This decoder decodes its value when found this way."""
>> +    try:
>> +        return "output", {"port": int(value)}
>> +    except ValueError:
>> +        return "output", {"port": value.strip('"')}
>> +
>> +
>> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
>> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
>> +"""
>> +The following IPv6 regexp is a modified version of the one in:
>> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
>> +
>> +It matches all these types of ipv6 addresses:
>> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
>> +fe80:0:0:0:204:61ff:fe9d:f156
>> +fe80::204:61ff:fe9d:f156
>> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
>> +fe80:0:0:0:0204:61ff:254.157.241.86
>> +fe80::204:61ff:254.157.241.86
>> +::1
>> +2001::
>> +fe80::
>> +"""
>> +ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501
>> +ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
>> +port_range = r":(\d+)(?:-(\d+))?"
>> +ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
>> +ipv4_port_regex = re.compile(
>> +    ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
>> +)
>> +ipv6_port_regex = re.compile(
>> +    ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
>> +)
>> +
>> +

Hi Eelco, what do you think about going back to the v1 version of this regexp 
that successfully captured the IPv4 and IPv6 addresses? As mentioned in another 
email, this regexp is only used to capture the substrings that _might_ be ipv4/6 
addresses, leaving the actual address parsing to netaddr.IPAddress. Therefore, 
we don't need it to be 100% accurate in terms of address format and we can favor 
maintanability.

>> +def decode_ip_port_range(value):
>> +    """
>> +    Decodes an IP and port range:
>> +        {ip_start}-{ip-end}:{port_start}-{port_end}
>> +
>> +    IPv6 addresses are surrounded by "[" and "]" if port ranges are also
>> +    present
>> +
>> +    Returns the following dictionary:
>> +        {
>> +            "addrs": {
>> +                "start": {ip_start}
>> +                "end": {ip_end}
>> +            }
>> +            "ports": {
>> +                "start": {port_start},
>> +                "end": {port_end}
>> +        }
>> +        (the "ports" key might be omitted)
>> +    """
>> +    if value.count(":") > 1:
>> +        match = ipv6_port_regex.match(value)
>> +    else:
>> +        match = ipv4_port_regex.match(value)
>> +
>> +    ip_start = match.group(1)
>> +    ip_end = match.group(2)
>> +    port_start = match.group(3)
>> +    port_end = match.group(4)
>> +
>> +    result = {
>> +        "addrs": {
>> +            "start": netaddr.IPAddress(ip_start),
>> +            "end": netaddr.IPAddress(ip_end or ip_start),
>> +        }
>> +    }
>> +    if port_start:
>> +        result["ports"] = {
>> +            "start": int(port_start),
>> +            "end": int(port_end or port_start),
>> +        }
>> +
>> +    return result
>> +
>> +
>> +def decode_nat(value):
>> +    """Decodes the 'nat' keyword of the ct action"""
> 
> You were going to add an example of what the decode would look like?
> 
>> +    if not value:
>> +        return True
> 
> You where going to add a comment here why return true:
> 
> “””
> 
>> Why returning True is no data is present? I would expect None.
> 
> In general keys without values are decoded as key: True. Other places in the code calls them "flags", e.g: "drop" action is decoded as {"drop": True}. In this case "ct" without a value is a flag.
> 
> But I can see how this can be confusing. I'll add a comment to clarify.
> 
> “””
> 
>> +
>> +    result = dict()
>> +    type_parts = value.split("=")
>> +    result["type"] = type_parts[0]
>> +
>> +    if len(type_parts) > 1:
>> +        value_parts = type_parts[1].split(",")
>> +        if len(type_parts) != 2:
>> +            raise ValueError("Malformed nat action: %s" % value)
>> +
>> +        ip_port_range = decode_ip_port_range(value_parts[0])
>> +
>> +        result = {"type": type_parts[0], **ip_port_range}
>> +
>> +        for flag in value_parts[1:]:
>> +            result[flag] = True
>> +
>> +    return result
>> diff --git a/python/ovs/flows/ofp.py b/python/ovs/flows/ofp.py
>> new file mode 100644
>> index 000000000..1af06fa01
>> --- /dev/null
>> +++ b/python/ovs/flows/ofp.py
>> @@ -0,0 +1,453 @@
>> +""" Defines the parsers needed to parse ofproto flows.
>> +"""
>> +
>> +import functools
>> +
>> +from ovs.flows.kv import KVParser, KVDecoders, nested_kv_decoder
>> +from ovs.flows.ofp_fields import field_decoders
>> +from ovs.flows.flow import Flow, Section
>> +from ovs.flows.list import ListDecoders, nested_list_decoder
>> +from ovs.flows.decoders import (
>> +    decode_default,
>> +    decode_flag,
>> +    decode_int,
>> +    decode_time,
>> +    decode_mask,
>> +    IPMask,
>> +    EthMask,
>> +    decode_free_output,
>> +    decode_nat,
>> +)
>> +from ovs.flows.ofp_act import (
>> +    decode_output,
>> +    decode_field,
>> +    decode_controller,
>> +    decode_bundle,
>> +    decode_bundle_load,
>> +    decode_encap_ethernet,
>> +    decode_load_field,
>> +    decode_set_field,
>> +    decode_move_field,
>> +    decode_dec_ttl,
>> +    decode_chk_pkt_larger,
>> +    decode_zone,
>> +    decode_exec,
>> +    decode_learn,
>> +)
>> +
>> +
>> +class OFPFlow(Flow):
>> +    """OFPFLow represents an OpenFlow Flow.
>> +
>> +    Attributes:
>> +        info: The info section.
>> +        match: The match section.
>> +        actions: The actions section.
>> +        id: The id object given at construction time.
>> +    """
>> +
>> +    """
>> +    These class variables are used to cache the KVDecoders instances. This
>> +    will speed up subsequent flow parsings.
>> +    """
>> +    _info_decoders = None
>> +    _match_decoders = None
>> +    _action_decoders = None
>> +
>> +    @staticmethod
>> +    def info_decoders():
>> +        """Return the KVDecoders instance to parse the info section.
>> +
>> +        Uses the cached version if available.
>> +        """
>> +        if not OFPFlow._info_decoders:
>> +            OFPFlow._info_decoders = OFPFlow._gen_info_decoders()
>> +        return OFPFlow._info_decoders
>> +
>> +    @staticmethod
>> +    def match_decoders():
>> +        """Return the KVDecoders instance to parse the match section.
>> +
>> +        Uses the cached version if available.
>> +        """
>> +        if not OFPFlow._match_decoders:
>> +            OFPFlow._match_decoders = OFPFlow._gen_match_decoders()
>> +        return OFPFlow._match_decoders
>> +
>> +    @staticmethod
>> +    def action_decoders():
>> +        """Return the KVDecoders instance to parse the actions section.
>> +
>> +        Uses the cached version if available.
>> +        """
>> +        if not OFPFlow._action_decoders:
>> +            OFPFlow._action_decoders = OFPFlow._gen_action_decoders()
>> +        return OFPFlow._action_decoders
>> +
>> +    def __init__(self, ofp_string, id=None):
>> +        """Create a OFPFlow from a flow string.
>> +
>> +        The string is expected to have the follwoing format:
> 
> following
> 
>> +
>> +            [flow data] [match] actions=[actions]
>> +
>> +        Args:
>> +            ofp_string(str): An OpenFlow flow string.
>> +            id(Any): Optional; any object used to uniquely identify this flow
>> +                from the rest.
>> +
>> +        Returns
>> +            An OFPFlow with the content of the flow string or None if there is
>> +            no flow information but the string is expected to be found in a a
> 
> a a
> 
>> +            flow dump.
>> +
>> +        Raises
>> +            ValueError if the string is malformed.
>> +            ParseError if an error in parsing occurs.
>> +        """
>> +        if " reply " in ofp_string:
>> +            return None
>> +
>> +        sections = list()
>> +        parts = ofp_string.split("actions=")
>> +        if len(parts) != 2:
>> +            raise ValueError("malformed ofproto flow: %s" % ofp_string)
>> +
>> +        actions = parts[1]
>> +
>> +        field_parts = parts[0].rstrip(" ").rpartition(" ")
>> +        if len(field_parts) != 3:
>> +            raise ValueError("malformed ofproto flow: %s" % ofp_string)
>> +
>> +        info = field_parts[0]
>> +        match = field_parts[2]
>> +
>> +        iparser = KVParser(info, OFPFlow.info_decoders())
>> +        iparser.parse()
>> +        isection = Section(
>> +            name="info",
>> +            pos=ofp_string.find(info),
>> +            string=info,
>> +            data=iparser.kv(),
>> +        )
>> +        sections.append(isection)
>> +
>> +        mparser = KVParser(match, OFPFlow.match_decoders())
>> +        mparser.parse()
>> +        msection = Section(
>> +            name="match",
>> +            pos=ofp_string.find(match),
>> +            string=match,
>> +            data=mparser.kv(),
>> +        )
>> +        sections.append(msection)
>> +
>> +        aparser = KVParser(actions, OFPFlow.action_decoders())
>> +        aparser.parse()
>> +        asection = Section(
>> +            name="actions",
>> +            pos=ofp_string.find(actions),
>> +            string=actions,
>> +            data=aparser.kv(),
>> +            is_list=True,
>> +        )
>> +        sections.append(asection)
>> +
>> +        super(OFPFlow, self).__init__(sections, ofp_string, id)
>> +
>> +    def __str__(self):
>> +        if self._orig:
>> +            return self._orig
>> +        else:
>> +            return self.to_string()
>> +
>> +    def to_string(self):
>> +        """Return a text representation of the flow."""
>> +        string = "Info: {} | ".format(self.info)
>> +        string += "Match : {} | ".format(self.match)
>> +        string += "Actions: {}".format(self.actions)
>> +        return string
>> +
>> +    @staticmethod
>> +    def _gen_info_decoders():
>> +        """Generate the info KVDecoders."""
>> +        args = {
>> +            "table": decode_int,
>> +            "duration": decode_time,
>> +            "n_packet": decode_int,
>> +            "n_bytes": decode_int,
>> +            "cookie": decode_int,
>> +            "idle_timeout": decode_time,
>> +            "hard_timeout": decode_time,
>> +            "hard_age": decode_time,
>> +        }
>> +        return KVDecoders(args)
>> +
>> +    @staticmethod
>> +    def _gen_match_decoders():
>> +        """Generate the match KVDecoders."""
>> +        args = {
>> +            **OFPFlow._field_decoder_args(),
>> +            **OFPFlow._extra_match_decoder_args(),
>> +        }
>> +
>> +        return KVDecoders(args)
>> +
>> +    @staticmethod
>> +    def _extra_match_decoder_args():
>> +        """Returns the extra KVDecoder arguments needed to decode the match
>> +        part of a flow (apart from the fields)."""
>> +        return {
>> +            "priority": decode_int,
>> +        }
>> +
>> +    @staticmethod
>> +    def _field_decoder_args():
>> +        """Returns the KVDecoder arguments needed to decode match fields
>> +        decoding match fields.
>> +        """
>> +        shorthands = [
>> +            "eth",
>> +            "ip",
>> +            "ipv6",
>> +            "icmp",
>> +            "icmp6",
>> +            "tcp",
>> +            "tcp6",
>> +            "udp",
>> +            "udp6",
>> +            "sctp",
>> +            "arp",
>> +            "rarp",
>> +            "mpls",
>> +            "mplsm",
>> +        ]
>> +
>> +        fields = {**field_decoders, **{key: decode_flag for key in shorthands}}
>> +
>> +        # vlan_vid field is special. Although it is technically 12 bit wide,
>> +        # bit 12 is allowed to be set to 1 to indicate that the vlan header is
>> +        # present (see section VLAN FIELDS in
>> +        # http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt)
>> +        # Therefore, override the generated vlan_vid field size.
>> +        fields["vlan_vid"] = decode_mask(13)
>> +        return fields
>> +
>> +    @staticmethod
>> +    def _gen_action_decoders():
>> +        """Generate the actions decoders."""
>> +
>> +        actions = {
>> +            **OFPFlow._output_actions_decoders_args(),
>> +            **OFPFlow._encap_actions_decoders_args(),
>> +            **OFPFlow._field_action_decoders_args(),
>> +            **OFPFlow._meta_action_decoders_args(),
>> +            **OFPFlow._fw_action_decoders_args(),
>> +            **OFPFlow._control_action_decoders_args(),
>> +            **OFPFlow._other_action_decoders_args(),
>> +        }
>> +        clone_actions = OFPFlow._clone_actions_decoders_args(actions)
>> +        actions.update(clone_actions)
>> +        return KVDecoders(actions, default_free=decode_free_output)
>> +
>> +    @staticmethod
>> +    def _output_actions_decoders_args():
>> +        """Returns the decoder arguments for the output actions."""
>> +        return {
>> +            "output": decode_output,
>> +            "drop": decode_flag,
>> +            "controller": decode_controller,
>> +            "enqueue": nested_list_decoder(
>> +                ListDecoders([("port", decode_default), ("queue", int)]),
>> +                delims=[",", ":"],
>> +            ),
>> +            "bundle": decode_bundle,
>> +            "bundle_load": decode_bundle_load,
>> +            "group": decode_default,
>> +        }
>> +
>> +    @staticmethod
>> +    def _encap_actions_decoders_args():
>> +        """Returns the decoders arguments for the encap actions."""
>> +
>> +        return {
>> +            "pop_vlan": decode_flag,
>> +            "strip_vlan": decode_flag,
>> +            "push_vlan": decode_default,
>> +            "decap": decode_flag,
>> +            "encap": nested_kv_decoder(
>> +                KVDecoders(
>> +                    {
>> +                        "nsh": nested_kv_decoder(
>> +                            KVDecoders(
>> +                                {
>> +                                    "md_type": decode_default,
>> +                                    "tlv": nested_list_decoder(
>> +                                        ListDecoders(
>> +                                            [
>> +                                                ("class", decode_int),
>> +                                                ("type", decode_int),
>> +                                                ("value", decode_int),
>> +                                            ]
>> +                                        )
>> +                                    ),
>> +                                }
>> +                            )
>> +                        ),
>> +                    },
>> +                    default=None,
>> +                    default_free=decode_encap_ethernet,
>> +                )
>> +            ),
>> +        }
>> +
>> +    @staticmethod
>> +    def _field_action_decoders_args():
>> +        """Returns the decoders arguments for field-modification actions."""
>> +        # Field modification actions
>> +        field_default_decoders = [
>> +            "set_mpls_label",
>> +            "set_mpls_tc",
>> +            "set_mpls_ttl",
>> +            "mod_nw_tos",
>> +            "mod_nw_ecn",
>> +            "mod_tcp_src",
>> +            "mod_tcp_dst",
>> +        ]
>> +        return {
>> +            "load": decode_load_field,
>> +            "set_field": functools.partial(
>> +                decode_set_field, KVDecoders(OFPFlow._field_decoder_args())
>> +            ),
>> +            "move": decode_move_field,
>> +            "mod_dl_dst": EthMask,
>> +            "mod_dl_src": EthMask,
>> +            "mod_nw_dst": IPMask,
>> +            "mod_nw_src": IPMask,
>> +            "dec_ttl": decode_dec_ttl,
>> +            "dec_mpls_ttl": decode_flag,
>> +            "dec_nsh_ttl": decode_flag,
>> +            "check_pkt_larger": decode_chk_pkt_larger,
>> +            **{field: decode_default for field in field_default_decoders},
>> +        }
>> +
>> +    @staticmethod
>> +    def _meta_action_decoders_args():
>> +        """Returns the decoders arguments for the metadata actions."""
>> +        meta_default_decoders = ["set_tunnel", "set_tunnel64", "set_queue"]
>> +        return {
>> +            "pop_queue": decode_flag,
>> +            **{field: decode_default for field in meta_default_decoders},
>> +        }
>> +
>> +    @staticmethod
>> +    def _fw_action_decoders_args():
>> +        """Returns the decoders arguments for the firewalling actions."""
>> +        return {
>> +            "ct": nested_kv_decoder(
>> +                KVDecoders(
>> +                    {
>> +                        "commit": decode_flag,
>> +                        "zone": decode_zone,
>> +                        "table": decode_int,
>> +                        "nat": decode_nat,
>> +                        "force": decode_flag,
>> +                        "exec": functools.partial(
>> +                            decode_exec,
>> +                            KVDecoders(
>> +                                {
>> +                                    **OFPFlow._encap_actions_decoders_args(),
>> +                                    **OFPFlow._field_action_decoders_args(),
>> +                                    **OFPFlow._meta_action_decoders_args(),
>> +                                }
>> +                            ),
>> +                        ),
>> +                        "alg": decode_default,
>> +                    }
>> +                )
>> +            ),
>> +            "ct_clear": decode_flag,
>> +        }
>> +
>> +    @staticmethod
>> +    def _control_action_decoders_args():
>> +        return {
>> +            "resubmit": nested_list_decoder(
>> +                ListDecoders(
>> +                    [
>> +                        ("port", decode_default),
>> +                        ("table", decode_int),
>> +                        ("ct", decode_flag),
>> +                    ]
>> +                )
>> +            ),
>> +            "push": decode_field,
>> +            "pop": decode_field,
>> +            "exit": decode_flag,
>> +            "multipath": nested_list_decoder(
>> +                ListDecoders(
>> +                    [
>> +                        ("fields", decode_default),
>> +                        ("basis", decode_int),
>> +                        ("algorithm", decode_default),
>> +                        ("n_links", decode_int),
>> +                        ("arg", decode_int),
>> +                        ("dst", decode_field),
>> +                    ]
>> +                )
>> +            ),
>> +        }
>> +
>> +    @staticmethod
>> +    def _clone_actions_decoders_args(action_decoders):
>> +        """Generate the decoder arguments for the clone actions.
>> +
>> +        Args:
>> +            action_decoders (dict): The decoders of the supported nested
>> +            actions.
>> +        """
>> +        return {
>> +            "learn": decode_learn(
>> +                {
>> +                    **action_decoders,
>> +                    "fin_timeout": nested_kv_decoder(
>> +                        KVDecoders(
>> +                            {
>> +                                "idle_timeout": decode_time,
>> +                                "hard_timeout": decode_time,
>> +                            }
>> +                        )
>> +                    ),
>> +                }
>> +            ),
>> +            "clone": functools.partial(
>> +                decode_exec, KVDecoders(action_decoders)
>> +            ),
>> +        }
>> +
>> +    @staticmethod
>> +    def _other_action_decoders_args():
>> +        """Generate the decoder arguments for other actions
>> +        (see man(7) ovs-actions)."""
>> +        return {
>> +            "conjunction": nested_list_decoder(
>> +                ListDecoders(
>> +                    [("id", decode_int), ("k", decode_int), ("n", decode_int)]
>> +                ),
>> +                delims=[",", "/"],
>> +            ),
>> +            "note": decode_default,
>> +            "sample": nested_kv_decoder(
>> +                KVDecoders(
>> +                    {
>> +                        "probability": decode_int,
>> +                        "collector_set_id": decode_int,
>> +                        "obs_domain_id": decode_int,
>> +                        "obs_point_id": decode_int,
>> +                        "sampling_port": decode_default,
>> +                        "ingress": decode_flag,
>> +                        "egress": decode_flag,
>> +                    }
>> +                )
>> +            ),
>> +        }
>> diff --git a/python/ovs/flows/ofp_act.py b/python/ovs/flows/ofp_act.py
>> new file mode 100644
>> index 000000000..9e04445ba
>> --- /dev/null
>> +++ b/python/ovs/flows/ofp_act.py
>> @@ -0,0 +1,243 @@
>> +""" Defines decoders for openflow actions.
>> +"""
>> +
>> +import functools
>> +
>> +from ovs.flows.kv import nested_kv_decoder, KVDecoders, KeyValue, KVParser
>> +from ovs.flows.decoders import (
>> +    decode_default,
>> +    decode_time,
>> +    decode_flag,
>> +    decode_int,
>> +)
>> +from ovs.flows.ofp_fields import field_decoders
>> +
>> +
>> +def decode_output(value):
>> +    """Decodes the output value.
>> +
>> +    Does not support field specification.
>> +    """
>> +    if len(value.split(",")) > 1:
>> +        return nested_kv_decoder()(value)
>> +    try:
>> +        return {"port": int(value)}
>> +    except ValueError:
>> +        return {"port": value.strip('"')}
>> +
>> +
>> +def decode_controller(value):
>> +    """Decodes the controller action."""
>> +    if not value:
>> +        return KeyValue("output", "controller")
>> +    else:
>> +        # Try controller:max_len
>> +        try:
>> +            max_len = int(value)
>> +            return {
>> +                "max_len": max_len,
>> +            }
>> +        except ValueError:
>> +            pass
>> +        # controller(key[=val], ...)
>> +        return nested_kv_decoder()(value)
>> +
>> +
>> +def decode_bundle_load(value):
>> +    return decode_bundle(value, True)
>> +
>> +
>> +def decode_bundle(value, load=False):
>> +    """Decode bundle action."""
>> +    result = {}
>> +    keys = ["fields", "basis", "algorithm", "ofport"]
>> +    if load:
>> +        keys.append("dst")
>> +
>> +    for key in keys:
>> +        parts = value.partition(",")
>> +        nvalue = parts[0]
>> +        value = parts[2]
>> +        if key == "ofport":
>> +            continue
>> +        result[key] = decode_default(nvalue)
>> +
>> +    # Handle members:
>> +    mvalues = value.split("members:")
>> +    result["members"] = [int(port) for port in mvalues[1].split(",")]
>> +    return result
>> +
>> +
>> +def decode_encap_ethernet(value):
>> +    """Decodes encap ethernet value."""
>> +    return "ethernet", int(value, 0)
>> +
>> +
>> +def decode_field(value):
>> +    """Decodes a field as defined in the 'Field Specification' of the actions
>> +    man page:
>> +    http://www.openvswitch.org/support/dist-docs/ovs-actions.7.txt."""
>> +    parts = value.strip("]\n\r").split("[")
>> +    result = {
>> +        "field": parts[0],
>> +    }
>> +
>> +    if len(parts) > 1 and parts[1]:
>> +        field_range = parts[1].split("..")
>> +        start = field_range[0]
>> +        end = field_range[1] if len(field_range) > 1 else start
>> +        if start:
>> +            result["start"] = int(start)
>> +        if end:
>> +            result["end"] = int(end)
>> +
>> +    return result
>> +
>> +
>> +def decode_load_field(value):
>> +    """Decodes LOAD actions such as: 'load:value->dst'."""
>> +    parts = value.split("->")
>> +    if len(parts) != 2:
>> +        raise ValueError("Malformed load action : %s" % value)
>> +
>> +    # If the load action is performed within a learn() action,
>> +    # The value can be specified as another field.
>> +    try:
>> +        return {"value": int(parts[0], 0), "dst": decode_field(parts[1])}
>> +    except ValueError:
>> +        return {"src": decode_field(parts[0]), "dst": decode_field(parts[1])}
>> +
>> +
>> +def decode_set_field(field_decoders, value):
>> +    """Decodes SET_FIELD actions such as: 'set_field:value/mask->dst'.
>> +
>> +    The value is decoded by field_decoders which is a KVDecoders instance.
>> +    Args:
>> +        field_decoders(KVDecoders): The KVDecoders to be used to decode the
>> +            field.
>> +    """
>> +    parts = value.split("->")
>> +    if len(parts) != 2:
>> +        raise ValueError("Malformed set_field action : %s" % value)
>> +
>> +    val = parts[0]
>> +    dst = parts[1]
>> +
>> +    val_result = field_decoders.decode(dst, val)
>> +
>> +    return {
>> +        "value": {val_result[0]: val_result[1]},
>> +        "dst": decode_field(dst),
>> +    }
>> +
>> +
>> +def decode_move_field(value):
>> +    """Decodes MOVE actions such as 'move:src->dst'."""
>> +    parts = value.split("->")
>> +    if len(parts) != 2:
>> +        raise ValueError("Malformed move action : %s" % value)
>> +
>> +    return {
>> +        "src": decode_field(parts[0]),
>> +        "dst": decode_field(parts[1]),
>> +    }
>> +
>> +
>> +def decode_dec_ttl(value):
>> +    """Decodes dec_ttl and dec_ttl(id, id[2], ...) actions."""
>> +    if not value:
>> +        return True
>> +    return [int(idx) for idx in value.split(",")]
>> +
>> +
>> +def decode_chk_pkt_larger(value):
>> +    """Decodes 'check_pkt_larger(pkt_len)->dst' actions."""
>> +    parts = value.split("->")
>> +    if len(parts) != 2:
>> +        raise ValueError("Malformed check_pkt_larger action : %s" % value)
>> +
>> +    pkt_len = int(parts[0].strip("()"))
>> +    dst = decode_field(parts[1])
>> +    return {"pkt_len": pkt_len, "dst": dst}
>> +
>> +
>> +# CT decoders
>> +def decode_zone(value):
>> +    """Decodes the value of the 'zone' keyword (part of the ct action)."""
>> +    try:
>> +        return int(value, 0)
>> +    except ValueError:
>> +        pass
>> +    return decode_field(value)
>> +
>> +
>> +def decode_exec(action_decoders, value):
>> +    """Decodes the value of the 'exec' keyword (part of the ct action).
>> +
>> +    Args:
>> +        decode_actions (KVDecoders): The decoders to be used to decode the
>> +            nested exec.
>> +        value (string): The string to be decoded.
>> +    """
>> +    exec_parser = KVParser(value, action_decoders)
>> +    exec_parser.parse()
>> +    return [{kv.key: kv.value} for kv in exec_parser.kv()]
>> +
>> +
>> +def decode_learn(action_decoders):
>> +    """Create the decoder to be used to decode the 'learn' action.
>> +
>> +    The learn action has two added complexities:
>> +    1) It can hold any valid action key-value. Therefore we must take
>> +    the precalculated action_decoders and use them. That's why we require
>> +    them as argument.
>> +
>> +    2) The way fields can be specified is augmented. Not only we have
>> +    'field=value', but we also have:
>> +        - 'field=_src_' (where _src_ is another field name)
>> +        - and just 'field'
>> +    For this we need to create a wrapper of field_decoders that, for each
>> +    "field=X" key-value we check if X is a field_name or if it's acually
>> +    a value that we need to send to the appropriate field_decoder to
>> +    process.
>> +
>> +    Args:
>> +        action_decoders (dict): Dictionary of decoders to be used in nested
>> +            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.
>> +        """
>> +        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()
>> +    }
>> +    learn_decoders = {
>> +        **action_decoders,
>> +        **learn_field_decoders,
>> +        "idle_timeout": decode_time,
>> +        "hard_timeout": decode_time,
>> +        "priority": decode_int,
>> +        "cookie": decode_int,
>> +        "send_flow_rem": decode_flag,
>> +        "table": decode_int,
>> +        "delete_learned": decode_flag,
>> +        "limit": decode_int,
>> +        "result_dst": decode_field,
>> +    }
>> +
>> +    return functools.partial(decode_exec, KVDecoders(learn_decoders))
>> -- 
>> 2.34.1
>
Eelco Chaudron Feb. 28, 2022, 11:54 a.m. UTC | #11
On 28 Feb 2022, at 10:03, Adrian Moreno wrote:

> On 2/11/22 15:12, Eelco Chaudron wrote:
>>
>> On 28 Jan 2022, at 17:04, Adrian Moreno wrote:
>>
>>> Introduce OFPFlow class and all its decoders.
>>>
>>> Most of the decoders are generic (from decoders.py). Some have special
>>> syntax and need a specific implementation.
>>>
>>> Decoders for nat are moved to the common decoders.py because it's syntax
>>> is shared with other types of flows (e.g: dpif flows).
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>
>> Some small comments below…
>>
>>> ---
>>>   python/automake.mk           |   2 +
>>>   python/ovs/flows/decoders.py | 108 +++++++++
>>>   python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>>>   python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>>>   4 files changed, 806 insertions(+)
>>>   create mode 100644 python/ovs/flows/ofp.py
>>>   create mode 100644 python/ovs/flows/ofp_act.py
>>>
>>> diff --git a/python/automake.mk b/python/automake.mk
>>> index b7debfbd9..7b6d6596f 100644
>>> --- a/python/automake.mk
>>> +++ b/python/automake.mk
>>> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>>>   	python/ovs/flows/flow.py \
>>>   	python/ovs/flows/kv.py \
>>>   	python/ovs/flows/list.py \
>>> +	python/ovs/flows/ofp.py \
>>> +	python/ovs/flows/ofp_act.py \
>>>   	python/ovs/json.py \
>>>   	python/ovs/jsonrpc.py \
>>>   	python/ovs/ovsuuid.py \
>>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>>> index 2f8e5bd0a..1462b0b9d 100644
>>> --- a/python/ovs/flows/decoders.py
>>> +++ b/python/ovs/flows/decoders.py
>>> @@ -6,6 +6,7 @@ object.
>>>   """
>>>
>>>   import netaddr
>>> +import re
>>>
>>>
>>>   class Decoder(object):
>>> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>>>
>>>       def to_json(self):
>>>           return str(self)
>>> +
>>> +
>>> +def decode_free_output(value):
>>> +    """The value of the output action can be found free, i.e: without the
>>> +    'output' keyword. This decoder decodes its value when found this way."""
>>> +    try:
>>> +        return "output", {"port": int(value)}
>>> +    except ValueError:
>>> +        return "output", {"port": value.strip('"')}
>>> +
>>> +
>>> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
>>> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
>>> +"""
>>> +The following IPv6 regexp is a modified version of the one in:
>>> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
>>> +
>>> +It matches all these types of ipv6 addresses:
>>> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
>>> +fe80:0:0:0:204:61ff:fe9d:f156
>>> +fe80::204:61ff:fe9d:f156
>>> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
>>> +fe80:0:0:0:0204:61ff:254.157.241.86
>>> +fe80::204:61ff:254.157.241.86
>>> +::1
>>> +2001::
>>> +fe80::
>>> +"""
>>> +ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501
>>> +ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
>>> +port_range = r":(\d+)(?:-(\d+))?"
>>> +ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
>>> +ipv4_port_regex = re.compile(
>>> +    ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
>>> +)
>>> +ipv6_port_regex = re.compile(
>>> +    ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
>>> +)
>>> +
>>> +
>
> Hi Eelco, what do you think about going back to the v1 version of this regexp that successfully captured the IPv4 and IPv6 addresses? As mentioned in another email, this regexp is only used to capture the substrings that _might_ be ipv4/6 addresses, leaving the actual address parsing to netaddr.IPAddress. Therefore, we don't need it to be 100% accurate in terms of address format and we can favor maintanability.

Yes, this makes sense to me for the v6! I assume you keep the v4 above?

<SNIP>
Adrian Moreno Feb. 28, 2022, 11:59 a.m. UTC | #12
On 2/28/22 12:54, Eelco Chaudron wrote:
> 
> 
> On 28 Feb 2022, at 10:03, Adrian Moreno wrote:
> 
>> On 2/11/22 15:12, Eelco Chaudron wrote:
>>>
>>> On 28 Jan 2022, at 17:04, Adrian Moreno wrote:
>>>
>>>> Introduce OFPFlow class and all its decoders.
>>>>
>>>> Most of the decoders are generic (from decoders.py). Some have special
>>>> syntax and need a specific implementation.
>>>>
>>>> Decoders for nat are moved to the common decoders.py because it's syntax
>>>> is shared with other types of flows (e.g: dpif flows).
>>>>
>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>
>>> Some small comments below…
>>>
>>>> ---
>>>>    python/automake.mk           |   2 +
>>>>    python/ovs/flows/decoders.py | 108 +++++++++
>>>>    python/ovs/flows/ofp.py      | 453 +++++++++++++++++++++++++++++++++++
>>>>    python/ovs/flows/ofp_act.py  | 243 +++++++++++++++++++
>>>>    4 files changed, 806 insertions(+)
>>>>    create mode 100644 python/ovs/flows/ofp.py
>>>>    create mode 100644 python/ovs/flows/ofp_act.py
>>>>
>>>> diff --git a/python/automake.mk b/python/automake.mk
>>>> index b7debfbd9..7b6d6596f 100644
>>>> --- a/python/automake.mk
>>>> +++ b/python/automake.mk
>>>> @@ -31,6 +31,8 @@ ovs_pyfiles = \
>>>>    	python/ovs/flows/flow.py \
>>>>    	python/ovs/flows/kv.py \
>>>>    	python/ovs/flows/list.py \
>>>> +	python/ovs/flows/ofp.py \
>>>> +	python/ovs/flows/ofp_act.py \
>>>>    	python/ovs/json.py \
>>>>    	python/ovs/jsonrpc.py \
>>>>    	python/ovs/ovsuuid.py \
>>>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>>>> index 2f8e5bd0a..1462b0b9d 100644
>>>> --- a/python/ovs/flows/decoders.py
>>>> +++ b/python/ovs/flows/decoders.py
>>>> @@ -6,6 +6,7 @@ object.
>>>>    """
>>>>
>>>>    import netaddr
>>>> +import re
>>>>
>>>>
>>>>    class Decoder(object):
>>>> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>>>>
>>>>        def to_json(self):
>>>>            return str(self)
>>>> +
>>>> +
>>>> +def decode_free_output(value):
>>>> +    """The value of the output action can be found free, i.e: without the
>>>> +    'output' keyword. This decoder decodes its value when found this way."""
>>>> +    try:
>>>> +        return "output", {"port": int(value)}
>>>> +    except ValueError:
>>>> +        return "output", {"port": value.strip('"')}
>>>> +
>>>> +
>>>> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
>>>> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
>>>> +"""
>>>> +The following IPv6 regexp is a modified version of the one in:
>>>> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
>>>> +
>>>> +It matches all these types of ipv6 addresses:
>>>> +fe80:0000:0000:0000:0204:61ff:fe9d:f156
>>>> +fe80:0:0:0:204:61ff:fe9d:f156
>>>> +fe80::204:61ff:fe9d:f156
>>>> +fe80:0000:0000:0000:0204:61ff:254.157.241.86
>>>> +fe80:0:0:0:0204:61ff:254.157.241.86
>>>> +fe80::204:61ff:254.157.241.86
>>>> +::1
>>>> +2001::
>>>> +fe80::
>>>> +"""
>>>> +ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501
>>>> +ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
>>>> +port_range = r":(\d+)(?:-(\d+))?"
>>>> +ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
>>>> +ipv4_port_regex = re.compile(
>>>> +    ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
>>>> +)
>>>> +ipv6_port_regex = re.compile(
>>>> +    ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
>>>> +)
>>>> +
>>>> +
>>
>> Hi Eelco, what do you think about going back to the v1 version of this regexp that successfully captured the IPv4 and IPv6 addresses? As mentioned in another email, this regexp is only used to capture the substrings that _might_ be ipv4/6 addresses, leaving the actual address parsing to netaddr.IPAddress. Therefore, we don't need it to be 100% accurate in terms of address format and we can favor maintanability.
> 
> Yes, this makes sense to me for the v6! I assume you keep the v4 above?
> 

Yes, v4 will remain as is since it's already easy-to-read.

Thanks
diff mbox series

Patch

diff --git a/python/automake.mk b/python/automake.mk
index b7debfbd9..7b6d6596f 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -31,6 +31,8 @@  ovs_pyfiles = \
 	python/ovs/flows/flow.py \
 	python/ovs/flows/kv.py \
 	python/ovs/flows/list.py \
+	python/ovs/flows/ofp.py \
+	python/ovs/flows/ofp_act.py \
 	python/ovs/json.py \
 	python/ovs/jsonrpc.py \
 	python/ovs/ovsuuid.py \
diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
index 2f8e5bd0a..1462b0b9d 100644
--- a/python/ovs/flows/decoders.py
+++ b/python/ovs/flows/decoders.py
@@ -6,6 +6,7 @@  object.
 """
 
 import netaddr
+import re
 
 
 class Decoder(object):
@@ -414,3 +415,110 @@  class IPMask(Decoder):
 
     def to_json(self):
         return str(self)
+
+
+def decode_free_output(value):
+    """The value of the output action can be found free, i.e: without the
+    'output' keyword. This decoder decodes its value when found this way."""
+    try:
+        return "output", {"port": int(value)}
+    except ValueError:
+        return "output", {"port": value.strip('"')}
+
+
+ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
+ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
+"""
+The following IPv6 regexp is a modified version of the one in:
+https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
+
+It matches all these types of ipv6 addresses:
+fe80:0000:0000:0000:0204:61ff:fe9d:f156
+fe80:0:0:0:204:61ff:fe9d:f156
+fe80::204:61ff:fe9d:f156
+fe80:0000:0000:0000:0204:61ff:254.157.241.86
+fe80:0:0:0:0204:61ff:254.157.241.86
+fe80::204:61ff:254.157.241.86
+::1
+2001::
+fe80::
+"""
+ipv6 = r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\
 d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"  # noqa: E501
+ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
+port_range = r":(\d+)(?:-(\d+))?"
+ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
+ipv4_port_regex = re.compile(
+    ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
+)
+ipv6_port_regex = re.compile(
+    ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
+)
+
+
+def decode_ip_port_range(value):
+    """
+    Decodes an IP and port range:
+        {ip_start}-{ip-end}:{port_start}-{port_end}
+
+    IPv6 addresses are surrounded by "[" and "]" if port ranges are also
+    present
+
+    Returns the following dictionary:
+        {
+            "addrs": {
+                "start": {ip_start}
+                "end": {ip_end}
+            }
+            "ports": {
+                "start": {port_start},
+                "end": {port_end}
+        }
+        (the "ports" key might be omitted)
+    """
+    if value.count(":") > 1:
+        match = ipv6_port_regex.match(value)
+    else:
+        match = ipv4_port_regex.match(value)
+
+    ip_start = match.group(1)
+    ip_end = match.group(2)
+    port_start = match.group(3)
+    port_end = match.group(4)
+
+    result = {
+        "addrs": {
+            "start": netaddr.IPAddress(ip_start),
+            "end": netaddr.IPAddress(ip_end or ip_start),
+        }
+    }
+    if port_start:
+        result["ports"] = {
+            "start": int(port_start),
+            "end": int(port_end or port_start),
+        }
+
+    return result
+
+
+def decode_nat(value):
+    """Decodes the 'nat' keyword of the ct action"""
+    if not value:
+        return True
+
+    result = dict()
+    type_parts = value.split("=")
+    result["type"] = type_parts[0]
+
+    if len(type_parts) > 1:
+        value_parts = type_parts[1].split(",")
+        if len(type_parts) != 2:
+            raise ValueError("Malformed nat action: %s" % value)
+
+        ip_port_range = decode_ip_port_range(value_parts[0])
+
+        result = {"type": type_parts[0], **ip_port_range}
+
+        for flag in value_parts[1:]:
+            result[flag] = True
+
+    return result
diff --git a/python/ovs/flows/ofp.py b/python/ovs/flows/ofp.py
new file mode 100644
index 000000000..1af06fa01
--- /dev/null
+++ b/python/ovs/flows/ofp.py
@@ -0,0 +1,453 @@ 
+""" Defines the parsers needed to parse ofproto flows.
+"""
+
+import functools
+
+from ovs.flows.kv import KVParser, KVDecoders, nested_kv_decoder
+from ovs.flows.ofp_fields import field_decoders
+from ovs.flows.flow import Flow, Section
+from ovs.flows.list import ListDecoders, nested_list_decoder
+from ovs.flows.decoders import (
+    decode_default,
+    decode_flag,
+    decode_int,
+    decode_time,
+    decode_mask,
+    IPMask,
+    EthMask,
+    decode_free_output,
+    decode_nat,
+)
+from ovs.flows.ofp_act import (
+    decode_output,
+    decode_field,
+    decode_controller,
+    decode_bundle,
+    decode_bundle_load,
+    decode_encap_ethernet,
+    decode_load_field,
+    decode_set_field,
+    decode_move_field,
+    decode_dec_ttl,
+    decode_chk_pkt_larger,
+    decode_zone,
+    decode_exec,
+    decode_learn,
+)
+
+
+class OFPFlow(Flow):
+    """OFPFLow represents an OpenFlow Flow.
+
+    Attributes:
+        info: The info section.
+        match: The match section.
+        actions: The actions section.
+        id: The id object given at construction time.
+    """
+
+    """
+    These class variables are used to cache the KVDecoders instances. This
+    will speed up subsequent flow parsings.
+    """
+    _info_decoders = None
+    _match_decoders = None
+    _action_decoders = None
+
+    @staticmethod
+    def info_decoders():
+        """Return the KVDecoders instance to parse the info section.
+
+        Uses the cached version if available.
+        """
+        if not OFPFlow._info_decoders:
+            OFPFlow._info_decoders = OFPFlow._gen_info_decoders()
+        return OFPFlow._info_decoders
+
+    @staticmethod
+    def match_decoders():
+        """Return the KVDecoders instance to parse the match section.
+
+        Uses the cached version if available.
+        """
+        if not OFPFlow._match_decoders:
+            OFPFlow._match_decoders = OFPFlow._gen_match_decoders()
+        return OFPFlow._match_decoders
+
+    @staticmethod
+    def action_decoders():
+        """Return the KVDecoders instance to parse the actions section.
+
+        Uses the cached version if available.
+        """
+        if not OFPFlow._action_decoders:
+            OFPFlow._action_decoders = OFPFlow._gen_action_decoders()
+        return OFPFlow._action_decoders
+
+    def __init__(self, ofp_string, id=None):
+        """Create a OFPFlow from a flow string.
+
+        The string is expected to have the follwoing format:
+
+            [flow data] [match] actions=[actions]
+
+        Args:
+            ofp_string(str): An OpenFlow flow string.
+            id(Any): Optional; any object used to uniquely identify this flow
+                from the rest.
+
+        Returns
+            An OFPFlow with the content of the flow string or None if there is
+            no flow information but the string is expected to be found in a a
+            flow dump.
+
+        Raises
+            ValueError if the string is malformed.
+            ParseError if an error in parsing occurs.
+        """
+        if " reply " in ofp_string:
+            return None
+
+        sections = list()
+        parts = ofp_string.split("actions=")
+        if len(parts) != 2:
+            raise ValueError("malformed ofproto flow: %s" % ofp_string)
+
+        actions = parts[1]
+
+        field_parts = parts[0].rstrip(" ").rpartition(" ")
+        if len(field_parts) != 3:
+            raise ValueError("malformed ofproto flow: %s" % ofp_string)
+
+        info = field_parts[0]
+        match = field_parts[2]
+
+        iparser = KVParser(info, OFPFlow.info_decoders())
+        iparser.parse()
+        isection = Section(
+            name="info",
+            pos=ofp_string.find(info),
+            string=info,
+            data=iparser.kv(),
+        )
+        sections.append(isection)
+
+        mparser = KVParser(match, OFPFlow.match_decoders())
+        mparser.parse()
+        msection = Section(
+            name="match",
+            pos=ofp_string.find(match),
+            string=match,
+            data=mparser.kv(),
+        )
+        sections.append(msection)
+
+        aparser = KVParser(actions, OFPFlow.action_decoders())
+        aparser.parse()
+        asection = Section(
+            name="actions",
+            pos=ofp_string.find(actions),
+            string=actions,
+            data=aparser.kv(),
+            is_list=True,
+        )
+        sections.append(asection)
+
+        super(OFPFlow, self).__init__(sections, ofp_string, id)
+
+    def __str__(self):
+        if self._orig:
+            return self._orig
+        else:
+            return self.to_string()
+
+    def to_string(self):
+        """Return a text representation of the flow."""
+        string = "Info: {} | ".format(self.info)
+        string += "Match : {} | ".format(self.match)
+        string += "Actions: {}".format(self.actions)
+        return string
+
+    @staticmethod
+    def _gen_info_decoders():
+        """Generate the info KVDecoders."""
+        args = {
+            "table": decode_int,
+            "duration": decode_time,
+            "n_packet": decode_int,
+            "n_bytes": decode_int,
+            "cookie": decode_int,
+            "idle_timeout": decode_time,
+            "hard_timeout": decode_time,
+            "hard_age": decode_time,
+        }
+        return KVDecoders(args)
+
+    @staticmethod
+    def _gen_match_decoders():
+        """Generate the match KVDecoders."""
+        args = {
+            **OFPFlow._field_decoder_args(),
+            **OFPFlow._extra_match_decoder_args(),
+        }
+
+        return KVDecoders(args)
+
+    @staticmethod
+    def _extra_match_decoder_args():
+        """Returns the extra KVDecoder arguments needed to decode the match
+        part of a flow (apart from the fields)."""
+        return {
+            "priority": decode_int,
+        }
+
+    @staticmethod
+    def _field_decoder_args():
+        """Returns the KVDecoder arguments needed to decode match fields
+        decoding match fields.
+        """
+        shorthands = [
+            "eth",
+            "ip",
+            "ipv6",
+            "icmp",
+            "icmp6",
+            "tcp",
+            "tcp6",
+            "udp",
+            "udp6",
+            "sctp",
+            "arp",
+            "rarp",
+            "mpls",
+            "mplsm",
+        ]
+
+        fields = {**field_decoders, **{key: decode_flag for key in shorthands}}
+
+        # vlan_vid field is special. Although it is technically 12 bit wide,
+        # bit 12 is allowed to be set to 1 to indicate that the vlan header is
+        # present (see section VLAN FIELDS in
+        # http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt)
+        # Therefore, override the generated vlan_vid field size.
+        fields["vlan_vid"] = decode_mask(13)
+        return fields
+
+    @staticmethod
+    def _gen_action_decoders():
+        """Generate the actions decoders."""
+
+        actions = {
+            **OFPFlow._output_actions_decoders_args(),
+            **OFPFlow._encap_actions_decoders_args(),
+            **OFPFlow._field_action_decoders_args(),
+            **OFPFlow._meta_action_decoders_args(),
+            **OFPFlow._fw_action_decoders_args(),
+            **OFPFlow._control_action_decoders_args(),
+            **OFPFlow._other_action_decoders_args(),
+        }
+        clone_actions = OFPFlow._clone_actions_decoders_args(actions)
+        actions.update(clone_actions)
+        return KVDecoders(actions, default_free=decode_free_output)
+
+    @staticmethod
+    def _output_actions_decoders_args():
+        """Returns the decoder arguments for the output actions."""
+        return {
+            "output": decode_output,
+            "drop": decode_flag,
+            "controller": decode_controller,
+            "enqueue": nested_list_decoder(
+                ListDecoders([("port", decode_default), ("queue", int)]),
+                delims=[",", ":"],
+            ),
+            "bundle": decode_bundle,
+            "bundle_load": decode_bundle_load,
+            "group": decode_default,
+        }
+
+    @staticmethod
+    def _encap_actions_decoders_args():
+        """Returns the decoders arguments for the encap actions."""
+
+        return {
+            "pop_vlan": decode_flag,
+            "strip_vlan": decode_flag,
+            "push_vlan": decode_default,
+            "decap": decode_flag,
+            "encap": nested_kv_decoder(
+                KVDecoders(
+                    {
+                        "nsh": nested_kv_decoder(
+                            KVDecoders(
+                                {
+                                    "md_type": decode_default,
+                                    "tlv": nested_list_decoder(
+                                        ListDecoders(
+                                            [
+                                                ("class", decode_int),
+                                                ("type", decode_int),
+                                                ("value", decode_int),
+                                            ]
+                                        )
+                                    ),
+                                }
+                            )
+                        ),
+                    },
+                    default=None,
+                    default_free=decode_encap_ethernet,
+                )
+            ),
+        }
+
+    @staticmethod
+    def _field_action_decoders_args():
+        """Returns the decoders arguments for field-modification actions."""
+        # Field modification actions
+        field_default_decoders = [
+            "set_mpls_label",
+            "set_mpls_tc",
+            "set_mpls_ttl",
+            "mod_nw_tos",
+            "mod_nw_ecn",
+            "mod_tcp_src",
+            "mod_tcp_dst",
+        ]
+        return {
+            "load": decode_load_field,
+            "set_field": functools.partial(
+                decode_set_field, KVDecoders(OFPFlow._field_decoder_args())
+            ),
+            "move": decode_move_field,
+            "mod_dl_dst": EthMask,
+            "mod_dl_src": EthMask,
+            "mod_nw_dst": IPMask,
+            "mod_nw_src": IPMask,
+            "dec_ttl": decode_dec_ttl,
+            "dec_mpls_ttl": decode_flag,
+            "dec_nsh_ttl": decode_flag,
+            "check_pkt_larger": decode_chk_pkt_larger,
+            **{field: decode_default for field in field_default_decoders},
+        }
+
+    @staticmethod
+    def _meta_action_decoders_args():
+        """Returns the decoders arguments for the metadata actions."""
+        meta_default_decoders = ["set_tunnel", "set_tunnel64", "set_queue"]
+        return {
+            "pop_queue": decode_flag,
+            **{field: decode_default for field in meta_default_decoders},
+        }
+
+    @staticmethod
+    def _fw_action_decoders_args():
+        """Returns the decoders arguments for the firewalling actions."""
+        return {
+            "ct": nested_kv_decoder(
+                KVDecoders(
+                    {
+                        "commit": decode_flag,
+                        "zone": decode_zone,
+                        "table": decode_int,
+                        "nat": decode_nat,
+                        "force": decode_flag,
+                        "exec": functools.partial(
+                            decode_exec,
+                            KVDecoders(
+                                {
+                                    **OFPFlow._encap_actions_decoders_args(),
+                                    **OFPFlow._field_action_decoders_args(),
+                                    **OFPFlow._meta_action_decoders_args(),
+                                }
+                            ),
+                        ),
+                        "alg": decode_default,
+                    }
+                )
+            ),
+            "ct_clear": decode_flag,
+        }
+
+    @staticmethod
+    def _control_action_decoders_args():
+        return {
+            "resubmit": nested_list_decoder(
+                ListDecoders(
+                    [
+                        ("port", decode_default),
+                        ("table", decode_int),
+                        ("ct", decode_flag),
+                    ]
+                )
+            ),
+            "push": decode_field,
+            "pop": decode_field,
+            "exit": decode_flag,
+            "multipath": nested_list_decoder(
+                ListDecoders(
+                    [
+                        ("fields", decode_default),
+                        ("basis", decode_int),
+                        ("algorithm", decode_default),
+                        ("n_links", decode_int),
+                        ("arg", decode_int),
+                        ("dst", decode_field),
+                    ]
+                )
+            ),
+        }
+
+    @staticmethod
+    def _clone_actions_decoders_args(action_decoders):
+        """Generate the decoder arguments for the clone actions.
+
+        Args:
+            action_decoders (dict): The decoders of the supported nested
+            actions.
+        """
+        return {
+            "learn": decode_learn(
+                {
+                    **action_decoders,
+                    "fin_timeout": nested_kv_decoder(
+                        KVDecoders(
+                            {
+                                "idle_timeout": decode_time,
+                                "hard_timeout": decode_time,
+                            }
+                        )
+                    ),
+                }
+            ),
+            "clone": functools.partial(
+                decode_exec, KVDecoders(action_decoders)
+            ),
+        }
+
+    @staticmethod
+    def _other_action_decoders_args():
+        """Generate the decoder arguments for other actions
+        (see man(7) ovs-actions)."""
+        return {
+            "conjunction": nested_list_decoder(
+                ListDecoders(
+                    [("id", decode_int), ("k", decode_int), ("n", decode_int)]
+                ),
+                delims=[",", "/"],
+            ),
+            "note": decode_default,
+            "sample": nested_kv_decoder(
+                KVDecoders(
+                    {
+                        "probability": decode_int,
+                        "collector_set_id": decode_int,
+                        "obs_domain_id": decode_int,
+                        "obs_point_id": decode_int,
+                        "sampling_port": decode_default,
+                        "ingress": decode_flag,
+                        "egress": decode_flag,
+                    }
+                )
+            ),
+        }
diff --git a/python/ovs/flows/ofp_act.py b/python/ovs/flows/ofp_act.py
new file mode 100644
index 000000000..9e04445ba
--- /dev/null
+++ b/python/ovs/flows/ofp_act.py
@@ -0,0 +1,243 @@ 
+""" Defines decoders for openflow actions.
+"""
+
+import functools
+
+from ovs.flows.kv import nested_kv_decoder, KVDecoders, KeyValue, KVParser
+from ovs.flows.decoders import (
+    decode_default,
+    decode_time,
+    decode_flag,
+    decode_int,
+)
+from ovs.flows.ofp_fields import field_decoders
+
+
+def decode_output(value):
+    """Decodes the output value.
+
+    Does not support field specification.
+    """
+    if len(value.split(",")) > 1:
+        return nested_kv_decoder()(value)
+    try:
+        return {"port": int(value)}
+    except ValueError:
+        return {"port": value.strip('"')}
+
+
+def decode_controller(value):
+    """Decodes the controller action."""
+    if not value:
+        return KeyValue("output", "controller")
+    else:
+        # Try controller:max_len
+        try:
+            max_len = int(value)
+            return {
+                "max_len": max_len,
+            }
+        except ValueError:
+            pass
+        # controller(key[=val], ...)
+        return nested_kv_decoder()(value)
+
+
+def decode_bundle_load(value):
+    return decode_bundle(value, True)
+
+
+def decode_bundle(value, load=False):
+    """Decode bundle action."""
+    result = {}
+    keys = ["fields", "basis", "algorithm", "ofport"]
+    if load:
+        keys.append("dst")
+
+    for key in keys:
+        parts = value.partition(",")
+        nvalue = parts[0]
+        value = parts[2]
+        if key == "ofport":
+            continue
+        result[key] = decode_default(nvalue)
+
+    # Handle members:
+    mvalues = value.split("members:")
+    result["members"] = [int(port) for port in mvalues[1].split(",")]
+    return result
+
+
+def decode_encap_ethernet(value):
+    """Decodes encap ethernet value."""
+    return "ethernet", int(value, 0)
+
+
+def decode_field(value):
+    """Decodes a field as defined in the 'Field Specification' of the actions
+    man page:
+    http://www.openvswitch.org/support/dist-docs/ovs-actions.7.txt."""
+    parts = value.strip("]\n\r").split("[")
+    result = {
+        "field": parts[0],
+    }
+
+    if len(parts) > 1 and parts[1]:
+        field_range = parts[1].split("..")
+        start = field_range[0]
+        end = field_range[1] if len(field_range) > 1 else start
+        if start:
+            result["start"] = int(start)
+        if end:
+            result["end"] = int(end)
+
+    return result
+
+
+def decode_load_field(value):
+    """Decodes LOAD actions such as: 'load:value->dst'."""
+    parts = value.split("->")
+    if len(parts) != 2:
+        raise ValueError("Malformed load action : %s" % value)
+
+    # If the load action is performed within a learn() action,
+    # The value can be specified as another field.
+    try:
+        return {"value": int(parts[0], 0), "dst": decode_field(parts[1])}
+    except ValueError:
+        return {"src": decode_field(parts[0]), "dst": decode_field(parts[1])}
+
+
+def decode_set_field(field_decoders, value):
+    """Decodes SET_FIELD actions such as: 'set_field:value/mask->dst'.
+
+    The value is decoded by field_decoders which is a KVDecoders instance.
+    Args:
+        field_decoders(KVDecoders): The KVDecoders to be used to decode the
+            field.
+    """
+    parts = value.split("->")
+    if len(parts) != 2:
+        raise ValueError("Malformed set_field action : %s" % value)
+
+    val = parts[0]
+    dst = parts[1]
+
+    val_result = field_decoders.decode(dst, val)
+
+    return {
+        "value": {val_result[0]: val_result[1]},
+        "dst": decode_field(dst),
+    }
+
+
+def decode_move_field(value):
+    """Decodes MOVE actions such as 'move:src->dst'."""
+    parts = value.split("->")
+    if len(parts) != 2:
+        raise ValueError("Malformed move action : %s" % value)
+
+    return {
+        "src": decode_field(parts[0]),
+        "dst": decode_field(parts[1]),
+    }
+
+
+def decode_dec_ttl(value):
+    """Decodes dec_ttl and dec_ttl(id, id[2], ...) actions."""
+    if not value:
+        return True
+    return [int(idx) for idx in value.split(",")]
+
+
+def decode_chk_pkt_larger(value):
+    """Decodes 'check_pkt_larger(pkt_len)->dst' actions."""
+    parts = value.split("->")
+    if len(parts) != 2:
+        raise ValueError("Malformed check_pkt_larger action : %s" % value)
+
+    pkt_len = int(parts[0].strip("()"))
+    dst = decode_field(parts[1])
+    return {"pkt_len": pkt_len, "dst": dst}
+
+
+# CT decoders
+def decode_zone(value):
+    """Decodes the value of the 'zone' keyword (part of the ct action)."""
+    try:
+        return int(value, 0)
+    except ValueError:
+        pass
+    return decode_field(value)
+
+
+def decode_exec(action_decoders, value):
+    """Decodes the value of the 'exec' keyword (part of the ct action).
+
+    Args:
+        decode_actions (KVDecoders): The decoders to be used to decode the
+            nested exec.
+        value (string): The string to be decoded.
+    """
+    exec_parser = KVParser(value, action_decoders)
+    exec_parser.parse()
+    return [{kv.key: kv.value} for kv in exec_parser.kv()]
+
+
+def decode_learn(action_decoders):
+    """Create the decoder to be used to decode the 'learn' action.
+
+    The learn action has two added complexities:
+    1) It can hold any valid action key-value. Therefore we must take
+    the precalculated action_decoders and use them. That's why we require
+    them as argument.
+
+    2) The way fields can be specified is augmented. Not only we have
+    'field=value', but we also have:
+        - 'field=_src_' (where _src_ is another field name)
+        - and just 'field'
+    For this we need to create a wrapper of field_decoders that, for each
+    "field=X" key-value we check if X is a field_name or if it's acually
+    a value that we need to send to the appropriate field_decoder to
+    process.
+
+    Args:
+        action_decoders (dict): Dictionary of decoders to be used in nested
+            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.
+        """
+        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()
+    }
+    learn_decoders = {
+        **action_decoders,
+        **learn_field_decoders,
+        "idle_timeout": decode_time,
+        "hard_timeout": decode_time,
+        "priority": decode_int,
+        "cookie": decode_int,
+        "send_flow_rem": decode_flag,
+        "table": decode_int,
+        "delete_learned": decode_flag,
+        "limit": decode_int,
+        "result_dst": decode_field,
+    }
+
+    return functools.partial(decode_exec, KVDecoders(learn_decoders))