[ovs-dev,v1,02/18] python: add mask, ip and eth decoders

Add more decoders that can be used by KVParser.

For IPv4 and IPv6 addresses, create a new class that wraps
netaddr.IPAddress.
For Ethernet addresses, create a new class that wraps netaddr.EUI.
For Integers, create a new class that performs basic bitwise mask
comparisons

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 python/ovs/flows/decoders.py | 341 +++++++++++++++++++++++++++++++++++
 python/setup.py              |   2 +-
 2 files changed, 342 insertions(+), 1 deletion(-)

On Mon, 22 Nov 2021 12:22:40 +0100
Adrian Moreno <amorenoz@redhat.com> wrote:

> Add more decoders that can be used by KVParser.
> 
> For IPv4 and IPv6 addresses, create a new class that wraps
> netaddr.IPAddress.
> For Ethernet addresses, create a new class that wraps netaddr.EUI.
> For Integers, create a new class that performs basic bitwise mask
> comparisons
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  python/ovs/flows/decoders.py | 341 +++++++++++++++++++++++++++++++++++
>  python/setup.py              |   2 +-
>  2 files changed, 342 insertions(+), 1 deletion(-)
> 
> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
> index bfb64e70e..bf7a94ae8 100644
> --- a/python/ovs/flows/decoders.py
> +++ b/python/ovs/flows/decoders.py
> @@ -5,6 +5,15 @@ A decoder is generally a callable that accepts a string and returns the value
>  object.
>  """
>  
> +import netaddr
> +
> +
> +class Decoder:
> +    """Base class for all decoder classes"""
> +
> +    def to_json(self):
> +        assert "function must be implemented by derived class"

I think it's better to use "raise NotImplementedError" here

See comments inline below.

On 22 Nov 2021, at 12:22, Adrian Moreno wrote:

> Add more decoders that can be used by KVParser.
>
> For IPv4 and IPv6 addresses, create a new class that wraps
> netaddr.IPAddress.
> For Ethernet addresses, create a new class that wraps netaddr.EUI.
> For Integers, create a new class that performs basic bitwise mask
> comparisons
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  python/ovs/flows/decoders.py | 341 +++++++++++++++++++++++++++++++++++
>  python/setup.py              |   2 +-
>  2 files changed, 342 insertions(+), 1 deletion(-)
>
> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
> index bfb64e70e..bf7a94ae8 100644
> --- a/python/ovs/flows/decoders.py
> +++ b/python/ovs/flows/decoders.py
> @@ -5,6 +5,15 @@ A decoder is generally a callable that accepts a string and returns the value
>  object.
>  """
>
> +import netaddr
> +
> +
> +class Decoder:
> +    """Base class for all decoder classes"""
> +
> +    def to_json(self):
> +        assert "function must be implemented by derived class"

I agree with Timothy here. All the asserts should be removed, as they are not included when debug is disabled. So “raise NotImplementedError” will work here.

> +
>
>  def decode_default(value):
>      """Default decoder.
> @@ -17,3 +26,335 @@ def decode_default(value):
>          return ival
>      except ValueError:
>          return value
> +
> +
> +def decode_flag(value):
> +    """Default a flag. It's exising is just flagged by returning True"""

It's exising -> “ Its existence” also end the sentence with a dot.

> +    return True
> +
> +
> +def decode_int(value):
> +    """integer decoder
> +
> +    Both base10 and base16 integers are supported

Add . at the end of the sentence.

> +
> +    Used for fields such as:
> +        n_bytes=34
> +        metadata=0x4
> +    """
> +    return int(value, 0)
> +
> +
> +def decode_time(value):
> +    """time decoder
> +
> +    Used for fields such as:
> +        duration=1234.123s
> +    """
> +    if value == "never":
> +        return value
> +
> +    time_str = value.rstrip("s")
> +    return float(time_str)
> +
> +
> +class IntMask(Decoder):
> +    """Base class for Integer Mask decoder classes

Add .
> +
> +    It supports decoding a value/mask pair. It has to be derived and size
> +    has to be set to a specific value

What about making this a bit more clear?

It supports decoding a value/mask pair. The class has to be derived, and the size attribute must be set.

> +    """
> +
> +    size = None  # size in bits
> +
> +    def __init__(self, string):
> +        if not self.size:
> +            assert "IntMask should be derived and size should be fixed"

Change to raise, see an earlier comment.

> +
> +        parts = string.split("/")
> +        if len(parts) > 1:
> +            self._value = int(parts[0], 0)
> +            self._mask = int(parts[1], 0)

If the input string is “123/“, we will error out above, probably with ValueError.
Do we want to catch this, or are we ok with the error?

> +            if self._mask.bit_length() > self.size:
> +                raise ValueError(
> +                    "Integer mask {} is bigger than size {}".format(
> +                        self._mask, self.size
> +                    )
> +                )
> +        else:
> +            self._value = int(parts[0], 0)
> +            self._mask = 2 ** self.size - 1

Guess you could use self._mask = self.max_mask()

> +
> +        if self._value.bit_length() > self.size:
> +            raise ValueError(
> +                "Integer value {} is bigger than size {}".format(
> +                    self._value, self.size
> +                )
> +            )
> +
> +    @property
> +    def value(self):
> +        return self._value
> +
> +    @property
> +    def mask(self):
> +        return self._mask
> +
> +    def max_mask(self):
> +        return 2 ** self.size - 1
> +
> +    def fully(self):
> +        """Returns if it's fully masked"""

Guess it always returns ;) Guess you wanted to say “””Returns True if it's fully masked.”””

> +        return self._mask == self.max_mask()
> +
> +    def min(self):
> +        return self._value & self._mask

I assumed that self._value was always masked with self._mask in the OVS cases, but I guess there are cases this is not true.

> +
> +    def max(self):
> +        return (self.max_mask() & ~self._mask) | (self._value & self._mask)

What is max() representing? Taking the 0x0001/00FF example, this will give max = FF01, or 0x0001/F00F = 0FF1, or 0xF0F0/0010 = 0F1F?

> +    def __str__(self):
> +        if self.fully():
> +            return str(self._value)
> +        else:
> +            return "{}/{}".format(hex(self._value), hex(self._mask))
> +
> +    def __repr__(self):
> +        return "%s('%s')" % (self.__class__.__name__, self)
> +
> +    def __eq__(self, other):
> +        if isinstance(other, IntMask):
> +            return self.value == other.value and self.mask == other.mask

I don’t know what the use case is (yet), but this compares the mask properties, not the value?
I was expecting this: self.value & self.mask == other.value & other.mask

> +        elif isinstance(other, int):
> +            return self.value == other and self.mask == self.max_mask()
> +        else:
> +            raise ValueError("Cannot compare against ", other)
> +
> +    def __contains__(self, other):
> +        if isinstance(other, IntMask):
> +            if other.fully():
> +                return other.value in self
> +            return other.min() in self and other.max() in self
> +        else:
> +            return other & self._mask == self._value & self._mask
> +
> +    def dict(self):
> +        return {"value": self._value, "mask": self._mask}
> +
> +    def to_json(self):
> +        return self.dict()
> +
> +
> +class Mask8(IntMask):
> +    size = 8
> +
> +
> +class Mask16(IntMask):
> +    size = 16
> +
> +
> +class Mask32(IntMask):
> +    size = 32
> +
> +
> +class Mask64(IntMask):
> +    size = 64
> +
> +
> +class Mask128(IntMask):
> +    size = 128
> +
> +
> +class Mask992(IntMask):
> +    size = 992
> +
> +
> +def decode_mask(mask_size):
> +    """value/mask decoder for values of specific size (bits)
> +
> +    Used for fields such as:
> +        reg0=0x248/0xff
> +    """
> +
> +    class Mask(IntMask):
> +        size = mask_size
> +        __name__ = "Mask{}".format(size)
> +
> +    return Mask
> +
> +
> +class EthMask(Decoder):
> +    """EthMask represents an Ethernet address with optional mask

Guess all doctext need dots at the end of a sentence, so I will no longer add individual comments.

> +
> +    It uses netaddr.EUI
> +
> +    Attributes:
> +        eth (netaddr.EUI): the ethernet address
> +        mask (netaddr.EUI): Optional, the ethernet address mask
> +
> +    Args:
> +        string (str): A string representing the masked ethernet address
> +            e.g: 00.11:22:33:44:55 or 01:00:22:00:33:00/01:00:00:00:00:00
> +    """
> +
> +    def __init__(self, string):
> +        mask_parts = string.split("/")
> +        self._eth = netaddr.EUI(mask_parts[0])
> +        if len(mask_parts) == 2:
> +            self._mask = netaddr.EUI(mask_parts[1])
> +        else:
> +            self._mask = None
> +
> +    @property
> +    def eth(self):
> +        """The ethernet address"""
> +        return self._eth
> +
> +    @property
> +    def mask(self):
> +        """The ethernet address mask"""
> +        return self._mask
> +
> +    def __eq__(self, other):
> +        """Returns True if this EthMask is numerically the same as other"""
> +        return self._mask == other._mask and self._eth == other._eth

See previous command, as self._eth might not be the same as self._eth & self._mask, how do we define numerically the same?

If it as above: self._mask == other._mask and self._eth == other._eth
Or:             self._mask &  self._eth  == other._mask & other._eth

> +
> +    def __contains__(self, other):
> +        """
> +        Args:
> +            other (netaddr.EUI): another Ethernet address
> +
> +        Returns:
> +            True if other falls into the masked address range
> +        """
> +        if isinstance(other, EthMask):
> +            if other._mask:
> +                raise ValueError("EthMask mask comparison not supported")
> +            return other._eth in self
> +
> +        if self._mask:
> +            return (other.value & self._mask.value) == (
> +                self._eth.value & self._mask.value
> +            )
> +        else:
> +            return other == self._eth
> +
> +    def __str__(self):
> +        if self._mask:
> +            return "/".join(
> +                [
> +                    self._eth.format(netaddr.mac_unix),
> +                    self._mask.format(netaddr.mac_unix),
> +                ]
> +            )
> +        else:
> +            return self._eth.format(netaddr.mac_unix)
> +
> +    def __repr__(self):
> +        return "%s('%s')" % (self.__class__.__name__, self)
> +
> +    def to_json(self):
> +        return str(self)
> +
> +
> +class IPMask(Decoder):
> +    """IPMask stores an IPv6 or IPv4 and a mask
> +
> +    It uses netaddr.IPAddress. The IPMask can be represented by a
> +    netaddr.IPNetwork (if it's a valid cidr) or by an ip and a mask
> +

Guess also be consistent with IP, so either IP all uppercase or all lowercase (ipv6 vs IPv6).

> +    Args:
> +        string (str): A string representing the ip/mask
> +    """
> +
> +    def __init__(self, string):
> +        """Constructor"""
> +        self._ipnet = None
> +        self._ip = None
> +        self._mask = None
> +        try:
> +            self._ipnet = netaddr.IPNetwork(string)
> +        except netaddr.AddrFormatError:
> +            pass
> +
> +        if not self._ipnet:
> +            # It's not a valid CIDR. Store ip and mask indendently

Guess you mean “independently”


> +            parts = string.split("/")
> +            if len(parts) != 2:
> +                raise ValueError(
> +                    "value {}: is not an ipv4 or ipv6 address".format(string)
> +                )
> +            try:
> +                self._ip = netaddr.IPAddress(parts[0])
> +                self._mask = netaddr.IPAddress(parts[1])
> +            except netaddr.AddrFormatError as exc:
> +                raise ValueError(
> +                    "value {}: is not an ipv4 or ipv6 address".format(string)
> +                ) from exc
> +
> +    def __eq__(self, other):
> +        """Returns True if this IPMask is numerically the same as other"""
> +        if isinstance(other, netaddr.IPNetwork):
> +            return self._ipnet and self._ipnet == other
> +        if isinstance(other, netaddr.IPAddress):
> +            return self._ipnet and self._ipnet.ip == other
> +        elif isinstance(other, IPMask):
> +            if self._ipnet:
> +                return self._ipnet == other._ipnet
> +
> +            return self._ip == other._ip and self._mask == other._mask

See earlier comments on __eq__

> +        else:
> +            return False
> +
> +    def __contains__(self, other):
> +        """
> +        Args:
> +            other (netaddr.IPAddres): another IP address
> +
> +        Returns:
> +            True if other falls into the masked ip range
> +        """
> +        if isinstance(other, IPMask):
> +            if not other._ipnet:
> +                raise ValueError("ip/mask comparisons not supported")
> +
> +            return (
> +                netaddr.IPAddress(other._ipnet.first) in self
> +                and netaddr.IPAddress(other._ipnet.last) in self
> +            )
> +
> +        elif isinstance(other, netaddr.IPAddress):
> +            if self._ipnet:
> +                return other in self._ipnet
> +            return (other & self._mask) == (self._ip & self._mask)
> +
> +    def cidr(self):
> +        """
> +        Returns True if the IPMask is a valid CIDR
> +        """
> +        return self._ipnet is not None
> +
> +    @property
> +    def ip(self):
> +        """The IP address"""
> +        if self._ipnet:
> +            return self._ipnet.ip
> +        return self._ip
> +
> +    @property
> +    def mask(self):
> +        """The IP mask"""
> +        if self._ipnet:
> +            return self._ipnet.netmask
> +        return self._mask
> +
> +    def __str__(self):
> +        if self._ipnet:
> +            return str(self._ipnet)
> +        return "/".join([str(self._ip), str(self._mask)])
> +
> +    def __repr__(self):
> +        return "%s('%s')" % (self.__class__.__name__, self)
> +
> +    def to_json(self):
> +        return str(self)
> diff --git a/python/setup.py b/python/setup.py
> index 0e6b0ea39..b06370bd9 100644
> --- a/python/setup.py
> +++ b/python/setup.py
> @@ -87,7 +87,7 @@ setup_args = dict(
>      ext_modules=[setuptools.Extension("ovs._json", sources=["ovs/_json.c"],
>                                        libraries=['openvswitch'])],
>      cmdclass={'build_ext': try_build_ext},
> -    install_requires=['sortedcontainers'],
> +    install_requires=['sortedcontainers', 'netaddr'],
>      extras_require={':sys_platform == "win32"': ['pywin32 >= 1.0']},
>  )
>
> -- 
> 2.31.1

On 11/22/21 14:07, Timothy Redaelli wrote:
> On Mon, 22 Nov 2021 12:22:40 +0100
> Adrian Moreno <amorenoz@redhat.com> wrote:
> 
>> Add more decoders that can be used by KVParser.
>>
>> For IPv4 and IPv6 addresses, create a new class that wraps
>> netaddr.IPAddress.
>> For Ethernet addresses, create a new class that wraps netaddr.EUI.
>> For Integers, create a new class that performs basic bitwise mask
>> comparisons
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>   python/ovs/flows/decoders.py | 341 +++++++++++++++++++++++++++++++++++
>>   python/setup.py              |   2 +-
>>   2 files changed, 342 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>> index bfb64e70e..bf7a94ae8 100644
>> --- a/python/ovs/flows/decoders.py
>> +++ b/python/ovs/flows/decoders.py
>> @@ -5,6 +5,15 @@ A decoder is generally a callable that accepts a string and returns the value
>>   object.
>>   """
>>   
>> +import netaddr
>> +
>> +
>> +class Decoder:
>> +    """Base class for all decoder classes"""
>> +
>> +    def to_json(self):
>> +        assert "function must be implemented by derived class"
> 
> I think it's better to use "raise NotImplementedError" here
> 

Ack, will do. Thanks.

On 12/17/21 14:10, Eelco Chaudron wrote:
> See comments inline below.
> 
> On 22 Nov 2021, at 12:22, Adrian Moreno wrote:
> 
>> Add more decoders that can be used by KVParser.
>>
>> For IPv4 and IPv6 addresses, create a new class that wraps
>> netaddr.IPAddress.
>> For Ethernet addresses, create a new class that wraps netaddr.EUI.
>> For Integers, create a new class that performs basic bitwise mask
>> comparisons
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>   python/ovs/flows/decoders.py | 341 +++++++++++++++++++++++++++++++++++
>>   python/setup.py              |   2 +-
>>   2 files changed, 342 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>> index bfb64e70e..bf7a94ae8 100644
>> --- a/python/ovs/flows/decoders.py
>> +++ b/python/ovs/flows/decoders.py
>> @@ -5,6 +5,15 @@ A decoder is generally a callable that accepts a string and returns the value
>>   object.
>>   """
>>
>> +import netaddr
>> +
>> +
>> +class Decoder:
>> +    """Base class for all decoder classes"""
>> +
>> +    def to_json(self):
>> +        assert "function must be implemented by derived class"
> 
> I agree with Timothy here. All the asserts should be removed, as they are not included when debug is disabled. So “raise NotImplementedError” will work here.
> 
>> +
>>
>>   def decode_default(value):
>>       """Default decoder.
>> @@ -17,3 +26,335 @@ def decode_default(value):
>>           return ival
>>       except ValueError:
>>           return value
>> +
>> +
>> +def decode_flag(value):
>> +    """Default a flag. It's exising is just flagged by returning True"""
> 
> It's exising -> “ Its existence” also end the sentence with a dot.
> 
>> +    return True
>> +
>> +
>> +def decode_int(value):
>> +    """integer decoder
>> +
>> +    Both base10 and base16 integers are supported
> 
> Add . at the end of the sentence.
> 
>> +
>> +    Used for fields such as:
>> +        n_bytes=34
>> +        metadata=0x4
>> +    """
>> +    return int(value, 0)
>> +
>> +
>> +def decode_time(value):
>> +    """time decoder
>> +
>> +    Used for fields such as:
>> +        duration=1234.123s
>> +    """
>> +    if value == "never":
>> +        return value
>> +
>> +    time_str = value.rstrip("s")
>> +    return float(time_str)
>> +
>> +
>> +class IntMask(Decoder):
>> +    """Base class for Integer Mask decoder classes
> 
> Add .
>> +
>> +    It supports decoding a value/mask pair. It has to be derived and size
>> +    has to be set to a specific value
> 
> What about making this a bit more clear?
> 
> It supports decoding a value/mask pair. The class has to be derived, and the size attribute must be set.
> 

Ack, will do. Thanks.

>> +    """
>> +
>> +    size = None  # size in bits
>> +
>> +    def __init__(self, string):
>> +        if not self.size:
>> +            assert "IntMask should be derived and size should be fixed"
> 
> Change to raise, see an earlier comment.
> 
>> +
>> +        parts = string.split("/")
>> +        if len(parts) > 1:
>> +            self._value = int(parts[0], 0)
>> +            self._mask = int(parts[1], 0)
> 
> If the input string is “123/“, we will error out above, probably with ValueError.
> Do we want to catch this, or are we ok with the error?
> 

You're right, is "123/" a valid integer mask in openflow or datapath flows?

>> +            if self._mask.bit_length() > self.size:
>> +                raise ValueError(
>> +                    "Integer mask {} is bigger than size {}".format(
>> +                        self._mask, self.size
>> +                    )
>> +                )
>> +        else:
>> +            self._value = int(parts[0], 0)
>> +            self._mask = 2 ** self.size - 1
> 
> Guess you could use self._mask = self.max_mask()

Ack, will do. Thanks.

> 
>> +
>> +        if self._value.bit_length() > self.size:
>> +            raise ValueError(
>> +                "Integer value {} is bigger than size {}".format(
>> +                    self._value, self.size
>> +                )
>> +            )
>> +
>> +    @property
>> +    def value(self):
>> +        return self._value
>> +
>> +    @property
>> +    def mask(self):
>> +        return self._mask
>> +
>> +    def max_mask(self):
>> +        return 2 ** self.size - 1
>> +
>> +    def fully(self):
>> +        """Returns if it's fully masked"""
> 
> Guess it always returns ;) Guess you wanted to say “””Returns True if it's fully masked.”””
> 

Yes! Thanks.

>> +        return self._mask == self.max_mask()
>> +
>> +    def min(self):
>> +        return self._value & self._mask
> 
> I assumed that self._value was always masked with self._mask in the OVS cases, but I guess there are cases this is not true.
> 
>> +
>> +    def max(self):
>> +        return (self.max_mask() & ~self._mask) | (self._value & self._mask)
> 
> What is max() representing? Taking the 0x0001/00FF example, this will give max = FF01, or 0x0001/F00F = 0FF1, or 0xF0F0/0010 = 0F1F?
> 

Yes, max represents the highest possible value included in the mask. But 
thinking again about this, I'm no longer sure if the reason behind is valid. The 
idea was to be able to quickly calculate "__contains__" of two IntMask as 
"return other.min() in self and other.max() in self" but that doesn't hold true 
if the mask represents a non-contiguous set of possible integers. Besides, I 
don't think we really need this for filtering. Most likely filtering will end up 
checking if the IntMask __contains__ a particular integer (not another IntMask).

So leaning towards removing this.

>> +    def __str__(self):
>> +        if self.fully():
>> +            return str(self._value)
>> +        else:
>> +            return "{}/{}".format(hex(self._value), hex(self._mask))
>> +
>> +    def __repr__(self):
>> +        return "%s('%s')" % (self.__class__.__name__, self)
>> +
>> +    def __eq__(self, other):
>> +        if isinstance(other, IntMask):
>> +            return self.value == other.value and self.mask == other.mask
> 
> I don’t know what the use case is (yet), but this compares the mask properties, not the value?
> I was expecting this: self.value & self.mask == other.value & other.mask

The usecase is intended to be filtering based on this fields. Currently filters 
based on IntMasks have two operations.

Equality:
ovs-ofparse --filter "ct.state=0x01/ff" : I want flows that have exactly that 
ct.state. Would we want it to match also a flow that has "ct.state=0x1/0f"? My 
initial thought was no, hence the this implementation. WDYT?

Inclusion:
ovs-ofparse --filter "ct.state~=0x01" : I want flows that have a ct.state that 
_includes_ 0x01. Both 0x01/ff and 0x01/0f will match

> 
>> +        elif isinstance(other, int):
>> +            return self.value == other and self.mask == self.max_mask()
>> +        else:
>> +            raise ValueError("Cannot compare against ", other)
>> +
>> +    def __contains__(self, other):
>> +        if isinstance(other, IntMask):
>> +            if other.fully():
>> +                return other.value in self
>> +            return other.min() in self and other.max() in self
>> +        else:
>> +            return other & self._mask == self._value & self._mask
>> +
>> +    def dict(self):
>> +        return {"value": self._value, "mask": self._mask}
>> +
>> +    def to_json(self):
>> +        return self.dict()
>> +
>> +
>> +class Mask8(IntMask):
>> +    size = 8
>> +
>> +
>> +class Mask16(IntMask):
>> +    size = 16
>> +
>> +
>> +class Mask32(IntMask):
>> +    size = 32
>> +
>> +
>> +class Mask64(IntMask):
>> +    size = 64
>> +
>> +
>> +class Mask128(IntMask):
>> +    size = 128
>> +
>> +
>> +class Mask992(IntMask):
>> +    size = 992
>> +
>> +
>> +def decode_mask(mask_size):
>> +    """value/mask decoder for values of specific size (bits)
>> +
>> +    Used for fields such as:
>> +        reg0=0x248/0xff
>> +    """
>> +
>> +    class Mask(IntMask):
>> +        size = mask_size
>> +        __name__ = "Mask{}".format(size)
>> +
>> +    return Mask
>> +
>> +
>> +class EthMask(Decoder):
>> +    """EthMask represents an Ethernet address with optional mask
> 
> Guess all doctext need dots at the end of a sentence, so I will no longer add individual comments.
> 

Yep, sorry. I'll fix them all

>> +
>> +    It uses netaddr.EUI
>> +
>> +    Attributes:
>> +        eth (netaddr.EUI): the ethernet address
>> +        mask (netaddr.EUI): Optional, the ethernet address mask
>> +
>> +    Args:
>> +        string (str): A string representing the masked ethernet address
>> +            e.g: 00.11:22:33:44:55 or 01:00:22:00:33:00/01:00:00:00:00:00
>> +    """
>> +
>> +    def __init__(self, string):
>> +        mask_parts = string.split("/")
>> +        self._eth = netaddr.EUI(mask_parts[0])
>> +        if len(mask_parts) == 2:
>> +            self._mask = netaddr.EUI(mask_parts[1])
>> +        else:
>> +            self._mask = None
>> +
>> +    @property
>> +    def eth(self):
>> +        """The ethernet address"""
>> +        return self._eth
>> +
>> +    @property
>> +    def mask(self):
>> +        """The ethernet address mask"""
>> +        return self._mask
>> +
>> +    def __eq__(self, other):
>> +        """Returns True if this EthMask is numerically the same as other"""
>> +        return self._mask == other._mask and self._eth == other._eth
> 
> See previous command, as self._eth might not be the same as self._eth & self._mask, how do we define numerically the same?
> 
> If it as above: self._mask == other._mask and self._eth == other._eth
> Or:             self._mask &  self._eth  == other._mask & other._eth
>

See previous comment. The idea was to be able to match exactly the same flow 
content. Maybe we could implement another operator?

>> +
>> +    def __contains__(self, other):
>> +        """
>> +        Args:
>> +            other (netaddr.EUI): another Ethernet address
>> +
>> +        Returns:
>> +            True if other falls into the masked address range
>> +        """
>> +        if isinstance(other, EthMask):
>> +            if other._mask:
>> +                raise ValueError("EthMask mask comparison not supported")
>> +            return other._eth in self
>> +
>> +        if self._mask:
>> +            return (other.value & self._mask.value) == (
>> +                self._eth.value & self._mask.value
>> +            )
>> +        else:
>> +            return other == self._eth
>> +
>> +    def __str__(self):
>> +        if self._mask:
>> +            return "/".join(
>> +                [
>> +                    self._eth.format(netaddr.mac_unix),
>> +                    self._mask.format(netaddr.mac_unix),
>> +                ]
>> +            )
>> +        else:
>> +            return self._eth.format(netaddr.mac_unix)
>> +
>> +    def __repr__(self):
>> +        return "%s('%s')" % (self.__class__.__name__, self)
>> +
>> +    def to_json(self):
>> +        return str(self)
>> +
>> +
>> +class IPMask(Decoder):
>> +    """IPMask stores an IPv6 or IPv4 and a mask
>> +
>> +    It uses netaddr.IPAddress. The IPMask can be represented by a
>> +    netaddr.IPNetwork (if it's a valid cidr) or by an ip and a mask
>> +
> 
> Guess also be consistent with IP, so either IP all uppercase or all lowercase (ipv6 vs IPv6).

Ack. Will do.

> 
>> +    Args:
>> +        string (str): A string representing the ip/mask
>> +    """
>> +
>> +    def __init__(self, string):
>> +        """Constructor"""
>> +        self._ipnet = None
>> +        self._ip = None
>> +        self._mask = None
>> +        try:
>> +            self._ipnet = netaddr.IPNetwork(string)
>> +        except netaddr.AddrFormatError:
>> +            pass
>> +
>> +        if not self._ipnet:
>> +            # It's not a valid CIDR. Store ip and mask indendently
> 
> Guess you mean “independently”

Sure, thanks.


> 
>> +            parts = string.split("/")
>> +            if len(parts) != 2:
>> +                raise ValueError(
>> +                    "value {}: is not an ipv4 or ipv6 address".format(string)
>> +                )
>> +            try:
>> +                self._ip = netaddr.IPAddress(parts[0])
>> +                self._mask = netaddr.IPAddress(parts[1])
>> +            except netaddr.AddrFormatError as exc:
>> +                raise ValueError(
>> +                    "value {}: is not an ipv4 or ipv6 address".format(string)
>> +                ) from exc
>> +
>> +    def __eq__(self, other):
>> +        """Returns True if this IPMask is numerically the same as other"""
>> +        if isinstance(other, netaddr.IPNetwork):
>> +            return self._ipnet and self._ipnet == other
>> +        if isinstance(other, netaddr.IPAddress):
>> +            return self._ipnet and self._ipnet.ip == other
>> +        elif isinstance(other, IPMask):
>> +            if self._ipnet:
>> +                return self._ipnet == other._ipnet
>> +
>> +            return self._ip == other._ip and self._mask == other._mask
> 
> See earlier comments on __eq__
> 
>> +        else:
>> +            return False
>> +
>> +    def __contains__(self, other):
>> +        """
>> +        Args:
>> +            other (netaddr.IPAddres): another IP address
>> +
>> +        Returns:
>> +            True if other falls into the masked ip range
>> +        """
>> +        if isinstance(other, IPMask):
>> +            if not other._ipnet:
>> +                raise ValueError("ip/mask comparisons not supported")
>> +
>> +            return (
>> +                netaddr.IPAddress(other._ipnet.first) in self
>> +                and netaddr.IPAddress(other._ipnet.last) in self
>> +            )
>> +
>> +        elif isinstance(other, netaddr.IPAddress):
>> +            if self._ipnet:
>> +                return other in self._ipnet
>> +            return (other & self._mask) == (self._ip & self._mask)
>> +
>> +    def cidr(self):
>> +        """
>> +        Returns True if the IPMask is a valid CIDR
>> +        """
>> +        return self._ipnet is not None
>> +
>> +    @property
>> +    def ip(self):
>> +        """The IP address"""
>> +        if self._ipnet:
>> +            return self._ipnet.ip
>> +        return self._ip
>> +
>> +    @property
>> +    def mask(self):
>> +        """The IP mask"""
>> +        if self._ipnet:
>> +            return self._ipnet.netmask
>> +        return self._mask
>> +
>> +    def __str__(self):
>> +        if self._ipnet:
>> +            return str(self._ipnet)
>> +        return "/".join([str(self._ip), str(self._mask)])
>> +
>> +    def __repr__(self):
>> +        return "%s('%s')" % (self.__class__.__name__, self)
>> +
>> +    def to_json(self):
>> +        return str(self)
>> diff --git a/python/setup.py b/python/setup.py
>> index 0e6b0ea39..b06370bd9 100644
>> --- a/python/setup.py
>> +++ b/python/setup.py
>> @@ -87,7 +87,7 @@ setup_args = dict(
>>       ext_modules=[setuptools.Extension("ovs._json", sources=["ovs/_json.c"],
>>                                         libraries=['openvswitch'])],
>>       cmdclass={'build_ext': try_build_ext},
>> -    install_requires=['sortedcontainers'],
>> +    install_requires=['sortedcontainers', 'netaddr'],
>>       extras_require={':sys_platform == "win32"': ['pywin32 >= 1.0']},
>>   )
>>
>> -- 
>> 2.31.1
>

On 20 Dec 2021, at 10:55, Adrian Moreno wrote:

> On 12/17/21 14:10, Eelco Chaudron wrote:
>> See comments inline below.
>>
>> On 22 Nov 2021, at 12:22, Adrian Moreno wrote:
>>
>>> Add more decoders that can be used by KVParser.
>>>
>>> For IPv4 and IPv6 addresses, create a new class that wraps
>>> netaddr.IPAddress.
>>> For Ethernet addresses, create a new class that wraps netaddr.EUI.
>>> For Integers, create a new class that performs basic bitwise mask
>>> comparisons
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>   python/ovs/flows/decoders.py | 341 +++++++++++++++++++++++++++++++++++
>>>   python/setup.py              |   2 +-
>>>   2 files changed, 342 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
>>> index bfb64e70e..bf7a94ae8 100644
>>> --- a/python/ovs/flows/decoders.py
>>> +++ b/python/ovs/flows/decoders.py
>>> @@ -5,6 +5,15 @@ A decoder is generally a callable that accepts a string and returns the value
>>>   object.
>>>   """
>>>
>>> +import netaddr
>>> +
>>> +
>>> +class Decoder:
>>> +    """Base class for all decoder classes"""
>>> +
>>> +    def to_json(self):
>>> +        assert "function must be implemented by derived class"
>>
>> I agree with Timothy here. All the asserts should be removed, as they are not included when debug is disabled. So “raise NotImplementedError” will work here.
>>
>>> +
>>>
>>>   def decode_default(value):
>>>       """Default decoder.
>>> @@ -17,3 +26,335 @@ def decode_default(value):
>>>           return ival
>>>       except ValueError:
>>>           return value
>>> +
>>> +
>>> +def decode_flag(value):
>>> +    """Default a flag. It's exising is just flagged by returning True"""
>>
>> It's exising -> “ Its existence” also end the sentence with a dot.
>>
>>> +    return True
>>> +
>>> +
>>> +def decode_int(value):
>>> +    """integer decoder
>>> +
>>> +    Both base10 and base16 integers are supported
>>
>> Add . at the end of the sentence.
>>
>>> +
>>> +    Used for fields such as:
>>> +        n_bytes=34
>>> +        metadata=0x4
>>> +    """
>>> +    return int(value, 0)
>>> +
>>> +
>>> +def decode_time(value):
>>> +    """time decoder
>>> +
>>> +    Used for fields such as:
>>> +        duration=1234.123s
>>> +    """
>>> +    if value == "never":
>>> +        return value
>>> +
>>> +    time_str = value.rstrip("s")
>>> +    return float(time_str)
>>> +
>>> +
>>> +class IntMask(Decoder):
>>> +    """Base class for Integer Mask decoder classes
>>
>> Add .
>>> +
>>> +    It supports decoding a value/mask pair. It has to be derived and size
>>> +    has to be set to a specific value
>>
>> What about making this a bit more clear?
>>
>> It supports decoding a value/mask pair. The class has to be derived, and the size attribute must be set.
>>
>
> Ack, will do. Thanks.
>
>>> +    """
>>> +
>>> +    size = None  # size in bits
>>> +
>>> +    def __init__(self, string):
>>> +        if not self.size:
>>> +            assert "IntMask should be derived and size should be fixed"
>>
>> Change to raise, see an earlier comment.
>>
>>> +
>>> +        parts = string.split("/")
>>> +        if len(parts) > 1:
>>> +            self._value = int(parts[0], 0)
>>> +            self._mask = int(parts[1], 0)
>>
>> If the input string is “123/“, we will error out above, probably with ValueError.
>> Do we want to catch this, or are we ok with the error?
>>
>
> You're right, is "123/" a valid integer mask in openflow or datapath flows?

I do not think OVS reports it as such, was thinking about parsing user input for filters.
But poping the error is fine by me.


>>> +            if self._mask.bit_length() > self.size:
>>> +                raise ValueError(
>>> +                    "Integer mask {} is bigger than size {}".format(
>>> +                        self._mask, self.size
>>> +                    )
>>> +                )
>>> +        else:
>>> +            self._value = int(parts[0], 0)
>>> +            self._mask = 2 ** self.size - 1
>>
>> Guess you could use self._mask = self.max_mask()
>
> Ack, will do. Thanks.
>
>>
>>> +
>>> +        if self._value.bit_length() > self.size:
>>> +            raise ValueError(
>>> +                "Integer value {} is bigger than size {}".format(
>>> +                    self._value, self.size
>>> +                )
>>> +            )
>>> +
>>> +    @property
>>> +    def value(self):
>>> +        return self._value
>>> +
>>> +    @property
>>> +    def mask(self):
>>> +        return self._mask
>>> +
>>> +    def max_mask(self):
>>> +        return 2 ** self.size - 1
>>> +
>>> +    def fully(self):
>>> +        """Returns if it's fully masked"""
>>
>> Guess it always returns ;) Guess you wanted to say “””Returns True if it's fully masked.”””
>>
>
> Yes! Thanks.
>
>>> +        return self._mask == self.max_mask()
>>> +
>>> +    def min(self):
>>> +        return self._value & self._mask
>>
>> I assumed that self._value was always masked with self._mask in the OVS cases, but I guess there are cases this is not true.
>>
>>> +
>>> +    def max(self):
>>> +        return (self.max_mask() & ~self._mask) | (self._value & self._mask)
>>
>> What is max() representing? Taking the 0x0001/00FF example, this will give max = FF01, or 0x0001/F00F = 0FF1, or 0xF0F0/0010 = 0F1F?
>>
>
> Yes, max represents the highest possible value included in the mask. But thinking again about this, I'm no longer sure if the reason behind is valid. The idea was to be able to quickly calculate "__contains__" of two IntMask as "return other.min() in self and other.max() in self" but that doesn't hold true if the mask represents a non-contiguous set of possible integers. Besides, I don't think we really need this for filtering. Most likely filtering will end up checking if the IntMask __contains__ a particular integer (not another IntMask).
>
> So leaning towards removing this.

If it’s not user I would suggest removing it to avoid confusion.

>>> +    def __str__(self):
>>> +        if self.fully():
>>> +            return str(self._value)
>>> +        else:
>>> +            return "{}/{}".format(hex(self._value), hex(self._mask))
>>> +
>>> +    def __repr__(self):
>>> +        return "%s('%s')" % (self.__class__.__name__, self)
>>> +
>>> +    def __eq__(self, other):
>>> +        if isinstance(other, IntMask):
>>> +            return self.value == other.value and self.mask == other.mask
>>
>> I don’t know what the use case is (yet), but this compares the mask properties, not the value?
>> I was expecting this: self.value & self.mask == other.value & other.mask
>
> The usecase is intended to be filtering based on this fields. Currently filters based on IntMasks have two operations.
>
> Equality:
> ovs-ofparse --filter "ct.state=0x01/ff" : I want flows that have exactly that ct.state. Would we want it to match also a flow that has "ct.state=0x1/0f"? My initial thought was no, hence the this implementation. WDYT?
>
> Inclusion:
> ovs-ofparse --filter "ct.state~=0x01" : I want flows that have a ct.state that _includes_ 0x01. Both 0x01/ff and 0x01/0f will match

With this context it makes sense, maybe you should add some comments to the individual __xx__ function to explain this.

>>
>>> +        elif isinstance(other, int):
>>> +            return self.value == other and self.mask == self.max_mask()
>>> +        else:
>>> +            raise ValueError("Cannot compare against ", other)
>>> +
>>> +    def __contains__(self, other):
>>> +        if isinstance(other, IntMask):
>>> +            if other.fully():
>>> +                return other.value in self
>>> +            return other.min() in self and other.max() in self
>>> +        else:
>>> +            return other & self._mask == self._value & self._mask
>>> +
>>> +    def dict(self):
>>> +        return {"value": self._value, "mask": self._mask}
>>> +
>>> +    def to_json(self):
>>> +        return self.dict()
>>> +
>>> +
>>> +class Mask8(IntMask):
>>> +    size = 8
>>> +
>>> +
>>> +class Mask16(IntMask):
>>> +    size = 16
>>> +
>>> +
>>> +class Mask32(IntMask):
>>> +    size = 32
>>> +
>>> +
>>> +class Mask64(IntMask):
>>> +    size = 64
>>> +
>>> +
>>> +class Mask128(IntMask):
>>> +    size = 128
>>> +
>>> +
>>> +class Mask992(IntMask):
>>> +    size = 992
>>> +
>>> +
>>> +def decode_mask(mask_size):
>>> +    """value/mask decoder for values of specific size (bits)
>>> +
>>> +    Used for fields such as:
>>> +        reg0=0x248/0xff
>>> +    """
>>> +
>>> +    class Mask(IntMask):
>>> +        size = mask_size
>>> +        __name__ = "Mask{}".format(size)
>>> +
>>> +    return Mask
>>> +
>>> +
>>> +class EthMask(Decoder):
>>> +    """EthMask represents an Ethernet address with optional mask
>>
>> Guess all doctext need dots at the end of a sentence, so I will no longer add individual comments.
>>
>
> Yep, sorry. I'll fix them all
>
>>> +
>>> +    It uses netaddr.EUI
>>> +
>>> +    Attributes:
>>> +        eth (netaddr.EUI): the ethernet address
>>> +        mask (netaddr.EUI): Optional, the ethernet address mask
>>> +
>>> +    Args:
>>> +        string (str): A string representing the masked ethernet address
>>> +            e.g: 00.11:22:33:44:55 or 01:00:22:00:33:00/01:00:00:00:00:00
>>> +    """
>>> +
>>> +    def __init__(self, string):
>>> +        mask_parts = string.split("/")
>>> +        self._eth = netaddr.EUI(mask_parts[0])
>>> +        if len(mask_parts) == 2:
>>> +            self._mask = netaddr.EUI(mask_parts[1])
>>> +        else:
>>> +            self._mask = None
>>> +
>>> +    @property
>>> +    def eth(self):
>>> +        """The ethernet address"""
>>> +        return self._eth
>>> +
>>> +    @property
>>> +    def mask(self):
>>> +        """The ethernet address mask"""
>>> +        return self._mask
>>> +
>>> +    def __eq__(self, other):
>>> +        """Returns True if this EthMask is numerically the same as other"""
>>> +        return self._mask == other._mask and self._eth == other._eth
>>
>> See previous command, as self._eth might not be the same as self._eth & self._mask, how do we define numerically the same?
>>
>> If it as above: self._mask == other._mask and self._eth == other._eth
>> Or:             self._mask &  self._eth  == other._mask & other._eth
>>
>
> See previous comment. The idea was to be able to match exactly the same flow content. Maybe we could implement another operator?
>
>>> +
>>> +    def __contains__(self, other):
>>> +        """
>>> +        Args:
>>> +            other (netaddr.EUI): another Ethernet address
>>> +
>>> +        Returns:
>>> +            True if other falls into the masked address range
>>> +        """
>>> +        if isinstance(other, EthMask):
>>> +            if other._mask:
>>> +                raise ValueError("EthMask mask comparison not supported")
>>> +            return other._eth in self
>>> +
>>> +        if self._mask:
>>> +            return (other.value & self._mask.value) == (
>>> +                self._eth.value & self._mask.value
>>> +            )
>>> +        else:
>>> +            return other == self._eth
>>> +
>>> +    def __str__(self):
>>> +        if self._mask:
>>> +            return "/".join(
>>> +                [
>>> +                    self._eth.format(netaddr.mac_unix),
>>> +                    self._mask.format(netaddr.mac_unix),
>>> +                ]
>>> +            )
>>> +        else:
>>> +            return self._eth.format(netaddr.mac_unix)
>>> +
>>> +    def __repr__(self):
>>> +        return "%s('%s')" % (self.__class__.__name__, self)
>>> +
>>> +    def to_json(self):
>>> +        return str(self)
>>> +
>>> +
>>> +class IPMask(Decoder):
>>> +    """IPMask stores an IPv6 or IPv4 and a mask
>>> +
>>> +    It uses netaddr.IPAddress. The IPMask can be represented by a
>>> +    netaddr.IPNetwork (if it's a valid cidr) or by an ip and a mask
>>> +
>>
>> Guess also be consistent with IP, so either IP all uppercase or all lowercase (ipv6 vs IPv6).
>
> Ack. Will do.
>
>>
>>> +    Args:
>>> +        string (str): A string representing the ip/mask
>>> +    """
>>> +
>>> +    def __init__(self, string):
>>> +        """Constructor"""
>>> +        self._ipnet = None
>>> +        self._ip = None
>>> +        self._mask = None
>>> +        try:
>>> +            self._ipnet = netaddr.IPNetwork(string)
>>> +        except netaddr.AddrFormatError:
>>> +            pass
>>> +
>>> +        if not self._ipnet:
>>> +            # It's not a valid CIDR. Store ip and mask indendently
>>
>> Guess you mean “independently”
>
> Sure, thanks.
>
>
>>
>>> +            parts = string.split("/")
>>> +            if len(parts) != 2:
>>> +                raise ValueError(
>>> +                    "value {}: is not an ipv4 or ipv6 address".format(string)
>>> +                )
>>> +            try:
>>> +                self._ip = netaddr.IPAddress(parts[0])
>>> +                self._mask = netaddr.IPAddress(parts[1])
>>> +            except netaddr.AddrFormatError as exc:
>>> +                raise ValueError(
>>> +                    "value {}: is not an ipv4 or ipv6 address".format(string)
>>> +                ) from exc
>>> +
>>> +    def __eq__(self, other):
>>> +        """Returns True if this IPMask is numerically the same as other"""
>>> +        if isinstance(other, netaddr.IPNetwork):
>>> +            return self._ipnet and self._ipnet == other
>>> +        if isinstance(other, netaddr.IPAddress):
>>> +            return self._ipnet and self._ipnet.ip == other
>>> +        elif isinstance(other, IPMask):
>>> +            if self._ipnet:
>>> +                return self._ipnet == other._ipnet
>>> +
>>> +            return self._ip == other._ip and self._mask == other._mask
>>
>> See earlier comments on __eq__
>>
>>> +        else:
>>> +            return False
>>> +
>>> +    def __contains__(self, other):
>>> +        """
>>> +        Args:
>>> +            other (netaddr.IPAddres): another IP address
>>> +
>>> +        Returns:
>>> +            True if other falls into the masked ip range
>>> +        """
>>> +        if isinstance(other, IPMask):
>>> +            if not other._ipnet:
>>> +                raise ValueError("ip/mask comparisons not supported")
>>> +
>>> +            return (
>>> +                netaddr.IPAddress(other._ipnet.first) in self
>>> +                and netaddr.IPAddress(other._ipnet.last) in self
>>> +            )
>>> +
>>> +        elif isinstance(other, netaddr.IPAddress):
>>> +            if self._ipnet:
>>> +                return other in self._ipnet
>>> +            return (other & self._mask) == (self._ip & self._mask)
>>> +
>>> +    def cidr(self):
>>> +        """
>>> +        Returns True if the IPMask is a valid CIDR
>>> +        """
>>> +        return self._ipnet is not None
>>> +
>>> +    @property
>>> +    def ip(self):
>>> +        """The IP address"""
>>> +        if self._ipnet:
>>> +            return self._ipnet.ip
>>> +        return self._ip
>>> +
>>> +    @property
>>> +    def mask(self):
>>> +        """The IP mask"""
>>> +        if self._ipnet:
>>> +            return self._ipnet.netmask
>>> +        return self._mask
>>> +
>>> +    def __str__(self):
>>> +        if self._ipnet:
>>> +            return str(self._ipnet)
>>> +        return "/".join([str(self._ip), str(self._mask)])
>>> +
>>> +    def __repr__(self):
>>> +        return "%s('%s')" % (self.__class__.__name__, self)
>>> +
>>> +    def to_json(self):
>>> +        return str(self)
>>> diff --git a/python/setup.py b/python/setup.py
>>> index 0e6b0ea39..b06370bd9 100644
>>> --- a/python/setup.py
>>> +++ b/python/setup.py
>>> @@ -87,7 +87,7 @@ setup_args = dict(
>>>       ext_modules=[setuptools.Extension("ovs._json", sources=["ovs/_json.c"],
>>>                                         libraries=['openvswitch'])],
>>>       cmdclass={'build_ext': try_build_ext},
>>> -    install_requires=['sortedcontainers'],
>>> +    install_requires=['sortedcontainers', 'netaddr'],
>>>       extras_require={':sys_platform == "win32"': ['pywin32 >= 1.0']},
>>>   )
>>>
>>> -- 
>>> 2.31.1
>>
>
> -- 
> Adrián Moreno