On 22 Nov 2021, at 12:22, 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 <[email protected]> > --- > python/automake.mk | 4 +- > python/ovs/flows/decoders.py | 93 ++++++++ > python/ovs/flows/ofp.py | 400 +++++++++++++++++++++++++++++++++++ > python/ovs/flows/ofp_act.py | 233 ++++++++++++++++++++ > 4 files changed, 729 insertions(+), 1 deletion(-) > 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 136da26bd..d1464d7f6 100644 > --- a/python/automake.mk > +++ b/python/automake.mk > @@ -46,7 +46,9 @@ ovs_pyfiles = \ > python/ovs/flows/decoders.py \ > python/ovs/flows/kv.py \ > python/ovs/flows/list.py \ > - python/ovs/flows/flow.py > + python/ovs/flows/flow.py \ > + python/ovs/flows/ofp.py \ > + python/ovs/flows/ofp_act.py > > # These python files are used at build time but not runtime, > # so they are not installed. > diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py > index bf7a94ae8..3def9f279 100644 > --- a/python/ovs/flows/decoders.py > +++ b/python/ovs/flows/decoders.py > @@ -6,6 +6,7 @@ object. > """ > > import netaddr > +import re > > > class Decoder: > @@ -358,3 +359,95 @@ class IPMask(Decoder): > > def to_json(self): > return str(self) > + > + > +def decode_free_output(value): > + """Decodes the output value when found free > + (without the 'output' keyword)""" This is the last time I’ll add it for the remaining patches to be reviewed. But please check that all sentences start with a Captical and end with a dot. > + try: > + return "output", {"port": int(value)} Any reason not to try to remove the “ here? return "output", {"port": int(value.strip('"'))} > + except ValueError: > + return "output", {"port": value.strip('"')} > + > + > +ipv4 = r"[\d\.]+" Should this not try to match at least 1 and at most 3 numbers? Maybe something like [\d{1,3}\.]+. > +ipv4_capture = r"({ipv4})".format(ipv4=ipv4) > +ipv6 = r"[\w:]+" > +ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6) > +port_range = r":(\d+)(?:-(\d+))?" Same as for IPv4, guess {0, 4} > +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) Do we need to check if there is a match? It might be good to error out if the string supplied does not match the expected input? Or if garbage is following the part we could decode? > + > + 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""" Can we add an example of the nat format? > + if not value: > + return True Why returning True is no data is present? I would expect None. > + > + 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..e56b08967 > --- /dev/null > +++ b/python/ovs/flows/ofp.py > @@ -0,0 +1,400 @@ > +""" 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""" Can we describe args; sections, orig, and id? Guess just copy them from Flow() > + > + def __init__(self, sections, orig="", id=None): > + """Constructor""" > + super(OFPFlow, self).__init__(sections, orig, id) > + > + def __str__(self): > + if self._orig: > + return self._orig > + else: > + return self.to_string() > + > + def to_string(self): > + """Print a text representation of the flow""" > + string = "Info: {}\n" + self.info > + string += "Match : {}\n" + self.match > + string += "Actions: {}\n " + self.actions It’s getting late, so maybe I should stop reviewing. But I’m wondering where self.info/self.match/self.actions are set? Also, _orig is only None, if explicitly set during initialization, else it’s “”. > + return string > + > + > +class OFPFlowFactory: Do we need OFPFlowFactory(object):, some times flake reports this as H238. But might no longer be valid with the latest python3. > + """OpenFlow Flow Factory is a class capable of creating OFPFLow > objects""" > + > + def __init__(self): > + self.info_decoders = self._info_decoders() > + self.match_decoders = KVDecoders( The name suggests that we return KVDecoders() as the __info_decoders() does, I think all three should return the same. > + {**self._field_decoders(), **self._flow_match_decoders()} > + ) > + self.act_decoders = self._act_decoders() > + > + def from_string(self, ofp_string, id=None): > + """Parse a ofproto flow string > + > + The string is expected to have the follwoing format: > + [flow data] [match] actions=[actions] Should we maybe add an example, as it’s not that straight forward, for example: cookie=0x0, duration=1.001s, table=0, n_packets=1, n_bytes=64, ip,in_port=enp5s0f0 actions=output:vnet0 [flow data], [match] actions=[actions] > + > + :param ofp_string: a ofproto string as dumped by ovs-ofctl tool An ofproto string, as dumped by the ovs-ofctl tool. > + :type ofp_string: str > + > + :return: an OFPFlow with the content of the flow string > + :rtype: OFPFlow > + """ > + 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(self.info_decoders) > + iparser.parse(info) > + isection = Section( > + name="info", > + pos=ofp_string.find(info), > + string=info, > + data=iparser.kv(), > + ) > + sections.append(isection) > + > + mparser = KVParser(self.match_decoders) > + mparser.parse(match) > + msection = Section( > + name="match", > + pos=ofp_string.find(match), > + string=match, > + data=mparser.kv(), > + ) > + sections.append(msection) > + > + aparser = KVParser(self.act_decoders) > + aparser.parse(actions) > + asection = Section( > + name="actions", > + pos=ofp_string.find(actions), > + string=actions, > + data=aparser.kv(), > + is_list=True, > + ) > + sections.append(asection) > + > + return OFPFlow(sections, ofp_string, id) > + > + @classmethod > + def _info_decoders(cls): > + """Generate the match decoders""" > + info = { > + "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(info) > + > + @classmethod > + def _flow_match_decoders(cls): > + """Returns the decoders for key-values that are part of the flow > match > + but not a flow field""" > + return { > + "priority": decode_int, > + } Guess this should be return KVDecoders({ "priority": decode_int, })) > + > + @classmethod > + def _field_decoders(cls): > + 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 This should return KVDecoders(fields). Guess this is true for all class methods below! Or, you should choose to always return the list. But not mix the two. > + > + @classmethod > + def _output_actions_decoders(cls): > + """Returns the decoders for the output actions""" If you have them, I think it might be useful, to add some example strings to the individual decoders. This way, it's easy to see what they intend to decode. > + 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, > + } > + > + @classmethod > + def _encap_actions_decoders(cls): > + """Returns the decoders 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, > + ) > + ), > + } > + > + @classmethod > + def _field_action_decoders(cls): > + """Returns the decoders for the 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(cls._field_decoders()) > + ), > + "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}, > + } > + > + @classmethod > + def _meta_action_decoders(cls): > + """Returns the decoders 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}, > + } > + > + @classmethod > + def _fw_action_decoders(cls): > + """Returns the decoders 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( > + { > + **cls._encap_actions_decoders(), > + **cls._field_action_decoders(), > + **cls._meta_action_decoders(), > + } > + ), > + ), > + "alg": decode_default, > + } > + ) > + ), > + "ct_clear": decode_flag, > + } > + > + @classmethod > + def _control_action_decoders(cls): > + 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), > + ] > + ) > + ), > + } > + > + @classmethod > + def _clone_actions_decoders(cls, action_decoders): > + """Generate the decoders for 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) > + ), > + } > + > + @classmethod > + def _other_action_decoders(cls): > + """Recoders 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, > + } > + ) > + ), > + } > + > + @classmethod > + def _act_decoders(cls): > + """Generate the actions decoders""" > + > + actions = { > + **cls._output_actions_decoders(), > + **cls._encap_actions_decoders(), > + **cls._field_action_decoders(), > + **cls._meta_action_decoders(), > + **cls._fw_action_decoders(), > + **cls._control_action_decoders(), > + **cls._other_action_decoders(), > + } > + clone_actions = cls._clone_actions_decoders(actions) > + actions.update(clone_actions) > + return KVDecoders(actions, default_free=decode_free_output) > diff --git a/python/ovs/flows/ofp_act.py b/python/ovs/flows/ofp_act.py > new file mode 100644 > index 000000000..bc6574999 > --- /dev/null > +++ b/python/ovs/flows/ofp_act.py > @@ -0,0 +1,233 @@ > +""" 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""" Was confused with ethertype here, but I think ethernet does not have a value does it? Just encap(ethernet)? > + 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:value->dst' actions""" > + 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:value/mask->dst' actions > + > + The value is decoded by field_decoders which is a KVDecoders instance > + Args: > + field_decoders > + """ > + 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:src->dst' actions""" > + 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 'zone' keyword of the ct action""" > + try: > + return int(value, 0) > + except ValueError: > + pass > + return decode_field(value) > + > + > +def decode_exec(action_decoders, value): > + """Decodes the 'exec' keyword 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(action_decoders) > + exec_parser.parse(value) > + return [{kv.key: kv.value} for kv in exec_parser.kv()] > + > + > +def decode_learn(action_decoders): It’s getting late, and I have a hard time (focussing ;) understanding where the value for this one comes from? I'll pick it up from here when I continue the review. > + """Create the decoder to be used to decode the 'learn' action. > + > + The learn action can include any nested action, therefore we need > decoders > + for all possible actions. > + > + 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, > + "cooke": 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.31.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
