Thanks for the review Jacob!
On Wed, Oct 14, 2026 at 5:04 PM Jacob Tanenbaum <[email protected]> wrote:
>
>
>
> 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.
I updated the type to be size_t.
>
>>
>> + 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?
After giving this some thought I decided that it would be a bad move
to make a working configuration that happened to have empty strings
for nexthops suddenly stop working. So I have excluded empty strings
from the IP address family check.
>
>>
>> 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
