| Message ID | 20221118081343.90227-1-wanjunjie@bytedance.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofp-flow: TLVs should be ordered | expand |
| 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 |
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
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
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 --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
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(-)