On Tue, Oct 6, 2026 at 1:50 PM Mark Michelson via dev < [email protected]> wrote:
> In the current code, en-lflow checks the valid nexthops on a routing > policy to ensure they all have the same configured address family just > before writing the policy logical flows. > > This commit performs the check in en-route-policies instead, so that by > the time logical flows are being written, it is guaranteed that all > valid nexthops are of the same address family. > > Signed-off-by: Mark Michelson <[email protected]> > --- > northd/en-route-policies.c | 26 +++++++++++++++++++++++++- > northd/northd.c | 20 -------------------- > 2 files changed, 25 insertions(+), 21 deletions(-) > > diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c > index 1288a4a93..fd2025294 100644 > --- a/northd/en-route-policies.c > +++ b/northd/en-route-policies.c > @@ -238,6 +238,31 @@ build_route_policies(struct ovn_datapath *od, > continue; > } > > + /* Check that all the nexthops belong to the same addr > family. */ > + bool is_ipv4 = true; > + bool ips_match = true; > + for (uint16_t j = 0; j < rule->n_nexthops; j++) { > nit: I know this is taken from the original code but here you use uint16_t as the iterator while the loop directly below uses size_t for the same rule->n_nexthops, using size_t would be consistent and match rule->n_nexthops decleration. > + bool nexthop_is_ipv4 = !!strchr(rule->nexthops[j], '.'); > + > + if (j == 0) { > + is_ipv4 = nexthop_is_ipv4; > + } > + > + if (nexthop_is_ipv4 != is_ipv4) { > + static struct vlog_rate_limit rl = > + VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "nexthop [%s] of the router policy > with " > + "the match [%s] does not belong to the > same " > + "address family as other next hops", > + rule->nexthops[j], rule->match); > + ips_match = false; > + break; > + } > + } > + if (!ips_match) { > + continue; > + } > + > Could you move the family check below the empty string filter? Before this patch nexthops = ["10.0.0.1", ""] would get accepted and installed as a valid single-nexthop reroute but your patch causes it to be rejected because the empty string evaluates as an IPv6 address. I know this is a misconfiguration but is degrading it the intent of this patch? > valid_nexthops = xcalloc(rule->n_nexthops, sizeof > *valid_nexthops); > for (size_t j = 0; j < rule->n_nexthops; j++) { > char *nexthop = rule->nexthops[j]; > @@ -246,7 +271,6 @@ build_route_policies(struct ovn_datapath *od, > } > > struct ovn_port *out_port = NULL; > - bool is_ipv4 = strchr(nexthop, '.') ? true : false; > > if (!find_policy_outport(od, rule, nexthop, is_ipv4, NULL, > &out_port)) { > diff --git a/northd/northd.c b/northd/northd.c > index 91e2fa601..c0de0e444 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -12328,29 +12328,9 @@ build_ecmp_routing_policy_flows(struct > lflow_table *lflows, > uint16_t ecmp_group_id, > struct lflow_ref *lflow_ref) > { > - bool nexthops_is_ipv4 = true; > const struct nbrec_logical_router_policy *rule = rp->rule; > ovs_assert(rp->n_valid_nexthops > 1); > > - /* Check that all the nexthops belong to the same addr family before > - * adding logical flows. */ > - for (uint16_t i = 0; i < rp->n_valid_nexthops; i++) { > - bool is_ipv4 = strchr(rp->valid_nexthops[i], '.') ? true : false; > - > - if (i == 0) { > - nexthops_is_ipv4 = is_ipv4; > - } > - > - if (is_ipv4 != nexthops_is_ipv4) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_WARN_RL(&rl, "nexthop [%s] of the router policy with " > - "the match [%s] do not belong to the same > address " > - "family as other next hops", > - rp->valid_nexthops[i], rule->match); > - return; > - } > - } > - > struct ds match = DS_EMPTY_INITIALIZER; > struct ds actions = DS_EMPTY_INITIALIZER; > > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Jacob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
