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 <[email protected]> >>> --- >>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
