| Message ID | 20231130073107.37096-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Commit | a34e306a0db44bacb824691e53dff9c56e83421a |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] ofp-ct: Return error for unknown property in CT flush. | 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 | fail | test: fail |
On 11/30/23 08:31, Ales Musil wrote: > CT flush extension would silently ignore unknown properties, > which could lead to potential surprise by deleting more than > it was requested to. Return error on unknown property instead > to avoid this problem and at the same time inform the user > that the specified property is not supported. > > Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic match.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Rebase on top of current master. > Address comments from Ilya: > - Add the check also into ofp_ct_tuple_decode_nested. > --- > lib/ofp-ct.c | 11 +++++++++++ > tests/ofp-print.at | 18 ++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > index 85a9d8bec..53964cf09 100644 > --- a/lib/ofp-ct.c > +++ b/lib/ofp-ct.c > @@ -31,6 +31,9 @@ > #include "openvswitch/ofp-prop.h" > #include "openvswitch/ofp-util.h" > #include "openvswitch/packets.h" > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(ofp_ct); > > static void > ofp_ct_tuple_format(struct ds *ds, const struct ofp_ct_tuple *tuple, > @@ -286,6 +289,10 @@ ofp_ct_tuple_decode_nested(struct ofpbuf *property, struct ofp_ct_tuple *tuple, > case NXT_CT_TUPLE_ICMP_CODE: > error = ofpprop_parse_u8(&inner, &tuple->icmp_code); > break; > + > + default: > + error = OFPPROP_UNKNOWN(false, "ofp_ct_tuple", type); > + break; > } > > if (error) { > @@ -377,6 +384,10 @@ ofp_ct_match_decode(struct ofp_ct_match *match, bool *with_zone, > } > error = ofpprop_parse_u16(&property, zone_id); > break; > + > + default: > + error = OFPPROP_UNKNOWN(false, "ofp_ct_match", type); > + break; > } > > if (error) { > diff --git a/tests/ofp-print.at b/tests/ofp-print.at > index 14aa55416..b4b4c6685 100644 > --- a/tests/ofp-print.at > +++ b/tests/ofp-print.at > @@ -4180,4 +4180,22 @@ AT_CHECK([ovs-ofctl ofp-print "\ > 00 01 00 20 00 00 00 00 \ > 00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 00 \ > " | grep -q OFPBPC_BAD_VALUE], [0]) > + > +AT_CHECK([ovs-ofctl ofp-print "\ > +01 04 00 20 00 00 00 03 00 00 23 20 00 00 00 20 \ > +06 \ > +00 00 00 00 00 00 00 \ > +00 80 00 08 00 00 00 00 \ > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > +AT_CHECK([grep -q "unknown ofp_ct_match property type 128" stderr], [0]) Hmm, the 'ofp_ct_match' is not really something user-visible. I think, it's better to point to the actual OpenFlow entity here, i.e. 'NXT_CT_FLUSH'. > + > +AT_CHECK([ovs-ofctl ofp-print "\ > +01 04 00 28 00 00 00 03 00 00 23 20 00 00 00 20 \ > +06 \ > +00 00 00 00 00 00 00 \ > +00 00 00 10 00 00 00 00 \ > +00 80 00 08 00 50 00 00 \ > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > +AT_CHECK([grep -q "unknown ofp_ct_tuple property type 128" stderr], [0]) Same here. We do not have a direct replacement, but something like 'NXT_CT_TUPLE' might make sense as it is a prefix of the actual properties here. If you agree, I can replace these while applying the patch. WDYT? Best regards, Ilya Maximets.
On Sat, Dec 2, 2023 at 12:21 AM Ilya Maximets <i.maximets@ovn.org> wrote: > On 11/30/23 08:31, Ales Musil wrote: > > CT flush extension would silently ignore unknown properties, > > which could lead to potential surprise by deleting more than > > it was requested to. Return error on unknown property instead > > to avoid this problem and at the same time inform the user > > that the specified property is not supported. > > > > Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic > match.") > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > v2: Rebase on top of current master. > > Address comments from Ilya: > > - Add the check also into ofp_ct_tuple_decode_nested. > > --- > > lib/ofp-ct.c | 11 +++++++++++ > > tests/ofp-print.at | 18 ++++++++++++++++++ > > 2 files changed, 29 insertions(+) > > > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > > index 85a9d8bec..53964cf09 100644 > > --- a/lib/ofp-ct.c > > +++ b/lib/ofp-ct.c > > @@ -31,6 +31,9 @@ > > #include "openvswitch/ofp-prop.h" > > #include "openvswitch/ofp-util.h" > > #include "openvswitch/packets.h" > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(ofp_ct); > > > > static void > > ofp_ct_tuple_format(struct ds *ds, const struct ofp_ct_tuple *tuple, > > @@ -286,6 +289,10 @@ ofp_ct_tuple_decode_nested(struct ofpbuf *property, > struct ofp_ct_tuple *tuple, > > case NXT_CT_TUPLE_ICMP_CODE: > > error = ofpprop_parse_u8(&inner, &tuple->icmp_code); > > break; > > + > > + default: > > + error = OFPPROP_UNKNOWN(false, "ofp_ct_tuple", type); > > + break; > > } > > > > if (error) { > > @@ -377,6 +384,10 @@ ofp_ct_match_decode(struct ofp_ct_match *match, > bool *with_zone, > > } > > error = ofpprop_parse_u16(&property, zone_id); > > break; > > + > > + default: > > + error = OFPPROP_UNKNOWN(false, "ofp_ct_match", type); > > + break; > > } > > > > if (error) { > > diff --git a/tests/ofp-print.at b/tests/ofp-print.at > > index 14aa55416..b4b4c6685 100644 > > --- a/tests/ofp-print.at > > +++ b/tests/ofp-print.at > > @@ -4180,4 +4180,22 @@ AT_CHECK([ovs-ofctl ofp-print "\ > > 00 01 00 20 00 00 00 00 \ > > 00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 00 > \ > > " | grep -q OFPBPC_BAD_VALUE], [0]) > > + > > +AT_CHECK([ovs-ofctl ofp-print "\ > > +01 04 00 20 00 00 00 03 00 00 23 20 00 00 00 20 \ > > +06 \ > > +00 00 00 00 00 00 00 \ > > +00 80 00 08 00 00 00 00 \ > > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > > +AT_CHECK([grep -q "unknown ofp_ct_match property type 128" stderr], [0]) > > Hmm, the 'ofp_ct_match' is not really something user-visible. > I think, it's better to point to the actual OpenFlow entity > here, i.e. 'NXT_CT_FLUSH'. > > > + > > +AT_CHECK([ovs-ofctl ofp-print "\ > > +01 04 00 28 00 00 00 03 00 00 23 20 00 00 00 20 \ > > +06 \ > > +00 00 00 00 00 00 00 \ > > +00 00 00 10 00 00 00 00 \ > > +00 80 00 08 00 50 00 00 \ > > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > > +AT_CHECK([grep -q "unknown ofp_ct_tuple property type 128" stderr], [0]) > > Same here. We do not have a direct replacement, but something like > 'NXT_CT_TUPLE' might make sense as it is a prefix of the actual > properties here. > > If you agree, I can replace these while applying the patch. > > WDYT? > That sounds reasonable. > > Best regards, Ilya Maximets. > > Thanks, Ales
Ales Musil <amusil@redhat.com> writes: > CT flush extension would silently ignore unknown properties, > which could lead to potential surprise by deleting more than > it was requested to. Return error on unknown property instead > to avoid this problem and at the same time inform the user > that the specified property is not supported. > > Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic match.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- This is a test - please don't change the state in patchwork yet. Recheck-request: github
On 12/4/23 06:11, Ales Musil wrote: > > > On Sat, Dec 2, 2023 at 12:21 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: > > On 11/30/23 08:31, Ales Musil wrote: > > CT flush extension would silently ignore unknown properties, > > which could lead to potential surprise by deleting more than > > it was requested to. Return error on unknown property instead > > to avoid this problem and at the same time inform the user > > that the specified property is not supported. > > > > Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic match.") > > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>> > > --- > > v2: Rebase on top of current master. > > Address comments from Ilya: > > - Add the check also into ofp_ct_tuple_decode_nested. > > --- > > lib/ofp-ct.c | 11 +++++++++++ > > tests/ofp-print.at <http://ofp-print.at> | 18 ++++++++++++++++++ > > 2 files changed, 29 insertions(+) > > > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > > index 85a9d8bec..53964cf09 100644 > > --- a/lib/ofp-ct.c > > +++ b/lib/ofp-ct.c > > @@ -31,6 +31,9 @@ > > #include "openvswitch/ofp-prop.h" > > #include "openvswitch/ofp-util.h" > > #include "openvswitch/packets.h" > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(ofp_ct); > > > > static void > > ofp_ct_tuple_format(struct ds *ds, const struct ofp_ct_tuple *tuple, > > @@ -286,6 +289,10 @@ ofp_ct_tuple_decode_nested(struct ofpbuf *property, struct ofp_ct_tuple *tuple, > > case NXT_CT_TUPLE_ICMP_CODE: > > error = ofpprop_parse_u8(&inner, &tuple->icmp_code); > > break; > > + > > + default: > > + error = OFPPROP_UNKNOWN(false, "ofp_ct_tuple", type); > > + break; > > } > > > > if (error) { > > @@ -377,6 +384,10 @@ ofp_ct_match_decode(struct ofp_ct_match *match, bool *with_zone, > > } > > error = ofpprop_parse_u16(&property, zone_id); > > break; > > + > > + default: > > + error = OFPPROP_UNKNOWN(false, "ofp_ct_match", type); > > + break; > > } > > > > if (error) { > > diff --git a/tests/ofp-print.at <http://ofp-print.at> b/tests/ofp-print.at <http://ofp-print.at> > > index 14aa55416..b4b4c6685 100644 > > --- a/tests/ofp-print.at <http://ofp-print.at> > > +++ b/tests/ofp-print.at <http://ofp-print.at> > > @@ -4180,4 +4180,22 @@ AT_CHECK([ovs-ofctl ofp-print "\ > > 00 01 00 20 00 00 00 00 \ > > 00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 00 \ > > " | grep -q OFPBPC_BAD_VALUE], [0]) > > + > > +AT_CHECK([ovs-ofctl ofp-print "\ > > +01 04 00 20 00 00 00 03 00 00 23 20 00 00 00 20 \ > > +06 \ > > +00 00 00 00 00 00 00 \ > > +00 80 00 08 00 00 00 00 \ > > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > > +AT_CHECK([grep -q "unknown ofp_ct_match property type 128" stderr], [0]) > > Hmm, the 'ofp_ct_match' is not really something user-visible. > I think, it's better to point to the actual OpenFlow entity > here, i.e. 'NXT_CT_FLUSH'. > > > + > > +AT_CHECK([ovs-ofctl ofp-print "\ > > +01 04 00 28 00 00 00 03 00 00 23 20 00 00 00 20 \ > > +06 \ > > +00 00 00 00 00 00 00 \ > > +00 00 00 10 00 00 00 00 \ > > +00 80 00 08 00 50 00 00 \ > > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > > +AT_CHECK([grep -q "unknown ofp_ct_tuple property type 128" stderr], [0]) > > Same here. We do not have a direct replacement, but something like > 'NXT_CT_TUPLE' might make sense as it is a prefix of the actual > properties here. > > If you agree, I can replace these while applying the patch. > > WDYT? > > > That sounds reasonable. Thanks! I made the change. Applied and backported down to 3.1. Best regards, Ilya Maximets. > > > > Best regards, Ilya Maximets. > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com <mailto:amusil@redhat.com> > > <https://red.ht/sig> >
On 12/4/23 19:22, Aaron Conole wrote: > Ales Musil <amusil@redhat.com> writes: > >> CT flush extension would silently ignore unknown properties, >> which could lead to potential surprise by deleting more than >> it was requested to. Return error on unknown property instead >> to avoid this problem and at the same time inform the user >> that the specified property is not supported. >> >> Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic match.") >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- > > This is a test - please don't change the state in patchwork yet. > > Recheck-request: github > I hope it was enough time for testing. :) I applied the change now. Best regards, Ilya Maximets.
diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c index 85a9d8bec..53964cf09 100644 --- a/lib/ofp-ct.c +++ b/lib/ofp-ct.c @@ -31,6 +31,9 @@ #include "openvswitch/ofp-prop.h" #include "openvswitch/ofp-util.h" #include "openvswitch/packets.h" +#include "openvswitch/vlog.h" + +VLOG_DEFINE_THIS_MODULE(ofp_ct); static void ofp_ct_tuple_format(struct ds *ds, const struct ofp_ct_tuple *tuple, @@ -286,6 +289,10 @@ ofp_ct_tuple_decode_nested(struct ofpbuf *property, struct ofp_ct_tuple *tuple, case NXT_CT_TUPLE_ICMP_CODE: error = ofpprop_parse_u8(&inner, &tuple->icmp_code); break; + + default: + error = OFPPROP_UNKNOWN(false, "ofp_ct_tuple", type); + break; } if (error) { @@ -377,6 +384,10 @@ ofp_ct_match_decode(struct ofp_ct_match *match, bool *with_zone, } error = ofpprop_parse_u16(&property, zone_id); break; + + default: + error = OFPPROP_UNKNOWN(false, "ofp_ct_match", type); + break; } if (error) { diff --git a/tests/ofp-print.at b/tests/ofp-print.at index 14aa55416..b4b4c6685 100644 --- a/tests/ofp-print.at +++ b/tests/ofp-print.at @@ -4180,4 +4180,22 @@ AT_CHECK([ovs-ofctl ofp-print "\ 00 01 00 20 00 00 00 00 \ 00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 00 \ " | grep -q OFPBPC_BAD_VALUE], [0]) + +AT_CHECK([ovs-ofctl ofp-print "\ +01 04 00 20 00 00 00 03 00 00 23 20 00 00 00 20 \ +06 \ +00 00 00 00 00 00 00 \ +00 80 00 08 00 00 00 00 \ +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) +AT_CHECK([grep -q "unknown ofp_ct_match property type 128" stderr], [0]) + +AT_CHECK([ovs-ofctl ofp-print "\ +01 04 00 28 00 00 00 03 00 00 23 20 00 00 00 20 \ +06 \ +00 00 00 00 00 00 00 \ +00 00 00 10 00 00 00 00 \ +00 80 00 08 00 50 00 00 \ +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) +AT_CHECK([grep -q "unknown ofp_ct_tuple property type 128" stderr], [0]) + AT_CLEANUP
CT flush extension would silently ignore unknown properties, which could lead to potential surprise by deleting more than it was requested to. Return error on unknown property instead to avoid this problem and at the same time inform the user that the specified property is not supported. Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic match.") Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Rebase on top of current master. Address comments from Ilya: - Add the check also into ofp_ct_tuple_decode_nested. --- lib/ofp-ct.c | 11 +++++++++++ tests/ofp-print.at | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+)