diff mbox series

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

Message ID 20221118081343.90227-1-wanjunjie@bytedance.com
State Changes Requested
Headers show
Series [ovs-dev] ofp-flow: TLVs should be ordered | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Wan Junjie Nov. 18, 2022, 8:13 a.m. UTC
According to the ovs-fields manpage, 'TLVs must be ordered so that
a field’s prerequisites are satisfied before if is parsed'.
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.

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

Comments

Adrián Moreno March 31, 2023, 4:39 p.m. UTC | #1
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.

> Signed-off-by: Wan Junjie <wanjunjie@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
Wan Junjie April 4, 2023, 7:19 a.m. UTC | #2
Hi Adrian,

On Sat, Apr 1, 2023 at 12:39 AM Adrian Moreno <amorenoz@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?


Regards,
Wan Junjie

>
> > Signed-off-by: Wan Junjie <wanjunjie@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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Adrián Moreno July 5, 2023, 1:33 p.m. UTC | #3
On 4/4/23 09:19, Wan Junjie wrote:
> Hi Adrian,
> 
> On Sat, Apr 1, 2023 at 12:39 AM Adrian Moreno <amorenoz@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 <wanjunjie@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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

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