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 <amore...@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
--
Adrián Moreno
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
