Re: [ovs-dev] [External] Re: [PATCH] ofp-flow: TLVs should be ordered

[Adrian Moreno]

On 4/4/23 09:19, Wan Junjie wrote:

Hi Adrian,

On Sat, Apr 1, 2023 at 12:39 AM Adrian Moreno <amore...@redhat.com> wrote:

Hi Wan,

Thanks for submitting this patch.

On 11/18/22 09:13, Wan Junjie wrote:

According to the ovs-fields manpage, 'TLVs must be ordered so that
a field’s prerequisites are satisfied before if is parsed'.

I believe this sentence is talking about the binary Openflow (>1.2) format where
match fields are encoded in TLV.

The way we represent flow matches in human-readable strings (for ovs-ofctl) is
more flexible, allowing the user to express the matches in any arbitrary order.
For example, we could add a flow such as:
"tcp_src=255,ip_src=1.1.1.1,ip,eth_src=ca:fe:ca:fe:ca:fe,tcp,actions=drop"

which is clearly in a very weird order, but if we looked at the wire we'd see
the TLV sections of the OFP packet are in the right order.

However, a match pattern like 'tcp,ipv6' can be parsed as 'tcp6',
meanwhile 'ipv6,tcp' is parsed as 'tcp'. So a limitation here make
sure that the order should be respected.

I understand this might be a bit confusing.
Firstly, according to the ovs-fields(7) these are the meanings of the 
shorthands:

ip:   eth_type=0x0800,
ipv6: eth_type=0x86dd
tcp:  eth_type=0x0800,ip_proto=6
tcp6: eth_type=0x86dd,ip_proto=6

If we expand the shorthands:

"tcp,ipv6" == "eth_type=0x0800,ip_proto=6,eth_type=0x86dd"
"ipv6,tcp" == "eth_type=0x86dd,eth_type=0x0800,ip_proto=6"

So the last "eth_type" match prevails.

Thanks for your clarification.
I saw the shorthands. IMHO flexibility input should not have any side
effect over correctness.
At least we should print a reminder to the user. Or add some documents instead?

Sorry Wan, this email completely got lost in my inbox.

Maybe we could add a Q/A in the Documentation/faq/configuration.rst?

Regards,
Wan Junjie

Signed-off-by: Wan Junjie <wanjun...@bytedance.com>
---
   lib/ofp-flow.c        |  3 +++
   tests/ofproto-dpif.at |  8 ++++----
   tests/ovs-ofctl.at    | 15 +++++++++++++++
   3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
index 3bc744f78..db9b75c7b 100644
--- a/lib/ofp-flow.c
+++ b/lib/ofp-flow.c
@@ -1580,6 +1580,9 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, 
char *string,
           char *error = NULL;

           if (ofp_parse_protocol(name, &p)) {
+            if (match.flow.nw_proto) {
+                return xstrdup("TLVs must be ordered");
+            }
               match_set_dl_type(&match, htons(p->dl_type));
               if (p->nw_proto) {
                   match_set_nw_proto(&match, p->nw_proto);
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 728243183..a4a7ab5fb 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -11629,14 +11629,14 @@ table=1,priority=1,action=drop
   dnl
   dnl Table 2
   dnl
-table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
-table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,tcp,action=ct(table=3,zone=2)
+table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,action=ct(commit,zone=1),ct(table=3,zone=2)
+table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,action=ct(table=3,zone=2)
   table=2,priority=1,action=drop
   dnl
   dnl Table 3
   dnl
-table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4
-table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,tcp,action=3
+table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,action=ct(commit,zone=2),4
+table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,action=3
   table=2,priority=1,action=drop
   ])

diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index a8934051e..b933924b8 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -381,6 +381,21 @@ do
   done
   AT_CLEANUP

+AT_SETUP([ovs-ofctl parse-flow with protocol order])
+for test_case in \
+    'tcp,ip' \
+    'tcp,ipv6' \
+    'udp,ip' \
+    'udp,ip6'
+do
+    set $test_case
+    field=$1
+    AT_CHECK_UNQUOTED([ovs-ofctl parse-flow "$field,actions=drop"], [1], [],
+[ovs-ofctl: TLVs must be ordered
+])
+done
+AT_CLEANUP
+
   AT_SETUP([ovs-ofctl action inconsistency (OpenFlow 1.1)])
   OVS_VSWITCHD_START
   add_of_ports br0 1 2 3

--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev