From: Numan Siddique <[email protected]>
If the value of pkt_mark in the router policy options is greater than
2147483647, we
are ignoring it. This is because we use smap_get_int(). This patch fixes this
issue
by adding a wrapper function - ovn_smap_get_uint(). The OpenvSwitch library
doesn't have
this smap helper function.
ovn_smap_get_uint() needs to be removed once the OpenvSwitch library has this
helper
function. A comment has been added in the code for this.
Fixes: a123ef0fb8fd("Support packet metadata marking for logical router
policies.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1878248
Reported-by: Alexander Constantinescu <[email protected]>
Signed-off-by: Numan Siddique <[email protected]>
---
v1 -> v2
--------
* Added a wrapper function ovn_smap_get_uint() in lib/util.c
lib/ovn-util.c | 20 ++++++++++++++++++++
lib/ovn-util.h | 5 +++++
northd/ovn-northd.c | 4 ++--
tests/ovn.at | 43 +++++++++++++++++++++++++++++++++++++------
4 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index cdb5e18fb0..47c7f57a0e 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -641,3 +641,23 @@ str_tolower(const char *orig)
return copy;
}
+
+/* This is a wrapper function which get the value associated with 'key' in
+ * 'smap' and converts it to an unsigned int. If 'key' is not in 'smap' or a
+ * valid unsigned integer can't be parsed from it's value, returns 'def'.
+ *
+ * Note: Remove this function once OpenvSwitch library (lib/smap.h) has this
+ * helper function.
+ */
+unsigned int
+ovn_smap_get_uint(const struct smap *smap, const char *key, unsigned int def)
+{
+ const char *value = smap_get(smap, key);
+ unsigned int u_value;
+
+ if (!value || !str_to_uint(value, 10, &u_value)) {
+ return def;
+ }
+
+ return u_value;
+}
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index d9aadcbc03..a597efb505 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -32,6 +32,7 @@ struct eth_addr;
struct sbrec_port_binding;
struct sbrec_datapath_binding;
struct unixctl_conn;
+struct smap;
struct ipv4_netaddr {
ovs_be32 addr; /* 192.168.10.123 */
@@ -152,6 +153,10 @@ char *normalize_ipv4_prefix(ovs_be32 ipv4, unsigned int
plen);
char *normalize_ipv6_prefix(struct in6_addr ipv6, unsigned int plen);
char *normalize_v46_prefix(const struct v46_ip *prefix, unsigned int plen);
+/* Temporary util function until ovs library has smap_get_unit. */
+unsigned int ovn_smap_get_uint(const struct smap *smap, const char *key,
+ unsigned int def);
+
/* Returns a lowercase copy of orig.
* Caller must free the returned string.
*/
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index cfec6a2c86..0c12a7437e 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7547,7 +7547,7 @@ build_routing_policy_flow(struct hmap *lflows, struct
ovn_datapath *od,
rule->priority, rule->nexthop);
return;
}
- uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0);
+ uint32_t pkt_mark = ovn_smap_get_uint(&rule->options, "pkt_mark", 0);
if (pkt_mark) {
ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
}
@@ -7568,7 +7568,7 @@ build_routing_policy_flow(struct hmap *lflows, struct
ovn_datapath *od,
} else if (!strcmp(rule->action, "drop")) {
ds_put_cstr(&actions, "drop;");
} else if (!strcmp(rule->action, "allow")) {
- uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0);
+ uint32_t pkt_mark = ovn_smap_get_uint(&rule->options, "pkt_mark", 0);
if (pkt_mark) {
ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
}
diff --git a/tests/ovn.at b/tests/ovn.at
index a6f1fb58f8..b2876b806e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -20589,7 +20589,7 @@ pol4=$(ovn-nbctl --bare --columns _uuid find
logical_router_policy priority=2001
pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
priority=1001)
ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4
-ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5
+ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=4294967295
ovn-nbctl --wait=hv sync
OVS_WAIT_UNTIL([
@@ -20607,9 +20607,11 @@ OVS_WAIT_UNTIL([
grep "load:0x4->NXM_NX_PKT_MARK" -c)
])
+as hv1 ovs-ofctl dump-flows br-int table=20 | grep NXM_NX_PKT_MARK
+
OVS_WAIT_UNTIL([
test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=20 | \
- grep "load:0x5->NXM_NX_PKT_MARK" -c)
+ grep "load:0xffffffff->NXM_NX_PKT_MARK" -c)
])
AT_CHECK([as hv1 ovs-ofctl del-flows br-phys])
@@ -20620,7 +20622,7 @@ table=0, priority=100, pkt_mark=0x64 actions=drop
table=0, priority=100, pkt_mark=0x2 actions=drop
table=0, priority=100, pkt_mark=0x3 actions=drop
table=0, priority=100, pkt_mark=0x4 actions=drop
-table=0, priority=100, pkt_mark=0x5 actions=drop
+table=0, priority=100, pkt_mark=0xffffffff actions=drop
])
AT_CHECK([as hv1 ovs-ofctl --protocols=OpenFlow13 add-flows br-phys flows.txt])
@@ -20690,7 +20692,7 @@ AT_CHECK([
AT_CHECK([
test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
- grep "priority=100,pkt_mark=0x5" | \
+ grep "priority=100,pkt_mark=0xffffffff" | \
grep "n_packets=0" -c)
])
@@ -20745,7 +20747,7 @@ send_icmp6_packet hv1 hv1-vif2 505400000004
00000000ff01 ${src_ip6} ${dst_ip6}
OVS_WAIT_UNTIL([
test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
- grep "priority=100,pkt_mark=0x5" | \
+ grep "priority=100,pkt_mark=0xffffffff" | \
grep "n_packets=1" -c)
])
@@ -20771,10 +20773,39 @@ OVS_WAIT_UNTIL([
AT_CHECK([
test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
- grep "priority=100,pkt_mark=0x5" | \
+ grep "priority=100,pkt_mark=0xffffffff" | \
grep "n_packets=1" -c)
])
+AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" |
sort], [0], [dnl
+ table=12(lr_in_policy ), priority=1001 , dnl
+match=(ip6), action=(pkt.mark = 4294967295; next;)
+])
+
+ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=-1
+AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" |
sort], [0], [dnl
+ table=12(lr_in_policy ), priority=1001 , dnl
+match=(ip6), action=(next;)
+])
+
+ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=2147483648
+AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" |
sort], [0], [dnl
+ table=12(lr_in_policy ), priority=1001 , dnl
+match=(ip6), action=(pkt.mark = 2147483648; next;)
+])
+
+ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=foo
+AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" |
sort], [0], [dnl
+ table=12(lr_in_policy ), priority=1001 , dnl
+match=(ip6), action=(next;)
+])
+
+ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=4294967296
+AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" |
sort], [0], [dnl
+ table=12(lr_in_policy ), priority=1001 , dnl
+match=(ip6), action=(next;)
+])
+
OVN_CLEANUP([hv1])
AT_CLEANUP
--
2.26.2
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
