| Message ID | 20230628162714.392047-4-aconole@redhat.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | selftests: openvswitch: add flow programming cases | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
Bleep bloop. Greetings Aaron Conole, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
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 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) 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 = True > + > if 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: >> 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) > > 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 = True >> + >> if not parsed: >> raise ValueError("Action str: '%s' not supported" % actstr) >>
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)
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(+)