The openvswitch selftests currently contain a few cases for managing the datapath, which includes creating datapath instances, adding interfaces, and doing some basic feature / upcall tests. This is useful to validate the control path.
Add the ability to program some of the more common flows with actions. This can be improved overtime to include regression testing, etc.
Aaron Conole (4): selftests: openvswitch: add an initial flow programming case selftests: openvswitch: add a test for ipv4 forwarding selftests: openvswitch: add basic ct test case parsing selftests: openvswitch: add ct-nat test case with ipv4
.../selftests/net/openvswitch/openvswitch.sh | 223 ++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 507 ++++++++++++++++++ 2 files changed, 730 insertions(+)
The openvswitch self-tests can test much of the control side of the module (ie: what a vswitchd implementation would process), but the actual packet forwarding cases aren't supported, making the testing of limited value.
Add some flow parsing and an initial ARP based test case using arping utility. This lets us display flows, add some basic output flows with simple matches, and test against a known good forwarding case.
Signed-off-by: Aaron Conole aconole@redhat.com --- NOTE: 3 lines flag the line-length checkpatch warning, but there didn't seem to bea good way of breaking the lines smaller for 2 of them. The third would still flag, even if broken at what looks like a good point to break it.
.../selftests/net/openvswitch/openvswitch.sh | 51 +++ .../selftests/net/openvswitch/ovs-dpctl.py | 408 ++++++++++++++++++ 2 files changed, 459 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 3117a4be0cd04..5cdacb3c8c925 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -11,6 +11,7 @@ VERBOSE=0 TRACING=0
tests=" + arp_ping eth-arp: Basic arp ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -127,6 +128,16 @@ ovs_add_netns_and_veths () { return 0 }
+ovs_add_flow () { + info "Adding flow to DP: sbx:$1 br:$2 flow:$3 act:$4" + ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py add-flow "$2" "$3" "$4" + if [ $? -ne 0 ]; then + echo "Flow [ $3 : $4 ] failed" >> ${ovs_dir}/debug.log + return 1 + fi + return 0 +} + usage() { echo echo "$0 [OPTIONS] [TEST]..." @@ -141,6 +152,46 @@ usage() { exit 1 }
+# arp_ping test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - send ARP ping between two ns +test_arp_ping () { + + which arping >/dev/null 2>&1 || return $ksft_skip + + sbx_add "test_arp_ping" || return $? + + ovs_add_dp "test_arp_ping" arpping || return 1 + + info "create namespaces" + for ns in client server; do + ovs_add_netns_and_veths "test_arp_ping" "arpping" "$ns" \ + "${ns:0:1}0" "${ns:0:1}1" || return 1 + done + + # Setup client namespace + ip netns exec client ip addr add 172.31.110.10/24 dev c1 + ip netns exec client ip link set c1 up + HW_CLIENT=`ip netns exec client ip link show dev c1 | grep -E 'link/ether [0-9a-f:]+' | awk '{print $2;}'` + info "Client hwaddr: $HW_CLIENT" + + # Setup server namespace + ip netns exec server ip addr add 172.31.110.20/24 dev s1 + ip netns exec server ip link set s1 up + HW_SERVER=`ip netns exec server ip link show dev s1 | grep -E 'link/ether [0-9a-f:]+' | awk '{print $2;}'` + info "Server hwaddr: $HW_SERVER" + + ovs_add_flow "test_arp_ping" arpping \ + "in_port(1),eth(),eth_type(0x0806),arp(sip=172.31.110.10,tip=172.31.110.20,sha=$HW_CLIENT,tha=ff:ff:ff:ff:ff:ff)" '2' || return 1 + ovs_add_flow "test_arp_ping" arpping \ + "in_port(2),eth(),eth_type(0x0806),arp()" '1' || return 1 + + ovs_sbx "test_arp_ping" ip netns exec client arping -I c1 172.31.110.20 -c 1 || return 1 + + return 0 +} + # netlink_validation # - Create a dp # - check no warning with "old version" simulation diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 1c8b36bc15d48..799bfb3064b90 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -9,9 +9,12 @@ import errno import ipaddress import logging import multiprocessing +import re import struct import sys import time +import types +import uuid
try: from pyroute2 import NDB @@ -59,6 +62,104 @@ def macstr(mac): return outstr
+def strspn(str1, str2): + tot = 0 + for char in str1: + if str2.find(char) == -1: + return tot + tot += 1 + return tot + + +def intparse(statestr, defmask="0xffffffff"): + totalparse = strspn(statestr, "0123456789abcdefABCDEFx/") + # scan until "/" + count = strspn(statestr, "x0123456789abcdefABCDEF") + + firstnum = statestr[:count] + if firstnum[-1] == "/": + firstnum = firstnum[:-1] + k = int(firstnum, 0) + + m = None + if defmask is not None: + secondnum = defmask + if statestr[count] == "/": + secondnum = statestr[count + 1 :] # this is wrong... + m = int(secondnum, 0) + + return statestr[totalparse + 1 :], k, m + + +def parse_flags(flag_str, flag_vals): + bitResult = 0 + maskResult = 0 + + if len(flag_str) == 0: + return flag_str, bitResult, maskResult + + if flag_str[0].isdigit(): + idx = 0 + while flag_str[idx].isdigit() or flag_str[idx] == "x": + idx += 1 + digits = flag_str[:idx] + flag_str = flag_str[idx:] + + bitResult = int(digits, 0) + maskResult = int(digits, 0) + + while len(flag_str) > 0 and (flag_str[0] == "+" or flag_str[0] == "-"): + if flag_str[0] == "+": + setFlag = True + elif flag_str[0] == "-": + setFlag = False + + flag_str = flag_str[1:] + + flag_len = 0 + while ( + flag_str[flag_len] != "+" + and flag_str[flag_len] != "-" + and flag_str[flag_len] != "," + and flag_str[flag_len] != ")" + ): + flag_len += 1 + + flag = flag_str[0:flag_len] + + if flag in flag_vals: + if maskResult & flag_vals[flag]: + raise KeyError( + "Flag %s set once, cannot be set in multiples" % flag + ) + + if setFlag: + bitResult |= flag_vals[flag] + + maskResult |= flag_vals[flag] + else: + raise KeyError("Missing flag value: %s" % flag) + + flag_str = flag_str[flag_len:] + + return flag_str, bitResult, maskResult + + +def parse_ct_state(statestr): + ct_flags = { + "new": 1 << 0, + "est": 1 << 1, + "rel": 1 << 2, + "rpl": 1 << 3, + "inv": 1 << 4, + "trk": 1 << 5, + "snat": 1 << 6, + "dnat": 1 << 7, + } + + return parse_flags(statestr, ct_flags) + + def convert_mac(mac_str, mask=False): if mac_str is None or mac_str == "": mac_str = "00:00:00:00:00:00" @@ -79,6 +180,61 @@ def convert_ipv4(ip, mask=False): return int(ipaddress.IPv4Address(ip))
+def parse_starts_block(block_str, scanstr, returnskipped, scanregex=False): + if scanregex: + m = re.search(scanstr, block_str) + if m is None: + if returnskipped: + return block_str + return False + if returnskipped: + block_str = block_str[len(m.group(0)) :] + return block_str + return True + + if block_str.startswith(scanstr): + if returnskipped: + block_str = block_str[len(scanstr) :] + else: + return True + + if returnskipped: + return block_str + + return False + + +def parse_extract_field( + block_str, fieldstr, scanfmt, convert, masked=False, defval=None +): + if fieldstr and not block_str.startswith(fieldstr): + return block_str, defval + + if fieldstr: + str_skiplen = len(fieldstr) + str_skipped = block_str[str_skiplen:] + if str_skiplen == 0: + return str_skipped, defval + else: + str_skiplen = 0 + str_skipped = block_str + + m = re.search(scanfmt, str_skipped) + if m is None: + raise ValueError("Bad fmt string") + + data = m.group(0) + if convert: + data = convert(m.group(0)) + + str_skipped = str_skipped[len(m.group(0)) :] + if masked: + if str_skipped[0] == "/": + raise ValueError("Masking support TBD...") + + return str_skipped, data + + class ovs_dp_msg(genlmsg): # include the OVS version # We need a custom header rather than just being able to rely on @@ -278,6 +434,52 @@ class ovsactions(nla):
return print_str
+ def parse(self, actstr): + parsed = False + while len(actstr) != 0: + if actstr.startswith("drop"): + # for now, drops have no explicit action, so we + # don't need to set any attributes. The final + # act of the processing chain will just drop the packet + return + + elif parse_starts_block(actstr, "^(\d+)", False, True): + actstr, output = parse_extract_field( + actstr, None, "(\d+)", lambda x: int(x), False, "0" + ) + actstr = actstr[strspn(actstr, ", ") :] + self["attrs"].append(["OVS_ACTION_ATTR_OUTPUT", output]) + parsed = True + elif parse_starts_block(actstr, "recirc(", False): + actstr, recircid = parse_extract_field( + actstr, + "recirc(", + "([0-9a-fA-Fx]+)", + lambda x: int(x, 0), + False, + 0, + ) + actstr = actstr[strspn(actstr, "), ") :] + self["attrs"].append(["OVS_ACTION_ATTR_RECIRC", recircid]) + parsed = True + + parse_flat_map = ( + ("ct_clear", "OVS_ACTION_ATTR_CT_CLEAR"), + ("pop_vlan", "OVS_ACTION_ATTR_POP_VLAN"), + ("pop_eth", "OVS_ACTION_ATTR_POP_ETH"), + ("pop_nsh", "OVS_ACTION_ATTR_POP_NSH"), + ) + + for flat_act in parse_flat_map: + if parse_starts_block(actstr, flat_act[0], False): + actstr += len(flat_act[0]) + self["attrs"].append([flat_act[1]]) + actstr = actstr[strspn(actstr, ", ") :] + parsed = True + + if not parsed: + raise ValueError("Action str: '%s' not supported" % actstr) +
class ovskey(nla): nla_flags = NLA_F_NESTED @@ -347,6 +549,53 @@ class ovskey(nla): init=init, )
+ def parse(self, flowstr, typeInst): + if not flowstr.startswith(self.proto_str): + return None, None + + k = typeInst() + m = typeInst() + + flowstr = flowstr[len(self.proto_str) :] + if flowstr.startswith("("): + flowstr = flowstr[1:] + + keybits = b"" + maskbits = b"" + for f in self.fields_map: + if flowstr.startswith(f[1]): + # the following assumes that the field looks + # something like 'field.' where '.' is a + # character that we don't exactly care about. + flowstr = flowstr[len(f[1]) + 1 :] + splitchar = 0 + for c in flowstr: + if c == "," or c == ")": + break + splitchar += 1 + data = flowstr[:splitchar] + flowstr = flowstr[splitchar:] + else: + data = None + + if len(f) > 4: + func = f[4] + else: + func = f[3] + k[f[0]] = func(data) + if len(f) > 4: + m[f[0]] = func(data, True) + else: + m[f[0]] = func(data) + + flowstr = flowstr[strspn(flowstr, ", ") :] + if len(flowstr) == 0: + return flowstr, k, m + + flowstr = flowstr[strspn(flowstr, "), ") :] + + return flowstr, k, m + def dpstr(self, masked=None, more=False): outstr = self.proto_str + "(" first = False @@ -810,6 +1059,71 @@ class ovskey(nla): class ovs_key_mpls(nla): fields = (("lse", ">I"),)
+ def parse(self, flowstr, mask=None): + for field in ( + ("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse), + ("OVS_KEY_ATTR_SKB_MARK", "skb_mark", intparse), + ("OVS_KEY_ATTR_RECIRC_ID", "recirc_id", intparse), + ("OVS_KEY_ATTR_DP_HASH", "dp_hash", intparse), + ("OVS_KEY_ATTR_CT_STATE", "ct_state", parse_ct_state), + ("OVS_KEY_ATTR_CT_ZONE", "ct_zone", intparse), + ("OVS_KEY_ATTR_CT_MARK", "ct_mark", intparse), + ("OVS_KEY_ATTR_IN_PORT", "in_port", intparse), + ( + "OVS_KEY_ATTR_ETHERNET", + "eth", + ovskey.ethaddr, + ), + ( + "OVS_KEY_ATTR_ETHERTYPE", + "eth_type", + lambda x: intparse(x, "0xffff"), + ), + ( + "OVS_KEY_ATTR_IPV4", + "ipv4", + ovskey.ovs_key_ipv4, + ), + ( + "OVS_KEY_ATTR_IPV6", + "ipv6", + ovskey.ovs_key_ipv6, + ), + ( + "OVS_KEY_ATTR_ARP", + "arp", + ovskey.ovs_key_arp, + ), + ( + "OVS_KEY_ATTR_TCP", + "tcp", + ovskey.ovs_key_tcp, + ), + ( + "OVS_KEY_ATTR_TCP_FLAGS", + "tcp_flags", + lambda x: parse_flags(x, None), + ), + ): + fld = field[1] + "(" + if not flowstr.startswith(fld): + continue + + if not isinstance(field[2], types.FunctionType): + nk = field[2]() + flowstr, k, m = nk.parse(flowstr, field[2]) + else: + flowstr = flowstr[len(fld) :] + flowstr, k, m = field[2](flowstr) + + if m and mask is not None: + mask["attrs"].append([field[0], m]) + self["attrs"].append([field[0], k]) + + flowstr = flowstr[strspn(flowstr, "),") :] + + return flowstr + def dpstr(self, mask=None, more=False): print_str = ""
@@ -1358,11 +1672,92 @@ class OvsFlow(GenericNetlinkSocket):
return print_str
+ def parse(self, flowstr, actstr, dpidx=0): + OVS_UFID_F_OMIT_KEY = 1 << 0 + OVS_UFID_F_OMIT_MASK = 1 << 1 + OVS_UFID_F_OMIT_ACTIONS = 1 << 2 + + self["cmd"] = 0 + self["version"] = 0 + self["reserved"] = 0 + self["dpifindex"] = 0 + + if flowstr.startswith("ufid:"): + count = 5 + while flowstr[count] != ",": + count += 1 + ufidstr = flowstr[5:count] + flowstr = flowstr[count + 1 :] + else: + ufidstr = str(uuid.uuid4()) + uuidRawObj = uuid.UUID(ufidstr).fields + + self["attrs"].append( + [ + "OVS_FLOW_ATTR_UFID", + [ + uuidRawObj[0], + uuidRawObj[1] << 16 | uuidRawObj[2], + uuidRawObj[3] << 24 + | uuidRawObj[4] << 16 + | uuidRawObj[5] & (0xFF << 32) >> 32, + uuidRawObj[5] & (0xFFFFFFFF), + ], + ] + ) + self["attrs"].append( + [ + "OVS_FLOW_ATTR_UFID_FLAGS", + int( + OVS_UFID_F_OMIT_KEY + | OVS_UFID_F_OMIT_MASK + | OVS_UFID_F_OMIT_ACTIONS + ), + ] + ) + + k = ovskey() + m = ovskey() + k.parse(flowstr, m) + self["attrs"].append(["OVS_FLOW_ATTR_KEY", k]) + self["attrs"].append(["OVS_FLOW_ATTR_MASK", m]) + + a = ovsactions() + a.parse(actstr) + self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a]) + def __init__(self): GenericNetlinkSocket.__init__(self)
self.bind(OVS_FLOW_FAMILY, OvsFlow.ovs_flow_msg)
+ def add_flow(self, dpifindex, flowmsg): + """ + Send a new flow message to the kernel. + + dpifindex should be a valid datapath obtained by calling + into the OvsDatapath lookup + + flowmsg is a flow object obtained by calling a dpparse + """ + + flowmsg["cmd"] = OVS_FLOW_CMD_NEW + flowmsg["version"] = OVS_DATAPATH_VERSION + flowmsg["reserved"] = 0 + flowmsg["dpifindex"] = dpifindex + + try: + reply = self.nlm_request( + flowmsg, + msg_type=self.prid, + msg_flags=NLM_F_REQUEST | NLM_F_ACK, + ) + reply = reply[0] + except NetlinkError as ne: + print(flowmsg) + raise ne + return reply + def dump(self, dpifindex, flowspec=None): """ Returns a list of messages containing flows. @@ -1514,6 +1909,11 @@ def main(argv): dumpflcmd = subparsers.add_parser("dump-flows") dumpflcmd.add_argument("dumpdp", help="Datapath Name")
+ addflcmd = subparsers.add_parser("add-flow") + addflcmd.add_argument("flbr", help="Datapath name") + addflcmd.add_argument("flow", help="Flow specification") + addflcmd.add_argument("acts", help="Flow actions") + args = parser.parse_args()
if args.verbose > 0: @@ -1589,6 +1989,14 @@ def main(argv): rep = ovsflow.dump(rep["dpifindex"]) for flow in rep: print(flow.dpstr(True if args.verbose > 0 else False)) + elif hasattr(args, "flbr"): + rep = ovsdp.info(args.flbr, 0) + if rep is None: + print("DP '%s' not found." % args.dumpdp) + return 1 + flow = OvsFlow.ovs_flow_msg() + flow.parse(args.flow, args.acts, rep["dpifindex"]) + ovsflow.add_flow(rep["dpifindex"], flow)
return 0
This is a simple ipv4 bidirectional connectivity test.
Signed-off-by: Aaron Conole aconole@redhat.com --- .../selftests/net/openvswitch/openvswitch.sh | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 5cdacb3c8c925..5d60a9466dab3 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -12,6 +12,7 @@ TRACING=0
tests=" arp_ping eth-arp: Basic arp ping between two NS + connect_v4 ip4-xon: Basic ipv4 ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -192,6 +193,45 @@ test_arp_ping () { return 0 }
+# connect_v4 test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +test_connect_v4 () { + + sbx_add "test_connect_v4" || return $? + + ovs_add_dp "test_connect_v4" cv4 || return 1 + + info "create namespaces" + for ns in client server; do + ovs_add_netns_and_veths "test_connect_v4" "cv4" "$ns" \ + "${ns:0:1}0" "${ns:0:1}1" || return 1 + done + + + ip netns exec client ip addr add 172.31.110.10/24 dev c1 + ip netns exec client ip link set c1 up + ip netns exec server ip addr add 172.31.110.20/24 dev s1 + ip netns exec server ip link set s1 up + + # Add forwarding for ARP and ip packets - completely wildcarded + ovs_add_flow "test_connect_v4" cv4 \ + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1 + ovs_add_flow "test_connect_v4" cv4 \ + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1 + ovs_add_flow "test_connect_v4" cv4 \ + 'in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' '2' || return 1 + ovs_add_flow "test_connect_v4" cv4 \ + 'in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20)' '1' || return 1 + + # do a ping + ovs_sbx "test_connect_v4" ip netns exec client ping 172.31.110.20 -c 3 || return 1 + + info "done..." + return 0 +} + # netlink_validation # - Create a dp # - check no warning with "old version" simulation
Forwarding via ct() action is an important use case for openvswitch, but generally would require using a full ovs-vswitchd to get working. Add a ct action parser for basic ct test case.
Signed-off-by: Aaron Conole aconole@redhat.com --- NOTE: 3 lines flag the line-length checkpatch warning, but there didnt seem to be a really good way of breaking the lines smaller.
.../selftests/net/openvswitch/openvswitch.sh | 68 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 39 +++++++++++ 2 files changed, 107 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 5d60a9466dab3..40a66c72af0f0 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -12,6 +12,7 @@ TRACING=0
tests=" arp_ping eth-arp: Basic arp ping between two NS + ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces" @@ -193,6 +194,73 @@ test_arp_ping () { return 0 }
+# ct_connect_v4 test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +# - only allow CT state stuff to pass through new in c -> s +test_ct_connect_v4 () { + + which nc >/dev/null 2>/dev/null || return $ksft_skip + + sbx_add "test_ct_connect_v4" || return $? + + ovs_add_dp "test_ct_connect_v4" ct4 || return 1 + info "create namespaces" + for ns in client server; do + ovs_add_netns_and_veths "test_ct_connect_v4" "ct4" "$ns" \ + "${ns:0:1}0" "${ns:0:1}1" || return 1 + done + + ip netns exec client ip addr add 172.31.110.10/24 dev c1 + ip netns exec client ip link set c1 up + ip netns exec server ip addr add 172.31.110.20/24 dev s1 + ip netns exec server ip link set s1 up + + # Add forwarding for ARP and ip packets - completely wildcarded + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1 + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1 + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'ct_state(-trk),eth(),eth_type(0x0800),ipv4()' \ + 'ct(commit),recirc(0x1)' || return 1 + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'recirc_id(0x1),ct_state(+trk+new),in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' \ + '2' || return 1 + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'recirc_id(0x1),ct_state(+trk+est),in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' \ + '2' || return 1 + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'recirc_id(0x1),ct_state(+trk+est),in_port(2),eth(),eth_type(0x0800),ipv4(dst=172.31.110.10)' \ + '1' || return 1 + ovs_add_flow "test_ct_connect_v4" ct4 \ + 'recirc_id(0x1),ct_state(+trk+inv),eth(),eth_type(0x0800),ipv4()' 'drop' || \ + return 1 + + # do a ping + ovs_sbx "test_ct_connect_v4" ip netns exec client ping 172.31.110.20 -c 3 || return 1 + + # create an echo server in 'server' + echo "server" | \ + ovs_netns_spawn_daemon "test_ct_connect_v4" "server" \ + nc -lvnp 4443 + ovs_sbx "test_ct_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.20 4443 || return 1 + + # Now test in the other direction (should fail) + echo "client" | \ + ovs_netns_spawn_daemon "test_ct_connect_v4" "client" \ + nc -lvnp 4443 + ovs_sbx "test_ct_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443 + if [ $? == 0 ]; then + info "ct connect to client was successful" + return 1 + fi + + info "done..." + return 0 +} + # connect_v4 test # - client has 1500 byte MTU # - server has 1500 byte MTU diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 799bfb3064b90..704cb4adf79a9 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -62,6 +62,15 @@ def macstr(mac): return outstr
+def strcspn(str1, str2): + tot = 0 + for char in str1: + if str2.find(char) != -1: + return tot + tot += 1 + return tot + + def strspn(str1, str2): tot = 0 for char in str1: @@ -477,6 +486,36 @@ class ovsactions(nla): actstr = actstr[strspn(actstr, ", ") :] parsed = True
+ if parse_starts_block(actstr, "ct(", False): + actstr = actstr[len("ct(") :] + ctact = ovsactions.ctact() + + for scan in ( + ("commit", "OVS_CT_ATTR_COMMIT", None), + ("force_commit", "OVS_CT_ATTR_FORCE_COMMIT", None), + ("zone", "OVS_CT_ATTR_ZONE", int), + ("mark", "OVS_CT_ATTR_MARK", int), + ("helper", "OVS_CT_ATTR_HELPER", lambda x, y: str(x)), + ("timeout", "OVS_CT_ATTR_TIMEOUT", lambda x, y: str(x)), + ): + if actstr.startswith(scan[0]): + actstr = actstr[len(scan[0]) :] + if scan[2] is not None: + if actstr[0] != "=": + raise ValueError("Invalid ct attr") + actstr = actstr[1:] + pos = strcspn(actstr, ",)") + datum = scan[2](actstr[:pos], 0) + ctact["attrs"].append([scan[1], datum]) + actstr = actstr[len(datum) :] + else: + ctact["attrs"].append([scan[1], None]) + actstr = actstr[strspn(actstr, ", ") :] + + self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact]) + actstr = actstr[strspn(actstr, "), ") :] + parsed = True + if not parsed: raise ValueError("Action str: '%s' not supported" % actstr)
Building on the previous work, add a very simplistic NAT case using ipv4. This just tests dnat transformation
Signed-off-by: Aaron Conole aconole@redhat.com --- .../selftests/net/openvswitch/openvswitch.sh | 64 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 60 +++++++++++++++++ 2 files changed, 124 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 40a66c72af0f0..dced4f612a78c 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -14,6 +14,7 @@ tests=" arp_ping eth-arp: Basic arp ping between two NS ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS + nat_connect_v4 ip4-nat-xon: Basic ipv4 tcp connection via NAT netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -300,6 +301,69 @@ test_connect_v4 () { return 0 }
+# nat_connect_v4 test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +# - only allow CT state stuff to pass through new in c -> s +test_nat_connect_v4 () { + which nc >/dev/null 2>/dev/null || return $ksft_skip + + sbx_add "test_nat_connect_v4" || return $? + + ovs_add_dp "test_nat_connect_v4" nat4 || return 1 + info "create namespaces" + for ns in client server; do + ovs_add_netns_and_veths "test_nat_connect_v4" "nat4" "$ns" \ + "${ns:0:1}0" "${ns:0:1}1" || return 1 + done + + ip netns exec client ip addr add 172.31.110.10/24 dev c1 + ip netns exec client ip link set c1 up + ip netns exec server ip addr add 172.31.110.20/24 dev s1 + ip netns exec server ip link set s1 up + + ip netns exec client ip route add default via 172.31.110.20 + + ovs_add_flow "test_nat_connect_v4" nat4 \ + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1 + ovs_add_flow "test_nat_connect_v4" nat4 \ + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1 + ovs_add_flow "test_nat_connect_v4" nat4 \ + "ct_state(-trk),in_port(1),eth(),eth_type(0x0800),ipv4(dst=192.168.0.20)" \ + "ct(commit,nat(dst=172.31.110.20)),recirc(0x1)" + ovs_add_flow "test_nat_connect_v4" nat4 \ + "ct_state(-trk),in_port(2),eth(),eth_type(0x0800),ipv4()" \ + "ct(commit,nat),recirc(0x2)" + + ovs_add_flow "test_nat_connect_v4" nat4 \ + "recirc_id(0x1),ct_state(+trk-inv),in_port(1),eth(),eth_type(0x0800),ipv4()" "2" + ovs_add_flow "test_nat_connect_v4" nat4 \ + "recirc_id(0x2),ct_state(+trk-inv),in_port(2),eth(),eth_type(0x0800),ipv4()" "1" + + # do a ping + ovs_sbx "test_nat_connect_v4" ip netns exec client ping 192.168.0.20 -c 3 || return 1 + + # create an echo server in 'server' + echo "server" | \ + ovs_netns_spawn_daemon "test_nat_connect_v4" "server" \ + nc -lvnp 4443 + ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 192.168.0.20 4443 || return 1 + + # Now test in the other direction (should fail) + echo "client" | \ + ovs_netns_spawn_daemon "test_nat_connect_v4" "client" \ + nc -lvnp 4443 + ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443 + if [ $? == 0 ]; then + info "connect to client was successful" + return 1 + fi + + info "done..." + return 0 +} + # netlink_validation # - Create a dp # - check no warning with "old version" simulation diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 704cb4adf79a9..12ba5265b88fb 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -511,6 +511,66 @@ class ovsactions(nla): else: ctact["attrs"].append([scan[1], None]) actstr = actstr[strspn(actstr, ", ") :] + # it seems strange to put this here, but nat() is a complex + # sub-action and this lets it sit anywhere in the ct() action + if actstr.startswith("nat"): + actstr = actstr[3:] + natact = ovsactions.ctact.natattr() + + if actstr.startswith("("): + t = None + actstr = actstr[1:] + if actstr.startswith("src"): + t = "OVS_NAT_ATTR_SRC" + actstr = actstr[3:] + elif actstr.startswith("dst"): + t = "OVS_NAT_ATTR_DST" + actstr = actstr[3:] + + actstr, ip_block_min = parse_extract_field( + actstr, "=", "([0-9a-fA-F:.]+)", str, False + ) + actstr, ip_block_max = parse_extract_field( + actstr, "-", "([0-9a-fA-F:.]+)", str, False + ) + actstr, proto_min = parse_extract_field( + actstr, ":", "(\d+)", int, False + ) + actstr, proto_max = parse_extract_field( + actstr, "-", "(\d+)", int, False + ) + if t is not None: + natact["attrs"].append([t, None]) + + if ip_block_min is not None: + natact["attrs"].append( + ["OVS_NAT_ATTR_IP_MIN", ip_block_min] + ) + if ip_block_max is not None: + natact["attrs"].append( + ["OVS_NAT_ATTR_IP_MAX", ip_block_max] + ) + if proto_min is not None: + natact["attrs"].append( + ["OVS_NAT_ATTR_PROTO_MIN", proto_min] + ) + if proto_max is not None: + natact["attrs"].append( + ["OVS_NAT_ATTR_PROTO_MAX", proto_max] + ) + + for natscan in ( + ("persist", "OVS_NAT_ATTR_PERSISTENT"), + ("hash", "OVS_NAT_ATTR_PROTO_HASH"), + ("random", "OVS_NAT_ATTR_PROTO_RANDOM"), + ): + if actstr.startswith(natscan[0]): + actstr = actstr[len(natscan[0]) :] + natact["attrs"].append([natscan[1], None]) + actstr = actstr[strspn(actstr, ", ") :] + + ctact["attrs"].append(["OVS_CT_ATTR_NAT", natact]) + actstr = actstr[strspn(actstr, ",) ") :]
self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact]) actstr = actstr[strspn(actstr, "), ") :]
On Wed, Jun 28, 2023 at 12:27:10PM -0400, Aaron Conole wrote:
The openvswitch selftests currently contain a few cases for managing the datapath, which includes creating datapath instances, adding interfaces, and doing some basic feature / upcall tests. This is useful to validate the control path.
Add the ability to program some of the more common flows with actions. This can be improved overtime to include regression testing, etc.
Hi Aaron,
sorry but:
[text from Jakub]
## Form letter - net-next-closed
The merge window for v6.5 has begun and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only.
Please repost when net-next reopens after July 10th.
RFC patches sent for review only are obviously welcome at any time.
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#developm... -- pw-bot: defer
Simon Horman simon.horman@corigine.com writes:
On Wed, Jun 28, 2023 at 12:27:10PM -0400, Aaron Conole wrote:
The openvswitch selftests currently contain a few cases for managing the datapath, which includes creating datapath instances, adding interfaces, and doing some basic feature / upcall tests. This is useful to validate the control path.
Add the ability to program some of the more common flows with actions. This can be improved overtime to include regression testing, etc.
Hi Aaron,
sorry but:
[text from Jakub]
## Form letter - net-next-closed
The merge window for v6.5 has begun and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only.
Please repost when net-next reopens after July 10th.
RFC patches sent for review only are obviously welcome at any time.
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#developm...
pw-bot: defer
Thanks Simon. I skipped looking at the ML this time and used the site setup for random German tourists: http://vger.kernel.org/~davem/net-next.html
I guess I'll stop using it and just check the ML as normal :) Sorry for the noise.
On Thu, Jun 29, 2023 at 08:25:35AM -0400, Aaron Conole wrote:
Simon Horman simon.horman@corigine.com writes:
On Wed, Jun 28, 2023 at 12:27:10PM -0400, Aaron Conole wrote:
The openvswitch selftests currently contain a few cases for managing the datapath, which includes creating datapath instances, adding interfaces, and doing some basic feature / upcall tests. This is useful to validate the control path.
Add the ability to program some of the more common flows with actions. This can be improved overtime to include regression testing, etc.
Hi Aaron,
sorry but:
[text from Jakub]
## Form letter - net-next-closed
The merge window for v6.5 has begun and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only.
Please repost when net-next reopens after July 10th.
RFC patches sent for review only are obviously welcome at any time.
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#developm...
pw-bot: defer
Thanks Simon. I skipped looking at the ML this time and used the site setup for random German tourists: http://vger.kernel.org/~davem/net-next.html
I guess I'll stop using it and just check the ML as normal :) Sorry for the noise.
Thanks Aaron,
unfortunately that page is not always up to date.
On 6/28/23 18:27, Aaron Conole wrote:
Forwarding via ct() action is an important use case for openvswitch, but generally would require using a full ovs-vswitchd to get working. Add a ct action parser for basic ct test case.
Signed-off-by: Aaron Conole aconole@redhat.com
NOTE: 3 lines flag the line-length checkpatch warning, but there didnt seem to be a really good way of breaking the lines smaller.
.../selftests/net/openvswitch/openvswitch.sh | 68 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 39 +++++++++++ 2 files changed, 107 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 5d60a9466dab3..40a66c72af0f0 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -12,6 +12,7 @@ TRACING=0 tests=" arp_ping eth-arp: Basic arp ping between two NS
- ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -193,6 +194,73 @@ test_arp_ping () { return 0 } +# ct_connect_v4 test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +# - only allow CT state stuff to pass through new in c -> s +test_ct_connect_v4 () {
- which nc >/dev/null 2>/dev/null || return $ksft_skip
- sbx_add "test_ct_connect_v4" || return $?
- ovs_add_dp "test_ct_connect_v4" ct4 || return 1
- info "create namespaces"
- for ns in client server; do
ovs_add_netns_and_veths "test_ct_connect_v4" "ct4" "$ns" \"${ns:0:1}0" "${ns:0:1}1" || return 1- done
- ip netns exec client ip addr add 172.31.110.10/24 dev c1
- ip netns exec client ip link set c1 up
- ip netns exec server ip addr add 172.31.110.20/24 dev s1
- ip netns exec server ip link set s1 up
- # Add forwarding for ARP and ip packets - completely wildcarded
- ovs_add_flow "test_ct_connect_v4" ct4 \
'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'ct_state(-trk),eth(),eth_type(0x0800),ipv4()' \'ct(commit),recirc(0x1)' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+new),in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' \'2' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+est),in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' \'2' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+est),in_port(2),eth(),eth_type(0x0800),ipv4(dst=172.31.110.10)' \'1' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+inv),eth(),eth_type(0x0800),ipv4()' 'drop' || \return 1- # do a ping
- ovs_sbx "test_ct_connect_v4" ip netns exec client ping 172.31.110.20 -c 3 || return 1
- # create an echo server in 'server'
- echo "server" | \
ovs_netns_spawn_daemon "test_ct_connect_v4" "server" \nc -lvnp 4443- ovs_sbx "test_ct_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.20 4443 || return 1
- # Now test in the other direction (should fail)
- echo "client" | \
ovs_netns_spawn_daemon "test_ct_connect_v4" "client" \nc -lvnp 4443- ovs_sbx "test_ct_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443
- if [ $? == 0 ]; then
info "ct connect to client was successful"return 1- fi
- info "done..."
- return 0
+}
- # connect_v4 test # - client has 1500 byte MTU # - server has 1500 byte MTU
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 799bfb3064b90..704cb4adf79a9 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -62,6 +62,15 @@ def macstr(mac): return outstr +def strcspn(str1, str2):
- tot = 0
- for char in str1:
if str2.find(char) != -1:return tottot += 1- return tot
- def strspn(str1, str2): tot = 0 for char in str1:
@@ -477,6 +486,36 @@ class ovsactions(nla): actstr = actstr[strspn(actstr, ", ") :] parsed = True
if parse_starts_block(actstr, "ct(", False):actstr = actstr[len("ct(") :]ctact = ovsactions.ctact()for scan in (("commit", "OVS_CT_ATTR_COMMIT", None),("force_commit", "OVS_CT_ATTR_FORCE_COMMIT", None),("zone", "OVS_CT_ATTR_ZONE", int),("mark", "OVS_CT_ATTR_MARK", int),("helper", "OVS_CT_ATTR_HELPER", lambda x, y: str(x)),("timeout", "OVS_CT_ATTR_TIMEOUT", lambda x, y: str(x)),):if actstr.startswith(scan[0]):actstr = actstr[len(scan[0]) :]if scan[2] is not None:if actstr[0] != "=":raise ValueError("Invalid ct attr")actstr = actstr[1:]pos = strcspn(actstr, ",)")datum = scan[2](actstr[:pos], 0)
It seems the scan function is only called with "0" as second argument. Wouldn't it be easier to omit that extra argument (which is the default value for "int" anyway) and simplify the lambda for "helper" and "timeout"?
Alternatively, we could have a function that tries both base-16 and base-10.
ctact["attrs"].append([scan[1], datum])actstr = actstr[len(datum) :]
"datum" can be of type int and ints don't have len(). Maybe just use "pos" directly?
else:ctact["attrs"].append([scan[1], None])actstr = actstr[strspn(actstr, ", ") :]self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact])actstr = actstr[strspn(actstr, "), ") :]parsed = Trueif not parsed: raise ValueError("Action str: '%s' not supported" % actstr)
On 6/28/23 18:27, Aaron Conole wrote:
Building on the previous work, add a very simplistic NAT case using ipv4. This just tests dnat transformation
Signed-off-by: Aaron Conole aconole@redhat.com
Hi Aaron,
I know that the goal is not to support the full syntax, and that nat is a specially convoluted action, so I'm just commenting on the low-hanging fruits (see below).
.../selftests/net/openvswitch/openvswitch.sh | 64 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 60 +++++++++++++++++ 2 files changed, 124 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 40a66c72af0f0..dced4f612a78c 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -14,6 +14,7 @@ tests=" arp_ping eth-arp: Basic arp ping between two NS ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS
- nat_connect_v4 ip4-nat-xon: Basic ipv4 tcp connection via NAT netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -300,6 +301,69 @@ test_connect_v4 () { return 0 } +# nat_connect_v4 test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +# - only allow CT state stuff to pass through new in c -> s +test_nat_connect_v4 () {
- which nc >/dev/null 2>/dev/null || return $ksft_skip
- sbx_add "test_nat_connect_v4" || return $?
- ovs_add_dp "test_nat_connect_v4" nat4 || return 1
- info "create namespaces"
- for ns in client server; do
ovs_add_netns_and_veths "test_nat_connect_v4" "nat4" "$ns" \"${ns:0:1}0" "${ns:0:1}1" || return 1- done
- ip netns exec client ip addr add 172.31.110.10/24 dev c1
- ip netns exec client ip link set c1 up
- ip netns exec server ip addr add 172.31.110.20/24 dev s1
- ip netns exec server ip link set s1 up
- ip netns exec client ip route add default via 172.31.110.20
- ovs_add_flow "test_nat_connect_v4" nat4 \
'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1- ovs_add_flow "test_nat_connect_v4" nat4 \
'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1- ovs_add_flow "test_nat_connect_v4" nat4 \
"ct_state(-trk),in_port(1),eth(),eth_type(0x0800),ipv4(dst=192.168.0.20)" \"ct(commit,nat(dst=172.31.110.20)),recirc(0x1)"- ovs_add_flow "test_nat_connect_v4" nat4 \
"ct_state(-trk),in_port(2),eth(),eth_type(0x0800),ipv4()" \"ct(commit,nat),recirc(0x2)"- ovs_add_flow "test_nat_connect_v4" nat4 \
"recirc_id(0x1),ct_state(+trk-inv),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"- ovs_add_flow "test_nat_connect_v4" nat4 \
"recirc_id(0x2),ct_state(+trk-inv),in_port(2),eth(),eth_type(0x0800),ipv4()" "1"- # do a ping
- ovs_sbx "test_nat_connect_v4" ip netns exec client ping 192.168.0.20 -c 3 || return 1
- # create an echo server in 'server'
- echo "server" | \
ovs_netns_spawn_daemon "test_nat_connect_v4" "server" \nc -lvnp 4443- ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 192.168.0.20 4443 || return 1
- # Now test in the other direction (should fail)
- echo "client" | \
ovs_netns_spawn_daemon "test_nat_connect_v4" "client" \nc -lvnp 4443- ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443
- if [ $? == 0 ]; then
info "connect to client was successful"return 1- fi
- info "done..."
- return 0
+}
- # netlink_validation # - Create a dp # - check no warning with "old version" simulation
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 704cb4adf79a9..12ba5265b88fb 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -511,6 +511,66 @@ class ovsactions(nla): else: ctact["attrs"].append([scan[1], None]) actstr = actstr[strspn(actstr, ", ") :]
# it seems strange to put this here, but nat() is a complex# sub-action and this lets it sit anywhere in the ct() actionif actstr.startswith("nat"):actstr = actstr[3:]natact = ovsactions.ctact.natattr()if actstr.startswith("("):t = Noneactstr = actstr[1:]if actstr.startswith("src"):t = "OVS_NAT_ATTR_SRC"actstr = actstr[3:]elif actstr.startswith("dst"):t = "OVS_NAT_ATTR_DST"actstr = actstr[3:]actstr, ip_block_min = parse_extract_field(actstr, "=", "([0-9a-fA-F:\.]+)", str, False)actstr, ip_block_max = parse_extract_field(actstr, "-", "([0-9a-fA-F:\.]+)", str, False)
Having the ":" character here makes this line parse the port as well (i.e: 1.1.1.1:6789 as ip_block_max) which then makes ip address parsing fail.
actstr, proto_min = parse_extract_field(actstr, ":", "(\d+)", int, False)actstr, proto_max = parse_extract_field(actstr, "-", "(\d+)", int, False)if t is not None:natact["attrs"].append([t, None])if ip_block_min is not None:natact["attrs"].append(["OVS_NAT_ATTR_IP_MIN", ip_block_min])if ip_block_max is not None:natact["attrs"].append(["OVS_NAT_ATTR_IP_MAX", ip_block_max])if proto_min is not None:natact["attrs"].append(["OVS_NAT_ATTR_PROTO_MIN", proto_min])if proto_max is not None:natact["attrs"].append(["OVS_NAT_ATTR_PROTO_MAX", proto_max])for natscan in (("persist", "OVS_NAT_ATTR_PERSISTENT"),("hash", "OVS_NAT_ATTR_PROTO_HASH"),("random", "OVS_NAT_ATTR_PROTO_RANDOM"),):
I think this is not taking into account the comma that separates ip:port from these keywords. A possible solution would be to add it to the natscan (e.g: s/persist/,persist/).
if actstr.startswith(natscan[0]):actstr = actstr[len(natscan[0]) :]natact["attrs"].append([natscan[1], None])actstr = actstr[strspn(actstr, ", ") :]ctact["attrs"].append(["OVS_CT_ATTR_NAT", natact])actstr = actstr[strspn(actstr, ",) ") :]self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact]) actstr = actstr[strspn(actstr, "), ") :]
On 6/28/23 18:27, Aaron Conole wrote:
The openvswitch self-tests can test much of the control side of the module (ie: what a vswitchd implementation would process), but the actual packet forwarding cases aren't supported, making the testing of limited value.
Add some flow parsing and an initial ARP based test case using arping utility. This lets us display flows, add some basic output flows with simple matches, and test against a known good forwarding case.
Signed-off-by: Aaron Conole aconole@redhat.com
NOTE: 3 lines flag the line-length checkpatch warning, but there didn't seem to bea good way of breaking the lines smaller for 2 of them. The third would still flag, even if broken at what looks like a good point to break it.
.../selftests/net/openvswitch/openvswitch.sh | 51 +++ .../selftests/net/openvswitch/ovs-dpctl.py | 408 ++++++++++++++++++ 2 files changed, 459 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 3117a4be0cd04..5cdacb3c8c925 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -11,6 +11,7 @@ VERBOSE=0 TRACING=0 tests="
- arp_ping eth-arp: Basic arp ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -127,6 +128,16 @@ ovs_add_netns_and_veths () { return 0 } +ovs_add_flow () {
- info "Adding flow to DP: sbx:$1 br:$2 flow:$3 act:$4"
- ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py add-flow "$2" "$3" "$4"
- if [ $? -ne 0 ]; then
echo "Flow [ $3 : $4 ] failed" >> ${ovs_dir}/debug.logreturn 1- fi
- return 0
+}
- usage() { echo echo "$0 [OPTIONS] [TEST]..."
@@ -141,6 +152,46 @@ usage() { exit 1 } +# arp_ping test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - send ARP ping between two ns +test_arp_ping () {
- which arping >/dev/null 2>&1 || return $ksft_skip
- sbx_add "test_arp_ping" || return $?
- ovs_add_dp "test_arp_ping" arpping || return 1
- info "create namespaces"
- for ns in client server; do
ovs_add_netns_and_veths "test_arp_ping" "arpping" "$ns" \"${ns:0:1}0" "${ns:0:1}1" || return 1- done
- # Setup client namespace
- ip netns exec client ip addr add 172.31.110.10/24 dev c1
- ip netns exec client ip link set c1 up
- HW_CLIENT=`ip netns exec client ip link show dev c1 | grep -E 'link/ether [0-9a-f:]+' | awk '{print $2;}'`
- info "Client hwaddr: $HW_CLIENT"
- # Setup server namespace
- ip netns exec server ip addr add 172.31.110.20/24 dev s1
- ip netns exec server ip link set s1 up
- HW_SERVER=`ip netns exec server ip link show dev s1 | grep -E 'link/ether [0-9a-f:]+' | awk '{print $2;}'`
- info "Server hwaddr: $HW_SERVER"
- ovs_add_flow "test_arp_ping" arpping \
"in_port(1),eth(),eth_type(0x0806),arp(sip=172.31.110.10,tip=172.31.110.20,sha=$HW_CLIENT,tha=ff:ff:ff:ff:ff:ff)" '2' || return 1- ovs_add_flow "test_arp_ping" arpping \
"in_port(2),eth(),eth_type(0x0806),arp()" '1' || return 1- ovs_sbx "test_arp_ping" ip netns exec client arping -I c1 172.31.110.20 -c 1 || return 1
- return 0
+}
- # netlink_validation # - Create a dp # - check no warning with "old version" simulation
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 1c8b36bc15d48..799bfb3064b90 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -9,9 +9,12 @@ import errno import ipaddress import logging import multiprocessing +import re import struct import sys import time +import types +import uuid try: from pyroute2 import NDB @@ -59,6 +62,104 @@ def macstr(mac): return outstr +def strspn(str1, str2):
- tot = 0
- for char in str1:
if str2.find(char) == -1:return tottot += 1- return tot
+def intparse(statestr, defmask="0xffffffff"):
- totalparse = strspn(statestr, "0123456789abcdefABCDEFx/")
- # scan until "/"
- count = strspn(statestr, "x0123456789abcdefABCDEF")
- firstnum = statestr[:count]
- if firstnum[-1] == "/":
firstnum = firstnum[:-1]- k = int(firstnum, 0)
- m = None
- if defmask is not None:
secondnum = defmaskif statestr[count] == "/":secondnum = statestr[count + 1 :] # this is wrong...m = int(secondnum, 0)- return statestr[totalparse + 1 :], k, m
+def parse_flags(flag_str, flag_vals):
- bitResult = 0
- maskResult = 0
- if len(flag_str) == 0:
return flag_str, bitResult, maskResult- if flag_str[0].isdigit():
idx = 0while flag_str[idx].isdigit() or flag_str[idx] == "x":idx += 1digits = flag_str[:idx]flag_str = flag_str[idx:]bitResult = int(digits, 0)maskResult = int(digits, 0)- while len(flag_str) > 0 and (flag_str[0] == "+" or flag_str[0] == "-"):
if flag_str[0] == "+":setFlag = Trueelif flag_str[0] == "-":setFlag = Falseflag_str = flag_str[1:]flag_len = 0while (flag_str[flag_len] != "+"and flag_str[flag_len] != "-"and flag_str[flag_len] != ","and flag_str[flag_len] != ")"):flag_len += 1flag = flag_str[0:flag_len]if flag in flag_vals:if maskResult & flag_vals[flag]:raise KeyError("Flag %s set once, cannot be set in multiples" % flag)if setFlag:bitResult |= flag_vals[flag]maskResult |= flag_vals[flag]else:raise KeyError("Missing flag value: %s" % flag)flag_str = flag_str[flag_len:]- return flag_str, bitResult, maskResult
+def parse_ct_state(statestr):
- ct_flags = {
"new": 1 << 0,"est": 1 << 1,"rel": 1 << 2,"rpl": 1 << 3,"inv": 1 << 4,"trk": 1 << 5,"snat": 1 << 6,"dnat": 1 << 7,- }
- return parse_flags(statestr, ct_flags)
- def convert_mac(mac_str, mask=False): if mac_str is None or mac_str == "": mac_str = "00:00:00:00:00:00"
@@ -79,6 +180,61 @@ def convert_ipv4(ip, mask=False): return int(ipaddress.IPv4Address(ip)) +def parse_starts_block(block_str, scanstr, returnskipped, scanregex=False):
- if scanregex:
m = re.search(scanstr, block_str)if m is None:if returnskipped:return block_strreturn Falseif returnskipped:block_str = block_str[len(m.group(0)) :]return block_strreturn True- if block_str.startswith(scanstr):
if returnskipped:block_str = block_str[len(scanstr) :]else:return True- if returnskipped:
return block_str- return False
+def parse_extract_field(
- block_str, fieldstr, scanfmt, convert, masked=False, defval=None
+):
- if fieldstr and not block_str.startswith(fieldstr):
return block_str, defval- if fieldstr:
str_skiplen = len(fieldstr)str_skipped = block_str[str_skiplen:]if str_skiplen == 0:return str_skipped, defval- else:
str_skiplen = 0str_skipped = block_str- m = re.search(scanfmt, str_skipped)
- if m is None:
raise ValueError("Bad fmt string")- data = m.group(0)
- if convert:
data = convert(m.group(0))- str_skipped = str_skipped[len(m.group(0)) :]
- if masked:
if str_skipped[0] == "/":raise ValueError("Masking support TBD...")- return str_skipped, data
- class ovs_dp_msg(genlmsg): # include the OVS version # We need a custom header rather than just being able to rely on
@@ -278,6 +434,52 @@ class ovsactions(nla): return print_str
- def parse(self, actstr):
parsed = Falsewhile len(actstr) != 0:if actstr.startswith("drop"):# for now, drops have no explicit action, so we# don't need to set any attributes. The final# act of the processing chain will just drop the packetreturnelif parse_starts_block(actstr, "^(\d+)", False, True):actstr, output = parse_extract_field(actstr, None, "(\d+)", lambda x: int(x), False, "0")actstr = actstr[strspn(actstr, ", ") :]self["attrs"].append(["OVS_ACTION_ATTR_OUTPUT", output])parsed = Trueelif parse_starts_block(actstr, "recirc(", False):actstr, recircid = parse_extract_field(actstr,"recirc(","([0-9a-fA-Fx]+)",lambda x: int(x, 0),False,0,)actstr = actstr[strspn(actstr, "), ") :]self["attrs"].append(["OVS_ACTION_ATTR_RECIRC", recircid])parsed = Trueparse_flat_map = (("ct_clear", "OVS_ACTION_ATTR_CT_CLEAR"),("pop_vlan", "OVS_ACTION_ATTR_POP_VLAN"),("pop_eth", "OVS_ACTION_ATTR_POP_ETH"),("pop_nsh", "OVS_ACTION_ATTR_POP_NSH"),)for flat_act in parse_flat_map:if parse_starts_block(actstr, flat_act[0], False):actstr += len(flat_act[0])self["attrs"].append([flat_act[1]])actstr = actstr[strspn(actstr, ", ") :]parsed = Trueif not parsed:raise ValueError("Action str: '%s' not supported" % actstr)class ovskey(nla): nla_flags = NLA_F_NESTED @@ -347,6 +549,53 @@ class ovskey(nla): init=init, )
def parse(self, flowstr, typeInst):if not flowstr.startswith(self.proto_str):return None, Nonek = typeInst()m = typeInst()flowstr = flowstr[len(self.proto_str) :]if flowstr.startswith("("):flowstr = flowstr[1:]keybits = b""maskbits = b""for f in self.fields_map:if flowstr.startswith(f[1]):# the following assumes that the field looks# something like 'field.' where '.' is a# character that we don't exactly care about.flowstr = flowstr[len(f[1]) + 1 :]splitchar = 0for c in flowstr:if c == "," or c == ")":breaksplitchar += 1data = flowstr[:splitchar]flowstr = flowstr[splitchar:]else:data = Noneif len(f) > 4:func = f[4]else:func = f[3]k[f[0]] = func(data)if len(f) > 4:m[f[0]] = func(data, True)else:m[f[0]] = func(data)flowstr = flowstr[strspn(flowstr, ", ") :]if len(flowstr) == 0:return flowstr, k, mflowstr = flowstr[strspn(flowstr, "), ") :]return flowstr, k, mdef dpstr(self, masked=None, more=False): outstr = self.proto_str + "(" first = False@@ -810,6 +1059,71 @@ class ovskey(nla): class ovs_key_mpls(nla): fields = (("lse", ">I"),)
- def parse(self, flowstr, mask=None):
for field in (("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),("OVS_KEY_ATTR_SKB_MARK", "skb_mark", intparse),("OVS_KEY_ATTR_RECIRC_ID", "recirc_id", intparse),("OVS_KEY_ATTR_DP_HASH", "dp_hash", intparse),("OVS_KEY_ATTR_CT_STATE", "ct_state", parse_ct_state),("OVS_KEY_ATTR_CT_ZONE", "ct_zone", intparse),("OVS_KEY_ATTR_CT_MARK", "ct_mark", intparse),("OVS_KEY_ATTR_IN_PORT", "in_port", intparse),("OVS_KEY_ATTR_ETHERNET","eth",ovskey.ethaddr,),("OVS_KEY_ATTR_ETHERTYPE","eth_type",lambda x: intparse(x, "0xffff"),),("OVS_KEY_ATTR_IPV4","ipv4",ovskey.ovs_key_ipv4,),("OVS_KEY_ATTR_IPV6","ipv6",ovskey.ovs_key_ipv6,),("OVS_KEY_ATTR_ARP","arp",ovskey.ovs_key_arp,),("OVS_KEY_ATTR_TCP","tcp",ovskey.ovs_key_tcp,),("OVS_KEY_ATTR_TCP_FLAGS","tcp_flags",lambda x: parse_flags(x, None),),):fld = field[1] + "("if not flowstr.startswith(fld):continueif not isinstance(field[2], types.FunctionType):nk = field[2]()flowstr, k, m = nk.parse(flowstr, field[2])else:flowstr = flowstr[len(fld) :]flowstr, k, m = field[2](flowstr)if m and mask is not None:mask["attrs"].append([field[0], m])self["attrs"].append([field[0], k])flowstr = flowstr[strspn(flowstr, "),") :]return flowstrdef dpstr(self, mask=None, more=False): print_str = ""@@ -1358,11 +1672,92 @@ class OvsFlow(GenericNetlinkSocket): return print_str
def parse(self, flowstr, actstr, dpidx=0):OVS_UFID_F_OMIT_KEY = 1 << 0OVS_UFID_F_OMIT_MASK = 1 << 1OVS_UFID_F_OMIT_ACTIONS = 1 << 2self["cmd"] = 0self["version"] = 0self["reserved"] = 0self["dpifindex"] = 0if flowstr.startswith("ufid:"):count = 5while flowstr[count] != ",":count += 1ufidstr = flowstr[5:count]flowstr = flowstr[count + 1 :]else:ufidstr = str(uuid.uuid4())uuidRawObj = uuid.UUID(ufidstr).fieldsself["attrs"].append(["OVS_FLOW_ATTR_UFID",[uuidRawObj[0],uuidRawObj[1] << 16 | uuidRawObj[2],uuidRawObj[3] << 24| uuidRawObj[4] << 16| uuidRawObj[5] & (0xFF << 32) >> 32,uuidRawObj[5] & (0xFFFFFFFF),],])self["attrs"].append(["OVS_FLOW_ATTR_UFID_FLAGS",int(OVS_UFID_F_OMIT_KEY| OVS_UFID_F_OMIT_MASK| OVS_UFID_F_OMIT_ACTIONS),])k = ovskey()m = ovskey()k.parse(flowstr, m)self["attrs"].append(["OVS_FLOW_ATTR_KEY", k])self["attrs"].append(["OVS_FLOW_ATTR_MASK", m])a = ovsactions()a.parse(actstr)self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a])def __init__(self): GenericNetlinkSocket.__init__(self)self.bind(OVS_FLOW_FAMILY, OvsFlow.ovs_flow_msg)
- def add_flow(self, dpifindex, flowmsg):
"""Send a new flow message to the kernel.dpifindex should be a valid datapath obtained by callinginto the OvsDatapath lookupflowmsg is a flow object obtained by calling a dpparse"""flowmsg["cmd"] = OVS_FLOW_CMD_NEWflowmsg["version"] = OVS_DATAPATH_VERSIONflowmsg["reserved"] = 0flowmsg["dpifindex"] = dpifindextry:reply = self.nlm_request(flowmsg,msg_type=self.prid,msg_flags=NLM_F_REQUEST | NLM_F_ACK,)reply = reply[0]except NetlinkError as ne:print(flowmsg)raise nereturn replydef dump(self, dpifindex, flowspec=None): """ Returns a list of messages containing flows.@@ -1514,6 +1909,11 @@ def main(argv): dumpflcmd = subparsers.add_parser("dump-flows") dumpflcmd.add_argument("dumpdp", help="Datapath Name")
- addflcmd = subparsers.add_parser("add-flow")
- addflcmd.add_argument("flbr", help="Datapath name")
- addflcmd.add_argument("flow", help="Flow specification")
- addflcmd.add_argument("acts", help="Flow actions")
args = parser.parse_args()if args.verbose > 0: @@ -1589,6 +1989,14 @@ def main(argv): rep = ovsflow.dump(rep["dpifindex"]) for flow in rep: print(flow.dpstr(True if args.verbose > 0 else False))
- elif hasattr(args, "flbr"):
These checks on the attributes means every command must have attributes with different names. So if we then add del-br it must not have an attribute called "flbr". We could rename it to "fladdbr" (following the other commands) or match on the subcommand name, which would be cleaner imho and less error, e.g: all the datapath attributes can be called the same (see below).
rep = ovsdp.info(args.flbr, 0)if rep is None:print("DP '%s' not found." % args.dumpdp)
"dumpdp" is not an attribute of this subcommand.
return 1flow = OvsFlow.ovs_flow_msg()flow.parse(args.flow, args.acts, rep["dpifindex"])ovsflow.add_flow(rep["dpifindex"], flow)return 0
Adrian Moreno amorenoz@redhat.com writes:
On 6/28/23 18:27, Aaron Conole wrote:
Forwarding via ct() action is an important use case for openvswitch, but generally would require using a full ovs-vswitchd to get working. Add a ct action parser for basic ct test case. Signed-off-by: Aaron Conole aconole@redhat.com
NOTE: 3 lines flag the line-length checkpatch warning, but there didnt seem to be a really good way of breaking the lines smaller. .../selftests/net/openvswitch/openvswitch.sh | 68 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 39 +++++++++++ 2 files changed, 107 insertions(+) diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 5d60a9466dab3..40a66c72af0f0 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -12,6 +12,7 @@ TRACING=0 tests=" arp_ping eth-arp: Basic arp ping between two NS
- ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces"
@@ -193,6 +194,73 @@ test_arp_ping () { return 0 } +# ct_connect_v4 test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +# - only allow CT state stuff to pass through new in c -> s +test_ct_connect_v4 () {
- which nc >/dev/null 2>/dev/null || return $ksft_skip
- sbx_add "test_ct_connect_v4" || return $?
- ovs_add_dp "test_ct_connect_v4" ct4 || return 1
- info "create namespaces"
- for ns in client server; do
ovs_add_netns_and_veths "test_ct_connect_v4" "ct4" "$ns" \"${ns:0:1}0" "${ns:0:1}1" || return 1- done
- ip netns exec client ip addr add 172.31.110.10/24 dev c1
- ip netns exec client ip link set c1 up
- ip netns exec server ip addr add 172.31.110.20/24 dev s1
- ip netns exec server ip link set s1 up
- # Add forwarding for ARP and ip packets - completely wildcarded
- ovs_add_flow "test_ct_connect_v4" ct4 \
'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'ct_state(-trk),eth(),eth_type(0x0800),ipv4()' \'ct(commit),recirc(0x1)' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+new),in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' \'2' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+est),in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10)' \'2' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+est),in_port(2),eth(),eth_type(0x0800),ipv4(dst=172.31.110.10)' \'1' || return 1- ovs_add_flow "test_ct_connect_v4" ct4 \
'recirc_id(0x1),ct_state(+trk+inv),eth(),eth_type(0x0800),ipv4()' 'drop' || \return 1- # do a ping
- ovs_sbx "test_ct_connect_v4" ip netns exec client ping 172.31.110.20 -c 3 || return 1
- # create an echo server in 'server'
- echo "server" | \
ovs_netns_spawn_daemon "test_ct_connect_v4" "server" \nc -lvnp 4443- ovs_sbx "test_ct_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.20 4443 || return 1
- # Now test in the other direction (should fail)
- echo "client" | \
ovs_netns_spawn_daemon "test_ct_connect_v4" "client" \nc -lvnp 4443- ovs_sbx "test_ct_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443
- if [ $? == 0 ]; then
info "ct connect to client was successful"return 1- fi
- info "done..."
- return 0
+}
- # connect_v4 test # - client has 1500 byte MTU # - server has 1500 byte MTU
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 799bfb3064b90..704cb4adf79a9 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -62,6 +62,15 @@ def macstr(mac): return outstr +def strcspn(str1, str2):
- tot = 0
- for char in str1:
if str2.find(char) != -1:return tottot += 1- return tot
- def strspn(str1, str2): tot = 0 for char in str1:
@@ -477,6 +486,36 @@ class ovsactions(nla): actstr = actstr[strspn(actstr, ", ") :] parsed = True
if parse_starts_block(actstr, "ct(", False):actstr = actstr[len("ct(") :]ctact = ovsactions.ctact()for scan in (("commit", "OVS_CT_ATTR_COMMIT", None),("force_commit", "OVS_CT_ATTR_FORCE_COMMIT", None),("zone", "OVS_CT_ATTR_ZONE", int),("mark", "OVS_CT_ATTR_MARK", int),("helper", "OVS_CT_ATTR_HELPER", lambda x, y: str(x)),("timeout", "OVS_CT_ATTR_TIMEOUT", lambda x, y: str(x)),):if actstr.startswith(scan[0]):actstr = actstr[len(scan[0]) :]if scan[2] is not None:if actstr[0] != "=":raise ValueError("Invalid ct attr")actstr = actstr[1:]pos = strcspn(actstr, ",)")datum = scan[2](actstr[:pos], 0)It seems the scan function is only called with "0" as second argument. Wouldn't it be easier to omit that extra argument (which is the default value for "int" anyway) and simplify the lambda for "helper" and "timeout"?
Alternatively, we could have a function that tries both base-16 and base-10.
I think '10' is the default argument for int() -
print("%d" % int("0x12"))
Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: invalid literal for int() with base 10: '0x12'
print("%d" % int("0x12", 0))
18
So, I will keep it as-is.
ctact["attrs"].append([scan[1], datum])actstr = actstr[len(datum) :]"datum" can be of type int and ints don't have len(). Maybe just use "pos" directly?
Good catch! I'll correct it.
else:ctact["attrs"].append([scan[1], None])actstr = actstr[strspn(actstr, ", ") :]self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact])actstr = actstr[strspn(actstr, "), ") :]parsed = Trueif not parsed: raise ValueError("Action str: '%s' not supported" % actstr)
Adrian Moreno amorenoz@redhat.com writes:
On 6/28/23 18:27, Aaron Conole wrote:
Building on the previous work, add a very simplistic NAT case using ipv4. This just tests dnat transformation Signed-off-by: Aaron Conole aconole@redhat.com
Hi Aaron,
I know that the goal is not to support the full syntax, and that nat is a specially convoluted action, so I'm just commenting on the low-hanging fruits (see below).
Thanks, Adrian!
.../selftests/net/openvswitch/openvswitch.sh | 64 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 60 +++++++++++++++++ 2 files changed, 124 insertions(+) diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 40a66c72af0f0..dced4f612a78c 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -14,6 +14,7 @@ tests=" arp_ping eth-arp: Basic arp ping between two NS ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct connect_v4 ip4-xon: Basic ipv4 ping between two NS
- nat_connect_v4 ip4-nat-xon: Basic ipv4 tcp connection via NAT netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces" @@ -300,6 +301,69 @@ test_connect_v4 () { return 0 } +# nat_connect_v4 test
+# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - use ICMP to ping in each direction +# - only allow CT state stuff to pass through new in c -> s +test_nat_connect_v4 () {
- which nc >/dev/null 2>/dev/null || return $ksft_skip
- sbx_add "test_nat_connect_v4" || return $?
- ovs_add_dp "test_nat_connect_v4" nat4 || return 1
- info "create namespaces"
- for ns in client server; do
ovs_add_netns_and_veths "test_nat_connect_v4" "nat4" "$ns" \"${ns:0:1}0" "${ns:0:1}1" || return 1- done
- ip netns exec client ip addr add 172.31.110.10/24 dev c1
- ip netns exec client ip link set c1 up
- ip netns exec server ip addr add 172.31.110.20/24 dev s1
- ip netns exec server ip link set s1 up
- ip netns exec client ip route add default via 172.31.110.20
- ovs_add_flow "test_nat_connect_v4" nat4 \
'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1- ovs_add_flow "test_nat_connect_v4" nat4 \
'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1- ovs_add_flow "test_nat_connect_v4" nat4 \
"ct_state(-trk),in_port(1),eth(),eth_type(0x0800),ipv4(dst=192.168.0.20)" \"ct(commit,nat(dst=172.31.110.20)),recirc(0x1)"- ovs_add_flow "test_nat_connect_v4" nat4 \
"ct_state(-trk),in_port(2),eth(),eth_type(0x0800),ipv4()" \"ct(commit,nat),recirc(0x2)"- ovs_add_flow "test_nat_connect_v4" nat4 \
"recirc_id(0x1),ct_state(+trk-inv),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"- ovs_add_flow "test_nat_connect_v4" nat4 \
"recirc_id(0x2),ct_state(+trk-inv),in_port(2),eth(),eth_type(0x0800),ipv4()" "1"- # do a ping
- ovs_sbx "test_nat_connect_v4" ip netns exec client ping 192.168.0.20 -c 3 || return 1
- # create an echo server in 'server'
- echo "server" | \
ovs_netns_spawn_daemon "test_nat_connect_v4" "server" \nc -lvnp 4443- ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 192.168.0.20 4443 || return 1
- # Now test in the other direction (should fail)
- echo "client" | \
ovs_netns_spawn_daemon "test_nat_connect_v4" "client" \nc -lvnp 4443- ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443
- if [ $? == 0 ]; then
info "connect to client was successful"return 1- fi
- info "done..."
- return 0
+}
- # netlink_validation # - Create a dp # - check no warning with "old version" simulation
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 704cb4adf79a9..12ba5265b88fb 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -511,6 +511,66 @@ class ovsactions(nla): else: ctact["attrs"].append([scan[1], None]) actstr = actstr[strspn(actstr, ", ") :]
# it seems strange to put this here, but nat() is a complex# sub-action and this lets it sit anywhere in the ct() actionif actstr.startswith("nat"):actstr = actstr[3:]natact = ovsactions.ctact.natattr()if actstr.startswith("("):t = Noneactstr = actstr[1:]if actstr.startswith("src"):t = "OVS_NAT_ATTR_SRC"actstr = actstr[3:]elif actstr.startswith("dst"):t = "OVS_NAT_ATTR_DST"actstr = actstr[3:]actstr, ip_block_min = parse_extract_field(actstr, "=", "([0-9a-fA-F:\.]+)", str, False)actstr, ip_block_max = parse_extract_field(actstr, "-", "([0-9a-fA-F:\.]+)", str, False)Having the ":" character here makes this line parse the port as well (i.e: 1.1.1.1:6789 as ip_block_max) which then makes ip address parsing fail.
Ugh... good catch. I'll re-work this section a bit. I was trying to keep it simple to catch both ipv4 and ipv6 syntax.
actstr, proto_min = parse_extract_field(actstr, ":", "(\d+)", int, False)actstr, proto_max = parse_extract_field(actstr, "-", "(\d+)", int, False)if t is not None:natact["attrs"].append([t, None])if ip_block_min is not None:natact["attrs"].append(["OVS_NAT_ATTR_IP_MIN", ip_block_min])if ip_block_max is not None:natact["attrs"].append(["OVS_NAT_ATTR_IP_MAX", ip_block_max])if proto_min is not None:natact["attrs"].append(["OVS_NAT_ATTR_PROTO_MIN", proto_min])if proto_max is not None:natact["attrs"].append(["OVS_NAT_ATTR_PROTO_MAX", proto_max])for natscan in (("persist", "OVS_NAT_ATTR_PERSISTENT"),("hash", "OVS_NAT_ATTR_PROTO_HASH"),("random", "OVS_NAT_ATTR_PROTO_RANDOM"),):I think this is not taking into account the comma that separates ip:port from these keywords. A possible solution would be to add it to the natscan (e.g: s/persist/,persist/).
I'll double check it, thanks!
if actstr.startswith(natscan[0]):actstr = actstr[len(natscan[0]) :]natact["attrs"].append([natscan[1], None])actstr = actstr[strspn(actstr, ", ") :]ctact["attrs"].append(["OVS_CT_ATTR_NAT", natact])actstr = actstr[strspn(actstr, ",) ") :] self["attrs"].append(["OVS_ACTION_ATTR_CT",ctact]) actstr = actstr[strspn(actstr, "), ") :]
Adrian Moreno amorenoz@redhat.com writes:
On 6/28/23 18:27, Aaron Conole wrote:
The openvswitch self-tests can test much of the control side of the module (ie: what a vswitchd implementation would process), but the actual packet forwarding cases aren't supported, making the testing of limited value. Add some flow parsing and an initial ARP based test case using arping utility. This lets us display flows, add some basic output flows with simple matches, and test against a known good forwarding case. Signed-off-by: Aaron Conole aconole@redhat.com
NOTE: 3 lines flag the line-length checkpatch warning, but there didn't seem to bea good way of breaking the lines smaller for 2 of them. The third would still flag, even if broken at what looks like a good point to break it. .../selftests/net/openvswitch/openvswitch.sh | 51 +++ .../selftests/net/openvswitch/ovs-dpctl.py | 408 ++++++++++++++++++ 2 files changed, 459 insertions(+) diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index 3117a4be0cd04..5cdacb3c8c925 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -11,6 +11,7 @@ VERBOSE=0 TRACING=0 tests="
- arp_ping eth-arp: Basic arp ping between two NS netlink_checks ovsnl: validate netlink attrs and settings upcall_interfaces ovs: test the upcall interfaces" @@ -127,6 +128,16 @@ ovs_add_netns_and_veths () { return 0 } +ovs_add_flow () {
- info "Adding flow to DP: sbx:$1 br:$2 flow:$3 act:$4"
- ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py add-flow "$2" "$3" "$4"
- if [ $? -ne 0 ]; then
echo "Flow [ $3 : $4 ] failed" >> ${ovs_dir}/debug.logreturn 1- fi
- return 0
+}
- usage() { echo echo "$0 [OPTIONS] [TEST]..."
@@ -141,6 +152,46 @@ usage() { exit 1 } +# arp_ping test +# - client has 1500 byte MTU +# - server has 1500 byte MTU +# - send ARP ping between two ns +test_arp_ping () {
- which arping >/dev/null 2>&1 || return $ksft_skip
- sbx_add "test_arp_ping" || return $?
- ovs_add_dp "test_arp_ping" arpping || return 1
- info "create namespaces"
- for ns in client server; do
ovs_add_netns_and_veths "test_arp_ping" "arpping" "$ns" \"${ns:0:1}0" "${ns:0:1}1" || return 1- done
- # Setup client namespace
- ip netns exec client ip addr add 172.31.110.10/24 dev c1
- ip netns exec client ip link set c1 up
- HW_CLIENT=`ip netns exec client ip link show dev c1 | grep -E
'link/ether [0-9a-f:]+' | awk '{print $2;}'`
- info "Client hwaddr: $HW_CLIENT"
- # Setup server namespace
- ip netns exec server ip addr add 172.31.110.20/24 dev s1
- ip netns exec server ip link set s1 up
- HW_SERVER=`ip netns exec server ip link show dev s1 | grep -E
'link/ether [0-9a-f:]+' | awk '{print $2;}'`
- info "Server hwaddr: $HW_SERVER"
- ovs_add_flow "test_arp_ping" arpping \
"in_port(1),eth(),eth_type(0x0806),arp(sip=172.31.110.10,tip=172.31.110.20,sha=$HW_CLIENT,tha=ff:ff:ff:ff:ff:ff)" '2' || return 1
- ovs_add_flow "test_arp_ping" arpping \
"in_port(2),eth(),eth_type(0x0806),arp()" '1' || return 1- ovs_sbx "test_arp_ping" ip netns exec client arping -I c1 172.31.110.20 -c 1 || return 1
- return 0
+}
- # netlink_validation # - Create a dp # - check no warning with "old version" simulation
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 1c8b36bc15d48..799bfb3064b90 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -9,9 +9,12 @@ import errno import ipaddress import logging import multiprocessing +import re import struct import sys import time +import types +import uuid try: from pyroute2 import NDB @@ -59,6 +62,104 @@ def macstr(mac): return outstr +def strspn(str1, str2):
- tot = 0
- for char in str1:
if str2.find(char) == -1:return tottot += 1- return tot
+def intparse(statestr, defmask="0xffffffff"):
- totalparse = strspn(statestr, "0123456789abcdefABCDEFx/")
- # scan until "/"
- count = strspn(statestr, "x0123456789abcdefABCDEF")
- firstnum = statestr[:count]
- if firstnum[-1] == "/":
firstnum = firstnum[:-1]- k = int(firstnum, 0)
- m = None
- if defmask is not None:
secondnum = defmaskif statestr[count] == "/":secondnum = statestr[count + 1 :] # this is wrong...m = int(secondnum, 0)- return statestr[totalparse + 1 :], k, m
+def parse_flags(flag_str, flag_vals):
- bitResult = 0
- maskResult = 0
- if len(flag_str) == 0:
return flag_str, bitResult, maskResult- if flag_str[0].isdigit():
idx = 0while flag_str[idx].isdigit() or flag_str[idx] == "x":idx += 1digits = flag_str[:idx]flag_str = flag_str[idx:]bitResult = int(digits, 0)maskResult = int(digits, 0)- while len(flag_str) > 0 and (flag_str[0] == "+" or flag_str[0] == "-"):
if flag_str[0] == "+":setFlag = Trueelif flag_str[0] == "-":setFlag = Falseflag_str = flag_str[1:]flag_len = 0while (flag_str[flag_len] != "+"and flag_str[flag_len] != "-"and flag_str[flag_len] != ","and flag_str[flag_len] != ")"):flag_len += 1flag = flag_str[0:flag_len]if flag in flag_vals:if maskResult & flag_vals[flag]:raise KeyError("Flag %s set once, cannot be set in multiples" % flag)if setFlag:bitResult |= flag_vals[flag]maskResult |= flag_vals[flag]else:raise KeyError("Missing flag value: %s" % flag)flag_str = flag_str[flag_len:]- return flag_str, bitResult, maskResult
+def parse_ct_state(statestr):
- ct_flags = {
"new": 1 << 0,"est": 1 << 1,"rel": 1 << 2,"rpl": 1 << 3,"inv": 1 << 4,"trk": 1 << 5,"snat": 1 << 6,"dnat": 1 << 7,- }
- return parse_flags(statestr, ct_flags)
- def convert_mac(mac_str, mask=False): if mac_str is None or mac_str == "": mac_str = "00:00:00:00:00:00"
@@ -79,6 +180,61 @@ def convert_ipv4(ip, mask=False): return int(ipaddress.IPv4Address(ip)) +def parse_starts_block(block_str, scanstr, returnskipped, scanregex=False):
- if scanregex:
m = re.search(scanstr, block_str)if m is None:if returnskipped:return block_strreturn Falseif returnskipped:block_str = block_str[len(m.group(0)) :]return block_strreturn True- if block_str.startswith(scanstr):
if returnskipped:block_str = block_str[len(scanstr) :]else:return True- if returnskipped:
return block_str- return False
+def parse_extract_field(
- block_str, fieldstr, scanfmt, convert, masked=False, defval=None
+):
- if fieldstr and not block_str.startswith(fieldstr):
return block_str, defval- if fieldstr:
str_skiplen = len(fieldstr)str_skipped = block_str[str_skiplen:]if str_skiplen == 0:return str_skipped, defval- else:
str_skiplen = 0str_skipped = block_str- m = re.search(scanfmt, str_skipped)
- if m is None:
raise ValueError("Bad fmt string")- data = m.group(0)
- if convert:
data = convert(m.group(0))- str_skipped = str_skipped[len(m.group(0)) :]
- if masked:
if str_skipped[0] == "/":raise ValueError("Masking support TBD...")- return str_skipped, data
- class ovs_dp_msg(genlmsg): # include the OVS version # We need a custom header rather than just being able to rely on
@@ -278,6 +434,52 @@ class ovsactions(nla): return print_str
- def parse(self, actstr):
parsed = Falsewhile len(actstr) != 0:if actstr.startswith("drop"):# for now, drops have no explicit action, so we# don't need to set any attributes. The final# act of the processing chain will just drop the packetreturnelif parse_starts_block(actstr, "^(\d+)", False, True):actstr, output = parse_extract_field(actstr, None, "(\d+)", lambda x: int(x), False, "0")actstr = actstr[strspn(actstr, ", ") :]self["attrs"].append(["OVS_ACTION_ATTR_OUTPUT", output])parsed = Trueelif parse_starts_block(actstr, "recirc(", False):actstr, recircid = parse_extract_field(actstr,"recirc(","([0-9a-fA-Fx]+)",lambda x: int(x, 0),False,0,)actstr = actstr[strspn(actstr, "), ") :]self["attrs"].append(["OVS_ACTION_ATTR_RECIRC", recircid])parsed = Trueparse_flat_map = (("ct_clear", "OVS_ACTION_ATTR_CT_CLEAR"),("pop_vlan", "OVS_ACTION_ATTR_POP_VLAN"),("pop_eth", "OVS_ACTION_ATTR_POP_ETH"),("pop_nsh", "OVS_ACTION_ATTR_POP_NSH"),)for flat_act in parse_flat_map:if parse_starts_block(actstr, flat_act[0], False):actstr += len(flat_act[0])self["attrs"].append([flat_act[1]])actstr = actstr[strspn(actstr, ", ") :]parsed = Trueif not parsed:raise ValueError("Action str: '%s' not supported" % actstr)- class ovskey(nla): nla_flags = NLA_F_NESTED
@@ -347,6 +549,53 @@ class ovskey(nla): init=init, )
def parse(self, flowstr, typeInst):if not flowstr.startswith(self.proto_str):return None, Nonek = typeInst()m = typeInst()flowstr = flowstr[len(self.proto_str) :]if flowstr.startswith("("):flowstr = flowstr[1:]keybits = b""maskbits = b""for f in self.fields_map:if flowstr.startswith(f[1]):# the following assumes that the field looks# something like 'field.' where '.' is a# character that we don't exactly care about.flowstr = flowstr[len(f[1]) + 1 :]splitchar = 0for c in flowstr:if c == "," or c == ")":breaksplitchar += 1data = flowstr[:splitchar]flowstr = flowstr[splitchar:]else:data = Noneif len(f) > 4:func = f[4]else:func = f[3]k[f[0]] = func(data)if len(f) > 4:m[f[0]] = func(data, True)else:m[f[0]] = func(data)flowstr = flowstr[strspn(flowstr, ", ") :]if len(flowstr) == 0:return flowstr, k, mflowstr = flowstr[strspn(flowstr, "), ") :]return flowstr, k, mdef dpstr(self, masked=None, more=False): outstr = self.proto_str + "(" first = False@@ -810,6 +1059,71 @@ class ovskey(nla): class ovs_key_mpls(nla): fields = (("lse", ">I"),)
- def parse(self, flowstr, mask=None):
for field in (("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),("OVS_KEY_ATTR_SKB_MARK", "skb_mark", intparse),("OVS_KEY_ATTR_RECIRC_ID", "recirc_id", intparse),("OVS_KEY_ATTR_DP_HASH", "dp_hash", intparse),("OVS_KEY_ATTR_CT_STATE", "ct_state", parse_ct_state),("OVS_KEY_ATTR_CT_ZONE", "ct_zone", intparse),("OVS_KEY_ATTR_CT_MARK", "ct_mark", intparse),("OVS_KEY_ATTR_IN_PORT", "in_port", intparse),("OVS_KEY_ATTR_ETHERNET","eth",ovskey.ethaddr,),("OVS_KEY_ATTR_ETHERTYPE","eth_type",lambda x: intparse(x, "0xffff"),),("OVS_KEY_ATTR_IPV4","ipv4",ovskey.ovs_key_ipv4,),("OVS_KEY_ATTR_IPV6","ipv6",ovskey.ovs_key_ipv6,),("OVS_KEY_ATTR_ARP","arp",ovskey.ovs_key_arp,),("OVS_KEY_ATTR_TCP","tcp",ovskey.ovs_key_tcp,),("OVS_KEY_ATTR_TCP_FLAGS","tcp_flags",lambda x: parse_flags(x, None),),):fld = field[1] + "("if not flowstr.startswith(fld):continueif not isinstance(field[2], types.FunctionType):nk = field[2]()flowstr, k, m = nk.parse(flowstr, field[2])else:flowstr = flowstr[len(fld) :]flowstr, k, m = field[2](flowstr)if m and mask is not None:mask["attrs"].append([field[0], m])self["attrs"].append([field[0], k])flowstr = flowstr[strspn(flowstr, "),") :]return flowstrdef dpstr(self, mask=None, more=False): print_str = ""
def parse(self, flowstr, actstr, dpidx=0):OVS_UFID_F_OMIT_KEY = 1 << 0OVS_UFID_F_OMIT_MASK = 1 << 1OVS_UFID_F_OMIT_ACTIONS = 1 << 2self["cmd"] = 0self["version"] = 0self["reserved"] = 0self["dpifindex"] = 0if flowstr.startswith("ufid:"):count = 5while flowstr[count] != ",":count += 1ufidstr = flowstr[5:count]flowstr = flowstr[count + 1 :]else:ufidstr = str(uuid.uuid4())uuidRawObj = uuid.UUID(ufidstr).fieldsself["attrs"].append(["OVS_FLOW_ATTR_UFID",[uuidRawObj[0],uuidRawObj[1] << 16 | uuidRawObj[2],uuidRawObj[3] << 24| uuidRawObj[4] << 16| uuidRawObj[5] & (0xFF << 32) >> 32,uuidRawObj[5] & (0xFFFFFFFF),],])self["attrs"].append(["OVS_FLOW_ATTR_UFID_FLAGS",int(OVS_UFID_F_OMIT_KEY| OVS_UFID_F_OMIT_MASK| OVS_UFID_F_OMIT_ACTIONS),])k = ovskey()m = ovskey()k.parse(flowstr, m)self["attrs"].append(["OVS_FLOW_ATTR_KEY", k])self["attrs"].append(["OVS_FLOW_ATTR_MASK", m])a = ovsactions()a.parse(actstr)self["attrs"].append(["OVS_FLOW_ATTR_ACTIONS", a])def __init__(self): GenericNetlinkSocket.__init__(self) self.bind(OVS_FLOW_FAMILY, OvsFlow.ovs_flow_msg)
- def add_flow(self, dpifindex, flowmsg):
"""Send a new flow message to the kernel.dpifindex should be a valid datapath obtained by callinginto the OvsDatapath lookupflowmsg is a flow object obtained by calling a dpparse"""flowmsg["cmd"] = OVS_FLOW_CMD_NEWflowmsg["version"] = OVS_DATAPATH_VERSIONflowmsg["reserved"] = 0flowmsg["dpifindex"] = dpifindextry:reply = self.nlm_request(flowmsg,msg_type=self.prid,msg_flags=NLM_F_REQUEST | NLM_F_ACK,)reply = reply[0]except NetlinkError as ne:print(flowmsg)raise nereturn replydef dump(self, dpifindex, flowspec=None): """ Returns a list of messages containing flows.@@ -1514,6 +1909,11 @@ def main(argv): dumpflcmd = subparsers.add_parser("dump-flows") dumpflcmd.add_argument("dumpdp", help="Datapath Name")
- addflcmd = subparsers.add_parser("add-flow")
- addflcmd.add_argument("flbr", help="Datapath name")
- addflcmd.add_argument("flow", help="Flow specification")
- addflcmd.add_argument("acts", help="Flow actions")
args = parser.parse_args() if args.verbose > 0:@@ -1589,6 +1989,14 @@ def main(argv): rep = ovsflow.dump(rep["dpifindex"]) for flow in rep: print(flow.dpstr(True if args.verbose > 0 else False))
- elif hasattr(args, "flbr"):
These checks on the attributes means every command must have attributes with different names. So if we then add del-br it must not have an attribute called "flbr". We could rename it to "fladdbr" (following the other commands) or match on the subcommand name, which would be cleaner imho and less error, e.g: all the datapath attributes can be called the same (see below).
I agree we can adjust this to use the subparser dest= and just check for the specific command passed. I will do that in a different series, just to keep it as a separate thing.
rep = ovsdp.info(args.flbr, 0)if rep is None:print("DP '%s' not found." % args.dumpdp)"dumpdp" is not an attribute of this subcommand.
I'll fix this.
return 1flow = OvsFlow.ovs_flow_msg()flow.parse(args.flow, args.acts, rep["dpifindex"])ovsflow.add_flow(rep["dpifindex"], flow) return 0
linux-kselftest-mirror@lists.linaro.org
-
Aaron Conole -
Adrian Moreno -
Simon Horman