The "nexthop" column in the northbound database is deprecated in favor of the "nexthops" column. However, the "nexthops" column only matters if the policy is a "reroute" policy. Therefore, we can ignore the fact that a deprecated column has its value set if the policy type does not rely on nexthops at all.
Signed-off-by: Mark Michelson <[email protected]> --- v1 -> v2: * Added a NEWS entry about this change. * Updated ovn-nb documentation so that it does not imply that the "nexthop" column is used. --- NEWS | 5 +++ northd/en-route-policies.c | 17 +++++----- ovn-nb.xml | 3 +- tests/ovn-northd.at | 66 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 10 deletions(-) diff --git a/NEWS b/NEWS index c4ebe5c39..f177b7f32 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,10 @@ Post v26.09.0 -------------- + - Router policies that used the deprecated "nexthop" column used to + always be skipped and not applied. Now these router policies are + only skipped if their action is "reroute". Router policies of type + "allow", "drop", and "jump" will still be accepted, even if the + "nexthop" column is set on those policies. - Learn ARP sender hardware address (arp.sha) and IPv6 Neighbor Discovery link-layer addresses (nd.tll, nd.sll) into the FDB table in addition to eth.src. This allows VRRP virtual MACs to diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c index 9597980ec..2110b8b60 100644 --- a/northd/en-route-policies.c +++ b/northd/en-route-policies.c @@ -176,14 +176,6 @@ build_route_policies(struct ovn_datapath *od, for (int i = 0; i < od->nbr->n_policies; i++) { const struct nbrec_logical_router_policy *rule = od->nbr->policies[i]; - if (rule->nexthop && rule->nexthop[0]) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "Logical router: %s, policy uses deprecated" - " column \"nexthop\", this column is ignored. Please" - "use \"nexthops\" column instead.", od->nbr->name); - continue; - } - size_t n_valid_nexthops = 0; char **valid_nexthops = NULL; uint32_t chain_id = 0; @@ -224,6 +216,15 @@ build_route_policies(struct ovn_datapath *od, } if (!strcmp(rule->action, "reroute")) { + if (rule->nexthop && rule->nexthop[0]) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "Logical router: %s, policy uses deprecated" + " column \"nexthop\", this policy is skipped. " + "Please use \"nexthops\" column instead.", + od->nbr->name); + continue; + } + if (rule->output_port && rule->n_nexthops != 1) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, diff --git a/ovn-nb.xml b/ovn-nb.xml index feea50a25..ebcd3e4d1 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -5368,8 +5368,7 @@ or </li> <li> - <code>reroute</code>: Reroute packet to <ref column="nexthop"/> or - <ref column="nexthops"/>. + <code>reroute</code>: Reroute packet to <ref column="nexthops"/> </li> <li> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 7bd0466c6..1401aecea 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -25135,3 +25135,69 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE OVN_CLEANUP_NORTHD AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Router policy selective nexthop deprecation warning]) +ovn_start + +check ovn-nbctl lr-add lr0 +check ovn-nbctl lrp-add lr0 lr0-p1 00:00:00:00:00:01 172.16.1.1/24 + +# Add a policy of each type to this router +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 10.0.0.0/24" allow +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 20.0.0.0/24" drop +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 30.0.0.0/24" reroute 172.16.1.50 +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 40.0.0.0/24" jump target +check ovn-nbctl --chain=target lr-policy-add lr0 200 "1" allow +check ovn-nbctl --wait=sb sync + +# Let's double-check that all of the expected flows are present +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | ovn_strip_lflows | sort], [0], [dnl + table=??(lr_in_policy ), priority=0 , match=(1), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.0.0/24)), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 20.0.0.0/24)), action=(drop;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 30.0.0.0/24)), action=(reg0 = 172.16.1.50; reg5 = 172.16.1.1; eth.src = 00:00:00:00:00:01; outport = "lr0-p1"; flags.loopback = 1; reg8[[0..15]] = 0; reg9[[9]] = 1; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 40.0.0.0/24)), action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);) + table=??(lr_in_policy ), priority=200 , match=(reg9[[16..31]] == 1 && (1)), action=(reg8[[0..15]] = 0; next;) +]) + +allow_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 action=allow) +drop_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 action=drop) +jump_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 action=jump) +reroute_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 action=reroute) + +# For the allow, drop, and jump policies, we'll set the "nexthop" column. The policies +# should remain unchanged, and we shouldn't see a warning in the log. +check ovn-nbctl set Logical_Router_Policy $allow_uuid nexthop=50.0.0.50 +check ovn-nbctl set Logical_Router_Policy $drop_uuid nexthop=50.0.0.50 +check ovn-nbctl set Logical_Router_Policy $jump_uuid nexthop=50.0.0.50 +check ovn-nbctl --wait=sb sync + +AT_CHECK([grep -qE "policy uses deprecated" northd/ovn-northd.log], [1]) + +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | ovn_strip_lflows | sort], [0], [dnl + table=??(lr_in_policy ), priority=0 , match=(1), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.0.0/24)), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 20.0.0.0/24)), action=(drop;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 30.0.0.0/24)), action=(reg0 = 172.16.1.50; reg5 = 172.16.1.1; eth.src = 00:00:00:00:00:01; outport = "lr0-p1"; flags.loopback = 1; reg8[[0..15]] = 0; reg9[[9]] = 1; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 40.0.0.0/24)), action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);) + table=??(lr_in_policy ), priority=200 , match=(reg9[[16..31]] == 1 && (1)), action=(reg8[[0..15]] = 0; next;) +]) + +# Now let's set the "nexthop" column on the reroute policy. This should result in the +# warning appearing and the reroute policy not showing up in the lflows. +check ovn-nbctl set Logical_Router_Policy $reroute_uuid nexthop=50.0.0.50 +check ovn-nbctl --wait=sb sync + +AT_CHECK([grep -qE "policy uses deprecated" northd/ovn-northd.log], [0]) +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | ovn_strip_lflows | sort], [0], [dnl + table=??(lr_in_policy ), priority=0 , match=(1), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.0.0/24)), action=(reg8[[0..15]] = 0; next;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 20.0.0.0/24)), action=(drop;) + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == 0 && (ip4.src == 40.0.0.0/24)), action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);) + table=??(lr_in_policy ), priority=200 , match=(reg9[[16..31]] == 1 && (1)), action=(reg8[[0..15]] = 0; next;) +]) + +OVN_CLEANUP_NORTHD +AT_CLEANUP +]) -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
